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