[rfc][icedtea-web] Fix support for signed applets with sandbox permissions in manifest
Jiri Vanek
jvanek at redhat.com
Tue Jul 1 11:27:22 UTC 2014
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.
More information about the distro-pkg-dev
mailing list