[rfc][icedteaweb] Support for Entry-Point manifest attribute
Jiri Vanek
jvanek at redhat.com
Mon Mar 16 16:00:46 UTC 2015
On 03/04/2015 10:01 AM, Jacob Wisor wrote:
> 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.
no backporting here!
>>>
>>> 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.
This behavior is intentional.
In past we had been many times bugged by similar issues - taht main class is not found - because its
right there.. no here.. aaaa overthere.
So yes, the logic to find mainclass get exactly to the shape you are describing.
All other attributes are following the same schema.
To fix it, probably means to brig those "bugs" back again.
I have two possibile hardening in mind
- to read other attributes only from jar with main class
- to read all attributes only from jar from codebase.
Unluckily, I'm guessing both will have some sidefects.
>
>> + }
>> +
>> /**
>> * 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.
What danger may be hidden here? I do not see any.
The no kinds of spaces are not allowed in classanmes, so from this point of view the spliting is ok.
If there is something else then valid class - then app will fail. And if so, then it os what it
deserves.
>
> Besides, please try giving a thought about context meaningful names, like entryPointAttributeValue
> for eps and splitEntryPoints for result.
sure
>
>> + 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?
Again. Why?
>
> Please test this patch more thoroughly.
Yes. It is necessary.
>
> Jacob
Thanx for check!
J.
More information about the distro-pkg-dev
mailing list