diff --git a/api/src/main/java/javax/faces/component/UIViewRoot.java b/api/src/main/java/javax/faces/component/UIViewRoot.java index 5e2cd47de2..e8d28c26b3 100644 --- a/api/src/main/java/javax/faces/component/UIViewRoot.java +++ b/api/src/main/java/javax/faces/component/UIViewRoot.java @@ -564,7 +564,13 @@ public void encodeChildren(FacesContext context) throws IOException PartialViewContext pContext = context.getPartialViewContext(); // If PartialViewContext.isAjaxRequest() returns true - if (pContext.isAjaxRequest()) + // Additionally require a postback: a genuine ajax request is always a postback and carries a + // jakarta.faces.ViewState. A request flagged as ajax but without any view state is not restoring a + // view, so partial-rendering it is meaningless - it would only build a PartialVisitContext from the + // attacker-controlled jakarta.faces.partial.render parameter against a freshly created view. Treating + // it as a normal (full) render keeps that parameter from being parsed on a non-postback and hardens + // the pre-authentication resource-exhaustion path. + if (pContext.isAjaxRequest() && context.isPostback()) { // Perform partial rendering by calling PartialViewContext.processPartial() with PhaseId.RENDER_RESPONSE. //sectin 13.4.3 of the jsf2 specification diff --git a/api/src/test/java/javax/faces/component/UIViewRootTest.java b/api/src/test/java/javax/faces/component/UIViewRootTest.java index da7166a3d9..2fedcf89a1 100644 --- a/api/src/test/java/javax/faces/component/UIViewRootTest.java +++ b/api/src/test/java/javax/faces/component/UIViewRootTest.java @@ -42,6 +42,8 @@ import javax.faces.application.ProjectStage; import javax.faces.application.ViewHandler; import javax.faces.context.ExternalContext; +import javax.faces.context.FacesContext; +import javax.faces.context.PartialViewContext; import javax.faces.event.AbortProcessingException; import javax.faces.event.ActionEvent; import javax.faces.event.ActionListener; @@ -597,6 +599,57 @@ public PhaseId getPhaseId() } } + /** + * a request flagged as ajax but without a javax.faces.ViewState + * is not a postback. UIViewRoot.encodeChildren must NOT trigger partial rendering for it, otherwise the + * attacker-controlled javax.faces.partial.render parameter would be parsed on a non-postback. It must + * fall back to a normal (full) render instead. + */ + @Test + public void testEncodeChildrenSkipsPartialRenderingWhenNotPostback() throws Exception + { + IMocksControl ctrl = EasyMock.createControl(); + FacesContext ctx = ctrl.createMock(FacesContext.class); + PartialViewContext pvc = ctrl.createMock(PartialViewContext.class); + + expect(ctx.getResponseComplete()).andReturn(false); + expect(ctx.getPartialViewContext()).andReturn(pvc); + expect(pvc.isAjaxRequest()).andReturn(true); + expect(ctx.isPostback()).andReturn(false); + // processPartial() is intentionally not expected: the strict mock fails the test if it is called. + ctrl.replay(); + + // rendered=false makes the full-render fallback (super.encodeChildren) a clean no-op, + // so only the gate's calls hit the mock. + _testimpl.setRendered(false); + _testimpl.encodeChildren(ctx); + + ctrl.verify(); + } + + /** + * Counterpart to {@link #testEncodeChildrenSkipsPartialRenderingWhenNotPostback()}: a genuine ajax + * postback (ViewState present) must still be partial-rendered. + */ + @Test + public void testEncodeChildrenDoesPartialRenderingOnAjaxPostback() throws Exception + { + IMocksControl ctrl = EasyMock.createControl(); + FacesContext ctx = ctrl.createMock(FacesContext.class); + PartialViewContext pvc = ctrl.createMock(PartialViewContext.class); + + expect(ctx.getResponseComplete()).andReturn(false); + expect(ctx.getPartialViewContext()).andReturn(pvc); + expect(pvc.isAjaxRequest()).andReturn(true); + expect(ctx.isPostback()).andReturn(true); + pvc.processPartial(PhaseId.RENDER_RESPONSE); + ctrl.replay(); + + _testimpl.encodeChildren(ctx); + + ctrl.verify(); + } + @Test public void testBroadcastEvents() { diff --git a/impl/src/main/java/org/apache/myfaces/component/visit/PartialVisitContext.java b/impl/src/main/java/org/apache/myfaces/component/visit/PartialVisitContext.java index b7f7302250..8ced29e193 100644 --- a/impl/src/main/java/org/apache/myfaces/component/visit/PartialVisitContext.java +++ b/impl/src/main/java/org/apache/myfaces/component/visit/PartialVisitContext.java @@ -47,6 +47,14 @@ public class PartialVisitContext extends VisitContext { + // Maximum NamingContainer nesting depth (number of separators) registered per client id. + // The number of separators in a client id equals its NamingContainer nesting depth; real views never + // nest more than a handful deep. Without a bound, a crafted client id made of many separators would make + // _addSubtreeClientId retain substring(0, i) for every separator, i.e. O(depth^2) characters and copies, + // which is an unauthenticated memory/CPU exhaustion vector. This keeps the work linear and acts + // as a backstop for any caller; the primary input caps live in PartialViewContextImpl. + private static final int MAX_NAMING_CONTAINER_DEPTH = 64; + /** * Creates a PartialVisitorContext instance. * @param facesContext the FacesContext for the current request @@ -287,7 +295,6 @@ private String _getVisitId(UIComponent component) } - // Converts an client id into a plain old id by ripping // out the trailing id segmetn. private String _getIdFromClientId(String clientId) @@ -323,10 +330,12 @@ private void _addSubtreeClientId(String clientId) // NamingContainer, add an entry into the map for the full client // id. final char separator = getFacesContext().getNamingContainerSeparatorChar(); - + int length = clientId.length(); - for (int i = 0; i < length; i++) + // Bound the nesting depth we register to keep this method linear (see MAX_NAMING_CONTAINER_DEPTH). + int depth = 0; + for (int i = 0; i < length && depth < MAX_NAMING_CONTAINER_DEPTH; i++) { if (clientId.charAt(i) == separator) { @@ -342,13 +351,14 @@ private void _addSubtreeClientId(String clientId) if (c == null) { - // TODO: smarter initial size? - c = new ArrayList(); + c = new ArrayList<>(5); _subtreeClientIds.put(namingContainerClientId, c); } // Stash away the client id c.add(clientId); + + depth++; } } } @@ -361,14 +371,14 @@ private void _removeSubtreeClientId(String clientId) // the client id to remove should be contained in the corresponding // collection - ie. whether the key (the NamingContainer client id) // is present at the start of the client id to remove. - for (String key : _subtreeClientIds.keySet()) + for (Map.Entry> stringCollectionEntry : _subtreeClientIds.entrySet()) { - if (clientId.startsWith(key)) + if (clientId.startsWith(stringCollectionEntry.getKey())) { // If the clientId starts with the key, we should // have an entry for this clientId in the corresponding // collection. Remove it. - Collection ids = _subtreeClientIds.get(key); + Collection ids = stringCollectionEntry.getValue(); ids.remove(clientId); } } diff --git a/impl/src/main/java/org/apache/myfaces/context/servlet/PartialViewContextImpl.java b/impl/src/main/java/org/apache/myfaces/context/servlet/PartialViewContextImpl.java index 47c930eeba..f19c043cb2 100644 --- a/impl/src/main/java/org/apache/myfaces/context/servlet/PartialViewContextImpl.java +++ b/impl/src/main/java/org/apache/myfaces/context/servlet/PartialViewContextImpl.java @@ -51,6 +51,7 @@ import java.util.Collection; import java.util.Collections; import java.util.EnumSet; +import java.util.LinkedHashSet; import java.util.List; import java.util.Map; import java.util.Set; @@ -71,8 +72,16 @@ public class PartialViewContextImpl extends PartialViewContext * will be changed for 2.1 to the official marker */ private static final String PARTIAL_IFRAME = "org.apache.myfaces.partial.iframe"; - - private static final Set PARTIAL_EXECUTE_HINTS = Collections.unmodifiableSet( + + // Upper bounds for the attacker-controllable javax.faces.partial.render / .execute client id lists. + // A legitimate ajax request references only a handful of short client ids, so these caps never affect + // real traffic; they keep an unauthenticated caller from driving unbounded memory/CPU when the ids are + // expanded into a PartialVisitContext (quadratic resource exhaustion). See also the nesting-depth + // backstop in PartialVisitContext#_addSubtreeClientId. + private static final int MAX_CLIENT_IDS = 256; + private static final int MAX_CLIENT_ID_LENGTH = 256; + + private static final Set PARTIAL_EXECUTE_HINTS = Collections.unmodifiableSet( EnumSet.of(VisitHint.EXECUTE_LIFECYCLE, VisitHint.SKIP_UNRENDERED)); // unrendered have to be skipped, transient definitely must be added to our list! @@ -244,19 +253,9 @@ public Collection getExecuteIds() //!PartialViewContext.NO_PARTIAL_PHASE_CLIENT_IDS.equals(executeMode) && !PartialViewContext.ALL_PARTIAL_PHASE_CLIENT_IDS.equals(executeMode)) { - - String[] clientIds - = StringUtils.splitShortString(_replaceTabOrEnterCharactersWithSpaces(executeMode), ' '); - //The collection must be mutable - List tempList = new ArrayList(); - for (String clientId : clientIds) - { - if (clientId.length() > 0) - { - tempList.add(clientId); - } - } + Collection tempList = parseClientIds(executeMode); + // The "javax.faces.source" parameter needs to be added to the list of // execute ids if missing (otherwise, we'd never execute an action associated // with, e.g., a button). @@ -268,7 +267,9 @@ public Collection getExecuteIds() { source = source.trim(); - if (!tempList.contains(source)) + // jakarta.faces.source is attacker-controlled as well; apply the same length bound so it + // cannot bypass the cap and be expanded into an oversized PartialVisitContext. + if (source.length() <= MAX_CLIENT_ID_LENGTH) { tempList.add(source); } @@ -302,6 +303,40 @@ private String _replaceTabOrEnterCharactersWithSpaces(String mode) return builder.toString(); } + /** + * Splits a space separated jakarta.faces.partial.render / .execute request parameter into its client ids. + *

