Commits

Anonymous committed 97fc0d5

Merge fixes for WW-2761 and XW-641 (Parameter Interceptor vulnerabilities fix)

git-svn-id: http://svn.opensymphony.com/svn/xwork/branches/2.0@1857e221344d-f017-0410-9bd5-d282ab1896d7

Comments (0)

Files changed (7)

src/java/com/opensymphony/xwork2/interceptor/ParametersInterceptor.java

     private static final Log LOG = LogFactory.getLog(ParametersInterceptor.class);
 
     boolean ordered = false;
-    Set<Pattern> excludeParams = Collections.EMPTY_SET;
-    Set<Pattern> acceptedParams = Collections.EMPTY_SET;
+    Set<Pattern> excludeParams = Collections.emptySet();
+    Set<Pattern> acceptParams = Collections.emptySet();
+    static boolean devMode = false;
 
     private String acceptedParamNames = "[\\p{Graph}&&[^,#:=]]*";
     private Pattern acceptedPattern = Pattern.compile(acceptedParamNames);
 
-    static boolean devMode = false;
-
     @Inject(value = "devMode", required = false)
     public static void setDevMode(String mode) {
         devMode = "true".equals(mode);
     }
 
-    public void setAcceptedParamNames(String commaDelim) {
+    public void setAcceptParamNames(String commaDelim) {
         Collection<String> acceptPatterns = asCollection(commaDelim);
         if (acceptPatterns != null) {
-            acceptedParams = new HashSet<Pattern>();
+            acceptParams = new HashSet<Pattern>();
             for (String pattern : acceptPatterns) {
-                acceptedParams.add(Pattern.compile(pattern));
+                acceptParams.add(Pattern.compile(pattern));
             }
         }
     }
             return l1 < l2 ? -1 : (l2 < l1 ? 1 : s1.compareTo(s2));
         }
 
-        ;
     };
 
     public String doIntercept(ActionInvocation invocation) throws Exception {
             params = new TreeMap(parameters);
         }
 
