RFR (12): 8191053: Provide a mechanism to make system's security manager immutable

[Sean Mullan]

Excellent comments, Stuart. Thanks for taking the time to review this.

I have posted another review that should address most of your comments, 
but also responded inline with replies below.

http://cr.openjdk.java.net/~mullan/webrevs/8191053/webrev.02/

I also posted the javadoc so you can see what it looks like, esp. the table:

http://cr.openjdk.java.net/~mullan/webrevs/8191053/webrev.02/docs/api/java.base/java/lang/SecurityManager.html
http://cr.openjdk.java.net/~mullan/webrevs/8191053/webrev.02/docs/api/java.base/java/lang/System.html


On 10/3/18 7:11 PM, Stuart Marks wrote:
> Hi Sean,
> 
> The new arrangement of using special tokens for java.security.manager 
> makes a lot more sense than having a separate property. Overall, the 
> proposed semantics seem reasonable to me.
> 
> 
> I have some suggestions to clarify the proposed specification. (But also 
> see below.) From webrev.01:
> 
>    81  * Environments using a security manager will typically set the 
> security
>    82  * manager at startup. In the JDK implementation, this is done by 
> setting
>    83  * the system property "{@code java.security.manager}" on the 
> command line to
>    84  * the class name of the security manager, or to the empty String 
> ("")
>    85  * or "{@code default}" to utilize the default security manager.
>    86  * If the "{@code java.security.manager}" system property is not 
> set, the
>    87  * default value is {@code null}, which means a security manager 
> will not be
>    88  * installed at startup.
> 
> I'd suggest using the term "special token" to describe the string 
> "default" here, to make it clear that this string is interpreted 
> specially and is not interpreted as a security manager class name. (The 
> FilePermission docs use the term "special token" to describe "<<ALL 
> FILES>>".)
> 
> Similarly, I'd suggest "special token" be used to describe "allow" and 
> "disallow" below.

Good suggestion, I added "special token" to those places as well as to 
the reference to "disallow" in System.setSecurityManager. I also broke 
up the first sentence above on lines 81-85 in two sentences to make it 
easier to read.

>    93  * In the JDK implementation, if the Java virtual machine is 
> started with
>    94  * the "{@code java.security.manager}" system property set to
>    95  * "{@code =disallow}" then the
>    96  * {@link System#setSecurityManager(SecurityManager) 
> setSecurityManager}
>    97  * method cannot be used to set a security manager and will throw an
>    98  * {@code UnsupportedOperationException}. The ability to 
> dynamically set the
> 
> (I assume this will be changed from "=disallow" to "disallow" and 
> similar for "allow" below.)

Oops. Yes. I had fixed that but forgot to upload it in the webrev. Fixed 
now.

> This should clarify that if "disallow" is used then no security manager 
> will be installed, in addition to preventing one from being installed in 
> the future.

Yes, fixed.

>    98  *                                        The ability to 
> dynamically set the
>    99  * security manager in a running system is an impediment to some 
> performance
>   100  * optimizations, even if the method is never called.
> 
> While I think this is true, it's a bit of rationale stuck into the 
> middle of the specification for the values of the system property, and 
> as such it sticks out. It kind of begs for more explanation. I'd suggest 
> removing it.

I was on the fence about including that as well. I have removed it (and 
also from the implNote in System.setSecurityManager). I think the JBS 
issue is the best place to keep this type of information for now.

>   100  *                                                 If a security 
> manager is
>   101  * set at startup, or if the property is set to "{@code =allow}", 
> then a
>   102  * security manager can be set dynamically.
> 
> I'd split this into two (or multiple) sentences, because there's 
> actually a lot going on here.
> 
> If the property is set to the special token "allow", no security manager 
> is installed at startup, but one can be set dynamically using the 
> setSecurityManager method. (Right?)

Correct.

> I think there are more cases that need to be covered than just "allow". 
> If the property is set to "allow", the class name of a security manager, 
> the empty string "", or the special token "default", then the 
> setSecurityManager() method can be used to attempt to set the security 
> manager dynamically. However, an already-installed security manager may 
> refuse this request. (Right?)

Correct. Initially I was a bit reluctant to document the behavior of 
System.setSecurityManager for all the different property values. It also 
depends on whether you are calling it for the first time or not. The 
only real difference in behavior is if "disallow" is set. Otherwise, it 
works exactly as the API is specified. But I can see how it can cause 
confusion since there are many different options for 
java.security.manager now.

> **
> 
> Whew, this is kind of a lot. The reason is that there are several 
> different values that the property can be set to, and they have an 
> effect on the initial SM that's set AND an effect on the behavior of 
> calls to setSecurityManager() at runtime. To get this straight, I wrote 
> down a table:
> 
> 
> Prop Value          SM installed initially          setSecurityManager() 
> works
> ------------------------------------------------------------------------------ 
> 
> null                none                            yes
> empty string        java.lang.SecurityManager       maybe
> "default"           java.lang.SecurityManager       maybe
> "disallow"          none                            no
> "allow"             none                            yes
> a class name        the named class                 maybe
> 
> 
> Note: "maybe" means that setSecurityManager() will attempt to set the 
> SM, in that it won't throw UOE; however, the current SM might disallow 
> it depending upon its policies and the privilege of the caller.
> 
>  From this table one can see that setting the property to the empty 
> string and to "default" have identical effects. Also, not setting the 
> property (i.e., its value is null) and setting it to "allow" have the 
> same effects. Did I get this right?

Yes, more or less. For the "yes" boxes, it is only if you assume it is 
the first time it is called and null is not the value of SM.

> Anyway, you can describe all of this in prose, but it has to be worded 
> very carefully in order to get all the details right. Or, you could put 
> a table directly into the spec. Or both! Up to you how you want to proceed.

I did both! For the table, my main issue was how to document the last 
column, and whether to assume setSecurityManager was being called for 
the first time or not. I assumed it was not, because this is really 
about static vs dynamic and not about how many times an SM can be set.
> **
> 
> An aside, possibly off topic. If the j.s.m property names a class that's 
> to be used as the security manager, presumably it must be a 
> j.l.SecurityManager or a subclass. Are there other requirements, such as 
> having a public no-arg constructor? Does the class need to be public, 
> and does it need to be in exported package or anything? Is the classpath 
> or the module path searched, and if the alternative SM is in a named 
> module, is there a syntax for naming it?

I don't think the package needs to be exported - it looks like the code 
tries to work around that by using reflection. It can be in a named or 
unnamed module.

I have kept it simple and added these 2 sentences: "If a class name is 
specified, it must be java.lang.SecurityManager or a public subclass and 
have a public no-arg constructor. The class is loaded by the system 
class loader." We could revisit this later if it should be more precise.

--Sean

> 
> Sorry for all the impertinent questions; I can't find where this is 
> documented. Feel free to redirect.
> 
> s'marks
> 
> 
> 
> 
> 
> 
> 
> 
> 
> On 10/2/18 8:34 AM, Sean Mullan wrote:
>> Thanks for all the comments so far, and the interesting discussions 
>> about the future of the SecurityManager. We will definitely return to 
>> those discussions in the near future, but for now I have a second 
>> webrev ready for review for this enhancement:
>>
>> http://cr.openjdk.java.net/~mullan/webrevs/8191053/webrev.01/
>>
>> The main changes since the initial revision are:
>>
>> 1. System.setSecurityManager is no longer deprecated. This type of 
>> change clearly needs more discussion and is not an essential part of 
>> this RFE.
>>
>> 2. After further thought, I took Bernd's suggestion [1] and instead of 
>> adding a new property to disallow the setting of a SecurityManager at 
>> runtime, have added new tokens to the existing "java.security.manager" 
>> system property, named "=disallow", and "=allow" to toggle this 
>> behavior. The "=" is to avoid any potential clashes with custom SM 
>> classes with those names. I think this is a cleaner approach. There 
>> are a couple of new paragraphs in the SecurityManager class 
>> description describing the "java.security.manager" property and how 
>> the new tokens work.
>>
>> 3. I also added a comment that Bernd had requested [2] on why 
>> System.setSecurityManager calls checkPackageAccess("java.lang").
>>
>> Also, the CSR has been updated: 
>> https://bugs.openjdk.java.net/browse/JDK-8203316
>>
>> Thanks,
>> Sean
>>
>> [1] 
>> http://mail.openjdk.java.net/pipermail/security-dev/2018-September/018217.html 
>>
>> [2] 
>> http://mail.openjdk.java.net/pipermail/security-dev/2018-September/018193.html 
>>
>>
>> On 9/13/18 4:02 PM, Sean Mullan wrote:
>>> This is a SecurityManager related change which warrants some 
>>> additional details for its motivation.
>>>
>>> The current System.setSecurityManager() API allows a SecurityManager 
>>> to be set at run-time. However, because of this mutability, it incurs 
>>> a performance overhead even for applications that never call it and 
>>> do not enable a SecurityManager dynamically, which is probably the 
>>> majority of applications.
>>>
>>> For example, there are lots of "SecurityManager sm = 
>>> System.getSecurityManager(); if (sm != null) ..." checks in the JDK. 
>>> If it was known that a SecurityManager could never be set at 
>>> run-time, these checks could be optimized using constant-folding.
>>>
>>> There are essentially two main parts to this change:
>>>
>>> 1. Deprecation of System.securityManager()
>>>
>>> Going forward, we want to discourage applications from calling 
>>> System.setSecurityManager(). Instead they should enable a 
>>> SecurityManager using the java.security.manager system property on 
>>> the command-line.
>>>
>>> 2. A new JDK-specific system property to disallow the setting of the 
>>> security manager at run-time: jdk.allowSecurityManager
>>>
>>> If set to false, it allows the run-time to optimize the code and 
>>> improve performance when it is known that an application will never 
>>> run with a SecurityManager. To support this behavior, the 
>>> System.setSecurityManager() API has been updated such that it can 
>>> throw an UnsupportedOperationException if it does not allow a 
>>> security manager to be set dynamically.
>>>
>>> webrev: http://cr.openjdk.java.net/~mullan/webrevs/8191053/webrev.00/
>>> CSR: https://bugs.openjdk.java.net/browse/JDK-8203316
>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8191053
>>>
>>> (I will likely also send this to core-libs for additional review later)
>>>
>>> --Sean