WW-5364 Automatically populate OGNL allowlist #800

kusalk · 2023-11-23T10:13:58Z

WW-5364

We automatically populate the allowlist from interceptors, actions and result types defined in XML configuration.
This will allow most applications to turn on the OGNL allowlist with no or limited additional configuration.
We can then consider turning this security option on by default in Struts 7.0.

Documentation: apache/struts-site#213

…lowlist

…terceptor classes

# Conflicts: # core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java

kusalk · 2023-11-26T08:37:29Z

WIP: Need to add more unit test coverage for this one

…oviderAllowlist

kusalk · 2023-12-04T11:27:46Z

apps/showcase/src/main/resources/struts.xml

@@ -34,6 +34,19 @@
    <constant name="struts.custom.i18n.resources" value="globalMessages" />
    <constant name="struts.action.extension" value="action,," />

+    <constant name="struts.allowlist.enable" value="true" />


kusalk · 2023-12-04T11:28:18Z

core/src/main/java/com/opensymphony/xwork2/XWorkJUnit4TestCase.java

Moved this here so that we can utilise it for tests in core

kusalk · 2023-12-04T11:29:09Z

core/src/main/java/com/opensymphony/xwork2/config/impl/DefaultConfiguration.java

@@ -395,6 +359,57 @@ protected Container createBootstrapContainer(List<ContainerProvider> providers)
        return builder.create(true);
    }

