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

Stuart Marks stuart.marks at oracle.com
Thu Oct 4 21:56:00 UTC 2018


Hi Sean,

Thanks for the updates. Looks like the details are all there now. One minor point:

   87  * If a class name is specified, it must be {@code java.lang.SecurityManager}
   88  * or a public subclass and have a public no-arg constructor. The class is
   89  * loaded by the {@linkplain ClassLoader#getSystemClassLoader()
   90  * system class loader}.

The class is loaded by the system class loader, only if the class name is 
something other than "java.lang.SecurityManager".

Editorial:

   83  * the system property "{@code java.security.manager}" on the command line to

Here and in several places, I don't think it's necessary to have the property 
name both in code font and in quotation marks. I think the code font is sufficient.

Thanks,

s'marks



On 10/4/18 2:04 PM, Sean Mullan wrote:
> 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



More information about the security-dev mailing list