[rfc][icedtea-web] implemented Application-Library-Allowable-Codebase Attribute

[Andrew Azores]

On 02/27/2014 12:11 PM, Jiri Vanek wrote:
> hi!
>
> Implementation of -  Application-Library-Allowable-Codebase Attribute. 
> Well it grow a bit.
>
> The implementation itself, is as straightforward as terrible 
> "specification" allowed  (please reviwer, study the specification too:( )
>
> However, many workarounds were needed:
>
>  *  netx/net/sourceforge/jnlp/JNLPFile.java and 
> netx/net/sourceforge/jnlp/util/ClasspathMatcher.java : It appeared, 
> that this attribute honors path in pattern. Luckily the matcher was 
> prepared for it, and now it is just conditionally enabled.
>
> * dialogues - still the same with one detail  - the remember option do 
> not work. I'm not sure if I wont to use already implemented whitelist 
> from Extended Applets Security... Well  why not? Becasue all alowed 
> appelts will be able to run rmeote context...But well.. why not? If I 
> will reuse it, then MatchingALACAttributePanel will be reworked.

Are you talking about using AppTrustWarningPanel? ;)

>
> * netx/net/sourceforge/jnlp/util/UrlUtils.java - this was most unlucky 
> - two new utility methods - to remove name filename from url path, and 
> to compare urls no meter if there is tailing slash.
>  - the exctraction of name is for puposes to find the uri of its 
> location, which is then matched against attribute
>  - the comparison without tailing slash is not so clear - There is 
> only one suecase of it :
>
> +        if (usedUrls.size() == 1) {
> +            if (UrlUtils.equalsIgnoreLastSlash(usedUrls.toArray(new 
> URL[]{})[0], codebase)
> +                    && 
> UrlUtils.equalsIgnoreLastSlash(usedUrls.toArray(new URL[]{})[0], 
> documentBase)) {
> +                //all resoources are from codebase or document base. 
> it is ok to proceeed.
> +                OutputController.getLogger().log("All applications 
> resources (" + usedUrls.toArray(new URL[]{})[0] + ") are from 
> codebas/documentbase " + codebase + "/" + documentBase + ", skipping 
> Application-Library-Allowable-Codebase Attribute check.");
> +                return;
> +            }
> +        }
>
>
> It happened that different applications have or have not the trailing 
> slash On codebase and so the implementation of removeFileName was 
> burdened by keep trailing slash or not. This is workarround.
>
>

Nit here: why "new URL[]{}" ? Why not "new URL[0]" ?

>
> *however* I'm a hesitating how to deal with it in Matcher. I adapted 
> it (if compare of paths is true) that some.url/some/path matches both 
> some.url/some/path/ and some.url/some/path, but some.url/some/path/ do 
> not match some.url/some/path (matches only some.url/some/path/)
> I consider it as lowest evil....:(
>
> All should be unittested as much as possible.
> I'm still taking deep breath before doing reproducers for both this 
> and permissions.
>
>
> Thanx in advance,
>  J.

Rest of comments here:

Messages.properties:
> # missing Application-Library-Allowable-Codebase dialogue
> ALACAMissingMainTitle=Application <span color='red'> {0} </span> \
> form codebase <span color='red'> {1} </span> is missing the  
> Application-Library-Allowable-Codebase attribute. \
> The resources this applications is opening are from remote 
> locations:<br/> {2} <br/>Are you sure to run this application to?
"This application uses resources from the following remote locations: 
{2} Are you sure you want to run this application?"
> ALACAMissingInfo=For more information you can visit:<br/>\
> <a 
> href="http://docs.oracle.com/javase/7/docs/technotes/guides/jweb/security/manifest.html#app_library"> 
> \
> http://docs.oracle.com/javase/7/docs/technotes/guides/jweb/security/manifest.html#app_library</a> 
> <br/> \
> and<br/> <a 
> href="http://docs.oracle.com/javase/7/docs/technotes/guides/jweb/security/no_redeploy.html"> 
> \
> http://docs.oracle.com/javase/7/docs/technotes/guides/jweb/security/no_redeploy.html</a>
> # matching Application-Library-Allowable-Codebase dialogue
> ALACAMatchingMainTitle=Application <span color='red'> {0} </span> \
> form codebase <span color='red'> {1} </span> is requiring valid 
> resources from different locations:<br/>{2} <br/> \
> Those resources are expected to be loaded. Do you agree to run this 
> application?
> ALACAMatchingInfo=For more information you can visit:<br/>\
> <a 
> href="http://docs.oracle.com/javase/7/docs/technotes/guides/jweb/security/manifest.html#app_library"> 
> \
> http://docs.oracle.com/javase/7/docs/technotes/guides/jweb/security/manifest.html#app_library</a> 
> <br/> \
> and<br/> <a 
> href="http://docs.oracle.com/javase/7/docs/technotes/guides/jweb/security/no_redeploy.html"> 
> \
> http://docs.oracle.com/javase/7/docs/technotes/guides/jweb/security/no_redeploy.html</a>

JNLPClassLoader:
> //do this check for unsigned apps? imho yes
> //if 
> (security.getSecurityType().equals(SecurityDesc.SANDBOX_PERMISSIONS)){
> //    return; /*when app isnot signed, then skip this check*/
> //}
Why should we do this check for unsigned? The spec specifies it's for 
"signed RIAs", and I don't think it's necessary to enforce it for 
unsigned. Partially signed is questionable. Anyway, this check would be 
better if it used the "signing" field instead, which has type 
SigningState and is meant for exactly this kind of check. There's also 
the new SecurityDelegate inner class (which was added after you sent 
this patch, I know), and maybe this whole method or at least large parts 
of it should be inside there instead.
> +        URL documentBase = null;
> +        if (file instanceof PluginBridge) {
> +            documentBase = ((PluginBridge) file).getSourceLocation();
> +        }
> +        if (documentBase == null) {
> +            documentBase = file.getCodeBase();
> +        }

         URL documentBase;
         if (file instanceof PluginBridge) {
             documentBase = ((PluginBridge) file).getSourceLocation();
         } else {
             documentBase = file.getCodeBase();
         }

This would be preferable, no?

> +        for (int i = 0; i < resourcesDescs.length; i++) {
> +            ResourcesDesc resourcesDesc = resourcesDescs[i];
> +                for (int j = 0; j < ex.length; j++) {
> +                    ExtensionDesc extensionDesc = ex[j];
> +                for (int k = 0; k < jars.length; k++) {
> +                    JARDesc jarDesc = jars[k];

Why not use for-each loops?

> +            ExtensionDesc[] ex = resourcesDesc.getExtensions();
> +            if (ex != null) {
> +            JARDesc[] jars = resourcesDesc.getJARs();
> +            if (jars != null) {

I think the null checks here are over-defensive. These methods appear to 
be designed to return empty arrays if there is no matching resource, not 
null.

> +        if (att == null) {
> +            int a = 
> SecurityDialogs.showMissingALACAttributePanel(file.getTitle(), 
> documentBase, usedUrls);
> +            if (a != 0) {

I'll come back to this later...

> +                throw new LaunchException("The application is using 
> resources from elsewhere then its base is, have missing 
> Application-Library-Allowable-Codebase attribute and you forbifd it to 
> run");

"The application uses non-codebase resources, has no 
Application-Library-Allowable-Codebase Attribute, and was blocked from 
running by the user"
Should these new LaunchException explanations not go in Messages.properties?

> +                    throw new LaunchException("The resource from " + 
> foundUrl + " do not have any match in 
> Application-Library-Allowable-Codebase Attribute " + att + ". Thats 
> fatal");

"The resource from " + foundUrl + " does not match the  location in 
Application-Library-Allowable-Codebase Attribute " + att + ". Blocking 
the application from running."

> +            throw new LaunchException("The application is using 
> resources from elsewhere then its base is which are matching 
> Application-Library-Allowable-Codebase attribute  but you forbifd it 
> to run");

"The application uses non-codebase resources, which do match its 
Application-Library-Allowable-Codebase Attribute, but was blocked from 
running by the user."


MatchingALACAttributePanel and MissingALACAttributePanel:

Why do these need main methods? Can you make an abstract parent class 
and have them subclass it? It seems like most of the code is shared.


SecurityDialogs:
> +     public static int showMissingALACAttributePanel(String title, 
> URL codeBase, Set<URL> remoteUrls) {
> +
> +        if (!shouldPromptUser()) {
> +            return 1;
> +        }
> +
> +        SecurityDialogMessage message = new SecurityDialogMessage();
> +        message.dialogType = DialogType.MISSING_ALACA;
> +        message.extras = new Object[]{title, codeBase.toString(), 
> UrlUtils.setOfUrlsToHtmlList(remoteUrls)};
> +        Object selectedValue = getUserResponse(message);
> +
> +        // result 0 = Yes, 1 = No
> +        if (selectedValue instanceof Integer) {
> +            // If the selected value can be cast to Integer, use that 
> value
> +            return ((Integer) selectedValue).intValue();
> +        } else {
> +            // Otherwise default to "no"
> +            return 1;
> +        }
> +    }
> +
> +     public static int showMatchingALACAttributePanel(String title, 
> URL codeBase, Set<URL> remoteUrls) {
> +
> +        if (!shouldPromptUser()) {
> +            return 1;
> +        }
> +
> +        SecurityDialogMessage message = new SecurityDialogMessage();
> +        message.dialogType = DialogType.MATCHING_ALACA;
> +        message.extras = new Object[]{title, codeBase.toString(), 
> UrlUtils.setOfUrlsToHtmlList(remoteUrls)};
> +        Object selectedValue = getUserResponse(message);
> +
> +        // result 0 = Yes, 1 = No
> +        if (selectedValue instanceof Integer) {
> +            // If the selected value can be cast to Integer, use that 
> value
> +            return ((Integer) selectedValue).intValue();
> +        } else {
> +            // Otherwise default to "no"
> +            return 1;
> +        }
> +    }

Please use either getIntegerResponseAsBoolean or 
getIntegerResponseAsAppletAction rather than returning 0/1 as ints for 
yes/no.


Thanks,

-- 
Andrew A