+        ValueStack newStack = ValueStackFactory.getFactory().createValueStack(stack);
+        if (newStack instanceof ClearableValueStack) {
+            //if the stack's context can be cleared, do that to prevent OGNL
+            //from having access to objects in the stack, see XW-641
+            ((ClearableValueStack)newStack).clearContextValues();
+            Map<String, Object> context = newStack.getContext();
+            OgnlContextState.setCreatingNullObjects(context, true);
+            OgnlContextState.setDenyMethodExecution(context, true);
+            OgnlContextState.setReportingConversionErrors(context, true);
+        }
+
+        boolean memberAccessStack = newStack instanceof MemberAccessValueStack;
+        if (memberAccessStack) {
+            //block or allow access to properties
+            //see WW-2761 for more details
+            MemberAccessValueStack accessValueStack = (MemberAccessValueStack) newStack;
+            accessValueStack.setAcceptProperties(acceptParams);
+            accessValueStack.setExcludeProperties(excludeParams);
+        }
+
         for (Iterator iterator = params.entrySet().iterator(); iterator.hasNext();) {
             Map.Entry entry = (Map.Entry) iterator.next();
             String name = entry.getKey().toString();
             if (acceptableName) {
                 Object value = entry.getValue();
                 try {
-                    stack.setValue(name, value);
+                    newStack.setValue(name, value);
                 } catch (RuntimeException e) {
                     if (devMode) {
                         String developerNotification = LocalizedTextUtil.findText(ParametersInterceptor.class, "devmode.notification", ActionContext.getContext().getLocale(), "Developer Notification:\n{0}", new Object[]{
     }
 
     protected boolean isAccepted(String paramName) {
-        if (!this.acceptedParams.isEmpty()) {
-            for (Pattern pattern : acceptedParams) {
+        if (!this.acceptParams.isEmpty()) {
+            for (Pattern pattern : acceptParams) {
                 Matcher matcher = pattern.matcher(paramName);
-                if (!matcher.matches()) {
-                    return false;
+                if (matcher.matches()) {
+                    return true;
                 }
             }
         }

src/java/com/opensymphony/xwork2/util/ClearableValueStack.java

+/*
+ * $Id$
+ *
+ * Copyright 2003-2004 The Apache Software Foundation.
+ *
+ * Licensed 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.util;
+
+/**
+ * ValueStacks implementing this interface provide a way to remove values from
+ * their contexts.
+ */
+public interface ClearableValueStack {
+    /**
+     * Remove all values from the context
+     */
+    void clearContextValues();
+}

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

+package com.opensymphony.xwork2.util;
+
+import java.util.Set;
+import java.util.regex.Pattern;
+
+/**
+ * ValueStacks implementing this interface provide a way to remove block or allow access
+ * to properties using regular expressions
+ */
+public interface MemberAccessValueStack {
+    void setExcludeProperties(Set<Pattern> excludeProperties);
+
+    void setAcceptProperties(Set<Pattern> acceptedProperties);
+}

src/java/com/opensymphony/xwork2/util/OgnlValueStack.java

 import java.util.List;
 import java.util.Map;
 import java.util.Set;
+import java.util.regex.Pattern;
 
 import ognl.ObjectPropertyAccessor;
 import ognl.Ognl;
  *
  * @author Patrick Lightbody
  * @author tm_jee
- * 
+ *
  * @version $Date$ $Id$
  */
-public class OgnlValueStack implements Serializable, ValueStack {
-	
+public class OgnlValueStack implements Serializable, ValueStack, ClearableValueStack, MemberAccessValueStack {
+
 	private static final long serialVersionUID = 370737852934925530L;
-	
+
 	private static CompoundRootAccessor accessor;
     private static Log LOG = LogFactory.getLog(OgnlValueStack.class);
     private static boolean devMode;
     Class defaultType;
     Map overrides;
 
+    transient SecurityMemberAccess securityMemberAccess;
 
     public OgnlValueStack() {
         setRoot(new CompoundRoot());
     public static CompoundRootAccessor getAccessor() {
         return accessor;
     }
-    
+
     @Inject(value = "devMode", required = false)
     public static void setDevMode(String mode) {
         devMode = "true".equals(mode);
     		this.overrides.putAll(overrides);
     	}
     }
-    
+
     /* (non-Javadoc)
      * @see com.opensymphony.xwork2.util.ValueStack#getExprOverrides()
      */
      */
     public void set(String key, Object o) {
     	//set basically is backed by a Map
-    	//pushed on the stack with a key 
-    	//being put on the map and the 
+    	//pushed on the stack with a key
+    	//being put on the map and the
     	//Object being the value
-    	
+
     	Map setMap=null;
-    	
-    	//check if this is a Map 
+
+    	//check if this is a Map
     	//put on the stack  for setting
     	//if so just use the old map (reduces waste)
     	Object topObj=peek();
-    	if (topObj instanceof Map 
+    	if (topObj instanceof Map
     			&&((Map)topObj).get(MAP_IDENTIFIER_KEY)!=null) {
-    		
+
     		setMap=(Map)topObj;
     	}	else {
     		setMap=new HashMap();
     		push(setMap);
     	}
     	setMap.put(key,o);
-    	
+
     }
-    
-    
+
+
     private static final String MAP_IDENTIFIER_KEY="com.opensymphony.xwork2.util.OgnlValueStack.MAP_IDENTIFIER_KEY";
-    
+
     /* (non-Javadoc)
      * @see com.opensymphony.xwork2.util.ValueStack#size()
      */
 
     private void setRoot(CompoundRoot compoundRoot) {
         this.root = compoundRoot;
+        this.securityMemberAccess =  new SecurityMemberAccess(allowStaticMethodAccess);
         this.context = Ognl.createDefaultContext(this.root, accessor, XWorkConverter.getInstance(),
-                new StaticMemberAccess(allowStaticMethodAccess));
+                this.securityMemberAccess);
         context.put(VALUE_STACK, this);
         Ognl.setClassResolver(context, accessor);
         ((OgnlContext) context).setTraceEvaluations(false);
 
         return aStack;
     }
+
+    public void clearContextValues() {
+        //this is an OGNL ValueStack so the context will be an OgnlContext
+        //it would be better to make context of type OgnlContext
+       ((OgnlContext)context).getValues().clear();
+    }
+
+    public void setAcceptProperties(Set<Pattern> acceptedProperties) {
+        securityMemberAccess.setAcceptProperties(acceptedProperties);
+    }
+
+    public void setExcludeProperties(Set<Pattern> excludeProperties) {
+       securityMemberAccess.setExcludeProperties(excludeProperties);
+    }
 }

src/java/com/opensymphony/xwork2/util/SecurityMemberAccess.java

+/*
+ * Copyright (c) 2002-2006 by OpenSymphony
+ * All rights reserved.
+ */
+package com.opensymphony.xwork2.util;
+
+import ognl.DefaultMemberAccess;
+
+import java.lang.reflect.Member;
+import java.lang.reflect.Method;
+import java.lang.reflect.Modifier;
+import java.util.Collections;
+import java.util.Map;
+import java.util.Set;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+/**
+ * Allows access decisions to be made on the basis of whether a member is static or not.
+ * Also blocks or allows access to properties.
+ */
+public class SecurityMemberAccess extends DefaultMemberAccess {
+
+    private boolean allowStaticMethodAccess;
+    Set<Pattern> excludeProperties = Collections.emptySet();
+    Set<Pattern> acceptProperties = Collections.emptySet();
+
+    public SecurityMemberAccess(boolean method) {
+        super(false);
+        allowStaticMethodAccess = method;
+    }
+
+    public boolean getAllowStaticMethodAccess() {
+        return allowStaticMethodAccess;
+    }
+
+    public void setAllowStaticMethodAccess(boolean allowStaticMethodAccess) {
+        this.allowStaticMethodAccess = allowStaticMethodAccess;
+    }
+
+    @Override
+    public boolean isAccessible(Map context, Object target, Member member,
+                                String propertyName) {
+
+        boolean allow = true;
+        int modifiers = member.getModifiers();
+        if (Modifier.isStatic(modifiers)) {
+            if (member instanceof Method && !getAllowStaticMethodAccess()) {
+                allow = false;
+                if (target instanceof Class) {
+                    Class clazz = (Class) target;
+                    Method method = (Method) member;
+                    if (Enum.class.isAssignableFrom(clazz) && method.getName().equals("values"))
+                        allow = true;
+                }
+            }
+        }
+
+        //failed static test
+        if (!allow)
+            return false;
+
+        // Now check for standard scope rules
+        if (!super.isAccessible(context, target, member, propertyName))
+            return false;
+
+        return isAcceptableProperty(propertyName);
+    }
+
+    protected boolean isAcceptableProperty(String name) {
+        if (isAccepted(name) && !isExcluded(name)) {
+            return true;
+        }
+        return false;
+    }
+
+    protected boolean isAccepted(String paramName) {
+        if (!this.acceptProperties.isEmpty()) {
+            for (Pattern pattern : acceptProperties) {
+                Matcher matcher = pattern.matcher(paramName);
+                if (matcher.matches()) {
+                    return true;
+                }
+            }
+
+            //no match, but acceptedParams is not empty
+            return false;
+        }
+
+        //empty acceptedParams
+        return true;
+    }
+
+    protected boolean isExcluded(String paramName) {
+        if (!this.excludeProperties.isEmpty()) {
+            for (Pattern pattern : excludeProperties) {
+                Matcher matcher = pattern.matcher(paramName);
+                if (matcher.matches()) {
+                    return true;
+                }
+            }
+        }
+        return false;
+    }
+
+    public void setExcludeProperties(Set<Pattern> excludeProperties) {
+        this.excludeProperties = excludeProperties;
+    }
+
+    public void setAcceptProperties(Set<Pattern> acceptedProperties) {
+        this.acceptProperties = acceptedProperties;
+    }
+}

src/java/com/opensymphony/xwork2/util/StaticMemberAccess.java

-/*
- * Copyright (c) 2002-2006 by OpenSymphony
- * All rights reserved.
- */
-package com.opensymphony.xwork2.util;
-
-import java.lang.reflect.Member;
-import java.lang.reflect.Method;
-import java.lang.reflect.Modifier;
-import java.util.Map;
-
-import ognl.DefaultMemberAccess;
-
-/**
- * Allows access decisions to be made on the basis of whether a member is static or not
- */
-public class StaticMemberAccess extends DefaultMemberAccess {
-
-    private boolean allowStaticMethodAccess;
-    
-    public StaticMemberAccess(boolean method) {
-        super(false);
-        allowStaticMethodAccess = method;
-    }
-
-    public boolean getAllowStaticMethodAccess() {
-        return allowStaticMethodAccess;
-    }
-
-    public void setAllowStaticMethodAccess(boolean allowStaticMethodAccess) {
-        this.allowStaticMethodAccess = allowStaticMethodAccess;
-    }
-
-    @Override
-    public boolean isAccessible(Map context, Object target, Member member,
-            String propertyName) {
-        
-        boolean allow = true;
-        int modifiers = member.getModifiers();
-        if (Modifier.isStatic(modifiers)) {
-            if (member instanceof Method && !getAllowStaticMethodAccess()) {
-                allow = false;
-            }
-        }
-        
-        // Now check for standard scope rules
-        if (allow) {
-            return super.isAccessible(context, target, member, propertyName);
-        }
-        
-        return false;
-    }
-    
-    
-    
-    
-
-}

src/test/com/opensymphony/xwork2/interceptor/ParametersInterceptorTest.java

  */
 package com.opensymphony.xwork2.interceptor;
 
-import com.opensymphony.xwork2.*;
-import com.opensymphony.xwork2.config.ConfigurationManager;
+import java.util.HashMap;
+import java.util.LinkedHashMap;
+import java.util.Map;
+
+import ognl.PropertyAccessor;
+
+import com.opensymphony.xwork2.Action;
+import com.opensymphony.xwork2.ActionContext;
+import com.opensymphony.xwork2.ActionProxy;
+import com.opensymphony.xwork2.ModelDrivenAction;
+import com.opensymphony.xwork2.SimpleAction;
+import com.opensymphony.xwork2.TestBean;
+import com.opensymphony.xwork2.TextProvider;
+import com.opensymphony.xwork2.XWorkTestCase;
+import com.opensymphony.xwork2.config.entities.ActionConfig;
 import com.opensymphony.xwork2.config.providers.MockConfigurationProvider;
 import com.opensymphony.xwork2.config.providers.XmlConfigurationProvider;
 import com.opensymphony.xwork2.mock.MockActionInvocation;
+import com.opensymphony.xwork2.util.CompoundRoot;
+import com.opensymphony.xwork2.util.CompoundRootAccessor;
 import com.opensymphony.xwork2.util.OgnlValueStack;
+import com.opensymphony.xwork2.util.OgnlValueStackFactory;
 import com.opensymphony.xwork2.util.ValueStack;
-
-import java.util.HashMap;
-import java.util.Map;
+import com.opensymphony.xwork2.util.ValueStackFactory;
+import com.opensymphony.xwork2.util.XWorkConverter;
 
 
 /**
 
     public void testParameterNameAware() {
         ParametersInterceptor pi = new ParametersInterceptor();
+        container.inject(pi);
         final Map actual = new HashMap();
-        ValueStack stack = new OgnlValueStack() {
+        final ValueStack stack = new OgnlValueStack() {
             public void setValue(String expr, Object value) {
                 actual.put(expr, value);
             }
         };
+        ValueStackFactory.setFactory(new ValueStackFactory() {
+
+            @Override
+            public ValueStack createValueStack() {
+                return stack;
+            }
+
+            @Override
+            public ValueStack createValueStack(ValueStack stack) {
+                return stack;
+            }
+
+        });
         final Map expected = new HashMap() {
             {
                 put("fooKey", "fooValue");
         };
         pi.setParameters(a, stack, parameters);
         assertEquals(expected, actual);
+
+        ValueStackFactory.setFactory(new OgnlValueStackFactory());
     }
 
     public void testDoesNotAllowMethodInvocations() throws Exception {
         params.put("blah", "This is blah");
         params.put("#session.foo", "Foo");
         params.put("\u0023session[\'user\']", "0wn3d");
+        params.put("\\u0023session[\'user\']", "0wn3d");
         params.put("\u0023session.user2", "0wn3d");
+        params.put("\\u0023session.user2", "0wn3d");
         params.put("('\u0023'%20%2b%20'session[\'user3\']')(unused)", "0wn3d");
         params.put("('\\u0023' + 'session[\\'user4\\']')(unused)", "0wn3d");
 
         assertEquals("This is blah", ((SimpleAction) proxy.getAction()).getBlah());
     }
 
+    public void testExcludedTrickyParameters() throws Exception {
+        Map<String, Object> params = new HashMap<String, Object>() {
+            {
+                put("blah", "This is blah");
+                put("name", "try_1");
+                put("(name)", "try_2");
+                put("['name']", "try_3");
+                put("['na' + 'me']", "try_4");
+                put("{name}[0]", "try_5");
+                put("(new string{'name'})[0]", "try_6");
+                put("#{key: 'name'}.key", "try_7");
+
+            }
+        };
+
+        HashMap<String, Object> extraContext = new HashMap<String, Object>();
+        extraContext.put(ActionContext.PARAMETERS, params);
+
+        ActionProxy proxy = actionProxyFactory.createActionProxy("", MockConfigurationProvider.PARAM_INTERCEPTOR_ACTION_NAME, extraContext);
+
+        ActionConfig config = configuration.getRuntimeConfiguration().getActionConfig("", MockConfigurationProvider.PARAM_INTERCEPTOR_ACTION_NAME);
+        ParametersInterceptor pi =(ParametersInterceptor) config.getInterceptors().get(0).getInterceptor();
+        pi.setExcludeParams("name");
+
+        proxy.execute();
+
+        SimpleAction action = (SimpleAction) proxy.getAction();
+        assertNull(action.getName());
+        assertEquals("This is blah", (action).getBlah());
+    }
+
+    public void testAcceptedTrickyParameters() throws Exception {
+        Map<String, Object> params = new HashMap<String, Object>() {
+            {
+                put("blah", "This is blah");
+                put("['baz']", "123");
+                put("name", "try_1");
+                put("(name)", "try_2");
+                put("['name']", "try_3");
+                put("['na' + 'me']", "try_4");
+                put("{name}[0]", "try_5");
+                put("(new string{'name'})[0]", "try_6");
+                put("#{key: 'name'}.key", "try_7");
+            }
+        };
+
+        HashMap<String, Object> extraContext = new HashMap<String, Object>();
+        extraContext.put(ActionContext.PARAMETERS, params);
+
+        ActionProxy proxy = actionProxyFactory.createActionProxy("", MockConfigurationProvider.PARAM_INTERCEPTOR_ACTION_NAME, extraContext);
+
+        ActionConfig config = configuration.getRuntimeConfiguration().getActionConfig("", MockConfigurationProvider.PARAM_INTERCEPTOR_ACTION_NAME);
+        ParametersInterceptor pi =(ParametersInterceptor) config.getInterceptors().get(0).getInterceptor();
+        pi.setAcceptParamNames("blah, baz");
+
+        proxy.execute();
+
+        SimpleAction action = (SimpleAction) proxy.getAction();
+        assertNull(action.getName());
+        assertEquals("This is blah", (action).getBlah());
+         assertEquals(123, action.getBaz());
+    }
+
     public void testNonexistentParametersGetLoggedInDevMode() throws Exception {
         Map params = new HashMap();
         params.put("not_a_property", "There is no action property named like this");
         OgnlValueStack.setDevMode("true");
         ParametersInterceptor.setDevMode("true");
 
+        ActionConfig config = configuration.getRuntimeConfiguration().getActionConfig("", MockConfigurationProvider.PARAM_INTERCEPTOR_ACTION_NAME);
+        container.inject(config.getInterceptors().get(0).getInterceptor());
         ActionProxy proxy = actionProxyFactory.createActionProxy("", MockConfigurationProvider.PARAM_INTERCEPTOR_ACTION_NAME, extraContext);
         proxy.execute();
         final String actionMessage = "" + ((SimpleAction) proxy.getAction()).getActionMessages().toArray()[0];
         extraContext.put(ActionContext.PARAMETERS, params);
         OgnlValueStack.setDevMode("false");
 
+        ActionConfig config = configuration.getRuntimeConfiguration().getActionConfig("", MockConfigurationProvider.PARAM_INTERCEPTOR_ACTION_NAME);
+        container.inject(config.getInterceptors().get(0).getInterceptor());
         ActionProxy proxy = actionProxyFactory.createActionProxy("", MockConfigurationProvider.PARAM_INTERCEPTOR_ACTION_NAME, extraContext);
         proxy.execute();
         assertTrue(((SimpleAction) proxy.getAction()).getActionMessages().isEmpty());
         interceptor.doIntercept(mai);
         interceptor.destroy();
     }
-    
+
     public void testExcludedParametersAreIgnored() throws Exception {
         ParametersInterceptor pi = new ParametersInterceptor();
         pi.setExcludeParams("dojo\\..*");
         final Map actual = new HashMap();
-        ValueStack stack = new OgnlValueStack() {
+        final ValueStack stack = new OgnlValueStack() {
             public void setValue(String expr, Object value) {
                 actual.put(expr, value);
             }
         };
+        ValueStackFactory.setFactory(new ValueStackFactory() {
+
+            @Override
+            public ValueStack createValueStack() {
+                return stack;
+            }
+
+            @Override
+            public ValueStack createValueStack(ValueStack stack) {
+                return stack;
+            }
+
+        });
+
         final Map expected = new HashMap() {
             {
                 put("fooKey", "fooValue");
         };
         pi.setParameters(new NoParametersAction(), stack, parameters);
         assertEquals(expected, actual);
+
+        ValueStackFactory.setFactory(new OgnlValueStackFactory());
+    }
+
+    private ValueStack createStubValueStack(final Map<String, Object> actual) {
+        ValueStack stack = new OgnlValueStack() {
+            @Override
+            public void setValue(String expr, Object value) {
+                actual.put(expr, value);
+            }
+        };
+        container.inject(stack);
+        return stack;
     }
-    
+
     /*
     public void testIndexedParameters() throws Exception {
         Map params = new HashMap();
Tip: Filter by directory path e.g. /media app.js to search for public/media/app.js.
Tip: Use camelCasing e.g. ProjME to search for ProjectModifiedEvent.java.
Tip: Filter by extension type e.g. /repo .js to search for all .js files in the /repo directory.
Tip: Separate your search with spaces e.g. /ssh pom.xml to search for src/ssh/pom.xml.
Tip: Use ↑ and ↓ arrow keys to navigate and return to view the file.
Tip: You can also navigate files with Ctrl+j (next) and Ctrl+k (previous) and view the file with Ctrl+o.
Tip: You can also navigate files with Alt+j (next) and Alt+k (previous) and view the file with Alt+o.