[rfc][icedteaweb] Support for Entry-Point manifest attribute

[Jacob Wisor]

Hello there! :-)

On 02/25/2015 06:25 PM, Jiri Vanek wrote:
> On 02/25/2015 06:19 PM, Jiri Vanek wrote:
>> Hello, this is impl of another security manifest att:
>> http://docs.oracle.com/javase/7/docs/technotes/guides/jweb/security/manifest.html#entry_pt
>>
>> I would like to go with it to 1.5 too.
>>
>> Speaking about the attributes:
>> does http://www.geogebra.org/webstart/geogebra.jnlp library loading promt
>> seems correct to you?
>>
>> J.
> Crap - improved one
>
> The check is valid only if RIA is signed. I had it removed for testing...

> diff -r b053e7638d7f netx/net/sourceforge/jnlp/JNLPFile.java
> --- a/netx/net/sourceforge/jnlp/JNLPFile.java	Wed Feb 25 15:40:44 2015 +0100
> +++ b/netx/net/sourceforge/jnlp/JNLPFile.java	Wed Feb 25 18:23:18 2015 +0100
> @@ -900,6 +900,8 @@
>          public static final String CODEBASE = "Codebase";
>          public static final String TRUSTED_ONLY = "Trusted-Only";
>          public static final String TRUSTED_LIBRARY = "Trusted-Library";
> +        public static final String ENTRY_POINT="Entry-Point";
> +
>          private JNLPClassLoader loader;
>
>
> @@ -925,6 +927,18 @@
>              return loader.getMainClass();
>          }
>
> +         /**
> +         *
> +         * http://docs.oracle.com/javase/7/docs/technotes/guides/jweb/security/manifest.html#entry_pt
> +         */
> +        public String[] getEntryPoints() {
> +            return splitEntryPoints(getEntryPointString());
> +        }
> +
> +        public String getEntryPointString() {
> +            return getAttribute(ENTRY_POINT);

Warning: This effectively searches *all* JAR resource files for an Entry-Point 
attribute if the "main JAR" file's manifest does not have an Entry-Point attribute.
getAttribute(ENTRY_POINT) -> JNLPFile.getAttribute(new Attributes.Name(name)) -> 
JNLPFile.loader.checkForAttributeInJars(Arrays.asList(getResources().getJARs()), 
name)

So, even though JNLPClassLoader.checkForAttributeInJars() checks the main JAR 
file (whatever this might be, I guess this is the /first/ JAR file with a proper 
Main-Class attribute in its manifest) for the Entry-Point attribute first, this 
JAR file may not actually have one. Then, it goes on checking all other resource 
JAR files and one of them may have one. Although an application suite of JAR 
files could theoretically cross-reference entry points (main classes) over 
different JAR files, I do not think this is legal and is not intended by the 
specification. We should make sure that no cross-referencing in JAR files is 
allowed, in other words that Entry-Point as well as Main-Class attributes can 
only reference classes in the local JAR file. I may be wrong but you should 
definitely create a test for this.

> +        }
> +
>          /**
>           * http://docs.oracle.com/javase/7/docs/technotes/guides/jweb/manifest.html#app_name
>           */
> @@ -1047,8 +1061,8 @@
>                  }
>              }
>          }
> -    }
> -
> +    }
> +
>      public String createJnlpVendorValue() {
>          final String location;
>          if (getSourceLocation() != null) {
> @@ -1088,7 +1102,17 @@
>              return createJnlpTitleValue();
>          }
>          return getTitle() + " from " + createJnlpTitleValue();
> -
> +    }
> +
> +    static String[] splitEntryPoints(String eps) {
> +        if (eps == null || eps.trim().isEmpty()) {
> +            return null;
> +        }
> +        String[] result = eps.trim().split("\\s+");

This is actually supposed to be splitting *fully* *qualified* *class* *names*, 
not just arbitrary strings separated by an arbitrary count of white space
([ \t\n\x0B\f\r])characters. In fact, the specification speaks of exactly *a* 
*space*. This reads to me like exactly *one* *space* (U+0020) character.

java.util.jar.Attributes.Name take care of checking proper formatting of the 
Entry-Point attribute name. But what about the proper formatting of the value? 
So I guess IcedTea-Web fails only when it comes to actually loading the classes 
or something that is supposed to be a fully qualified class name. Hence, one 
could get away with non-existent class names, class names in other JAR files, or 
whatever string garbage as long as it is separated by white spaces. I don't 
know, this seems a little bit over tolerant for me, more like lax than tolerant.

Besides, please try giving a thought about context meaningful names, like 
entryPointAttributeValue for eps and splitEntryPoints for result.

> +        if (result.length == 0) {
> +            return null;
> +        }
> +        return result;

return result.length > 0 ? result : null; ;-)

>      }
>  }
>
> diff -r b053e7638d7f netx/net/sourceforge/jnlp/runtime/ManifestAttributesChecker.java
> --- a/netx/net/sourceforge/jnlp/runtime/ManifestAttributesChecker.java	Wed Feb 25 15:40:44 2015 +0100
> +++ b/netx/net/sourceforge/jnlp/runtime/ManifestAttributesChecker.java	Wed Feb 25 18:23:18 2015 +0100
> @@ -81,6 +81,7 @@
>              checkCodebaseAttribute();
>              checkPermissionsAttribute();
>              checkApplicationLibraryAllowableCodebaseAttribute();
> +            checkEntryPoint();
>          } else {
>              OutputController.getLogger().log(OutputController.Level.WARNING_ALL, "Manifest attribute checks are disabled.");
>          }
> @@ -90,6 +91,28 @@
>          final String deploymentProperty = JNLPRuntime.getConfiguration().getProperty(DeploymentConfiguration.KEY_ENABLE_MANIFEST_ATTRIBUTES_CHECK);
>          return Boolean.parseBoolean(deploymentProperty);
>      }
> +
> +    /*
> +     * http://docs.oracle.com/javase/7/docs/technotes/guides/jweb/security/manifest.html#entry_pt
> +     */
> +    private void checkEntryPoint() throws LaunchException {
> +        if (signing == SigningState.NONE) {
> +            return; /*when app is not signed at all, then skip this check*/
> +        }
> +        final String[] eps = file.getManifestsAttributes().getEntryPoints();
> +        String mainClass = file.getLaunchInfo().getMainClass();
> +        if (eps == null) {
> +            OutputController.getLogger().log(OutputController.Level.MESSAGE_DEBUG, "Entry-Point manifest attribute for yours '" + mainClass + "'not found. Continuing.");
> +            return;
> +        }
> +        for (String ep : eps) {
> +            if (ep.equals(mainClass)) {
> +                OutputController.getLogger().log(OutputController.Level.MESSAGE_DEBUG, "Entry-Point od " + ep + " mathches " + mainClass + " continuing.");
> +                return;
> +            }
> +        }
> +        throw new LaunchException("None of '" + file.getManifestsAttributes().getEntryPointString() + "' matched yours " + mainClass + ". Thats fatal.");
> +    }

So, no check for fully qualified (and locally existing) class names even here?

Please test this patch more thoroughly.

Jacob