Fix populate a malformed remote ip / host header value by remote ip valve (filter)

java/org/apache/catalina/filters/RemoteIpFilter.java

@@ -726,8 +726,11 @@ public void doFilter(HttpServletRequest request, HttpServletResponse response, F
             // loop on remoteIpHeaderValue to find the first trusted remote ip and to build the proxies chain
             for (idx = remoteIpHeaderValue.length - 1; idx >= 0; idx--) {
                 String currentRemoteIp = remoteIpHeaderValue[idx];
+                if (!isValidRemoteIp(currentRemoteIp)) {
+                    // Ignore invalid remote ip header value
+                    continue;
+                }
                 remoteIp = currentRemoteIp;
-
                 if (isInternalProxy(currentRemoteIp)) {
                     // do nothing, internalProxies IPs are not appended to the
                 } else if (isTrustedProxy(currentRemoteIp)) {
@@ -880,6 +883,28 @@ private boolean checkIsCidr(NetMaskSet netMaskSet, String remoteIp) {
         return false;
     }
 
+    /**
+     * Check the remote ip value with a basic syntax formating.
+     * @param remoteIp the header value from request
+     * @return <code>false</code> if the remote ip is null, empty, or malformed. Otherwise returns <code>true</code>
+     */
+    private boolean isValidRemoteIp(String remoteIp) {
+        if (remoteIp == null || remoteIp.isBlank()) {
+            return false;
+        }
+        // acceptable regular chars: 0-9/A-Z/a-z
+        // acceptable special chars: []:.-
+        for (char c : remoteIp.toCharArray()) {
+            if ((c >= '0' && c <= '9') || (c >= 'A' && c <= 'Z') || (c >= 'a' && c <= 'z') || c == '[' || c == ']'
+                    || c == ':' || c == '.' || c == '-') {
+                continue;
+            } else {
+                return false;
+            }
+        }
+        return true;
+    }
+
     /*
      * Considers the value to be secure if it exclusively holds forwards for {@link #protocolHeaderHttpsValue}.
      */

java/org/apache/catalina/valves/RemoteIpValve.java

@@ -555,6 +555,10 @@ public void invoke(Request request, Response response) throws IOException, Servl
             // loop on remoteIpHeaderValue to find the first trusted remote ip and to build the proxies chain
             for (idx = remoteIpHeaderValue.length - 1; idx >= 0; idx--) {
                 String currentRemoteIp = remoteIpHeaderValue[idx];
+                if(!isValidRemoteIp(currentRemoteIp)) {
+                    // Ignore invalid remote ip header value
+                    continue;
+                }
                 remoteIp = currentRemoteIp;
                 if (isInternalProxy(currentRemoteIp)) {
                     // do nothing, internalProxies IPs are not appended to the
@@ -737,6 +741,28 @@ private boolean checkIsCidr(NetMaskSet netMaskSet, String remoteIp) {
         return false;
     }
 
+    /**
+     * Check the remote ip value with a basic syntax formating.
+     * @param remoteIp the header value from request
+     * @return <code>false</code> if the remote ip is null, empty, or malformed. Otherwise returns <code>true</code>
+     */
+    private boolean isValidRemoteIp(String remoteIp) {
+        if (remoteIp == null || remoteIp.isBlank()) {
+            return false;
+        }
+        // acceptable regular chars: 0-9/A-Z/a-z
+        // acceptable special chars: []:.-
+        for (char c : remoteIp.toCharArray()) {
+            if ((c >= '0' && c <= '9') || (c >= 'A' && c <= 'Z') || (c >= 'a' && c <= 'z') || c == '[' || c == ']'
+                    || c == ':' || c == '.' || c == '-') {
+                continue;
+            } else {
+                return false;
+            }
+        }
+        return true;
+    }
+
     /*
      * Considers the value to be secure if it exclusively holds forwards for {@link #protocolHeaderHttpsValue}.
      */

test/org/apache/catalina/filters/TestRemoteIpFilter.java

@@ -558,6 +558,40 @@ public void testInvokeUntrustedProxyInTheChain() throws Exception {
         Assert.assertEquals("remoteHost", "untrusted-proxy", actualRemoteHost);
     }
 
+    @Test
+    public void testInvokeInvalidProxyInTheChain() throws Exception {
+        // PREPARE
+        FilterDef filterDef = new FilterDef();
+        filterDef.addInitParameter("internalProxies", "192.168.0.10/31");
+        filterDef.addInitParameter("trustedProxies", "200.0.0.1,200.0.0.2,200.0.0.3");
+        filterDef.addInitParameter("remoteIpHeader", "x-forwarded-for");
+        filterDef.addInitParameter("proxiesHeader", "x-forwarded-by");
+
+        MockHttpServletRequest request = new MockHttpServletRequest();
+
+        request.setRemoteAddr("192.168.0.10");
+        request.setRemoteHost("remote-host-original-value");
+        request.setHeader("x-forwarded-for", "140.211.11.130, 200.0.0.1, untrusted-proxy, invalid-proxy 01, 200.0.0.2");
+
+        // TEST
+        HttpServletRequest actualRequest = testRemoteIpFilter(filterDef, request).getRequest();
+
+        // VERIFY
+        String actualXForwardedFor = actualRequest.getHeader("x-forwarded-for");
+        Assert.assertEquals("syntax invalid proxy should ignore, ip/host before untrusted-proxy must appear in x-forwarded-for", "140.211.11.130,200.0.0.1",
+                actualXForwardedFor);
+
+        String actualXForwardedBy = actualRequest.getHeader("x-forwarded-by");
+        Assert.assertEquals("syntax invalid proxy should ignore, ip/host after untrusted-proxy must appear in  x-forwarded-by", "200.0.0.2",
+                actualXForwardedBy);
+
+        String actualRemoteAddr = actualRequest.getRemoteAddr();
+        Assert.assertEquals("remoteAddr", "untrusted-proxy", actualRemoteAddr);
+
+        String actualRemoteHost = actualRequest.getRemoteHost();
+        Assert.assertEquals("remoteHost", "untrusted-proxy", actualRemoteHost);
+    }
+
     @Test
     public void testInvokeXforwardedHost() throws Exception {
         // PREPARE

test/org/apache/catalina/valves/TestRemoteIpValve.java

@@ -1103,6 +1103,34 @@ public void testRequestForwarded() throws Exception {
                 request.getAttribute(Globals.REQUEST_FORWARDED_ATTRIBUTE));
     }
 
+    @Test
+    public void testIgnoreInvalidRequestForwardedFor() throws Exception {
+
+        // PREPARE
+        RemoteIpValve remoteIpValve = new RemoteIpValve();
+        remoteIpValve.setRemoteIpHeader("x-forwarded-for");
+        remoteIpValve.setProtocolHeader("x-forwarded-proto");
+        RemoteAddrAndHostTrackerValve remoteAddrAndHostTrackerValve = new RemoteAddrAndHostTrackerValve();
+        remoteIpValve.setNext(remoteAddrAndHostTrackerValve);
+
+        Request request = new MockRequest(new org.apache.coyote.Request());
+        // client ip
+        request.setRemoteAddr("192.168.0.10");
+        request.setRemoteHost("192.168.0.10");
+        request.getCoyoteRequest().getMimeHeaders().addValue("x-forwarded-for").setString("140.211.11.130,\"proxy \"01");
+        // protocol
+        request.setServerPort(8080);
+        request.getCoyoteRequest().scheme().setString("http");
+
+        // TEST
+        remoteIpValve.invoke(request, null);
+
+        // VERIFY
+        Assert.assertEquals("org.apache.tomcat.request.forwarded", Boolean.TRUE,
+                request.getAttribute(Globals.REQUEST_FORWARDED_ATTRIBUTE));
+        Assert.assertEquals("Valid remote ip header expected", "140.211.11.130", request.getAttribute(AccessLog.REMOTE_ADDR_ATTRIBUTE));
+    }
+
     private void assertArrayEquals(String[] expected, String[] actual) {
         if (expected == null) {
             Assert.assertNull(actual);

webapps/docs/changelog.xml

@@ -210,6 +210,11 @@
         <bug>70000</bug>: fix duplication of special headers in the response
         after commit, following fix for <bug>69967</bug>. (remm)
       </fix>
+      <fix>
+        Fix remote ip/host header population in remote ip valve or filter,
+        ignore malformed header value. Those unexpected malformed remote ip/host
+        header value may mislead downstream components. Patch submitted by Chenjp.
+      </fix>
     </changelog>
   </subsection>
   <subsection name="Coyote">