+ * The result is a mutable, insertion-ordered, duplicate-free collection. Empty tokens are dropped, client + * ids longer than {@link #MAX_CLIENT_ID_LENGTH} are rejected and at most {@link #MAX_CLIENT_IDS} ids are + * returned. These bounds keep an unauthenticated caller from expanding this attacker-controlled parameter + * into an oversized PartialVisitContext; legitimate requests stay well below the limits. + */ + private Collection parseClientIds(String mode) + { + String[] clientIds = StringUtils.splitShortString(_replaceTabOrEnterCharactersWithSpaces(mode), ' '); + + // LinkedHashSet: collapse duplicate client ids once, here, instead of carrying them through the + // request, while preserving order. + Collection result = new LinkedHashSet<>(); + for (String clientId : clientIds) + { + int length = clientId.length(); + if (length == 0 || length > MAX_CLIENT_ID_LENGTH) + { + // skip empty tokens and reject implausibly long client ids + continue; + } + + result.add(clientId); + + if (result.size() >= MAX_CLIENT_IDS) + { + break; + } + } + return result; + } + @Override public Collection getRenderIds() { @@ -317,19 +352,8 @@ public Collection getRenderIds() //!PartialViewContext.NO_PARTIAL_PHASE_CLIENT_IDS.equals(renderMode) && !PartialViewContext.ALL_PARTIAL_PHASE_CLIENT_IDS.equals(renderMode)) { - String[] clientIds - = StringUtils.splitShortString(_replaceTabOrEnterCharactersWithSpaces(renderMode), ' '); - //The collection must be mutable - List tempList = new ArrayList(); - for (String clientId : clientIds) - { - if (clientId.length() > 0) - { - tempList.add(clientId); - } - } - _renderClientIds = tempList; + _renderClientIds = parseClientIds(renderMode); } else { diff --git a/impl/src/test/java/org/apache/myfaces/context/ExecutePhaseClientIdsTest.java b/impl/src/test/java/org/apache/myfaces/context/ExecutePhaseClientIdsTest.java index bf433b6d39..438ce273ec 100644 --- a/impl/src/test/java/org/apache/myfaces/context/ExecutePhaseClientIdsTest.java +++ b/impl/src/test/java/org/apache/myfaces/context/ExecutePhaseClientIdsTest.java @@ -25,6 +25,8 @@ import org.apache.myfaces.context.servlet.FacesContextImpl; import org.apache.myfaces.test.base.AbstractJsfTestCase; +import org.junit.Assert; +import org.junit.Test; /** * @@ -133,4 +135,48 @@ public void testRequestParams6() { // // assertTrue("Value match", pprContext.getExecuteIds().get(3).equals("component4")); } + + /** + * a single, implausibly long execute id must not be expanded. + */ + @Test + public void testOverlongClientIdIsRejected() { + StringBuilder colons = new StringBuilder(); + for (int i = 0; i < 100000; i++) { + colons.append(':'); + } + Map requestParamMap = new HashMap(); + requestParamMap.put(PartialViewContext.PARTIAL_EXECUTE_PARAM_NAME, colons.toString()); + ContextTestRequestWrapper wrapper = new ContextTestRequestWrapper(request, requestParamMap); + + FacesContext context = new FacesContextImpl(servletContext, wrapper, response); + + PartialViewContext pprContext = context.getPartialViewContext(); + + Assert.assertTrue(pprContext.getExecuteIds().isEmpty()); + } + + /** + * the attacker-controlled javax.faces.source parameter must be + * length-bounded too, otherwise it bypasses the execute-id cap. + */ + @Test + public void testOverlongSourceIsRejected() { + StringBuilder colons = new StringBuilder(); + for (int i = 0; i < 100000; i++) { + colons.append(':'); + } + Map requestParamMap = new HashMap(); + requestParamMap.put(PartialViewContext.PARTIAL_EXECUTE_PARAM_NAME, "form:input"); + requestParamMap.put("javax.faces.source", colons.toString()); + ContextTestRequestWrapper wrapper = new ContextTestRequestWrapper(request, requestParamMap); + + FacesContext context = new FacesContextImpl(servletContext, wrapper, response); + + PartialViewContext pprContext = context.getPartialViewContext(); + + // only the valid execute id survives; the oversized source is dropped + Assert.assertEquals(1, pprContext.getExecuteIds().size()); + Assert.assertTrue(pprContext.getExecuteIds().contains("form:input")); + } } diff --git a/impl/src/test/java/org/apache/myfaces/context/RenderPhaseClientIdsTest.java b/impl/src/test/java/org/apache/myfaces/context/RenderPhaseClientIdsTest.java index e36bdd79d7..1a103d5493 100644 --- a/impl/src/test/java/org/apache/myfaces/context/RenderPhaseClientIdsTest.java +++ b/impl/src/test/java/org/apache/myfaces/context/RenderPhaseClientIdsTest.java @@ -24,6 +24,8 @@ import org.apache.myfaces.context.servlet.FacesContextImpl; import org.apache.myfaces.test.base.AbstractJsfTestCase; +import org.junit.Assert; +import org.junit.Test; /** * Testcases for the request parameter handling @@ -136,4 +138,64 @@ public void testRequestParams6() { // // assertTrue("Value match",pprContext.getRenderIds().get(3).equals("component4")); } + + /** + * duplicate client ids must be collapsed so the parameter + * cannot be inflated with repeated ids. + */ + @Test + public void testDuplicateClientIdsAreCollapsed() { + String params = "form:input form:input form:input"; + Map requestParamMap = new HashMap(); + requestParamMap.put(PartialViewContext.PARTIAL_RENDER_PARAM_NAME, params); + ContextTestRequestWrapper wrapper = new ContextTestRequestWrapper(request, requestParamMap); + + FacesContext context = new FacesContextImpl(servletContext, wrapper, response); + + PartialViewContext pprContext = context.getPartialViewContext(); + + Assert.assertEquals(1, pprContext.getRenderIds().size()); + Assert.assertTrue(pprContext.getRenderIds().contains("form:input")); + } + + /** + * a single, implausibly long client id (e.g. a run of thousands + * of NamingContainer separators) must not be expanded into a PartialVisitContext. + */ + @Test + public void testOverlongClientIdIsRejected() { + StringBuilder colons = new StringBuilder(); + for (int i = 0; i < 100000; i++) { + colons.append(':'); + } + Map requestParamMap = new HashMap(); + requestParamMap.put(PartialViewContext.PARTIAL_RENDER_PARAM_NAME, colons.toString()); + ContextTestRequestWrapper wrapper = new ContextTestRequestWrapper(request, requestParamMap); + + FacesContext context = new FacesContextImpl(servletContext, wrapper, response); + + PartialViewContext pprContext = context.getPartialViewContext(); + + Assert.assertTrue(pprContext.getRenderIds().isEmpty()); + } + + /** + * the number of client ids read from the request is capped. + */ + @Test + public void testClientIdCountIsCapped() { + StringBuilder params = new StringBuilder(); + for (int i = 0; i < 5000; i++) { + params.append("id").append(i).append(' '); + } + Map requestParamMap = new HashMap(); + requestParamMap.put(PartialViewContext.PARTIAL_RENDER_PARAM_NAME, params.toString()); + ContextTestRequestWrapper wrapper = new ContextTestRequestWrapper(request, requestParamMap); + + FacesContext context = new FacesContextImpl(servletContext, wrapper, response); + + PartialViewContext pprContext = context.getPartialViewContext(); + + Assert.assertEquals(256, pprContext.getRenderIds().size()); + } }