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

[Jiri Vanek]

On 04/01/2014 03:12 PM, Andrew Azores wrote:
> 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,

So here are the tests and small refactroiun using toHtml/toJnlp string where it had sense.

Once approved, do you mind to post the main part of your patch?


J.
>

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