Skip to content

Commit

Permalink
MYFACES-3405 includeViewParameters re-evaluates param/model values as…
Browse files Browse the repository at this point in the history
… EL expressions [CVE-2011-4343]
  • Loading branch information
Leonardo Uribe committed Nov 22, 2011
1 parent 7c1ddcf commit caee86e
Show file tree
Hide file tree
Showing 7 changed files with 231 additions and 38 deletions.
6 changes: 4 additions & 2 deletions api/src/main/java/javax/faces/application/NavigationCase.java
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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<String,List<String>> getParameters()
Expand Down
107 changes: 107 additions & 0 deletions api/src/main/java/javax/faces/application/_NavigationUtils.java
Original file line number Diff line number Diff line change
@@ -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<String, List<String> > getEvaluatedNavigationParameters(Map<String, List<String> > parameters)
{
Map<String,List<String>> evaluatedParameters = null;
if (parameters != null && parameters.size() > 0)
{
evaluatedParameters = new HashMap<String, List<String>>();
for (Map.Entry<String, List<String>> 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<String> _evaluateValueExpressions(List<String> 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<String> target = new ArrayList<String>(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;
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -765,7 +765,7 @@ private String encodeURL(String baseUrl, Map<String, List<String>> parameters)
{
if (pair.getKey() != null && pair.getKey().trim().length() != 0)
{
paramMap.put(pair.getKey(), _evaluateValueExpressions(pair.getValue()));
paramMap.put(pair.getKey(), pair.getValue());
}
}
}
Expand Down Expand Up @@ -815,37 +815,6 @@ private String encodeURL(String baseUrl, Map<String, List<String>> 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<String> _evaluateValueExpressions(List<String> 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<String> target = new ArrayList<String>(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
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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()
Expand Down
Original file line number Diff line number Diff line change
@@ -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<String, List<String> > getEvaluatedNavigationParameters(Map<String, List<String> > parameters)
{
Map<String,List<String>> evaluatedParameters = null;
if (parameters != null && parameters.size() > 0)
{
evaluatedParameters = new HashMap<String, List<String>>();
for (Map.Entry<String, List<String>> 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<String> _evaluateValueExpressions(List<String> 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<String> target = new ArrayList<String>(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;
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -1589,8 +1590,8 @@ public static String getOutcomeTargetHref(FacesContext facesContext,
}
}
// handle NavigationCase parameters
Map<String, List<String>> navigationCaseParams = navigationCase
.getParameters();
Map<String, List<String>> navigationCaseParams =
NavigationUtils.getEvaluatedNavigationParameters(navigationCase.getParameters());
if (navigationCaseParams != null)
{
if (parameters == null)
Expand Down

0 comments on commit caee86e

Please sign in to comment.