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

[Jiri Vanek]

On 06/18/2014 07:44 PM, Andrew Azores wrote:
> On 06/18/2014 12:01 PM, Jiri Vanek wrote:
>> On 05/27/2014 06:23 PM, Andrew Azores wrote:
>>> Hi,
>>>
>>> This patch allows signed applets with sandbox permissions specified in
>>> their manifests to actually
>>
>>
>> How it is dealing with mixed (signed + unsigned code) apps?
>
> I don't have any examples of mixed signing apps with a Permissions manifest attribute, but a
> reproducer could be prepared for this case.

It seems to be under construction...
>
>>> be run sandboxed. This is in contrast to the current behaviour where
>>> such applets will fail to
>>> launch, and the failure message presented asks the user to try
>>> launching via the Sandbox button next
>>> time. This was because the dialog which presented the Sandbox button
>>> appeared very early in the
>>> JNLPClassLoader's life cycle - early enough that no security settings
>>> had yet been set for the
>>> classloader or any of the applet's JAR locations - whereas the
>>> manifest checks were done later,
>>> after these settings would have already been initialized. Fixing the
>>> issue was not as simple as
>>> doing the manifest checks before presenting the security dialog
>>> because the dialog was presented
>>> part way through the initialization process, where JARs are being
>>> downloaded and checked for
>>> signing, so that the appropriate security dialog could be shown to the
>>> user. Putting the manifest
>>> checks first would therefore not work properly because the JARs were
>>> not yet available. This patch
>>> resolves the issue by moving the manifest checks inside the method
>>> which initializes the relevant
>>> security settings, such that the required resources are available, it
>>> is known what type of applet
>>> is about to be run, but the security settings for the JAR locations
>>> have not yet been initialized
>>> and the applet can thus still be set to run sandboxed safely.
>>>
>>> Additionally, the ManifestAttributesChecker check for the Permissions
>>> attribute is no longer skipped
>>> when Extended Applet Security is set to the Low level, since this
>>> allows for signed applets with
>>> Sandbox permissions specified in their manifests to run with full
>>> permissions when Low security is set.
>>>
>>
>> Well this concept I do not like.
>>
>> Imho the correct approach is to check the attributes, but do not take
>> any actions (except print warnings eg "normally I would fail because
>> of..,but low security is on".
>>
>> With exception on sandbox attribute - here probably warning (strong one)
>> requesting users attention.
>>
>>   What do you think? - Well this dialogue would be good place for run in
>> sandbox button too :)
>>
>> Otherwise the only way how to disable manifest check willbe the
>> deployment property which I added.
>
> Sure. I just *really* am not comfortable with leaving it as-is, which means Low Security allows

I agree.
> applets to be silently granted elevated permissions. I can imagine this too easily being abused by a
> malicious applet developer. I'm fine with adding an extra prompt here when on Low Security though,
> warning the user that due to the Low Security the applet may be granted elevated permissions if it
> is run.

The dialogue canbe added as separate changeset.

But for current changeset's behavior I would like the low security is not affecting *only* 
permissions attribute. Other attributes checks will be skipped.
>
>>
...
>>> -        setSecurity();
>>
>> I'm wondering, setSecurity moved, but setPermissions remained?
>
> You mean initializePermissions? Yea, it didn't have to be moved.

Still surprising me.

>
>>> -
>>> -        ManifestAttributesChecker mac = new
>>> ManifestAttributesChecker(security, file, signing, securityDelegate);
>>> -        mac.checkAll();
>>> -
>>>           installShutdownHooks();
>>>
>>>
>>> @@ -592,14 +586,12 @@ public class JNLPClassLoader extends URL
>>>               //Check if main jar is found within extensions
>>>               foundMainJar = foundMainJar || hasMainInExtensions();
>>>
>>> +            setSecurity();
>>> +            initializeManifestAttributesChecker();
>>> +            mac.checkAll();
>>>               return;
>>>           }
>>> -        /*
>>> -        if (jars == null || jars.length == 0) {
>>> -                throw new LaunchException(null, null, R("LSFatal"),
>>> -                                    R("LCInit"),
>>> R("LFatalVerification"), "No jars!");
>>> -        }
>>> -        */
>>
>> This should probably cam in as separate changset. Hmm.. When it was
>> commented Out actually? :)
>

then please remove as separate changeset.  Together with the hunk a bit lower.