+    public static ContainerBuilder bootstrapFactories(ContainerBuilder builder) {


Extracted code into this method for reuse by the StrutsDefaultConfigurationProvider

kusalk · 2023-12-04T11:33:08Z

core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java

@@ -57,6 +59,19 @@ public class SecurityMemberAccess implements MemberAccess {

    private static final Logger LOG = LogManager.getLogger(SecurityMemberAccess.class);

+    private static final Set<String> ALLOWLIST_REQUIRED_PACKAGES = unmodifiableSet(new HashSet<>(Arrays.asList(


Required system allowlist defined here - it's possible I've missed some packages/classes, can add as needed

kusalk · 2023-12-04T11:36:30Z

core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java

@@ -65,12 +80,14 @@ public class SecurityMemberAccess implements MemberAccess {
    private Set<String> excludedPackageNames = emptySet();
    private Set<String> excludedPackageExemptClasses = emptySet();
    private boolean enforceAllowlistEnabled = false;
-    private Set<String> allowlistClasses = emptySet();
+    private Set<Class<?>> allowlistClasses = emptySet();


While we use String comparison for the exclusion list, we use Class comparison for the allowlist to prevent accidentally allowing through another Class with the same name (in applications with multiple classloaders)

kusalk · 2023-12-04T11:38:19Z

...st/java/com/opensymphony/xwork2/config/providers/ConfigurationProviderOgnlAllowlistTest.java

+                Class.forName("com.opensymphony.xwork2.TextProvider"),
+                Class.forName("org.apache.struts2.interceptor.NoOpInterceptor"),
+                Class.forName("com.opensymphony.xwork2.interceptor.Interceptor"),
+                Class.forName("java.lang.Object"),


Even though java.lang.Object is added to the allowlist as it is a superclass, the exclusion list always takes precedence and so this class will not be allowed in practice

kusalk · 2023-12-04T11:39:15Z

plugins/junit/src/main/java/org/apache/struts2/junit/XWorkJUnit4TestCase.java

-        });
-    }
-
+public abstract class XWorkJUnit4TestCase extends com.opensymphony.xwork2.XWorkJUnit4TestCase {


Extended the moved class for compat

kusalk · 2023-12-04T11:39:41Z

plugins/junit/src/main/java/org/apache/struts2/junit/XWorkTestCase.java

+ */
+package org.apache.struts2.junit;
+
+public abstract class XWorkTestCase extends com.opensymphony.xwork2.XWorkTestCase {


Added this for consistency

kusalk · 2023-12-04T11:41:32Z

core/src/main/resources/struts-default.xml

@@ -44,8 +44,6 @@

        <interceptors>
            <interceptor name="alias" class="com.opensymphony.xwork2.interceptor.AliasInterceptor"/>
-            <interceptor name="autowiring"


This class isn't provided by core

kusalk · 2023-12-04T11:42:59Z

core/src/main/java/com/opensymphony/xwork2/config/providers/XmlDocConfigurationProvider.java

Made some changes to this file to attempt to load every class defined in a package and add it to the allowlist

sonarcloud · 2023-12-04T11:46:26Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
10 Code Smells

84.4% Coverage
2.2% Duplication

kusalk force-pushed the WW-5364-populate-allowlist branch 3 times, most recently from 6a47136 to 324f825 Compare November 24, 2023 09:55

kusalk added 2 commits November 24, 2023 21:07

WW-5364 Modify XmlDocConfigurationProvider to be able to load into al…

ebdf019

…lowlist

WW-5364 Make allowlist classloader specific

1d76bff

kusalk force-pushed the WW-5364-populate-allowlist branch from 324f825 to d431531 Compare November 24, 2023 10:07

kusalk added 3 commits November 24, 2023 21:32

WW-5364 Implement provider allowlist

198812f

WW-5364 Inject ProviderAllowlist into SecurityMemberAccess

3bf3e5f

WW-5364 Enable allowlist for showcase

ee442db

kusalk force-pushed the WW-5364-populate-allowlist branch from d431531 to cf178dd Compare November 24, 2023 10:54

kusalk added 3 commits November 24, 2023 22:41

WW-5364 Add Struts components to allowlist

39c3e33

WW-5364 Don't throw ConfigurationException on unloadable action or in…

6657e01

…terceptor classes

WW-5364 Replace some allowlist classes with packages

d7df9ce

kusalk force-pushed the WW-5364-populate-allowlist branch from b6bd2ee to d7df9ce Compare November 24, 2023 11:41

kusalk mentioned this pull request Nov 26, 2023

WW-5364 Document new OGNL security features apache/struts-site#213

Merged

Merge branch 'WW-5343-sec-extend' into WW-5364-populate-allowlist

0566a20

# Conflicts: # core/src/main/java/com/opensymphony/xwork2/ognl/SecurityMemberAccess.java

kusalk force-pushed the WW-5364-populate-allowlist branch from f164e4a to 0566a20 Compare November 26, 2023 07:24

Base automatically changed from WW-5343-sec-extend to master November 26, 2023 08:36

kusalk added 6 commits November 26, 2023 23:46

WW-5343 Collect bootstrap factories

d030532

Merge branch 'master' into WW-5364-populate-allowlist

9aff37a

WW-5343 Add unit test coverage for ProviderAllowlist

5e33c7f

WW-5343 Move JUnit4 test case into Struts-core

16f822a

WW-5343 Add integration tests for ConfigurationProvider populating Pr…

a268233

…oviderAllowlist

WW-5343 Add missing licenses

589219b

kusalk force-pushed the WW-5364-populate-allowlist branch from 2a70545 to 589219b Compare December 4, 2023 10:58

WW-5343 Make StrutsTestCase extend same package

a7d273c

kusalk commented Dec 4, 2023

View reviewed changes

kusalk requested a review from lukaszlenart December 4, 2023 11:45

kusalk marked this pull request as ready for review December 4, 2023 11:45

lukaszlenart approved these changes Dec 5, 2023

View reviewed changes

kusalk merged commit 80e8361 into master Dec 5, 2023
10 checks passed

kusalk deleted the WW-5364-populate-allowlist branch December 5, 2023 05:57

kusalk mentioned this pull request Dec 17, 2023

Rebase struts-7-0-x branch #809

Merged

kusalk mentioned this pull request Jan 9, 2024

WW-5352 Introducing the StrutsParameter annotation #832

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WW-5364 Automatically populate OGNL allowlist #800

WW-5364 Automatically populate OGNL allowlist #800

kusalk commented Nov 23, 2023 •

edited

Loading

kusalk commented Nov 26, 2023

kusalk Dec 4, 2023

kusalk Dec 4, 2023

kusalk Dec 4, 2023

kusalk Dec 4, 2023

kusalk Dec 4, 2023

kusalk Dec 4, 2023

kusalk Dec 4, 2023

kusalk Dec 4, 2023

kusalk Dec 4, 2023

kusalk Dec 4, 2023

sonarcloud bot commented Dec 4, 2023

		@@ -57,6 +59,19 @@ public class SecurityMemberAccess implements MemberAccess {

		private static final Logger LOG = LogManager.getLogger(SecurityMemberAccess.class);

		private static final Set<String> ALLOWLIST_REQUIRED_PACKAGES = unmodifiableSet(new HashSet<>(Arrays.asList(

WW-5364 Automatically populate OGNL allowlist #800

WW-5364 Automatically populate OGNL allowlist #800

Conversation

kusalk commented Nov 23, 2023 • edited Loading

kusalk commented Nov 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Dec 4, 2023

kusalk commented Nov 23, 2023 •

edited

Loading