[rfc][icedtea-web] Fix support for signed applets with sandbox permissions in manifest
Lukasz Dracz
ldracz at redhat.com
Wed Jul 2 15:34:34 UTC 2014
Hello,
Yeah, I don't mind taking care of it for both.
Thank you,
Lukasz
----- Original Message -----
> From: "Andrew Azores" <aazores at redhat.com>
> To: "Jiri Vanek" <jvanek at redhat.com>, "IcedTea" <distro-pkg-dev at openjdk.java.net>, "Lukasz Dracz" <ldracz at redhat.com>
> Sent: Wednesday, July 2, 2014 10:01:14 AM
> Subject: Re: [rfc][icedtea-web] Fix support for signed applets with sandbox permissions in manifest
>
> 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