|
History
| Log In
|
|
|
|
CPU usage is probably related to the garbage collector continually executing.
You have to enable gc log to see this. some simple testing shows that the field value is simply evaluated...
try to put on a struts textfield %{1+1} submit and you'll get "2" on the field... Cool but don't think it should be the default behaviour. What constructs can trigger recursion ? Unfortunately beside being DOS it is indeed a remote exploit. You have an access to whole stack and actually if you know OGNL well enough you can easily do almost anything. Just tested reading a file from file system, it worked:(
Not so fast :). Use Struts2.diff and xwork2.diff
IMHO if we are going to go with this solution it should not be optional but mandatory. In other words Struts should always exclude values matching this pattern. Else someone will upgrade, or define his/her own stack and forget this one parameter, making his/her application vulnerable.
Since there is no escape string for OGNL, how about escaping the strings that are inside expression: %{exprStr} would be converted to %{'exprStr'} then at least exprStr would be set as a value instead of erasing it. What if they submit %{foo %{bar} } ? The escaped string won't be valid for OGNL.
You are right, stack.findValue for "foo %{'bar'}" will return null, which doesn't give us much - same effect as removing the value in the interceptor. So in the long run there is a need for escape character.
I confirm that using <s:textfield name="xxx" value=""/> if you enter %{xxx} as the field value on the browser the infinite loop is not triggered BUT the expression is still evaluated (i.e. %{1+1} gives 2).
To me it seems that there are TWO different problem. One related to the infinite loop (and DoS), the other is arbitrary remote execution with servlet container privileges. I haven't examinated the source code carefully but I think that there must be two different solutions One should prevent ANY future infinite loop using a loop counter or something else to break out of the loop at a predefined level of expression complexity. This limit somewhat the expressions you can use but at least we haven't a possible cause of infinite loop for any cause in the future. Just write a log line if the limit is reached and/or let the limit value be configurable. The other solution should let us use the value parameter as in jsp EL. Here we are talking of two different things. One is specifing a value as a parameter to a tag. The value is passed to the tag class using setter methods so in the tag WE KNOW that the value is passed by the programmer in the jsp source code. In this case we can and we should keep the evaluation. The other is when a value is passed to an action by means of a HTTP parameter. In this case the evaluation should be turned off. I am correct ? For the second of the two problems, a simple test shows that the parameter value is passed "untouched" to the action class. So if <s:textfield name="xxx"/> and action has parameter xxx, if the user writes %{1+1} the string xxx into the action class contains %{1+1}. Sorry if it's obvious to you struts experts :)
So the evaluation that gives us a "2" in the textfield is run on the "view" part. So I think we should be able to tell the view tag if the value expression must be evaluated recursively or not. So <s:textfield name="xxx"/> that is "equivalent" to <s:textfield name="xxx" value="%{xxx}"/> when xxx is equal to "%{1+1}" should give us %{1+1} when NOT recursively evaluated or "2" when recursively evaluated. We can use two different parameters (like value and valueeval). Or request recursive evaluation with a parameter receval="true". We should also have a new param on com/opensymphony/xwork2/util/TextParseUtil.translateVariables to specify if we want recursive evaluation or not. I don't have all the tools and knowledge needed to try to patch myself and recompile xwork but if someone can try I think this can be a good solution. Please see translateVariable.txt
This seems to work. Don't know if it breaks anything :) It needs to be integrated with logging and maxLoopCount parameter handling. If maxLoopCount is set to 1 the infinite loop DoS is prevented AND expression evaluation is prevented. This is the default if the "old" method is called. If maxLoopCount>1 expression evaluation is on for maxLoopCount levels but the DoS is prevented. Now we need to call the correct method IF we want the recoursive evaluation (i.e. when the value is specified in the jsp source code). We should be able to specify the maxLoopCount in a configuration parameter. sorry
the 2 is correct because on the 1 there is a "debug" line. Andrea,
please attach the patch in diff format, so it can be applied easily. And remember to check the box about confirming the use of your patch under the ASL. Antonio Antonio, I haven't make a diff file because the attachment is only a proof of concept. It needs to be slightly modified for logging at least.
Just realized that my patch will break calls to TextParseUtil.translateVariables that have more than one %{something} at the same level.
For example it does not work when expression is "%{foo} %{bar}" since if foo doesn't eval to another %{something} string, when it's time to eval %{bar} we are on the second loop. I'll try to fix it splitting the expression and processing %{something} that are at the same level in the same main loop iteration. Does anyone know how to LOG in xwork ? I haven't find a line of loggin.... but I think it would be nice to be able to see that an expression was not processed because of the recursive limitation. Thanks I'm a little confused by all the attachments to this issue. Which ones (if any) will fix the DOS/remote exploit problem? Thanks!
Struts2.diff and Xwork2.diff will prevent parameters with values like %{...} to be set/evaluated
use struts2 and xwork2.
Users can't input something like %{blablabla} on your forms but it should be a minor problem... They solve the DOS and the remote exploit. could someone attach a patched struts2-core-2.0.8.1.jar please for the laziest of us (including myself)?
+1 on releasing an a 2.0.8.1 version (but not publishing here, it's not possible).
I think that many websites are in real danger, a quick fix MUST be released ASAP. The patch is on xwork-2.0.3.jar
The struts patch is very simple (just a couple of lines on struts.xml) If it's ok I can attach the patched xwork.jar here Are you sure? It has been released in may... Sorry
I wanna say that the patch is against xwork NOT struts. So I think that the xwork guys must patch and distribute the new version. We can't simply have a new struts-core.jar... If it's ok I can attach here the patched xwork.jar version I'm using. I downloaded xwork-2.0.3, patched and built. The problem is that your JAR will not be signed (currently you are not an official Struts committer, though I wish you will :-) ) and has not passed under a vote and an official release. Rainer told me on IRC yesterday that the patch seemed ok to him and he would probably make a release in a few days
Changed title and description to be clearer.
I believe the solution is use Andrea's solution and basically turn off recursive ognl parsing unless explicitly allowed. From my tests, this seems to resolve all known issues and should fix a few other holes, like when a l18n message uses an ognl expression:
The name needs at least %{minSize} characters and a malicious form overrides the property: <input type="hidden" name="minSize" value="%{@java.lang.System@exit(0)}" /> Most every existing application, at least as far as I can tell using our tests, should be ok, unless they do some crazy stuff that requires such recursion. As these cases become apparent, we'll just add 'recursion' attributes to the appropriate tags and methods, but I think it is better to be secure by default, rather than the other way around. I'm attaching the xwork 2 patch describing the fix I'm committing. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
 |
|
Bug
Resolved
Critical
User input is evaluated as an OGNL expression
In my case, this caused a 100% CPU consumption.
I noticed that it didn't happen if you add the value attribute to the tag. No need to set a value to it, its presence acts like a workaround.
Moreover, you can reproduce this issue with the <s:password> tag.