>>>
>>> -            jarLocationSecurityMap.put(jarDesc.getLocation(),
>>> jarSecurity);
>>> +            if (containsUnsignedJar && containsSignedJar) {
>>> +                break;
>>> +            }
>>>           }
>>>
>>>           if (containsSignedJar && containsUnsignedJar) {
>>>               checkPartialSigningWithUser();
>>>           }
>>>
>>> +        initializeManifestAttributesChecker();
>>> +        mac.checkAll();
>>> +
>>> +        for (JARDesc jarDesc : validJars) {
>>
>> There I'm missing something. Why is this secoond field even needed?
>> In any case, please mention  it in changelog.
>
> Which second field? "mac"?

Oh sorry. no. - The validJars. Can you expalin on them?
>
>>> +            final URL codebase = getJnlpFileCodebase();
>>> +            final SecurityDesc jarSecurity =
>>> securityDelegate.getCodebaseSecurityDesc(jarDesc, codebase.getHost());
>>> +            jarLocationSecurityMap.put(jarDesc.getLocation(),
>>> jarSecurity);
>>> +        }
>>> +
>>>           activateJars(initialJars);
>>>       }
>>> +
>>> +    private void initializeManifestAttributesChecker() throws
>>> LaunchException {
>>> +        if (mac == null) {
>>> +            file.getManifestsAttributes().setLoader(this);
>>> +            mac = new ManifestAttributesChecker(security, file,
>>> signing, securityDelegate);
>>> +        }
>>> +    }
>>> +
>>> +    private URL getJnlpFileCodebase() {
>>> +        final URL codebase;
>>> +        if (file.getCodeBase() != null) {
>>> +            codebase = file.getCodeBase();
>>> +        } else {
>>> +            // FIXME: codebase should be the codebase of the Main Jar
>>> not
>>> +            // the location. Although, it still works in the current
>>> state.
>>> +            codebase = file.getResources().getMainJAR().getLocation();
>>> +        }
>>> +        return codebase;
>>> +    }
>>>
>>>        /***
>>>        * Checks for the jar that contains the attribute.
>>> @@ -762,13 +780,12 @@ public class JNLPClassLoader extends URL
>>>        * @param  name attribute to be found
>>>        */
>>>       public String checkForAttributeInJars(List<JARDesc> jars,
>>> Attributes.Name name) {
>>> -
>>>           if (jars.isEmpty()) {
>>>               return null;
>>>           }
>>>
>>>           String result = null;
>>> -
>>> +
>>>           // Check main jar
>>>           JARDesc mainJarDesc = ResourcesDesc.getMainJAR(jars);
>>>           result = getManifestAttribute(mainJarDesc.getLocation(), name);
>>> @@ -776,7 +793,7 @@ public class JNLPClassLoader extends URL
>>>           if (result != null) {
>>>               return result;
>>>           }
>>> -
>>> +
>>>           // Check first jar
>>>           JARDesc firstJarDesc = jars.get(0);
>>>           result = getManifestAttribute(firstJarDesc.getLocation(),name);
>>> @@ -2380,12 +2397,15 @@ public class JNLPClassLoader extends URL
>>>           }
>>>
>>>           public void setRunInSandbox() throws LaunchException {
>>> -            if (promptedForSandbox || classLoader.security != null
>>> -                    || classLoader.jarLocationSecurityMap.size() != 0) {
>>> +            if (runInSandbox && classLoader.security != null
>>> +                    && classLoader.jarLocationSecurityMap.size() != 0) {
>>
>> Why have this condition changed in thisway? I would expect
>> promptedForSandbox || runInSandbox || classLoader.security != null  && ...
>
> The ClassLoader can end up attempting to automatically set the applet to run in sandbox multiple
> times, so that condition would cause it to fail in this situation. The condition was very strong
> before on purpose because it was easily possible for the security settings to have already been
> applied before the sandboxing option appeared to the user. Now, the sandboxing dialog appears well
> beforehand, and automatic sandboxing also happens early enough, so this condition has to be slightly
> weakened so that it does allow the call to be performed multiple times, but only if the security
> settings have not yet been applied to the classloader/applet. I haven't tested this but this
> condition should also work fine:


/me consuimg this and hoping /me understand well
%-~

>
> runInSandbox && (classLoader.security != null || jarLocationSecurityMap.size() != 0)
>
> The classLoader.security and jarLocationSecurityMap conditions should really always be "in sync"
> with each other in this regard though.
hmmh.

>
>>
>>>                   throw new LaunchException(classLoader.file, null,

>> +Permissions: sandbox
>>
>>
>> Do we  have/need also one with  Permissions: all-permissions ? I recall
>> there is something...
>
> I don't think so, but maybe there is. I couldn't find anything with a quick grep.
>
...in progress s I guess
>>
..
>>
>>
>> Why do you need custom reproducers? The manifest is handled by engine
>
> Ah, I forgot about this. Although a custom reproducer would still be needed for the mixed signing
> case, so I can convert this one to mixed and make normal "simple" and "signed" ones as well I suppose.
>

good!
...snip...
>> +        assertTrue("Applet should have printed its exit string",
>> pr.stdout.contains(CLOSE_STRING));
>> +    }
>> +}
>>
>> As this test is already known to fail, it can go (after manifest is
>> resolved)  in as independent changeset.
>>
>>

Well  except the issue with low security (and few more expalnationa) I'm ashamed to not have muc 
more comments :(


J.