WW-5343 Make SecurityMemberAccess an extensible bean #791

kusalk · 2023-11-14T13:35:41Z

WW-5343

This will allow applications to easily extend the capabilities of SecurityMemberAccess. It additionally allows us to significantly simplify the configuration loading.

…s bean

…rAccess bean

…ccess

kusalk · 2023-11-22T14:12:13Z

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

@@ -133,7 +134,6 @@ public class DefaultConfiguration implements Configuration {
        Map<String, Object> constants = new HashMap<>();
        constants.put(StrutsConstants.STRUTS_DEVMODE, Boolean.FALSE);
        constants.put(StrutsConstants.STRUTS_OGNL_LOG_MISSING_PROPERTIES, Boolean.FALSE);
-        constants.put(StrutsConstants.STRUTS_OGNL_ENABLE_EVAL_EXPRESSION, Boolean.FALSE);


This constant is not required for bootstrapping

kusalk · 2023-11-22T14:13:17Z

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

    }

-    private static Set<String> toClassesSet(String newDelimitedClasses) throws ConfigurationException {


Moved these utility methods into their own class

kusalk · 2023-11-22T14:13:50Z

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

    private boolean enableEvalExpression;

-    private Set<String> excludedClasses = emptySet();


All the configuration is now injected directly into SecurityMemberAccess except for devMode configuration

kusalk · 2023-11-22T14:14:33Z

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

    public Set<String> getExcludedClasses() {
-        return excludedClasses;
+        return toClassesSet(container.getInstance(String.class, StrutsConstants.STRUTS_EXCLUDED_CLASSES));


Deprecated getters are still functional for API compatibility

kusalk · 2023-11-22T14:15:08Z

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

@@ -922,8 +855,7 @@ protected Map<String, Object> createDefaultContext(Object root, ClassResolver cl
            resolver = container.getInstance(CompoundRootAccessor.class);
        }

-        SecurityMemberAccess memberAccess = new SecurityMemberAccess(allowStaticFieldAccess);
-        memberAccess.disallowProxyMemberAccess(disallowProxyMemberAccess);
+        SecurityMemberAccess memberAccess = container.getInstance(SecurityMemberAccess.class);


This is a prototype bean so will be a new instance per context

kusalk · 2023-11-22T14:17:42Z

core/src/main/java/com/opensymphony/xwork2/util/MemberAccessValueStack.java

@@ -31,15 +31,19 @@ public interface MemberAccessValueStack {
     * @deprecated please use {@link #useExcludeProperties(Set)}
     */
    @Deprecated
-    void setExcludeProperties(Set<Pattern> excludeProperties);
+    default void setExcludeProperties(Set<Pattern> excludeProperties) {
+        useExcludeProperties(excludeProperties);


Lifted the deprecated implementations into the interface for cleanliness

kusalk · 2023-11-22T14:18:00Z

core/src/main/java/org/apache/struts2/StrutsConstants.java

@@ -234,6 +234,8 @@ public final class StrutsConstants {
    /** The name of the parameter to determine whether static field access will be allowed in OGNL expressions or not */
    public static final String STRUTS_ALLOW_STATIC_FIELD_ACCESS = "struts.ognl.allowStaticFieldAccess";

+    public static final String STRUTS_MEMBER_ACCESS = "struts.securityMemberAccess";


Extension point!

kusalk · 2023-11-22T14:18:34Z

core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilStrutsTest.java

@@ -31,33 +31,6 @@ public void setUp() throws Exception {
        ognlUtil = container.getInstance(OgnlUtil.class);
    }

-    public void testDefaultExcludes() {


Deleted tests whose functionality is covered elsewhere

kusalk · 2023-11-22T14:19:28Z

core/src/test/java/com/opensymphony/xwork2/ognl/OgnlValueStackTest.java

-     * Test a raw OgnlValueStackFactory and OgnlValueStack generated by it
-     * when no static access flags are set (not present in configuration).
-     */
-    public void testOgnlValueStackFromOgnlValueStackFactoryNoFlagsSet() {


This flag is now a required constant so this test is no longer applicable

kusalk · 2023-11-22T14:19:55Z

core/src/test/java/com/opensymphony/xwork2/ognl/SecurityMemberAccessTest.java

+    }
+
+    @Test
+    public void defaultExclusionList() throws Exception {


Moved/rewrote some tests here from OgnlUtilTest

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

lukaszlenart · 2023-11-24T13:57:17Z

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

@@ -385,6 +386,7 @@ protected Container createBootstrapContainer(List<ContainerProvider> providers)
        builder.factory(ExpressionCacheFactory.class, DefaultOgnlExpressionCacheFactory.class, Scope.SINGLETON);
        builder.factory(BeanInfoCacheFactory.class, DefaultOgnlBeanInfoCacheFactory.class, Scope.SINGLETON);
        builder.factory(OgnlUtil.class, Scope.SINGLETON);
+        builder.factory(SecurityMemberAccess.class, Scope.PROTOTYPE);


I wonder if this is a good idea, attacker could get access to the SMA just by having access to the Container

Given it's prototype scope, I don't believe it to make any difference

That is, the container would return a new instance, and not the one that is being used by the ValueStack

I meant it will be possible creating a new instance via containers methods, yet we excluded the whole package com.opensymphony.xwork2.inject which means attacker cannot use container in the first place.

True, I also don't think there's anything useful an attacker can do with a fresh SecurityMemberAccess instance

sonarqubecloud · 2023-11-26T06:08:32Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
21 Code Smells

83.6% Coverage
0.0% Duplication

lukaszlenart

I'm good but please update the docs to address that a new extension has been added (or at least update the ticket to inform users about it)
https://struts.apache.org/plugins/plugins-architecture#extension-points

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

Documentation PR is here: apache/struts-site#213

kusalk added 10 commits November 14, 2023 20:43

WW-5343 Delete unused code and consolidate constructors

79ffc86

WW-5343 Extract ConfigParseUtil

0825329

WW-5343 Extract deprecated methods as default interface methods

b0b80ba

WW-5343 Deprecate unnecessary setter

9e556e9

WW-5343 Make SecurityMemberAccess a prototype bean

90344b3

WW-5343 Refactor OgnlValueStackFactory to utilise SecurityMemberAcces…

7e92a8d

…s bean

WW-5343 Update OgnlUtil#createDefaultContext to utilise SecurityMembe…

b518635

…rAccess bean

WW-5343 Move configuration injection from OgnlUtil to SecurityMemberA…

4490d9d

…ccess

WW-5343 Fix OgnlUtilTest#testBeanMapExpressions

8bf47b3

WW-5343 Fix unit test compilation errors

62988f7

kusalk mentioned this pull request Nov 14, 2023

WW-5350 Implement OGNL Allowlist capability #781

Merged

Base automatically changed from WW-5350-allowlist-2 to master November 14, 2023 13:39

kusalk added 8 commits November 16, 2023 23:06

WW-5343 Remove unnecessary method

ceff6cd

WW-5343 Add missing license

f87d7d7

WW-5343 Revert and fix serializability

a402e5c

WW-5343 Fix MemberAccess access blocked tests

d0d10d9

WW-5343 Remove defunct test now that constant is required

68f5584

Merge branch 'master' into WW-5343-sec-extend

05a9973

WW-5343 Migrate tests to SecurityMemberAccessTest

9640f5b

WW-5343 Fix final test

eba79a7

kusalk commented Nov 22, 2023

View reviewed changes

kusalk marked this pull request as ready for review November 22, 2023 14:20

lukaszlenart reviewed Nov 24, 2023

View reviewed changes

WW-5343 Clean up bootstrap constants

85d2c74

kusalk requested a review from lukaszlenart November 26, 2023 05:55

WW-5343 Address SonarCloud code smells

7929d86

kusalk mentioned this pull request Nov 26, 2023

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

Merged

lukaszlenart approved these changes Nov 26, 2023

View reviewed changes

kusalk merged commit a60842a into master Nov 26, 2023
10 checks passed

kusalk deleted the WW-5343-sec-extend branch November 26, 2023 08:36

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-5343 Make SecurityMemberAccess an extensible bean #791

WW-5343 Make SecurityMemberAccess an extensible bean #791

kusalk commented Nov 14, 2023

kusalk Nov 22, 2023

kusalk Nov 22, 2023

kusalk Nov 22, 2023

kusalk Nov 22, 2023

kusalk Nov 22, 2023

kusalk Nov 22, 2023

kusalk Nov 22, 2023

kusalk Nov 22, 2023

kusalk Nov 22, 2023

kusalk Nov 22, 2023

lukaszlenart Nov 24, 2023

kusalk Nov 26, 2023

kusalk Nov 26, 2023

lukaszlenart Nov 26, 2023

kusalk Nov 26, 2023

sonarqubecloud bot commented Nov 26, 2023

lukaszlenart left a comment

kusalk commented Nov 26, 2023

		}

		private static Set<String> toClassesSet(String newDelimitedClasses) throws ConfigurationException {

		private boolean enableEvalExpression;

		private Set<String> excludedClasses = emptySet();

WW-5343 Make SecurityMemberAccess an extensible bean #791

WW-5343 Make SecurityMemberAccess an extensible bean #791

Conversation

kusalk commented Nov 14, 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

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

sonarqubecloud bot commented Nov 26, 2023

lukaszlenart left a comment

Choose a reason for hiding this comment

kusalk commented Nov 26, 2023