On 4/23/2014 1:10 PM, Sean Mullan wrote:
Just a few comments:
1. When you write a test that uses the jtreg /policy option, the
policy file overrides the system policy file. If the test depends on a
standard extension, then you may get SecurityExceptions unless
additional perms are granted. Thus, there are quite a few tests that
define their own policy files and duplicate the grant statement for
extensions from the system policy:
grant codeBase "file:${{java.ext.dirs}}/*" {
permission java.security.AllPermission;
}
These tests should be modified to only grant the necessary
permissions. However, ideally I think that a better solution would be
to add a jtreg /policy option that doesn't override the system policy
file, but rather appends to it, for example, using "==" :
I suspect most of the tests only want to grant the permissions for the
test itself rather than overriding the system policy file. Having a new
jtreg/policy option not to override the system policy file may be a
better solution. This would avoid updating the test's policy file
every time the system's policy is modified. On the other hand, I think
the test policy may not need to grant permissions to the extensions
directory if it doesn't use the classes in extensions. It's a good
opportunity to clean that up. This will require some closer look at the
tests.
If you are okay, I'd like to separate the test's custom policy update as
a follow-on work.
@run main/othervm/policy==test.policy
(this is the reverse behavior of the java.security.policy system
property, so it might be a little confusing, so maybe it is better to
add a new option)
"==" is a confusing syntax. -Djava.security.policy==test.policy
overrides the system policy file ("=" prefix) whereas jtreg uses the
reverse syntax? I think it would be better to make jtreg/policy
consistent with -Djava.security.policy (i.e. default is to extend the
system java.security).
2. test/lib/security/java.policy/Ext_AllPolicy.java
I think you should also add a check for AllPermission.
Will look into it.
3. jdk/nio/zipfs/ZipFileSystem.java
If I understand the changes, the previous code would throw
SecurityExceptions when run under a SecurityManager? It's not
specifically related to this bug, is it?
It's a bug in the zipfs provider and I can file a new JBS issue for the
zipfs change. I prefer to push them in the same changeset that reduces
zipfs's privileges and added tests to run with security manager.
4. lib/security/java.policy
grant codeBase "file:${java.home}/lib/ext/zipfs.jar" {
permission java.io.FilePermission "<<ALL FILES>>",
"read,write,delete";
Hmm, granting that likely means you are just a hop away from getting
AllPermission ... not sure what to advise here, but there are several
cases like this for certain permissions (ex: RuntimePermission
"createClassLoader" is another one).
I think we could grant "delete" only to the temp files that zipfs
creates as implementation details. I'll let Sherman follow up and
fine-tune the permission set.
Thanks for the feedback. I'll send an updated webrev that will also
have the change for os-specific version of java.policy.
Mandy
--Sean
On 04/22/2014 03:39 PM, Mandy Chung wrote:
This change proposes to remove granting all permissions for extensions
as the default and implements the principle of least privilege.In JDK 9,
we want to reduce the privileges of as many system classes as possible.
http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8040059/webrev.00/
This patch has reduced the zipfs, localedata and cldrdata to grant the
permissions they require. It grants AllPermission to other jar files in
the lib/ext directory shipped with JDK and this change is intended to
enable the component teams to identify the minimum permissions and fix
any issue, if any.
Libraries installed in the extensions directory depending on
AllPermission granted by default are impacted. Making this change as
early in JDK 9 allows us to identify any customer impacted by this
change.
Mandy
