Bug 66370 - AccessControlException and default behavior change with org.apache.el.GET_CLASSLOADER_USE_PRIVILEGED
Summary: AccessControlException and default behavior change with org.apache.el.GET_CLA...
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 10
Classification: Unclassified
Component: EL (show other bugs)
Version: 10.1.2
Hardware: PC All
: P2 normal (vote)
Target Milestone: ------
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-12-01 16:32 UTC by Isaac Rivera Rivas
Modified: 2023-01-06 16:03 UTC (History)
0 users



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Isaac Rivera Rivas 2022-12-01 16:32:43 UTC
Hey there,

Running tomcat el 10.1.2 with java 2 security enabled, we discovered an issue with an AccessControlException due to Boolean.getBoolean(). It seems this was introduced in this commit https://github.com/apache/tomcat/commit/28ea2b9b2e781d20e0651cb5e0b65bacd464150c#diff-b5962d24af20591547a4804838aa91c84b0151645b2121ac4f244a1b9c1213e8R46-R47 with the addition of the new property org.apache.el.GET_CLASSLOADER_USE_PRIVILEGED. See exception below

java.security.AccessControlException: Access denied ("java.util.PropertyPermission" "org.apache.el.GET_CLASSLOADER_USE_PRIVILEGED" "read")
        at java.base/java.security.AccessController.throwACE(AccessController.java:176)
        at java.base/java.security.AccessController.checkPermissionHelper(AccessController.java:238)
        at java.base/java.security.AccessController.checkPermission(AccessController.java:385)
        at java.base/java.lang.SecurityManager.checkPermission(SecurityManager.java:322)
        at com.ibm.ws.kernel.launch.internal.MissingDoPrivDetectionSecurityManager.checkPermission(MissingDoPrivDetectionSecurityManager.java:45)
        at java.base/java.lang.SecurityManager.checkPropertyAccess(SecurityManager.java:1066)
        at java.base/java.lang.System.getProperty(System.java:581)
        at java.base/java.lang.System.getProperty(System.java:564)
        at java.base/java.lang.Boolean.getBoolean(Boolean.java:265)
        at jakarta.el.Util.<clinit>(Util.java:47)
        at jakarta.el.ExpressionFactory.newInstance(ExpressionFactory.java:92)
        at jakarta.el.ExpressionFactory.newInstance(ExpressionFactory.java:79)
        at org.apache.jasper.runtime.JspApplicationContextImpl.<clinit>(JspApplicationContextImpl.java:59)
        at org.apache.jasper.runtime.JspFactoryImpl.getJspApplicationContext(JspFactoryImpl.java:265)
        at com.ibm._jsp._login._jspInit(_login.java:66)
        at com.ibm.ws.jsp.runtime.HttpJspBase.init(HttpJspBase.java:77)
        at com.ibm.ws.webcontainer.servlet.ServletWrapper.init(ServletWrapper.java:299)



Worked locally on a fix for this and discovered that with the addition of this new property, a behavior change was introduced which obligates us to set the property if we don't want to hit another AccessControlException as seen below. I understand why the change was introduced with this new property but I would expect that it would not affect the expected behavior by default. I would expect to see a change only if it was set by the user. Any input as to why this was done the way it was? Any way we can change this to keep the expected behavior by default and change it if set?

java.security.AccessControlException: Access denied ("java.lang.RuntimePermission" "getClassLoader")
        at java.base/java.security.AccessController.throwACE(AccessController.java:176)
        at java.base/java.security.AccessController.checkPermissionHelper(AccessController.java:238)
        at java.base/java.security.AccessController.checkPermission(AccessController.java:385)
        at java.base/java.lang.SecurityManager.checkPermission(SecurityManager.java:322)
        at com.ibm.ws.kernel.launch.internal.MissingDoPrivDetectionSecurityManager.checkPermission(MissingDoPrivDetectionSecurityManager.java:45)
        at java.base/java.lang.Thread.getContextClassLoader(Thread.java:578)
        at jakarta.el.Util.getContextClassLoader(Util.java:665)
        at jakarta.el.ExpressionFactory.newInstance(ExpressionFactory.java:92)
        at jakarta.el.ExpressionFactory.newInstance(ExpressionFactory.java:79)
        at org.apache.jasper.runtime.JspApplicationContextImpl.<clinit>(JspApplicationContextImpl.java:59)
        at org.apache.jasper.runtime.JspFactoryImpl.getJspApplicationContext(JspFactoryImpl.java:265)
        at com.ibm._jsp._login._jspInit(_login.java:72)
        at com.ibm.ws.jsp.runtime.HttpJspBase.init(HttpJspBase.java:77)
        at com.ibm.ws.webcontainer.servlet.ServletWrapper.init(ServletWrapper.java:299)
