Mark,

On 11/8/22 12:41, Mark Thomas wrote:
On 08/11/2022 16:52, Christopher Schultz wrote:
Mark,

Wouldn't it be "safer" to have this doPrivileged be an "opt-out" permission rather than an "opt-in" permission?

Good question.

Nobody is going to know that they need to enable this options in order to get "proper protection".

Until they see the exception.

It will be an ugly permission error, and they'll assume their SecurityManager hasn't been configured properly. If we could throw an error saying "you should enable this system property if you need to use Tomcat EL in this way" than it would be nice. But we can't. :/

This change is not exactly backward-compatible. It may break people who are otherwise happily using the Tomcat EL package by requiring them to add a system property to get it to work.

I think the doPrivileged should be present /by default/ and the preference should be opt-out if only to maintain backward-compatibility. Evidently, only one user on the planet needs to disable this privilege re-acquisition.

No test case was every provided for BZ 62080. I suspect it was a theoretical issue rather than one observed in real code.

The performance issue is an issue for everyone using a SecurityManager.

Another factor I considered is that the SecurityManager is deprecated and support for it is likely to be removed in Jakarta EE 11.

I went for disabled by default because I thought that was the best solution for the majority - possible all - users.

I think we should mention this in the "Notable Changes" section of the UG.

-chris

On 11/8/22 08:16, ma...@apache.org wrote:
This is an automated email from the ASF dual-hosted git repository.

markt pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/tomcat.git


The following commit(s) were added to refs/heads/main by this push:
      new 28ea2b9b2e Fix BZ 66294. Make use of privileged block optional. Performance hotspot
28ea2b9b2e is described below

commit 28ea2b9b2e781d20e0651cb5e0b65bacd464150c
Author: Mark Thomas <ma...@apache.org>
AuthorDate: Tue Nov 8 13:16:32 2022 +0000

     Fix BZ 66294. Make use of privileged block optional. Performance hotspot
     https://bz.apache.org/bugzilla/show_bug.cgi?id=66294
---
  java/jakarta/el/Util.java           | 5 ++++-
  webapps/docs/changelog.xml          | 7 +++++++
  webapps/docs/config/systemprops.xml | 9 +++++++++
  3 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/java/jakarta/el/Util.java b/java/jakarta/el/Util.java
index b0a995c59b..71527d2429 100644
--- a/java/jakarta/el/Util.java
+++ b/java/jakarta/el/Util.java
@@ -43,6 +43,9 @@ class Util {
      private static final Class<?>[] EMPTY_CLASS_ARRAY = new Class<?>[0];
      private static final Object[] EMPTY_OBJECT_ARRAY = new Object[0];
+    private static final boolean GET_CLASSLOADER_USE_PRIVILEGED =
+ Boolean.getBoolean("org.apache.el.GET_CLASSLOADER_USE_PRIVILEGED");
+
      /**
       * Checks whether the supplied Throwable is one that needs to be
       * rethrown and swallows all others.
@@ -655,7 +658,7 @@ class Util {
      static ClassLoader getContextClassLoader() {
          ClassLoader tccl;
-        if (System.getSecurityManager() != null) {
+        if (System.getSecurityManager() != null && GET_CLASSLOADER_USE_PRIVILEGED) {               PrivilegedAction<ClassLoader> pa = new PrivilegedGetTccl();
              tccl = AccessController.doPrivileged(pa);
          } else {
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index 22c06cb070..33800616d7 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -167,6 +167,13 @@
    </subsection>
    <subsection name="Jasper">
      <changelog>
+      <fix>
+        <bug>66294</bug>: Make the use of a privileged block to obtain the +        thread context class loader added to address <bug>62080</bug> optional
+        and disabled by default. This is now controlled by the
+        <code>org.apache.el.GET_CLASSLOADER_USE_PRIVILEGED</code> system
+        property. (markt)
+      </fix>
        <fix>
          <bug>66317</bug>: Fix for Lambda coercion security manager missing           privileges. Based on pull request #557 by Isaac Rivera Rivas (lihan) diff --git a/webapps/docs/config/systemprops.xml b/webapps/docs/config/systemprops.xml
index 4225fd2bec..0def5feb97 100644
--- a/webapps/docs/config/systemprops.xml
+++ b/webapps/docs/config/systemprops.xml
@@ -74,6 +74,15 @@
  <section name="Expression Language">
    <properties>
+    <property name="org.apache.el. GET_CLASSLOADER_USE_PRIVILEGED">
+      <p>Controls whether the EL API classes make use of a privileged block to +      obtain the thread context class loader. When using the EL API within +      Apache Tomcat this does not need to be set as all calls are already +      wrapped in a privileged block further up the stack. It may be required if +      using the EL API under a SecurityManager outside of Apache Tomcat.</p> +      <p>If not specified, the default of <code>false</code> will be used.</p>
+    </property>
+
      <property name="org.apache.el.BeanELResolver. CACHE_SIZE">
        <p>The number of jakarta.el.BeanELResolver.BeanProperties objects that will
        be cached by the EL Parser.</p>


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

Reply via email to