[rfc][icedtea-web] Fix support for signed applets with sandbox permissions in manifest
Andrew Azores
aazores at redhat.com
Thu Jun 19 15:22:25 UTC 2014
On 06/19/2014 11:03 AM, Jiri Vanek wrote:
> 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.
If you look inside what initializePermissions does it makes more sense
:) maybe that method should be renamed, because its name sounds a lot
more wide-reaching than it truly is.
>
>>
>>>> -
>>>> - 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?
Right, so rather than iterating over JARs and immediately applying
security settings to them, any valid JAR that is discovered is added to
that collection instead. Then we can all at once determine if the applet
is fully signed vs partially signed (including discovering if it has an
external main-class to make it partial) and can perform the manifest
checks (this is the important part). Then, after we have gathered all
that information, we can actually apply the settings to each JAR by
iterating over validJars. If we try to apply settings to each JAR as we
go along, as it is without this patch, then it becomes impossible to
perform the manifest check for the Permissions attribute before any JARs
have their security settings applied (which is the asserted condition in
setRunInSandbox), since we won't have found and processed the manifest
before beginning to populate the jarLocationSecurityMap. This part of
the changeset is really the heart of the fix.
>>
>>>> + 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
> %-~
It hopefully makes more sense with the explanation above. It can be set
multiple times if multiple manifests are found and multiple
"Permissions: sandbox" entries are found, for example.
>
>>
>> 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.
>
Take your time on reviewing this, it's a huge one and although I feel
pretty confident in it, I really don't want it to go in if there are any
errors. The Sandbox button is a decent workaround at the moment at
least, other than occasional configuration errors eg. applets having
been previously Always Trusted, so the Sandbox button isn't presented.
Thanks,
--
Andrew A
More information about the distro-pkg-dev
mailing list