Comment 1 Isaac Rivera Rivas 2022-12-01 16:35:34 UTC
Proposed fix for the Boolean.getBoolean java 2 security issue https://github.com/apache/tomcat/pull/572 doesn't address the behavior difference with the new property
Comment 2 Isaac Rivera Rivas 2022-12-07 14:49:03 UTC
Updated the PR with a proposed change to restore default behavior and keep a property to control when to use the security manager.
Comment 3 Christopher Schultz 2022-12-07 23:06:17 UTC
You should read both https://lists.apache.org/thread/66djc4j4ybz45ponly0x5g94oyt844cm and https://lists.apache.org/thread/7w4x90zwp3qhn9qopbhmg5wponcc468n in their entirety to see the thought-process(es) here.
Comment 4 Mark Thomas 2022-12-08 15:15:23 UTC
The default for the property is unlikely to change but if there is a code path in standard Tomcat usage that reaches that point without entering a doPrivileged() block then we can look at that as it likely needs fixing anyway.
Comment 5 Paul Nicolucci 2022-12-08 16:51:24 UTC
Hi,

We use the Tomcat ExpressionLanguage within Open Liberty. We hit this issue after updating to the 10.1.2 (from 10.1.1) version of the Expression Language. I've read through the following thread: https://lists.apache.org/thread/7w4x90zwp3qhn9qopbhmg5wponcc468n

Although the SecurityManager is deprecated and may go away in Jakarta EE11 it is still used heavily in Jakarta EE10.

The problem is two fold:

1) When a SecurityManager is being used the following Exception occurs when looking up the value of the new system property:

java.security.AccessControlException: Access denied ("java.util.PropertyPermission" "org.apache.el.GET_CLASSLOADER_USE_PRIVILEGED" "read")
        at java.base/java.security.AccessController.throwACE(AccessController.java:176)
        at java.base/java.security.AccessController.checkPermissionHelper(AccessController.java:238)
        at java.base/java.security.AccessController.checkPermission(AccessController.java:385)
        at java.base/java.lang.SecurityManager.checkPermission(SecurityManager.java:322)
        at com.ibm.ws.kernel.launch.internal.MissingDoPrivDetectionSecurityManager.checkPermission(MissingDoPrivDetectionSecurityManager.java:45)
        at java.base/java.lang.SecurityManager.checkPropertyAccess(SecurityManager.java:1066)
        at java.base/java.lang.System.getProperty(System.java:581)
        at java.base/java.lang.System.getProperty(System.java:564)
        at java.base/java.lang.Boolean.getBoolean(Boolean.java:265)
        at jakarta.el.Util.<clinit>(Util.java:47)

see: https://docs.oracle.com/javase/8/docs/api/java/lang/Boolean.html#getBoolean-java.lang.String-

2) Anyone who is using a SecurityManager and wants the protection offered by it will now need to enable the new system property as well. As stated in the thread I referenced earlier this is a backward incompatible change going from 10.1.1  to 10.1.2. Even the thread referenced says this was likely a theoretical issue. 

Given the above I would argue a few things:

A) The default should be opt out so by default using the Security Manager gives you the expected behavior without having to set a system property.

B) If in Jakarta EE11 the use of the SecurityManager is removed or made optional then this property could then potentially be removed or opt-int.

C) At the very least when using Boolean.getBoolean the AccessControlException needs to be avoided.

If changing the default of the property isn't a possibility and won't be accepted by the community, can we agree to at least fix C?
Comment 6 Mark Thomas 2022-12-09 12:47:53 UTC
Looking at comment 0, this line appears in the stack trace:

org.apache.jasper.runtime.JspFactoryImpl.getJspApplicationContext(JspFactoryImpl.java:265)

