[rfc][icedtea-web] implemented Application-Library-Allowable-Codebase Attribute
Jiri Vanek
jvanek at redhat.com
Tue Mar 11 19:25:35 UTC 2014
All your nits have been fixed. However:
I'm still not using the AppTrustWarningPanel, so rember button is useless. It will come as another changeset (if ever)
- this patch broke elluminate - one of the deadly throws is probably redundant and should be repalced by security prompt.
- jsut hint - download indicator is broken
- elluminat works, but freaze to, same as http://www.walter-fendt.de/ph14e/resultant.htm
I guess all above are older issues not related to this patch.
J.
On 03/05/2014 04:02 PM, Andrew Azores wrote:
> 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
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: alaca_02.patch
Type: text/x-patch
Size: 47416 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20140311/c25e617a/alaca_02-0001.patch>
More information about the distro-pkg-dev
mailing list