From 48a82feadad6d0f46b04898bebbe848abf1af164 Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Tue, 5 Dec 2023 12:54:27 +1100 Subject: [PATCH 1/2] WW-5339 Make ClassResolver a bean --- .../xwork2/config/impl/DefaultConfiguration.java | 2 ++ .../java/com/opensymphony/xwork2/ognl/OgnlUtil.java | 9 +++++---- .../xwork2/ognl/OgnlValueStackFactory.java | 13 ++++++------- core/src/main/resources/struts-beans.xml | 3 +++ 4 files changed, 16 insertions(+), 11 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 index 2d2a4a2b1f..4a6ee1373e 100644 --- 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 index 18a73c47ac..c4ee4b655b 100644 --- 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 createDefaultContext(Object root) { return createDefaultContext(root, null); } - protected Map createDefaultContext(Object root, ClassResolver classResolver) { - ClassResolver resolver = classResolver; + protected Map 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 index 111a44d793..66798c0349 100644 --- 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/resources/struts-beans.xml b/core/src/main/resources/struts-beans.xml index 8c751c0c19..1238c9e056 100644 --- a/core/src/main/resources/struts-beans.xml +++ b/core/src/main/resources/struts-beans.xml @@ -174,6 +174,9 @@ + + Date: Tue, 5 Dec 2023 13:06:33 +1100 Subject: [PATCH 2/2] WW-5339 Add option to block custom OGNL maps --- .../ognl/accessor/CompoundRootAccessor.java | 15 +++++++++- .../org/apache/struts2/StrutsConstants.java | 2 ++ .../opensymphony/xwork2/ognl/MyCustomMap.java | 28 +++++++++++++++++++ .../xwork2/ognl/OgnlUtilTest.java | 12 ++++++++ 4 files changed, 56 insertions(+), 1 deletion(-) create mode 100644 core/src/test/java/com/opensymphony/xwork2/ognl/MyCustomMap.java 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 index 25bedba661..4600c7c97c 100644 --- 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 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 index f5fe67a50d..1dc891ee24 100644 --- 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/test/java/com/opensymphony/xwork2/ognl/MyCustomMap.java b/core/src/test/java/com/opensymphony/xwork2/ognl/MyCustomMap.java new file mode 100644 index 0000000000..ef56833868 --- /dev/null +++ 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 extends HashMap { + @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 index 5df410e2bc..05e3408fe8 100644 --- 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.