[rfc][icedtea-web] Fix support for signed applets with sandbox permissions in manifest
Andrew Azores
aazores at redhat.com
Wed Jul 2 14:01:14 UTC 2014
On 07/01/2014 07:27 AM, Jiri Vanek wrote:
> On 06/26/2014 05:04 PM, Andrew Azores wrote:
>> 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.
>>
>
> As for partially signed -
>
> Why KnownTofail? Why just not set alaca? Then it willl match,and no
> dialogue will apear :)
>
> (?)
>
> If we cannot do this, wee ned to fix the reproducers engine. Adding
> ton of knowntofail tests have no meaning.
>
>
> J.
>
Hmm, good idea...
Lukasz, would you mind taking care of this enhancement? ;) For both of
the tests patches.
Thanks,
--
Andrew A
More information about the distro-pkg-dev
mailing list