From 1396362d918a4d74f213749ef7dc4717cab5bbd1 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 | 14 +++- .../servlet/PartialViewContextImpl.java | 78 ++++++++++++------- .../context/ExecutePhaseClientIdsTest.java | 44 +++++++++++ .../context/RenderPhaseClientIdsTest.java | 60 ++++++++++++++ 4 files changed, 168 insertions(+), 28 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 67e1ada178..54a691d4d3 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 @@ -49,6 +49,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; + // The client ids to visit private final Collection _clientIds; @@ -331,7 +339,9 @@ private void _addSubtreeClientId(String clientId) 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) { @@ -352,6 +362,8 @@ private void _addSubtreeClientId(String clientId) // Stash away the client id c.add(clientId); + + depth++; } } } 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 37174bcf37..1989fdebba 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 @@ -24,6 +24,7 @@ import java.util.Collections; import java.util.EnumSet; import java.util.IdentityHashMap; +import java.util.LinkedHashSet; import java.util.List; import java.util.Map; import java.util.Set; @@ -73,7 +74,15 @@ public class PartialViewContextImpl extends PartialViewContext private static final String PARTIAL_AJAX = "partial/ajax"; private static final String PARTIAL_AJAX_REQ = "jakarta.faces.partial.ajax"; private static final String PARTIAL_PROCESS = "partial/process"; - + + // Upper bounds for the attacker-controllable jakarta.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)); @@ -209,19 +218,9 @@ public Collection getExecuteIds() if (executeMode != null && !executeMode.isEmpty() && !PartialViewContext.ALL_PARTIAL_PHASE_CLIENT_IDS.equals(executeMode)) { - - String[] clientIds - = StringUtils.splitShortString(_replaceTabOrEnterCharactersWithSpaces(executeMode), ' '); - //The collection must be mutable - List tempList = new ArrayList<>(clientIds.length); - for (String clientId : clientIds) - { - if (clientId.length() > 0) - { - tempList.add(clientId); - } - } + Collection tempList = parseClientIds(executeMode); + // The "jakarta.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). @@ -233,7 +232,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); } @@ -276,10 +277,44 @@ private String _replaceTabOrEnterCharactersWithSpaces(String mode) { return String.valueOf(escaped); } - + return mode; } + /** + * 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() { @@ -294,19 +329,8 @@ public Collection getRenderIds() if (renderMode != null && !renderMode.isEmpty() && !PartialViewContext.ALL_PARTIAL_PHASE_CLIENT_IDS.equals(renderMode)) { - String[] clientIds - = StringUtils.splitShortString(_replaceTabOrEnterCharactersWithSpaces(renderMode), ' '); - //The collection must be mutable - List tempList = new ArrayList<>(clientIds.length); - 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 0396fbba81..d724a92859 100644 --- a/impl/src/test/java/org/apache/myfaces/context/ExecutePhaseClientIdsTest.java +++ b/impl/src/test/java/org/apache/myfaces/context/ExecutePhaseClientIdsTest.java @@ -138,4 +138,48 @@ public void testRequestParams6() { // // Assertions.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(); + + Assertions.assertTrue(pprContext.getExecuteIds().isEmpty()); + } + + /** + * the attacker-controlled jakarta.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("jakarta.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 + Assertions.assertEquals(1, pprContext.getExecuteIds().size()); + Assertions.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 29f54226ba..20de58179e 100644 --- a/impl/src/test/java/org/apache/myfaces/context/RenderPhaseClientIdsTest.java +++ b/impl/src/test/java/org/apache/myfaces/context/RenderPhaseClientIdsTest.java @@ -140,4 +140,64 @@ public void testRequestParams6() { // // Assertions.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(); + + Assertions.assertEquals(1, pprContext.getRenderIds().size()); + Assertions.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(); + + Assertions.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(); + + Assertions.assertEquals(256, pprContext.getRenderIds().size()); + } }