|
[
Permlink
| « Hide
]
Rainer Hermanns added a comment - 12/Jun/08 05:34 AM
fixed in trunk and 2.0 branch
I believe the test case is flawed and the vulnerability still exists.
Consider testParametersDoesNotAffectSession() in ParametersInterceptorTest.java. Some backslashes in string literals have not been escaped properly. For instance, params.put("\u0023sess... should rather be params.put("\\u0023sess... After these minor modifications, the test case actually fails because the session object can still be modified. (trunk, rev. 1850) We have to check this, I think ...
Just an idea, why don't we create an empty value stack, where the parameters are set, acting like a black box. After that, we copy from that stack to the real stack only parameters that do not contain "keywords", like "session", "context", "params" and others. I think this way we could avoid all kind of trickery with OGNL notation.
I'd like to suggest something else: Restricting OGNL. After all, the OGNL interpreter definitely knows which objects will be accessed. It should be easier (and less error-prone) to check access permissions *after* the interpreter has evaluated parameter names.
This requires two different flavors of OGNL: "Result Page OGNL" and "Parameter OGNL." The former remains as-is; Only the latter includes additional verification, e.g. something like this (pseudo code): class ParameterMethodAccessor implements ObjectMethodAccessor { void callSomething(...) { fail("May not call anything."); } } class ParameterPropertyAccessor implements ObjectPropertyAccessor { Object getProperty(...) { if(!isAccessAllowed(...)) fail("May not touch this.") else return reallyGetProperty(...); } void setProperty(...) { if(!isAccessAllowed(...)) fail("May not touch this.") else reallySetProperty(...); } boolean isAccessAllowed(Object target, String propertyName) { Object action = evaluationContext.getCurrentAction(); if(target == action) { if(getter or setter is part of any Interceptor-Aware interface such as SessionAware from struts2) return false; // Only interceptors should use those else return true; // Allow access to other (probably harmless) action properties } // Allow access to model as well if(action instanceof ModelDriven && target == action.getModel()) return true; // Forbid access to all other objects by default (including session etc.) return false; } } Furthermore, ParameterNameAware should be extended accordingly, so actions can precisely control which objects may or may not be modified. This approach avoids many (all?) kinds of ambiguities. For example, "foo", "model.foo", "myfoo.parent.foreign.foo", "[1-1].foo", "('\u0066oo')([0])" etc. might all refer to the same object. However, this fact is obvious to the OGNL interpreter: In all cases it eventually demands access to property "foo" of some object. On the other hand, it is cumbersome and error-prone to test for every possible case using regular expressions, unless the expression is very simple and restrictive (e.g. only numbers and letters - thereby completely defeating OGNL). What do you think? I see your point, but there are cases when you want to submit a parameter which is never to be bound to the action. In that case the access would be denied right?
Certainly there must be a way to adapt access rights as required. Nevertheless, isAccessAllowed above seems to be a reasonable default.
Note that here "deny" means "OGNL will not automagically update the value referred to by this parameter." It is not necessary to remove or alter the parameters in any way; they simply would be ignored as far as OGNL is concerned. The other problem is that given A.B.C=X as a parameter, if we limit the the allowed targets of setProperty, then OGNL would fail to set B in A. Setting A in the action would succeed. I have been playing with the idea of an empty stack, but that is a no go, as there is a bunch of stuff there that is needed, like type conversion. I started a discussion on @dev to see if we can get rid of OGNL for parameter binding, it creates more problems than it actually solves.
This is not a real patch, but just an idea. The important code is:
Map<String, Object> context = new HashMap<String, Object>(); ReflectionContextState.setCreatingNullObjects(context, true); ReflectionContextState.setDenyMethodExecution(context, true); ReflectionContextState.setReportingConversionErrors(context, true); ognlUtil.setProperties(acceptableParameters, action, context); stack.getContext().putAll(acceptableParameters); OGNL would be used to bind parameters, but on an empty context. One thing that could break is type conversion. The reason why some tests will fail are: 1. They are no injecting the params interceptor (Nullpointer exception when ognlUtil is used) 2. The relay on setValue(x,y) being called on the value stack, instead of checking if those values are in the context never mind my "idea", it breaks way too many things ;)
Code review for a fix here: http://fisheye6.atlassian.com/cru/CR-9/
I committed the changes to xwork trunk, please test it and let me know if you find any problems
2.0.5 does not fix the bug,
when input ('\u0023'%20%2b%20'session[\'user\']')(unused)='123', the parameterInterceptor doesn't accept the parameter as expected, but when use ('\u0023'%2b'session[\'user\']')(unused)='123', the parameterInterceptor accept the parameter and set the session. |
|||||||||||||||||||||||||||||||||||||||||
 |
|
Bug
Resolved
Critical
XWork ParameterInterceptors bypass (OGNL statement execution)