[RFC][icedtea-web]: Added support for signed JNLP file

Saad Mohammad smohammad at redhat.com
Tue Jun 28 12:14:21 PDT 2011


On 06/27/2011 06:42 PM, Deepak Bhole wrote:
> The indentation above is incorrect... it should follow the same format
> as other entries in the ChangeLog. Also, not sure what the [2] means..
> it shouldn't be there.
Not sure, why the changelog is out of format. Nor do I know what the [2] 
is for, but here is the same formatted changelog I sent in the first 
email =):

2011-06-23  Saad Mohammad <smohammad at redhat.com>

     * netx/net/sourceforge/jnlp/JNLPVerify.java:
       Created this class to compares signed JNLP file with the 
launching JNLP file.
       It is able to compare both method of signing of a JNLP file: 
APPLICATION_TEMPLATE.JNLP
       and APPLICATION.JNLP
     * netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java:
       Added a custom exception: JNLPVerifyException. Also added 
JNLPVerifyException to methods
       that throws the custom exception.
       (initializeResources): Checks if there is any signed JNLP file 
within the signed jars.
        If signed JNLP file fails to match or fails to be verified, the 
application throws
       a JNLPVerifyException.
     * netx/net/sourceforge/jnlp/Node.java:
       Created a method that retrieves the attribute names of the Node in a
       private string [] member. The method returns the attribute names.
     * netx/net/sourceforge/nanoxml/XMLElement.java:
       (sanitizeInput): Changed parameter type of isr from 
InputStreamReader to Reader.

