[rfc][icedtea-web] Fix support for signed applets with sandbox permissions in manifest
Andrew Azores
aazores at redhat.com
Thu Jun 26 15:04:01 UTC 2014
On 06/24/2014 11:39 AM, Andrew Azores wrote:
> On 06/24/2014 10:40 AM, Jiri Vanek wrote:
>> On 06/24/2014 03:58 PM, Andrew Azores wrote:
>>> 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.
>>>>
>>>>>
>>>>>>> -
>>>>>>> - 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.
>>>>
>>>
>>> Latest version of the patch is attached. initializePermissions was
>>> renamed, the comment hunks were
>>> removed and done in a separate changeset, and all manifest checks
>>> are skipped when on low security
>>> with a WARNING_ALL level message logged. Otherwise, no changes from
>>> the last patch.
>>>
>>
>> No no. THis is not what I wonted.
>>
>> 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,
>
And attached is the promised update to the Signed reproducer.
Lukasz, if you could please also send the Partial Signing test that we
put together.
Thanks,
--
Andrew A
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pr1769-reproducer-2.patch
Type: text/x-patch
Size: 20557 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20140626/1c2d367d/pr1769-reproducer-2-0001.patch>
More information about the distro-pkg-dev
mailing list