[rfc][icedtea-web] Fix support for signed applets with sandbox permissions in manifest
Andrew Azores
aazores at redhat.com
Tue Jun 24 13:58:51 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.
>
>>
>>>> -
>>>> - 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.
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.
Thanks,
--
Andrew A
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pr1769-5.patch
Type: text/x-patch
Size: 14154 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20140624/94752d99/pr1769-5-0001.patch>
More information about the distro-pkg-dev
mailing list