|
Sort Order:
You don't even need an input text field. For example if you use an URL like this: http://.../webpage.ext?value=%23 The EL expression will get evaluated if the next navigation case uses includeViewParams. In that case I suspect your workaround will not work. Don't use includeViewParams before this is fixed. It seems the same problem exists in MyFaces. A workaround could be to attach a String-to-String converter to the view parameters that automatically removes every occurrence of '#{' in the parameter value. To fix this issue UIViewParameter could do something similar in the decode method by default to increase the security of the default behavior.
Sounds like a good first quick fix, since this is the place where dangerous values enter the system. Instead of removing the '#{' characters, escaping might also be an option, for instance '#{myBean.methodThatShouldBeSecure}' could be turned into '#{'#{'}myBean.methodThatShouldBeSecure}', so that the result of evaluating the expression will yield a string. Care must be taken of course that the resulting string is not evaluated again. However, as BalusC identified the problem to be within the URLBuilder, javax.faces.context.ExternalContext#encodeRedirectURL is also vulnerable, since this ultimately uses the same addParameters method. Should a warning be added to this method that values have to be "el-escaped" if el evaluation is undesirable, or should no el evaluation be done at all? Hi guys, I created a little project that shows the vulnerability: http://code.google.com/a/apache-extras.org/p/jsf-includeviewparams-security-hole-example/ Also, I created the following blog entry, describing the problem: http://www.jakobk.com/2011/11/jsf-value-expression-injection-vulnerability/ Since this is a really big security hole and there are a lot of versions of JSF out there, which have it (Mojarra and MyFaces), we should really try to spread the word! Regards, The problem seems to be that Mojarra's implementation of ExternalContext.encodeRedirectURL() is explicitly performing EL evaluation on the values passed in via the "parameters" map. The spec says nothing about this behavior. I believe that it should be the caller's responsibility to perform EL evaluation on parameter values before calling encodeRedirectURL() if EL evaluation is desired.
After giving this some more thought I'd more or less came to the same conclusion. When I simply removed the EL evaluation in .com.sun.faces.context.URLBuilde#addParameters, the test com.sun.faces.application.TestNavigationHandler fails at the following assertion: // ensure redirect parameter el evaluation is performed more than once NavigationCase ncase = cnh.getNavigationCase(getFacesContext(), null, "redirectOutcome3"); String url = getFacesContext().getExternalContext().encodeRedirectURL("/path.xhtml", ncase.getParameters()); System.out.println("URL: " + url); assertTrue(url.contains("param=1")); This reveals why the EL evaluation was needed in the first place for the encodeRedirectURL method, as the navigation case contains this: <navigation-case> <from-outcome>redirectOutcome3</from-outcome> <to-view-id>/page2.xhtml</to-view-id> <redirect> <view-param> <name>param</name> <value>#{navbean.increment}</value> </view-param> </redirect> </navigation-case> @arjan tijms - Great, thank for tracking this down. Looks like the problem is that NavigationHandlerImpl assumes that ViewHandler.getRedirectURL() (which calls ExternalContext.encodeRedirectURL()) will perform EL evaluation of the parameter values that are passed in. This is unspecified/undesired (and as we've seen, unsecured) behavior. The proper solution is to: 1. Remove the EL evaluation code from ExternalContext.encodeRedirectURL().
That's what I was thinking too. javax.faces.application.NavigationCase#getRedirectURL probably has to be done as well. So, for instance something like: NavigationCase public URL getRedirectURL(FacesContext context) throws MalformedURLException { ExternalContext extContext = context.getExternalContext(); return new URL(extContext.getRequestScheme(), extContext.getRequestServerName(), extContext.getRequestServerPort(), context.getApplication().getViewHandler().getRedirectURL(context, getToViewId(context), Util.evaluateExpressions(getParameters()), isIncludeViewParams())); } and NavigationHandlerImpl#handleNavigation if (caseStruct.navCase.isRedirect() || isUIViewActionBroadcastAndViewdsDiffer) { // perform a 302 redirect. String redirectUrl = viewHandler.getRedirectURL(context, caseStruct.viewId, Util.evaluateExpressions(caseStruct.navCase.getParameters()), caseStruct.navCase.isIncludeViewParams()); An initial quick patch that implements the idea discussed here. Evaluation is moved away from the UrlBuilder and into the locations where navigation outcomes are computed. Because of the API/RI distinction, I had to copy paste the evaluation code to NavigationCase, which IMO is not so pretty. Unit tests pass, but I haven't done any other testing or checking whether this is correct. The method isExpression in the attached patch and in com.sun.faces.el.ELUtils seems to contain a duplicate calculation of the start index. It also contains another bug if the tested string contains a closing brace before start of an el expression. This is easily fixable by rewriting it using the two-parameter version of indexOf as follows: public static boolean isExpression(String expression) { if (null == expression) { return false; } //check to see if attribute has an expression int start = expression.indexOf("#{"); return start != -1 && expression.indexOf('}', start+2) != -1; } Some testcases, the last one fails in the current version: Assert.assertTrue(isExpression("#{test}")); Assert.assertTrue(isExpression("abc#{test}")); Assert.assertTrue(isExpression("#{test}abc")); Assert.assertTrue(isExpression("abc#{test}abc")); Assert.assertTrue(isExpression("x}x#{test}")); I would like to suggest an alternate approach. Dan Allen, the original author of this code, was clearly adding a useful feature here, but I think it was exposed too broadly. We want to allow expressions to be evaluated if they originate from the Application Configuration Resources, but not if they originate from the query string or POST data. I will attempt to produce a fix that follows that approach. Do not allow the value of UIViewParam to be an EL Expression http://java.net/jira/browse/JAVASERVERFACES-2247 SECTION: Modified Files
+severe.uiviewparam_value_is_expression=The value of a UIParameter must not be an expression literal. Ignoring expression value {0}. A jsf-tools/template-src/SharedUtils.java
M jsf-api/src/main/java/javax/faces/component/UIViewParameter.java
M jsf-test/build.xml
A jsf-test/
Committed to 2.1 branch: Sending jsf-api/build-pre-maven-rename.xml Ed - I think that your alternate fix misidentifies the source of the problem. This fix assumes that an EL-bound UIViewParameter value must not evaluate to a String that in turn contains an EL expression. While this may be unusual, this is not forbidden, and is not inherently problematic. The real problem is that the result of the UIViewParameter's EL-evaluated value is being run through EL evaluation a second time. This should never happen. This is only happening due to an implementation bug - ie. due to the fact that the implementation of ExternalContext.encodeRedirectURL() happens to implicitly perform EL evaluation on parameter values, even though this is not specified (or desired) behavior. The fix is simple:
I very much recommend that we go with a solution along these lines. Thanks for your analysis, Andy. I will commit another patch that does what you suggest. In the meantime, I have migrated my existing fix to the trunk, for consistency. Sending jsf-api/build-pre-maven-rename.xml Hello Andy, if I do as you suggest, then this case, which the spec does require to work, no longer works. <navigation-rule> I am about to attach a stack trace that shows the necessity of this invocation sequence in light of your suggestion regarding what happens in the implementation of ExternalContext.encodeRedirectUrl(). Hi Ed, Is this issue considered closed? Also, is 2.1.5 considered released? It does not show up on the downloads page but it does show up in this releases repo: Committed to 2.0 branch. Sending jsf-api Re: Stan: yes, 2.1.5 is considered released. I'm behind in updating the page. Re: Andy. I misunderstood your recommendation. I thought you were suggesting we simply don't perform the EL evaluation at all. I can see now that you clearly stated this in your original comment on 23 November. I'll keep this open until I can fix it the way you suggest.
The patch I provided was just a quick shot at the problem, but I think it does exactly this:
Committed to trunk. Relocate EL expression evaluation outside of UrlBuilder http://java.net/jira/browse/JAVASERVERFACES-2247 SECTION: Modified Files
M jsf-ri/test/com/sun/faces/application/TestNavigationHandler.java
M jsf-ri/src/main/java/com/sun/faces/context/UrlBuilder.java
M jsf-ri/src/main/java/com/sun/faces/el/ELUtils.java
M jsf-ri/build.xml
M jsf-api/src/main/java/javax/faces/LogStrings_ko.properties
M jsf-api/src/main/java/javax/faces/component/UIViewParameter.java
A jsf-ri/test/com/sun/faces/application/packagehack
Sending jsf-api/build.xml Committed to trunk. Relocate EL expression evaluation outside of UrlBuilder http://java.net/jira/browse/JAVASERVERFACES-2247 SECTION: Modified Files
Sending jsf-api/build-pre-maven-rename.xml Committed to 2.1 branch. Sending jsf-api/build-pre-maven-rename.xml Committed to 2.0 branch. Sending jsf-api/build-pre-maven-rename.xml |
|||||||||||||||||||||||||||||||||||||||||||||||
Bug
Closed
Major
includeViewParameters re-evaluates param/model values as EL expressions
The "workaround" is by the way to use a normal GET form instead and do the action job in a preRenderView listener:
But this removes the possible benefit of using <f:ajax> in the form to do for example the validations.