[rfc][icedtea-web] Permissions manifest attribute fix

[Andrew Azores]

On 04/01/2014 07:38 AM, Jiri Vanek wrote:
> On 03/31/2014 11:30 PM, Omair Majid wrote:
>> * Andrew Azores <aazores at redhat.com> [2014-03-31 16:35]:
>>> On 03/31/2014 04:10 PM, Andrew Azores wrote:
>>>> On 03/31/2014 01:28 PM, Andrew Azores wrote:
>>>> Small refactor. Rather than the new RequestedPermissionLevel being
>>>> available from SecurityDesc and PluginBridge only, it's also
>>>> available from JNLPFile. PluginBridge, being a JNLPFile subclass,
>>>> then overrides the method and provides the correct implementation
>>>> for HTML applets. This just makes things more coherent IMO.
>>>>
>>>> Thanks,
>>>>
>>>
>>> And another, as discussed with Omair on IRC. Just extracted the
>>> common checks for HTML and JNLP into a new method and call this
>>> method once, before the split check for HTML/JNLP divergence. So
>>> long as the spec doesn't diverge the two any further, then this
>>> should be okay. Also remove an unnecessary typecast (made
>>> unnecessary in the last refactor due to new method and override, but
>>> forgot to fix).
>>
>> I am far from an expert on this code, so please take this 'review' with
>> a grain of salt.
>>
>> Also, I am a little hesitant about such (potentially) invasive code
>> going in at the last minute.

So am I, but it's because it's a fix for a security feature 
implementation that didn't quite work right, which was discovered with 
last minute testing...

>>
>>> +++ b/netx/net/sourceforge/jnlp/runtime/ManifestAttributesChecker.java
>>
>>> +        final RequestedPermissionLevel requested = 
>>> file.getRequestedPermissionLevel();
>>> + 
>>> validateRequestedPermissionLevelMatchesManifestPermissions(requested, permissions); 
>>>
>>> +        if (file instanceof PluginBridge) { // HTML applet
>>> +            if (requested == RequestedPermissionLevel.NONE) {
>>> +                if (permissions == ManifestBoolean.TRUE && signing 
>>> != SigningState.NONE) {
>>> +                    // 
>>> http://docs.oracle.com/javase/7/docs/technotes/guides/jweb/security/manifest.html#permissions
>>> +                    // FIXME: attempting to follow the spec, but it 
>>> is too late now to actually set the applet
>>> +                    // to run in sandbox, so the applet will not be 
>>> run at all, rather than run sandboxed!
>>> +                    //securityDelegate.setRunInSandbox();
>>
>> I read this comment a few times and I couldn't figure out (without
>> asking you) how it relates to the current state.
>>
>> The "applet will not be run at all" bit completely threw me off. I read
>> this comment as: we can not set the sandbox state now (for some reason)
>> so by leaving this commented out, the applet will not run at all. Can
>> you clarify that not calling sandbox will let it run without any issues?

The comment was meant to convey that if we did perform the sandbox call, 
in its current state, then it would block the applet from running at 
all, so it is commented out. The applet will currently be allowed to 
run, however it will not be automatically run sandboxed as defined in 
the spec. I edited the comment, hopefully it's clearer now.

>>
>>> +            if (requested == RequestedPermissionLevel.NONE) {
>>> +                if (permissions == ManifestBoolean.TRUE && signing 
>>> != SigningState.NONE) {
>>
>> Maybe call this variable `sandbox` rather than `permissions` to clarify
>> that TRUE means sandbox?
>
> I think that all *except* changes in ManifestAttributesChecker can go 
> in as seaprate changeset. They are good an necessary to go.
>
> Nits -
>
>  public enum RequestedPermissionLevel {
> +        NONE, SANDBOX, J2SE, ALL
> +    }
>
>
> may have toString, or if differe  -  toJnlpString and toHtmlString 
> which will return the correct values, from which are created.
>
>
>
>  NONE = > null or "null"
>  SANDBOX => "sandbox"
>  J2SE
>   ALL

Done in newest patch.

>
> It is defintley necessary to avoid duplicated, hardcoded strings. I 
> also think that those strings already *are* somewhere in ITW! Well, I 
> have not found it. So this can go later as separate chnageset, which 
> will replace all hardcoed Strings with 
> RequestedPermissionLevel.toString (including new, tests)
>
>
> Unittests for above  functionality are attached
>
>
> As for ManifestAttributesChecker itself - why it is so complex???

Because the spec has a lot of cases?

>
> I would expect you just repalce
>   AppletSecurityLevel level = 
> AppletStartupSecuritySettings.getInstance().getSecurityLevel();
>  by
>   new getRequestedPermissionLevel

No, these do completely different things. The first one gets the 
security setting from itweb-settings and the second one tells you if the 
applet requested sandbox vs all-permission vs not requesting anything at 
all.

>
>
> Why the  if nstacneof ? it seesm tome completely redundant...

The spec makes a distinction between applets which use JNLP and those 
which do not (so plugin applets). Do you have a proposal for dealing 
with this distinction otherwise?

>
>
> Thax for checking thi sissue and quick fix!
>
>
> J.
>

I also renamed a few of the variables in the ManifestAttributesChecker 
so that it should hopefully be more intuitive to read. If this is all 
still OK then I will go ahead and push everything minus the 
ManifestAttributesChecker, which I guess with more thorough testing can 
make it for 1.5.1. Until then, signed applets requesting sandbox 
permissions will be broken - unless the user chooses to employ the nifty 
new Sandbox button ;)

Thanks,

-- 
Andrew A

-------------- next part --------------
A non-text attachment was scrubbed...
Name: permissions-attr-check-4.patch
Type: text/x-patch
Size: 17991 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20140401/ad1ca156/permissions-attr-check-4-0001.patch>