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 2595cdbd5c..22fbd6940e 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 29f9cd8192..f0df5da52b 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 @@ -23,6 +23,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; @@ -69,7 +70,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)); @@ -204,19 +213,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). @@ -228,7 +227,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); } @@ -271,10 +272,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() { @@ -289,19 +324,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 7c5d11570d..e17b76f5a0 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 fee70a66d5..429b1406b8 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()); + } }