>> >  References
>> >  
>> >      1.http://jcp.org/aboutJava/communityprocess/maintenance/jsr056/jnlp-7_0-changes.html
>> >      2.mailto:smohammad at redhat.com
> ...
>> >  +    public static JNLPVerify getInstanceOf(Reader appTemplateFile, Reader launchJNLPFile,
>> >  +            boolean isTemplate) throws Exception {
> Jiri already pointed this out.. but throwing a generic Exception is a
> bad idea because it forces callers to catch all exceptions when they may
> not want to -- consider throwing a more restricted exception.
>
> Also, getInstanceOf suggests this that is a singleton class which it is
> not. The method name should be changed. Though frankly, I think this
> method shouldn't exist at all. Let the caller create the class and
> handle any exceptions from the constructor rather than having a wrapper
> just to re-throw the exception.
>
>> >  +        try {
>> >  +            JNLPVerify ret = new JNLPVerify(appTemplateFile, launchJNLPFile, isTemplate);
>> >  +            return ret;
>> >  +
>> >  +        } catch (Exception e) {
>> >  +            throw new Exception(
>> >  +                    "Failed to create an instance of JNLPVerify with specified Readers: "
>> >  +                            + e.getMessage());
>> >  +        }
>> >  +    }
>> >  +
>> >  +    /**
>> >  +     * Private constructor
>> >  +     *
>> >  +     * @param appTemplate
>> >  +     *            the reader stream of the signed APPLICATION.jnlp or
>> >  +     *            APPLICATION_TEMPLATE.jnlp
>> >  +     * @param launchJNLP
>> >  +     *            the reader stream of the launching JNLP file
>> >  +     * @param isTemplate
>> >  +     *            a boolean that specifies if appTemplateFile is a template
>> >  +     *
>> >  +     * @throws XMLParseException
>> >  +     *             if appTemplate and launchJNLP could not be parsed
>> >  +     * @throws IOException
>> >  +     *             if PipedOutputStream or InputStreamReader failed to be
>> >  +     *             created
>> >  +     */
>> >  +    private JNLPVerify(Reader appTemplate, Reader launchJNLP, boolean isTemplate)
>> >  +            throws XMLParseException, IOException {
>> >  +
> This is not a singleton class nor is it a factory and all of the
> constructor parameters are available to the caller, so there is no real
> need for a private constructor.
>
> ...
>> >  +
>> >  +        if (appTemplate != null&&  launchJNLP != null) {
>> >  +
>> >  +            Node appTemplateNode = (Node) appTemplate;
>> >  +            Node launchJNLPNode = (Node) launchJNLP;
>> >  +
>> >  +            //Store children of Node
>> >  +            LinkedList<Node>  appTemplateChild = new LinkedList<Node>(Arrays.asList(appTemplateNode
>> >  +                    .getChildNodes()));
>> >  +            LinkedList<Node>  launchJNLPChild = new LinkedList<Node>(Arrays.asList(launchJNLPNode
>> >  +                    .getChildNodes()));
>> >  +
>> >  +            // Compare only if both Nodes have the same name, else return false
>> >  +            if (appTemplateNode.getNodeName().equals(launchJNLPNode.getNodeName())) {
>> >  +
>> >  +                if (appTemplateChild.size() == launchJNLPChild.size()) { // Compare children
>> >  +
>> >  +                    int childLength = appTemplateChild.size();
>> >  +
>> >  +                    for (int i = 0; i<  childLength;) {
>> >  +                        for (int j = 0; j<  childLength; j++) {
>> >  +                            boolean isSame = compareNodes(appTemplateChild.get(i),
>> >  +                                    launchJNLPChild.get(j));
>> >  +
>> >  +                            if (!isSame&&  j == childLength - 1)
>> >  +                                return false;
>> >  +                            else if (isSame) { // If both child matches, remove them from the list of children
>> >  +                                appTemplateChild.remove(i);
>> >  +                                launchJNLPChild.remove(j);
>> >  +                                --childLength;
>> >  +                                break;
>> >  +                            }
>> >  +                        }
>> >  +                    }
>> >  +
> This logic seems more complex than it needs to be and is O(n2). You can
> define a comparator and use Arrays.sort to sort the arrays that the
> getChildNodes() methods return. Then you only need to iterate over the
> array once -- resulting in O(nlogn) performance and much cleaner code.
>
> ...
>> >  +
>> >  +            ArrayList<String>  appTemplateAttributes = new ArrayList<String>(Arrays.asList(appTemplateNode.getAttributeNames()));
>> >  +            ArrayList<String>  launchJNLPAttributes = new ArrayList<String>(Arrays.asList(launchJNLPNode.getAttributeNames()));
>> >  +
> Same change as above can be made here.
>
> ...
>
>> >  
>> >  +            if (js.anyJarsSigned()) {
>> >  +                //If there are any signed Jars, check if JNLP file is signed
>> >  +
>> >  +                if (JNLPRuntime.isDebug())
>> >  +                    System.out.println("STARTING check for signed JNLP file...");
>> >  +
>> >  +                final String template = "JNLP-INF/APPLICATION_TEMPLATE.JNLP";
>> >  +                final String application = "JNLP-INF/APPLICATION.JNLP";
>> >  +
>> >  +                for (int i = 0; i<  jars.length; i++) {
>> >  +                    List<JARDesc>  eachJar = new ArrayList<JARDesc>();
>> >  +                    JarSigner signer = new JarSigner();
>> >  +                    eachJar.add(jars[i]); //Adds only the single jar to check if the jar has a valid signature
>> >  +
>> >  +                    tracker.addResource(jars[i].getLocation(), jars[i].getVersion(),
>> >  +                            getDownloadOptionsForJar(jars[i]),
>> >  +                            jars[i].isCacheable() ? JNLPRuntime.getDefaultUpdatePolicy()
>> >  +                                    : UpdatePolicy.FORCE);
>> >  +
>> >  +                    try {
>> >  +                        signer.verifyJars(eachJar, tracker);
>> >  +
>> >  +                        if (signer.allJarsSigned()) { //If the jar is signed
>> >  +                            URL location = jars[i].getLocation();
>> >  +                            File localFile = tracker.getCacheFile(location);
>> >  +
>> >  +                            if (localFile != null) {
> There is a security issue here -- what if local file is null? Then no check will happen?
>
>> >  +                                try {
>> >  +                                    JarFile jarFile = new JarFile(localFile);
>> >  +                                    Enumeration<JarEntry>  entries = jarFile.entries();
>> >  +                                    JarEntry je;
>> >  +
>> >  +                                    while (entries.hasMoreElements()) {
>> >  +                                        je = entries.nextElement();
>> >  +                                        String jeName = je.getName().toUpperCase();
>> >  +
>> >  +                                        if (jeName.equals(template) || jeName.equals(application)) {
>> >  +
> The specification states that the file names should be recognized
> regardless of case. The above code does not allow that.
>
jeName is used to store the jar entry name in all uppercase before 
comparing it in the if statement. "String jeName = 
je.getName().toUpperCase();"
I think this should work regardless of the case!

-- 
Cheers,
Saad Mohammad

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20110628/0169f77f/attachment.html 


More information about the distro-pkg-dev mailing list