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

[Andrew Azores]

On 07/30/2014 09:37 AM, Jiri Vanek wrote:
> On 07/30/2014 03:01 PM, Jiri Vanek wrote:
>> ...snip...
>>>>
>>>> As you mentioned - the signed, but with permissions:sandbox app can 
>>>> not be run with all permissions.
>>>> So skip all attrributes, except permissions one
>>>>
>>>> The sandbox one should be checked:
>>>> signed: permissions:all-permissions - all permissions
>>>> unsigned: permissions:all-permissions - sandbox
>>>> unsigned: permissions:sandbox - sandbox
>>>> signed: permissions:sandbox - sandbox
>>>> signed: permissions missing - all permissions
>>>> unsigned: permissions missing - sandbox
>>>>
>>>> On low permissions, no asking.
>>>>
>>>> For other then low permissions user should be asked for each except
>>>> signed: permissions:all-permissions
>>>> unsigned: permissions:sandbox
>>>> cases.
>>>>
>>>> But the dialogue can be done as separate changeset (with 
>>>> custom-permissions button:)
>>>>
>>>>
>>>>
>>>>> New tests are also on the way and with the new tests will come the 
>>>>> fixed version of the previously
>>>>> sent test, which will make be a signed reproducer rather than custom.
>>>>>
>>>>
>>>> J.
>>>>
>>>
>>> Okay, I wasn't sure what you had meant. It didn't seem right to skip 
>>> them all but I thought maybe
>>> you had something else in mind to implement to fix it. So here's 
>>> another attempt.
>>>
>>> Setting deployment.manifest.attributes.check to false causes all 
>>> checks to be skipped, except for
>>> the Permissions attribute, which is always checked regardless. Using 
>>> Low Extended Applet Security
>>> causes the ALACA and Permissions checks to not generate dialogs and 
>>> assumes that it's OK to run the
>>> applet(s). In the case of the Permissions attribute, it will still 
>>> auto sandbox if necessary. Is
>>> this what you wanted?
>>>
>>> Thanks,
>>>
>>> -- 
>>> Andrew A
>>>
>>
>> I have not applied and not tested the patch.
>>
>> It looks good in design and code.
>>
>>
>> assuming the:
>> +        // If the attribute is not specified in the manifest, prompt 
>> the user. Oracle's spec says
>> that the
>> +        // attribute is required, but this breaks a lot of existing 
>> applets. Therefore, when on the
>> highest
>> +        // security level, we refuse to run these applets. On the 
>> standard security level, we ask.
>> And on the
>> +        // lowest security level, we simply proceed without asking.
>>
>> really works, I'm ok with this to go to head . I also think it should 
>> go to 1.5 but I left final
>> decision to you. (ok for 1.5 from my side)
>>
>>
>>
>>>
>>> pr1769-6.patch
>>>
>>>
>>> diff --git a/ChangeLog b/ChangeLog
>>> --- a/ChangeLog
>>> +++ b/ChangeLog
>>> @@ -1,3 +1,43 @@
>>> +2014-06-24  Andrew Azores<aazores at redhat.com>
>>> +
>>> +    Fixed support for signed applets which specify the Permissions 
>>> attribute
>>> +    as "sandbox" in their manifests. These applets are now properly 
>>> run
>>> +    sandboxed automatically, rather than requiring the user to 
>>> click the
>>> +    "Sandbox" run button.
>>> +    * netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
>>> +    (JNLPClassLoader): manifest attributes checking and security 
>>> settings
>>> +    moved inside initializeResources
>>> +    (initializePermissions): renamed 
>>> (initializeReadJarPermissions). Redundant
>>> +    null-check removed
>>> +    (initializeResources): do not set entries in 
>>> jarLocationSecurityMap until
>>> +    after prompting the user on whether to run the applet as well as
>>> +    performing manifest attribute checks. A new Collection 
>>> (validJars) is used
>>> +    to hold available and valid JARs between discovering the JARs 
>>> and applying
>>> +    any security settings to the them
>>> +    (initializeManifestAttributesChecker): new method
>>> +    (getJnlpFileCodebase): new method, extracted from 
>>> initializeResources
>>> +    (SecurityDelegateImpl.setRunInSandbox): throw exception if 
>>> already forced
>>> +    to run in sandbox, rather than if already prompted
>>> +    * netx/net/sourceforge/jnlp/runtime/ManifestAttributesChecker.java
>>> +    (isLowSecurity): new method
>>> +    (checkAll): Extended Applet Security on Low disables all 
>>> manifest checks
>>> +    except for Permissions
>>> +    (checkPermissionsAttribute): do not skip checking if Extended 
>>> Applet
>>> +    Security is Low. Remove try/catch on setRunInSandbox call as 
>>> this is now
>>> +    supported.
>>> +    (checkApplicationLibraryAllowableCodebaseAttribute): do not 
>>> display dialog
>>> +    prompts on low security, allow the applet to run without prompt
>>
>> I think the tests are not included in this patch. Please adjust the 
>> changelog to current state.
>>
>>> +    *
>>> tests/reproducers/custom/SignedAppletManifestSpecifiesSandbox/testcases/SignedAppletManifestSpecifiesSandboxTests.java: 
>>>
>>>
>>> +    new test case
>>> +    *
>>> tests/reproducers/custom/SignedAppletManifestSpecifiesSandbox/resources/SignedAppletManifestSpecifiesSandbox.html: 
>>>
>>>
>>> +    same
>>> +    * 
>>> tests/reproducers/custom/SignedAppletManifestSpecifiesSandbox/srcs/MANIFEST.MF:
>>> +    same
>>> +    * 
>>> tests/reproducers/custom/SignedAppletManifestSpecifiesSandbox/srcs/Makefile:
>>> +    same
>>> +    *
>>> tests/reproducers/custom/SignedAppletManifestSpecifiesSandbox/srcs/SignedAppletManifestSpecifiesSandbox.java: 
>>>
>>>
>>> +    same
>>> +
>> ...snip...
>>
>> Thank you very much,
>> J.
>>
>
> ouch it broken some unittests
>
> Please double check and fix.
>

Nice catch! The Dummy JNLP classes were actually broken and the test was 
expecting a result that we've defined to not be allowable - I'm not sure 
how this was passing before the patch but to me that sounds like a bug. 
The only change here from -6 is changing the test to have an allowable 
Permissions manifest attribute, and making the Dummies initialize the 
required field properly.

Thanks,

-- 
Andrew A

-------------- next part --------------
A non-text attachment was scrubbed...
Name: pr1769-7.patch
Type: text/x-patch
Size: 22078 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20140730/fb6c25b9/pr1769-7-0001.patch>