[2.3.x] Update Partial Rendering

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<String>();
+          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<String, Collection<String>> 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<String> ids = _subtreeClientIds.get(key);
+        Collection<String> ids = stringCollectionEntry.getValue();
         ids.remove(clientId);
       }
     }

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<VisitHint> 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<VisitHint> 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<String> 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<String> tempList = new ArrayList<String>();
-                for (String clientId : clientIds)
-                {
-                    if (clientId.length() > 0)
-                    {
-                        tempList.add(clientId);
-                    }
-                }
+                Collection<String> 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<String> 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.
+     * <p>
+     * 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<String> 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<String> 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<String> getRenderIds()
     {
@@ -317,19 +352,8 @@ public Collection<String> 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<String> tempList = new ArrayList<String>();
-                for (String clientId : clientIds)
-                {
-                    if (clientId.length() > 0)
-                    {
-                        tempList.add(clientId);
-                    }
-                }
-                _renderClientIds = tempList;
+                _renderClientIds = parseClientIds(renderMode);
             }
             else
             {

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<String, String> requestParamMap = new HashMap<String, String>();
+        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<String, String> requestParamMap = new HashMap<String, String>();
+        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"));
+    }
 }

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<String, String> requestParamMap = new HashMap<String, String>();
+        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<String, String> requestParamMap = new HashMap<String, String>();
+        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<String, String> requestParamMap = new HashMap<String, String>();
+        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());
+    }
 }