From caee86e71ab8c5f038186158e9955887ed72a0fd Mon Sep 17 00:00:00 2001 From: Leonardo Uribe Date: Tue, 22 Nov 2011 22:40:59 +0000 Subject: [PATCH] MYFACES-3405 includeViewParameters re-evaluates param/model values as EL expressions [CVE-2011-4343] --- .../faces/application/NavigationCase.java | 6 +- .../faces/application/_NavigationUtils.java | 107 ++++++++++++++++++ .../application/NavigationHandlerImpl.java | 8 +- .../servlet/ServletExternalContextImpl.java | 33 +----- .../ServletExternalContextImplTest.java | 3 +- .../shared/application/NavigationUtils.java | 107 ++++++++++++++++++ .../renderkit/html/HtmlRendererUtils.java | 5 +- 7 files changed, 231 insertions(+), 38 deletions(-) create mode 100644 api/src/main/java/javax/faces/application/_NavigationUtils.java create mode 100644 shared/src/main/java/org/apache/myfaces/shared/application/NavigationUtils.java diff --git a/api/src/main/java/javax/faces/application/NavigationCase.java b/api/src/main/java/javax/faces/application/NavigationCase.java index e1e827b985..14ffafc963 100644 --- a/api/src/main/java/javax/faces/application/NavigationCase.java +++ b/api/src/main/java/javax/faces/application/NavigationCase.java @@ -137,7 +137,8 @@ public URL getBookmarkableURL(FacesContext context) throws MalformedURLException return new URL(externalContext.getRequestScheme(), externalContext.getRequestServerName(), externalContext.getRequestServerPort(), - context.getApplication().getViewHandler().getBookmarkableURL(context, getToViewId(context), getParameters(), isIncludeViewParams())); + context.getApplication().getViewHandler().getBookmarkableURL(context, getToViewId(context), + _NavigationUtils.getEvaluatedNavigationParameters(getParameters()), isIncludeViewParams())); } public URL getResourceURL(FacesContext context) throws MalformedURLException @@ -153,7 +154,8 @@ public URL getRedirectURL(FacesContext context) throws MalformedURLException return new URL(externalContext.getRequestScheme(), externalContext.getRequestServerName(), externalContext.getRequestServerPort(), - context.getApplication().getViewHandler().getRedirectURL(context, getToViewId(context), getParameters(), isIncludeViewParams())); + context.getApplication().getViewHandler().getRedirectURL(context, getToViewId(context), + _NavigationUtils.getEvaluatedNavigationParameters(getParameters()), isIncludeViewParams())); } public Map> getParameters() diff --git a/api/src/main/java/javax/faces/application/_NavigationUtils.java b/api/src/main/java/javax/faces/application/_NavigationUtils.java new file mode 100644 index 0000000000..da4a24332a --- /dev/null +++ b/api/src/main/java/javax/faces/application/_NavigationUtils.java @@ -0,0 +1,107 @@ +/* + * 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 javax.faces.application; + +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +import javax.faces.context.FacesContext; + +/** + * + * @author Leonardo Uribe + * + */ +class _NavigationUtils +{ + /** + * Evaluate all EL expressions found as parameters and return a map that can be used for + * redirect or render bookmark links + * + * @param parameters parameter map retrieved from NavigationCase.getParameters() + * @return + */ + public static Map > getEvaluatedNavigationParameters(Map > parameters) + { + Map> evaluatedParameters = null; + if (parameters != null && parameters.size() > 0) + { + evaluatedParameters = new HashMap>(); + for (Map.Entry> pair : parameters.entrySet()) + { + boolean containsEL = false; + for (String value : pair.getValue()) + { + if (_isExpression(value)) + { + containsEL = true; + break; + } + } + if (containsEL) + { + evaluatedParameters.put(pair.getKey(), _evaluateValueExpressions(pair.getValue())); + } + else + { + evaluatedParameters.put(pair.getKey(), pair.getValue()); + } + } + } + else + { + evaluatedParameters = parameters; + } + return evaluatedParameters; + } + + /** + * Checks the Strings in the List for EL expressions and evaluates them. + * Note that the returned List will be a copy of the given List, because + * otherwise it will have unwanted side-effects. + * @param values + * @return + */ + private static List _evaluateValueExpressions(List values) + { + // note that we have to create a new List here, because if we + // change any value on the given List, it will be changed in the + // NavigationCase too and the EL expression won't be evaluated again + List target = new ArrayList(values.size()); + FacesContext context = FacesContext.getCurrentInstance(); + for (String value : values) + { + if (_isExpression(value)) + { + // evaluate the ValueExpression + value = context.getApplication().evaluateExpressionGet(context, value, String.class); + } + target.add(value); + } + return target; + } + + private static boolean _isExpression(String text) + { + return text.indexOf("#{") != -1; + } + +} diff --git a/impl/src/main/java/org/apache/myfaces/application/NavigationHandlerImpl.java b/impl/src/main/java/org/apache/myfaces/application/NavigationHandlerImpl.java index b97d42b5a6..b43ee6785a 100755 --- a/impl/src/main/java/org/apache/myfaces/application/NavigationHandlerImpl.java +++ b/impl/src/main/java/org/apache/myfaces/application/NavigationHandlerImpl.java @@ -50,6 +50,7 @@ import org.apache.myfaces.config.RuntimeConfig; import org.apache.myfaces.config.element.NavigationRule; +import org.apache.myfaces.shared.application.NavigationUtils; import org.apache.myfaces.shared.util.HashMapUtils; import org.apache.myfaces.shared.util.StringUtils; import org.apache.myfaces.view.facelets.tag.jsf.PreDisposeViewEvent; @@ -101,7 +102,12 @@ public void handleNavigation(FacesContext facesContext, String fromAction, Strin ExternalContext externalContext = facesContext.getExternalContext(); ViewHandler viewHandler = facesContext.getApplication().getViewHandler(); String toViewId = navigationCase.getToViewId(facesContext); - String redirectPath = viewHandler.getRedirectURL(facesContext, toViewId, navigationCase.getParameters(), navigationCase.isIncludeViewParams()); + + + String redirectPath = viewHandler.getRedirectURL( + facesContext, toViewId, + NavigationUtils.getEvaluatedNavigationParameters(navigationCase.getParameters()) , + navigationCase.isIncludeViewParams()); //Clear ViewMap if we are redirecting to other resource UIViewRoot viewRoot = facesContext.getViewRoot(); diff --git a/impl/src/main/java/org/apache/myfaces/context/servlet/ServletExternalContextImpl.java b/impl/src/main/java/org/apache/myfaces/context/servlet/ServletExternalContextImpl.java index 015fd8602d..f215ddedcf 100755 --- a/impl/src/main/java/org/apache/myfaces/context/servlet/ServletExternalContextImpl.java +++ b/impl/src/main/java/org/apache/myfaces/context/servlet/ServletExternalContextImpl.java @@ -765,7 +765,7 @@ private String encodeURL(String baseUrl, Map> parameters) { if (pair.getKey() != null && pair.getKey().trim().length() != 0) { - paramMap.put(pair.getKey(), _evaluateValueExpressions(pair.getValue())); + paramMap.put(pair.getKey(), pair.getValue()); } } } @@ -815,37 +815,6 @@ private String encodeURL(String baseUrl, Map> parameters) return newUrl.toString(); } - /** - * Checks the Strings in the List for EL expressions and evaluates them. - * Note that the returned List will be a copy of the given List, because - * otherwise it will have unwanted side-effects. - * @param values - * @return - */ - private List _evaluateValueExpressions(List values) - { - // note that we have to create a new List here, because if we - // change any value on the given List, it will be changed in the - // NavigationCase too and the EL expression won't be evaluated again - List target = new ArrayList(values.size()); - FacesContext context = FacesContext.getCurrentInstance(); - for (String value : values) - { - if (_isExpression(value)) - { - // evaluate the ValueExpression - value = context.getApplication().evaluateExpressionGet(context, value, String.class); - } - target.add(value); - } - return target; - } - - private boolean _isExpression(String text) - { - return text.indexOf("#{") != -1; - } - /** * @since 2.0 */ diff --git a/impl/src/test/java/org/apache/myfaces/context/servlet/ServletExternalContextImplTest.java b/impl/src/test/java/org/apache/myfaces/context/servlet/ServletExternalContextImplTest.java index 84880b8abf..0bd0929ba4 100644 --- a/impl/src/test/java/org/apache/myfaces/context/servlet/ServletExternalContextImplTest.java +++ b/impl/src/test/java/org/apache/myfaces/context/servlet/ServletExternalContextImplTest.java @@ -65,6 +65,7 @@ public void tearDown() throws Exception * Tests if encodeRedirectURL() and encodeBookmarkableURL() correctly * evaluate ValueExpressions as parameters values. */ + /* TODO: Invalid test, because EL evaluation should be done before call these methods. @Test @SuppressWarnings("unchecked") public void testEncodeURLHandlesValueExpressionParameters() @@ -94,7 +95,7 @@ public void testEncodeURLHandlesValueExpressionParameters() Assert.assertTrue(bookmarkableUrl.contains("param1=literalvalue")); Assert.assertTrue(bookmarkableUrl.contains("param1=myvalue1")); Assert.assertTrue(bookmarkableUrl.contains("param2=myvalue2")); - } + }*/ @Test public void testEncodeRedirectUrlWithEmptyParamInBaseUrl() diff --git a/shared/src/main/java/org/apache/myfaces/shared/application/NavigationUtils.java b/shared/src/main/java/org/apache/myfaces/shared/application/NavigationUtils.java new file mode 100644 index 0000000000..8eef3ac87a --- /dev/null +++ b/shared/src/main/java/org/apache/myfaces/shared/application/NavigationUtils.java @@ -0,0 +1,107 @@ +/* + * 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 org.apache.myfaces.shared.application; + +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +import javax.faces.context.FacesContext; + +/** + * + * @author Leonardo Uribe + * + */ +public class NavigationUtils +{ + /** + * Evaluate all EL expressions found as parameters and return a map that can be used for + * redirect or render bookmark links + * + * @param parameters parameter map retrieved from NavigationCase.getParameters() + * @return + */ + public static Map > getEvaluatedNavigationParameters(Map > parameters) + { + Map> evaluatedParameters = null; + if (parameters != null && parameters.size() > 0) + { + evaluatedParameters = new HashMap>(); + for (Map.Entry> pair : parameters.entrySet()) + { + boolean containsEL = false; + for (String value : pair.getValue()) + { + if (_isExpression(value)) + { + containsEL = true; + break; + } + } + if (containsEL) + { + evaluatedParameters.put(pair.getKey(), _evaluateValueExpressions(pair.getValue())); + } + else + { + evaluatedParameters.put(pair.getKey(), pair.getValue()); + } + } + } + else + { + evaluatedParameters = parameters; + } + return evaluatedParameters; + } + + /** + * Checks the Strings in the List for EL expressions and evaluates them. + * Note that the returned List will be a copy of the given List, because + * otherwise it will have unwanted side-effects. + * @param values + * @return + */ + private static List _evaluateValueExpressions(List values) + { + // note that we have to create a new List here, because if we + // change any value on the given List, it will be changed in the + // NavigationCase too and the EL expression won't be evaluated again + List target = new ArrayList(values.size()); + FacesContext context = FacesContext.getCurrentInstance(); + for (String value : values) + { + if (_isExpression(value)) + { + // evaluate the ValueExpression + value = context.getApplication().evaluateExpressionGet(context, value, String.class); + } + target.add(value); + } + return target; + } + + private static boolean _isExpression(String text) + { + return text.indexOf("#{") != -1; + } + +} diff --git a/shared/src/main/java/org/apache/myfaces/shared/renderkit/html/HtmlRendererUtils.java b/shared/src/main/java/org/apache/myfaces/shared/renderkit/html/HtmlRendererUtils.java index 5ca6d5d667..335c089789 100644 --- a/shared/src/main/java/org/apache/myfaces/shared/renderkit/html/HtmlRendererUtils.java +++ b/shared/src/main/java/org/apache/myfaces/shared/renderkit/html/HtmlRendererUtils.java @@ -63,6 +63,7 @@ import javax.faces.model.SelectItem; import javax.faces.model.SelectItemGroup; +import org.apache.myfaces.shared.application.NavigationUtils; import org.apache.myfaces.shared.component.DisplayValueOnlyCapable; import org.apache.myfaces.shared.component.EscapeCapable; import org.apache.myfaces.shared.config.MyfacesConfig; @@ -1589,8 +1590,8 @@ public static String getOutcomeTargetHref(FacesContext facesContext, } } // handle NavigationCase parameters - Map> navigationCaseParams = navigationCase - .getParameters(); + Map> navigationCaseParams = + NavigationUtils.getEvaluatedNavigationParameters(navigationCase.getParameters()); if (navigationCaseParams != null) { if (parameters == null)