Review request: 8040059 Change default policy for extensions to no permission

[Mandy Chung]

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