[rfc][icedtea-web] Fix support for signed applets with sandbox permissions in manifest

[Andrew Azores]

On 07/30/2014 01:07 PM, Jiri Vanek wrote:
> On 07/30/2014 06:49 PM, Jiri Vanek wrote:
>> On 07/30/2014 06:24 PM, Jiri Vanek wrote:
>>> On 07/30/2014 05:29 PM, Andrew Azores wrote:
>>>> On 07/30/2014 11:20 AM, Jiri Vanek wrote:
>>>>>
>>>>>>>> +++ 
>>>>>>>> b/tests/netx/unit/net/sourceforge/jnlp/runtime/JNLPFileTest.java
>>>>>>>> @@ -129,7 +129,7 @@ public class JNLPFileTest extends NoStdO
>>>>>>>>           /*
>>>>>>>>            *  "sandbox" or "all-permissions"
>>>>>>>>            */
>>>>>>>> -        manifest6.getMainAttributes().put(new
>>>>>>>> Attributes.Name(JNLPFile.ManifestsAttributes.PERMISSIONS), 
>>>>>>>> "sandbox");
>>>>>>>> +        manifest6.getMainAttributes().put(new
>>>>>>>> Attributes.Name(JNLPFile.ManifestsAttributes.PERMISSIONS), 
>>>>>>>> "all-permissions");
>>>>>>>
>>>>>>> Why this change??? It should not be here...
>>>>>>>
>>>>>>>> manifest6.getMainAttributes().put(new
>>>>>>>> Attributes.Name(JNLPFile.ManifestsAttributes.TRUSTED_LIBRARY), 
>>>>>>>> "false");
>>>>>>>>           manifest6.getMainAttributes().put(new
>>>>>>>> Attributes.Name(JNLPFile.ManifestsAttributes.TRUSTED_ONLY), 
>>>>>>>> "false");
>>>>>>>>
>>>>>>>> @@ -180,7 +180,7 @@ public class JNLPFileTest extends NoStdO
>>>>>>>>           Assert.assertEquals("*.comhttps://*.cz",
>>>>>>>> jnlpFile.getManifestsAttributes().getAttribute(new
>>>>>>>> Attributes.Name(JNLPFile.ManifestsAttributes.APP_LIBRARY_ALLOWABLE))); 
>>>>>>>>
>>>>>>>> Assert.assertEquals("*.netftp://*uu.co.uk",
>>>>>>>> jnlpFile.getManifestsAttributes().getAttribute(new
>>>>>>>> Attributes.Name(JNLPFile.ManifestsAttributes.CALLER_ALLOWABLE)));
>>>>>>>>           Assert.assertEquals("*.com *.net *.cz *.co.uk",
>>>>>>>> jnlpFile.getManifestsAttributes().getAttribute(new
>>>>>>>> Attributes.Name(JNLPFile.ManifestsAttributes.CODEBASE)));
>>>>>>>> - 
>>>>>>>> Assert.assertEquals(SecurityDesc.RequestedPermissionLevel.SANDBOX.toHtmlString(),
>>>>>>>> jnlpFile.getManifestsAttributes().getAttribute(new
>>>>>>>> Attributes.Name(JNLPFile.ManifestsAttributes.PERMISSIONS)));
>>>>>>>> + 
>>>>>>>> Assert.assertEquals(SecurityDesc.RequestedPermissionLevel.ALL.toHtmlString(),
>>>>>>>> jnlpFile.getManifestsAttributes().getAttribute(new
>>>>>>>> Attributes.Name(JNLPFile.ManifestsAttributes.PERMISSIONS)));
>>>>>>>
>>>>>>> So this one,
>>>>>>>> Assert.assertEquals("false", 
>>>>>>>> jnlpFile.getManifestsAttributes().getAttribute(new
>>>>>>>> Attributes.Name(JNLPFile.ManifestsAttributes.TRUSTED_LIBRARY)));
>>>>>>>>           Assert.assertEquals("false", 
>>>>>>>> jnlpFile.getManifestsAttributes().getAttribute(new
>>>>>>>> Attributes.Name(JNLPFile.ManifestsAttributes.TRUSTED_ONLY)));
>>>>>>>>
>>>>>>>> @@ -206,7 +206,7 @@ public class JNLPFileTest extends NoStdO
>>>>>>>>           Assert.assertEquals(true, 
>>>>>>>> jnlpFile.getManifestsAttributes().getCodebase().matches(new
>>>>>>>> URL("ftp://aa.bb.net")));
>>>>>>>>           Assert.assertEquals(true, 
>>>>>>>> jnlpFile.getManifestsAttributes().getCodebase().matches(new
>>>>>>>> URL("https://x.net")));
>>>>>>>>           Assert.assertEquals(false, 
>>>>>>>> jnlpFile.getManifestsAttributes().getCodebase().matches(new
>>>>>>>> URL("http://aa.bb/com")));
>>>>>>>> - Assert.assertEquals(JNLPFile.ManifestBoolean.TRUE,
>>>>>>>> jnlpFile.getManifestsAttributes().isSandboxForced());
>>>>>>>> + Assert.assertEquals(JNLPFile.ManifestBoolean.FALSE,
>>>>>>>> jnlpFile.getManifestsAttributes().isSandboxForced());
>>>>>>>
>>>>>>> and this one. Why so?
>>>>>>>> Assert.assertEquals(JNLPFile.ManifestBoolean.FALSE,
>>>>>>>> jnlpFile.getManifestsAttributes().isTrustedLibrary());
>>>>>>>> Assert.assertEquals(JNLPFile.ManifestBoolean.FALSE,
>>>>>>>> jnlpFile.getManifestsAttributes().isTrustedOnly());
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>> The JNLPClassLoader is determining the applet to be signed, and 
>>>>>> the ManifestAttributesChecker does
>>>>>> not allow for JNLP applets to have a mismatch between their 
>>>>>> signing state and the permissions
>>>>>> level
>>>>>> they request (I believe this is in the spec for the Permissions 
>>>>>> attribute). Is the applet not
>>>>>> actually signed? I looked into how the JARs are being created and 
>>>>>> couldn't see if they're signed
>>>>>> one
>>>>>> way or the other.
>>>>>>
>>>>>
>>>>> Well thats the issue. The testing "applet" (its dummyjnlp) is not 
>>>>> signed. So this change sholdnot
>>>>> be needed. Something different msut be wrong.
>>>>>
>>>>>
>>>>> J.
>>>>>
>>>>
>>>> The classloader's JarCertVerifier 'jcv' is returning true for 
>>>> 'jcv.isFullySigned()' on all of these
>>>> tests, even without the patch applied. It falls into the 
>>>> 'isTriviallySigned' case. If this needs to
>>>> be fixed then I think it's perhaps out of scope of this patch, no?
>>>>
>>>> Thanks,
>>>>
>>>
>>>
>>> ufff.. not so sure.
>>>
>>> Well the issue is here:
>>>
>>> public boolean isTriviallySigned() {
>>>          return getTotalJarEntries(jarSignableEntries) <= 0
>>>                  && certs.size() <= 0;
>>>      }
>>>
>>>
>>>
>>> I'm not sure if it is case of DummyJnlp only or if it hides 
>>> something more serious.
>>>
>>>
>>>
>>> It seems to me that once there are no jars, and no certificates, 
>>> then it is considered signed (like
>>> class only applet?/??) If I'm correct that this return is terribly 
>>> wrong. Not sure if your current
>>> patch made it more visible, made new hole, or just pointed to 
>>> something rotten already inside...
>>>
>>
>> Looking deeper, into
>>
>> VerifyResult verifyJarEntryCerts
>>
>>
>> I really think your patch disturbed something.  The loop here is 
>> counting the entries, and is
>> distinguishing among  maniefested, not manifested jars.  So I guess 
>> this is an cause.
>>
>>
>> J.
>>
>
> yes this wouldbe it:
> (initializeResources): do not set entries in jarLocationSecurityMap until
> +    after prompting the user on whether to run the applet as well as
>
>
> and -  inside that - you are asking about signature satus....\
>
> /me moreover brainstorming

But verifyJarEntryCerts is called from verifyJar, from verifyJars, from 
add, and add is still done before isFullySigned is called, same as before...

Like I said, even without this patch applied, isTriviallySigned is still 
returning true for dummy JNLPs (and so the classloader thinks they're 
signed). So it isn't this patch that's making the dummy JNLP show up as 
signed - it already is doing that in HEAD right now. AFAICT this is just 
"something rotten already inside" :(

-- 
Andrew A