I can't match that to any current or historical Tomcat version. It also appears to be missing a privileged block (which Tomcat versions all have) which would avoid the problem.

Are you using a modified JspFactoryImpl? If so, this is an issue that needs to be addressed in that modified JspFactoryImpl.


Looking at comment 4, I'd like to see the full stack trace to see what is calling jakarta.el.Util since that is a package private class.


 
Looking at the history here:

Bug 62080
Some non-Tomcat users of the EL API Jar provided by Tomcat stated a need for additional privileged blocks. While they are known not to be required by Tomcat, they were added.

Bug 66294
Tomcat users report that the additional privilege blocks causes performance issues. The solution was to make the use of the additional privilege blocks optional. This was controlled by a system property. The complication is that this also must be called from within a privileged block. Again, this is not an issue for Tomcat.

This bug
At least one non-Tomcat user of the EL API JAR doesn't use privileged blocks where Tomcat does.


Things are not helped by the limited information provided in bug 62080 that started all of this.


At this point I see three possible ways forward:

1. Users of the Tomcat provided EL (Open Liberty and others) use privilege blocks for the same calls Tomcat uses them for.

2. The changes for bug 66294 and bug 62080 are reverted.

3. Users stop using the SecurityManager as it is going to go away eventually and this is an opportunity to find an alternative solution to whatever problem using a security manager is currently solving for them.


Option 3 seems highly unlikely.

Some variation of option 1 might well turn out to the fix for the problem described in bug 62080 but given the lack of information in that bug it is hard to tell.

I'm currently leaning towards 2 and if bug 62080 is re-opened, we can dig into exactly why it is happening.
Comment 7 Isaac Rivera Rivas 2022-12-14 14:13:36 UTC
Following the discussion on comment 6, I’ve tried a couple of things for this in Open Liberty. I worked on wrapping up some of the function calls that call out to Expression Language. That required a couple of changes and did fix some of the issues but I realized that other open source we use specifically the hibernate-validator also have calls to EL specifically here https://github.com/hibernate/hibernate-validator/blob/8ed05f71e569b2a9d2fefcaf2c14187443f55be8/engine/src/main/java/org/hibernate/validator/messageinterpolation/ResourceBundleMessageInterpolator.java#L174 which reaches here https://github.com/apache/tomcat/blob/8e2aa5e45ce13388da62386e3cb1dbfa3b242b4b/java/jakarta/el/ELManager.java#L30 in Tomcat EL calling the Util class and hitting the issue. This is just one I’ve found but there could be many other examples of other open source out there which use Tomcat’s EL and run into the same issue using the security manager. In my opinion, the source of the issue is in EL itself and instead of fixing or wrapping the calls to EL in other open sources with privilege blocks, it would be best to fix it in the source of the issue itself.

FYI This is the Open Liberty issue following the changes https://github.com/OpenLiberty/open-liberty/issues/23543
Comment 8 Mark Thomas 2022-12-15 16:18:24 UTC
That is a fair point that the necessary privilege blocks should be in place inside the EL.

If the default for the GET_CLASSLOADER_USE_PRIVILEGED system property was changed, and the code that obtained the value for that system property was obtained inside a privilege block, would that address the issues Open Liberty is seeing? I ask as I might have a (less ugly than my previous ideas) way to have a different default depending on whether or not the code is running on Tomcat which would also address the performance concerns.
Comment 9 Isaac Rivera Rivas 2022-12-16 02:00:40 UTC
Yes, if the default is changed and the property lookup is wrapped in a privilege block it should fix at least the issues seen in Open Liberty. I've tested with the changes in the proposed PR https://github.com/apache/tomcat/pull/572 (not exactly changing the default but in essence reverting to the default behavior) and have seen the issues fixed.
Comment 10 Mark Thomas 2023-01-05 10:01:49 UTC
Fixed in:
- 11.0.x for 11.0.0-M2 onwards
- 10.1.x for 10.1.5 onwards
-  9.0.x for  9.0.71 onwards
-  8.5.x for  8.5.85 onwards
Comment 11 Isaac Rivera Rivas 2023-01-06 16:03:06 UTC
Verified the updates worked locally by building the jar and testing with it. Thanks for making the changes!