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

[Jiri Vanek]

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.

* 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.



*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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: alaca_01.patch
Type: text/x-patch
Size: 46870 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20140227/b51a3048/alaca_01-0001.patch

[Andrew Azores]

Post by Jiri Vanek
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:( )
* 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? ;)

Post by Jiri Vanek
* 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
+ 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]" ?

Post by Jiri Vanek
*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.
# 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?"

Post by Jiri Vanek
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>
//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.

Post by Jiri Vanek
+ 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?

Post by Jiri Vanek
+ 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?

Post by Jiri Vanek
+ 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.

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

I'll come back to this later...

Post by Jiri Vanek
+ 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?

Post by Jiri Vanek
+ 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."

Post by Jiri Vanek
+ 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.

Post by Jiri Vanek
+ 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,

[Jiri Vanek]

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.

Post by Andrew Azores

Post by Jiri Vanek
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:( )
* 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? ;)

Post by Jiri Vanek
* 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
+ 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]" ?

Post by Jiri Vanek
*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.
# 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?"

Post by Jiri Vanek
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>
//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.

Post by Jiri Vanek
+ 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?

Post by Jiri Vanek
+ 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?

Post by Jiri Vanek
+ 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.

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

I'll come back to this later...

Post by Jiri Vanek
+ 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?

Post by Jiri Vanek
+ 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."

Post by Jiri Vanek
+ 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."
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.

Post by Jiri Vanek
+ 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>

[Andrew Azores]

Post by Jiri Vanek
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.

Not okay to push if elluminate is broken by this... have you figured
out what to do about it?

Post by Jiri Vanek
- 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.

Thanks,

[Andrew Azores]

Post by Andrew Azores

Post by Jiri Vanek
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.

Not okay to push if elluminate is broken by this... have you figured
out what to do about it?

Post by Jiri Vanek
- 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.

Thanks,

Apparently it was just a broken cache problem. Elluminate does work.

Post by Andrew Azores
+ URL documentBase = null;
+ if (file instanceof PluginBridge) {
+ documentBase = ((PluginBridge) file).getSourceLocation();
+ }
+ if (documentBase == null) {
+ documentBase = file.getCodeBase();
+ }

Good to go otherwise, I think.

Thanks,

[Jiri Vanek]

Post by Andrew Azores

Post by Jiri Vanek
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.

Not okay to push if elluminate is broken by this... have you figured out what to do about it?

Post by Jiri Vanek
- 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.

Thanks,

Apparently it was just a broken cache problem. Elluminate does work.

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

I have kept it as it was. I think there can be state, when (PluginBridge) file).getSourceLocation()
will really remian null. But codebase is never null.

J.