Merge pull request #806 from apache/WW-5339-astmap-block

WW-5339 Add option to block custom OGNL maps
apache · Dec 5, 2023 · 6fcb501 · 6fcb501
2 parents 80e8361 + 002e598
commit 6fcb501
Show file tree

Hide file tree

Showing 8 changed files with 72 additions and 12 deletions.
diff --git a/core/src/main/java/com/opensymphony/xwork2/config/impl/DefaultConfiguration.java b/core/src/main/java/com/opensymphony/xwork2/config/impl/DefaultConfiguration.java
@@ -100,6 +100,7 @@
 import com.opensymphony.xwork2.util.fs.DefaultFileManagerFactory;
 import com.opensymphony.xwork2.util.location.LocatableProperties;
 import com.opensymphony.xwork2.util.reflection.ReflectionProvider;
+import ognl.ClassResolver;
 import ognl.PropertyAccessor;
 import org.apache.commons.lang3.StringUtils;
 import org.apache.logging.log4j.LogManager;
@@ -390,6 +391,7 @@ public static ContainerBuilder bootstrapFactories(ContainerBuilder builder) {
 
                 .factory(ObjectTypeDeterminer.class, DefaultObjectTypeDeterminer.class, Scope.SINGLETON)
                 .factory(PropertyAccessor.class, CompoundRoot.class.getName(), CompoundRootAccessor.class, Scope.SINGLETON)
+                .factory(ClassResolver.class, CompoundRoot.class.getName(), CompoundRootAccessor.class, Scope.SINGLETON)
 
                 .factory(ExpressionCacheFactory.class, DefaultOgnlExpressionCacheFactory.class, Scope.SINGLETON)
                 .factory(BeanInfoCacheFactory.class, DefaultOgnlBeanInfoCacheFactory.class, Scope.SINGLETON)

diff --git a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java
@@ -21,7 +21,6 @@
 import com.opensymphony.xwork2.conversion.impl.XWorkConverter;
 import com.opensymphony.xwork2.inject.Container;
 import com.opensymphony.xwork2.inject.Inject;
-import com.opensymphony.xwork2.ognl.accessor.CompoundRootAccessor;
 import com.opensymphony.xwork2.util.CompoundRoot;
 import com.opensymphony.xwork2.util.reflection.ReflectionException;
 import ognl.ClassResolver;
@@ -856,10 +855,12 @@ protected Map<String, Object> createDefaultContext(Object root) {
         return createDefaultContext(root, null);
     }
 
-    protected Map<String, Object> createDefaultContext(Object root, ClassResolver classResolver) {
-        ClassResolver resolver = classResolver;
+    protected Map<String, Object> createDefaultContext(Object root, ClassResolver resolver) {
         if (resolver == null) {
-            resolver = container.getInstance(CompoundRootAccessor.class);
+            resolver = container.getInstance(ClassResolver.class, CompoundRoot.class.getName());
+            if (resolver == null) {
+                throw new IllegalStateException("Cannot find ClassResolver");
+            }
         }
 
         SecurityMemberAccess memberAccess = container.getInstance(SecurityMemberAccess.class);

diff --git a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStackFactory.java b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlValueStackFactory.java
@@ -24,9 +24,9 @@
 import com.opensymphony.xwork2.inject.Container;
 import com.opensymphony.xwork2.inject.Inject;
 import com.opensymphony.xwork2.ognl.accessor.CompoundRootAccessor;
-import com.opensymphony.xwork2.util.CompoundRoot;
 import com.opensymphony.xwork2.util.ValueStack;
 import com.opensymphony.xwork2.util.ValueStackFactory;
+import ognl.ClassResolver;
 import ognl.MethodAccessor;
 import ognl.OgnlRuntime;
 import ognl.PropertyAccessor;
@@ -50,6 +50,11 @@ protected void setXWorkConverter(XWorkConverter converter) {
         this.xworkConverter = converter;
     }
 
+    @Inject(value = "com.opensymphony.xwork2.util.CompoundRoot")
+    protected void setClassResolver(ClassResolver classResolver) {
+        this.compoundRootAccessor = (CompoundRootAccessor) classResolver;
+    }
+
     @Inject("system")
     protected void setTextProvider(TextProvider textProvider) {
         this.textProvider = textProvider;
@@ -75,9 +80,6 @@ protected void setContainer(Container container) throws ClassNotFoundException {
         for (String name : names) {
             Class<?> cls = Class.forName(name);
             OgnlRuntime.setPropertyAccessor(cls, container.getInstance(PropertyAccessor.class, name));
-            if (compoundRootAccessor == null && CompoundRoot.class.isAssignableFrom(cls)) {
-                compoundRootAccessor = (CompoundRootAccessor) container.getInstance(PropertyAccessor.class, name);
-            }
         }
 
         names = container.getInstanceNames(MethodAccessor.class);
@@ -91,9 +93,6 @@ protected void setContainer(Container container) throws ClassNotFoundException {
             Class<?> cls = Class.forName(name);
             OgnlRuntime.setNullHandler(cls, new OgnlNullHandlerWrapper(container.getInstance(NullHandler.class, name)));
         }
-        if (compoundRootAccessor == null) {
-            throw new IllegalStateException("Couldn't find the compound root accessor");
-        }
         this.container = container;
     }
 

diff --git a/core/src/main/java/com/opensymphony/xwork2/ognl/accessor/CompoundRootAccessor.java b/core/src/main/java/com/opensymphony/xwork2/ognl/accessor/CompoundRootAccessor.java
@@ -42,7 +42,6 @@
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Map;
-import java.util.Set;
 import java.util.SortedSet;
 import java.util.TreeSet;
 import java.util.concurrent.ConcurrentHashMap;
@@ -77,12 +76,18 @@ public String getSourceSetter(OgnlContext context, Object target, Object index)
     private final static Class[] EMPTY_CLASS_ARRAY = new Class[0];
     private static final Map<MethodCall, Boolean> invalidMethods = new ConcurrentHashMap<>();
     private boolean devMode;
+    private boolean disallowCustomOgnlMap;
 
     @Inject(StrutsConstants.STRUTS_DEVMODE)
     protected void setDevMode(String mode) {
         this.devMode = BooleanUtils.toBoolean(mode);
     }
 
+    @Inject(value = StrutsConstants.STRUTS_DISALLOW_CUSTOM_OGNL_MAP, required = false)
+    public void useDisallowCustomOgnlMap(String disallowCustomOgnlMap) {
+        this.disallowCustomOgnlMap = BooleanUtils.toBoolean(disallowCustomOgnlMap);
+    }
+
     public void setProperty(Map context, Object target, Object name, Object value) throws OgnlException {
         CompoundRoot root = (CompoundRoot) target;
         OgnlContext ognlContext = (OgnlContext) context;
@@ -275,6 +280,14 @@ public Object callStaticMethod(Map transientVars, Class aClass, String s, Object
     public Class classForName(String className, Map context) throws ClassNotFoundException {
         Object root = Ognl.getRoot(context);
 
+        if (disallowCustomOgnlMap) {
+            String nodeClassName = ((OgnlContext) context).getCurrentNode().getClass().getName();
+            if ("ognl.ASTMap".equals(nodeClassName)) {
+                LOG.error("Constructing OGNL ASTMap's from custom classes is forbidden. Attempted class: {}", className);
+                return null;
+            }
+        }
+
         try {
             if (root instanceof CompoundRoot) {
                 if (className.startsWith("vs")) {

diff --git a/core/src/main/java/org/apache/struts2/StrutsConstants.java b/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_DISALLOW_CUSTOM_OGNL_MAP = "struts.ognl.disallowCustomOgnlMap";
+
     public static final String STRUTS_MEMBER_ACCESS = "struts.securityMemberAccess";
 
     public static final String STRUTS_OGNL_GUARD = "struts.ognlGuard";

diff --git a/core/src/main/resources/struts-beans.xml b/core/src/main/resources/struts-beans.xml
@@ -174,6 +174,9 @@
     <bean type="com.opensymphony.xwork2.util.TextParser" name="struts"
           class="com.opensymphony.xwork2.util.OgnlTextParser" scope="singleton"/>
 
+    <bean type="ognl.ClassResolver" name="com.opensymphony.xwork2.util.CompoundRoot"
+          class="com.opensymphony.xwork2.ognl.accessor.CompoundRootAccessor"/>
+
     <bean type="ognl.PropertyAccessor" name="com.opensymphony.xwork2.util.CompoundRoot"
           class="com.opensymphony.xwork2.ognl.accessor.CompoundRootAccessor"/>
     <bean type="ognl.PropertyAccessor" name="java.lang.Object"

diff --git a/core/src/test/java/com/opensymphony/xwork2/ognl/MyCustomMap.java b/core/src/test/java/com/opensymphony/xwork2/ognl/MyCustomMap.java
@@ -0,0 +1,28 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *  http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package com.opensymphony.xwork2.ognl;
+
+import java.util.HashMap;
+
+public class MyCustomMap<K, V> extends HashMap<K, V> {
+    @Override
+    public V get(Object key) {
+        return (V) "System compromised";
+    }
+}
diff --git a/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilTest.java b/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilTest.java
@@ -26,6 +26,7 @@
 import com.opensymphony.xwork2.conversion.impl.XWorkConverter;
 import com.opensymphony.xwork2.inject.ContainerBuilder;
 import com.opensymphony.xwork2.interceptor.ChainingInterceptor;
+import com.opensymphony.xwork2.ognl.accessor.CompoundRootAccessor;
 import com.opensymphony.xwork2.test.StubConfigurationProvider;
 import com.opensymphony.xwork2.test.User;
 import com.opensymphony.xwork2.util.Bar;
@@ -35,6 +36,7 @@
 import com.opensymphony.xwork2.util.ValueStack;
 import com.opensymphony.xwork2.util.location.LocatableProperties;
 import com.opensymphony.xwork2.util.reflection.ReflectionContextState;
+import ognl.ClassResolver;
 import ognl.InappropriateExpressionException;
 import ognl.MethodFailedException;
 import ognl.NoSuchPropertyException;
@@ -1623,6 +1625,16 @@ public void testOgnlDefaultCacheFactoryCoverage() {
         assertEquals("Eviction limit for cache mismatches limit for factory ?", 15, ognlCache.getEvictionLimit());
     }
 
+    public void testCustomOgnlMapBlocked() throws Exception {
+        String vulnerableExpr = "#@com.opensymphony.xwork2.ognl.MyCustomMap@{}.get(\"ye\")";
+        assertEquals("System compromised", ognlUtil.getValue(vulnerableExpr, ognlUtil.createDefaultContext(null), null));
+
+        ((CompoundRootAccessor) container.getInstance(ClassResolver.class, CompoundRoot.class.getName()))
+                .useDisallowCustomOgnlMap(Boolean.TRUE.toString());
+
+        assertThrows(OgnlException.class, () -> ognlUtil.getValue(vulnerableExpr, ognlUtil.createDefaultContext(null), null));
+    }
+
     /**
      * Generate a new OgnlUtil instance (not configured by the {@link ContainerBuilder}) that can be used for
      * basic tests, with its Expression and BeanInfo factories set to LRU mode.