From 2a78b98789d02aae8c29a34a713bff4649391319 Mon Sep 17 00:00:00 2001 From: tandraschko Date: Tue, 18 Aug 2026 21:16:30 +0200 Subject: [PATCH] Bound the partial render/execute client-id list The jakarta.faces.partial.render / .execute request parameters (and the jakarta.faces.source parameter) were split into client ids without any limit on the number or length of the ids. PartialVisitContext then retained one prefix substring per naming-container separator of each id, so a single very long id was expensive to process. Cap the parsed list to 256 ids of at most 256 chars each, collapse duplicates, and bound the naming-container depth registered per id in PartialVisitContext so that work stays linear in the id length. Update Partial Context comments --- .../component/visit/PartialVisitContext.java | 26 +++++-- .../servlet/PartialViewContextImpl.java | 78 ++++++++++++------- .../context/ExecutePhaseClientIdsTest.java | 46 +++++++++++ .../context/RenderPhaseClientIdsTest.java | 62 +++++++++++++++ 4 files changed, 177 insertions(+), 35 deletions(-) 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()); + } }