From da8c04c9e504b804fe39158a92c39d78d5d51c2e Mon Sep 17 00:00:00 2001 From: Fabien Chebel Date: Tue, 4 Jul 2023 20:21:10 +0200 Subject: [PATCH 1/2] Add support for CIDR notation in `RemoteIpFilter` # Context Tomcat's `RemoteIpFilter` currently allows configuring trusted/internal proxies using regexp. When integrating with reverse proxies with a large number of IP addresses, regexp configuration gets cumbersome. # Suggestion I suggest adding support for IP ranges in CIDR notation to make it easier to setup the filter in these cases. For backward compatibility, matching with masks is only performed when the trusted/internal proxies patterns are null. Depending on the feedback I receive on this PR, I may add the same changes to Tomcat's `RemoteIpValve`. --- .../catalina/filters/RemoteIpFilter.java | 109 +++++++++++++++++- .../catalina/filters/TestRemoteIpFilter.java | 72 ++++++++++++ 2 files changed, 177 insertions(+), 4 deletions(-) diff --git a/java/org/apache/catalina/filters/RemoteIpFilter.java b/java/org/apache/catalina/filters/RemoteIpFilter.java index 13a0eb7bf9b7..8781a470d298 100644 --- a/java/org/apache/catalina/filters/RemoteIpFilter.java +++ b/java/org/apache/catalina/filters/RemoteIpFilter.java @@ -21,6 +21,7 @@ import java.net.InetAddress; import java.net.UnknownHostException; import java.util.ArrayDeque; +import java.util.Arrays; import java.util.Collections; import java.util.Deque; import java.util.Enumeration; @@ -44,6 +45,7 @@ import org.apache.catalina.AccessLog; import org.apache.catalina.Globals; import org.apache.catalina.connector.RequestFacade; +import org.apache.catalina.util.NetMask; import org.apache.catalina.util.RequestUtil; import org.apache.juli.logging.Log; import org.apache.juli.logging.LogFactory; @@ -667,6 +669,8 @@ public PushBuilder newPushBuilder() { protected static final String INTERNAL_PROXIES_PARAMETER = "internalProxies"; + protected static final String INTERNAL_PROXIES_MASKS_PARAMETER = "internalProxiesMasks"; + // Log must be non-static as loggers are created per class-loader and this // Filter may be used in multiple class loaders private transient Log log = LogFactory.getLog(RemoteIpFilter.class); @@ -690,6 +694,8 @@ public PushBuilder newPushBuilder() { protected static final String TRUSTED_PROXIES_PARAMETER = "trustedProxies"; + protected static final String TRUSTED_PROXIES_MASKS_PARAMETER = "trustedProxiesMasks"; + protected static final String ENABLE_LOOKUPS_PARAMETER = "enableLookups"; /** @@ -725,6 +731,11 @@ protected static String[] commaDelimitedListToStringArray(String commaDelimitedS "172\\.1[6-9]{1}\\.\\d{1,3}\\.\\d{1,3}|" + "172\\.2[0-9]{1}\\.\\d{1,3}\\.\\d{1,3}|" + "172\\.3[0-1]{1}\\.\\d{1,3}\\.\\d{1,3}|" + "0:0:0:0:0:0:0:1|::1"); + /** + * @see #setInternalProxiesMasks(String) + */ + private List internalProxiesMasks = Arrays.asList(new NetMask("10.0.0.0/8"), new NetMask("192.168.0.0/16"), new NetMask("169.254.0.0/16"), new NetMask("127.0.0.0/8"), new NetMask("100.64.0.0/10"), new NetMask("172.16.0.0/12"), new NetMask("::1/128")); + /** * @see #setProtocolHeader(String) */ @@ -760,14 +771,19 @@ protected static String[] commaDelimitedListToStringArray(String commaDelimitedS */ private Pattern trustedProxies = null; + /** + * @see #setTrustedProxiesMasks(String) + */ + private List trustedProxiesMasks = null; + private boolean enableLookups; public void doFilter(HttpServletRequest request, HttpServletResponse response, FilterChain chain) throws IOException, ServletException { - boolean isInternal = internalProxies != null && internalProxies.matcher(request.getRemoteAddr()).matches(); + boolean isInternal = matchesInternalProxies(request.getRemoteAddr()); - if (isInternal || (trustedProxies != null && trustedProxies.matcher(request.getRemoteAddr()).matches())) { + if (isInternal || matchesTrustedProxies(request.getRemoteAddr())) { String remoteIp = null; Deque proxiesHeaderValue = new ArrayDeque<>(); StringBuilder concatRemoteIpHeaderValue = new StringBuilder(); @@ -789,9 +805,9 @@ public void doFilter(HttpServletRequest request, HttpServletResponse response, F for (idx = remoteIpHeaderValue.length - 1; idx >= 0; idx--) { String currentRemoteIp = remoteIpHeaderValue[idx]; remoteIp = currentRemoteIp; - if (internalProxies != null && internalProxies.matcher(currentRemoteIp).matches()) { + if (matchesInternalProxies(currentRemoteIp)) { // do nothing, internalProxies IPs are not appended to the - } else if (trustedProxies != null && trustedProxies.matcher(currentRemoteIp).matches()) { + } else if (matchesTrustedProxies(currentRemoteIp)) { proxiesHeaderValue.addFirst(currentRemoteIp); } else { idx--; // decrement idx because break statement doesn't do it @@ -907,6 +923,31 @@ public void doFilter(HttpServletRequest request, HttpServletResponse response, F } + private boolean matchesInternalProxies(String remoteAddr) { + return matchesPatternOrNetMasks(remoteAddr, internalProxies, internalProxiesMasks); + } + + private boolean matchesTrustedProxies(String remoteAddr) { + return matchesPatternOrNetMasks(remoteAddr, trustedProxies, trustedProxiesMasks); + } + + private boolean matchesPatternOrNetMasks(String remoteAddr, Pattern pattern, List masks) { + if (pattern != null) { + return pattern.matcher(remoteAddr).matches(); + } else if (masks != null && !masks.isEmpty()) { + try { + InetAddress address = InetAddress.getByName(remoteAddr); + return masks.stream() + .anyMatch(mask -> mask.matches(address)); + } catch (UnknownHostException e) { + // should never happen, as no lookups are performed with dotted-decimal address format + log.debug("unable to map remote address to InetAddress", e); + return false; + } + } + return false; + } + /* * Considers the value to be secure if it exclusively holds forwards for {@link #protocolHeaderHttpsValue}. */ @@ -975,6 +1016,10 @@ public Pattern getInternalProxies() { return internalProxies; } + public List getInternalProxiesMasks() { + return internalProxiesMasks; + } + public String getProtocolHeader() { return protocolHeader; } @@ -1018,6 +1063,11 @@ public void init() throws ServletException { setInternalProxies(getInitParameter(INTERNAL_PROXIES_PARAMETER)); } + if (getInitParameter(INTERNAL_PROXIES_MASKS_PARAMETER) != null) { + setInternalProxiesMasks(getInitParameter(INTERNAL_PROXIES_MASKS_PARAMETER)); + setInternalProxies(null); + } + if (getInitParameter(PROTOCOL_HEADER_PARAMETER) != null) { setProtocolHeader(getInitParameter(PROTOCOL_HEADER_PARAMETER)); } @@ -1054,6 +1104,11 @@ public void init() throws ServletException { setTrustedProxies(getInitParameter(TRUSTED_PROXIES_PARAMETER)); } + if (getInitParameter(TRUSTED_PROXIES_MASKS_PARAMETER) != null) { + setTrustedProxiesMasks(getInitParameter(TRUSTED_PROXIES_MASKS_PARAMETER)); + setTrustedProxies(null); + } + if (getInitParameter(HTTP_SERVER_PORT_PARAMETER) != null) { try { setHttpServerPort(Integer.parseInt(getInitParameter(HTTP_SERVER_PORT_PARAMETER))); @@ -1157,6 +1212,35 @@ public void setInternalProxies(String internalProxies) { } } + /** + *

+ * List of network masks defining the internal proxies. + * This value will be used if internalProxies is not defined + *

+ *

+ * Default value: + *

    + *
  • 10.0.0.0/8
    • + *
  • 192.168.0.0/16
    • + *
  • 169.254.0.0/16
    • + *
  • 127.0.0.0/8
    • + *
  • 100.64.0.0/10
    • + *
  • 172.16.0.0/12
    • + *
  • ::1/128
    • + *
+ *

+ * @param internalProxiesMasks The list of network masks + */ + public void setInternalProxiesMasks(String internalProxiesMasks) { + if (internalProxiesMasks == null || internalProxiesMasks.length() == 0) { + this.internalProxiesMasks = null; + } else { + this.internalProxiesMasks = Arrays.stream(internalProxiesMasks.split(",")) + .map(NetMask::new) + .toList(); + } + } + /** *

* Header that holds the incoming host, usually named X-Forwarded-Host. @@ -1292,6 +1376,23 @@ public void setTrustedProxies(String trustedProxies) { } } + /** + *

+ * List of network masks defining the trusted proxies. + * This value will be used if trustedProxies is not defined + *

+ * @param trustedProxiesMasks The list of network masks + */ + public void setTrustedProxiesMasks(String trustedProxiesMasks) { + if (trustedProxiesMasks == null || trustedProxiesMasks.length() == 0) { + this.trustedProxiesMasks = null; + } else { + this.trustedProxiesMasks = Arrays.stream(trustedProxiesMasks.split(",")) + .map(NetMask::new) + .toList(); + } + } + public void setEnableLookups(boolean enableLookups) { this.enableLookups = enableLookups; } diff --git a/test/org/apache/catalina/filters/TestRemoteIpFilter.java b/test/org/apache/catalina/filters/TestRemoteIpFilter.java index 82c8bb8f7948..942419ff470e 100644 --- a/test/org/apache/catalina/filters/TestRemoteIpFilter.java +++ b/test/org/apache/catalina/filters/TestRemoteIpFilter.java @@ -18,8 +18,11 @@ import java.io.IOException; import java.io.PrintWriter; +import java.io.UncheckedIOException; import java.net.HttpURLConnection; +import java.net.InetAddress; import java.net.URI; +import java.net.UnknownHostException; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; @@ -37,6 +40,7 @@ import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpServletResponse; +import org.apache.catalina.util.NetMask; import org.junit.Assert; import org.junit.Test; @@ -492,6 +496,40 @@ public void testInvokeAllProxiesAreTrustedOrInternal() throws Exception { Assert.assertEquals("remoteHost", "140.211.11.130", actualRemoteHost); } + @Test + public void testInvokeAllProxiesAreTrustedOrInternal_withNetMasks() throws Exception { + + // PREPARE + FilterDef filterDef = new FilterDef(); + filterDef.addInitParameter("internalProxiesMasks", "192.168.1.0/24,192.168.5.0/24"); + filterDef.addInitParameter("trustedProxiesMasks", "192.0.2.0/24,198.51.100.0/24"); + 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, 192.0.2.50, 198.51.100.33, 192.168.1.100, 192.168.5.1"); + + // TEST + HttpServletRequest actualRequest = testRemoteIpFilter(filterDef, request).getRequest(); + + // VERIFY + String actualXForwardedFor = actualRequest.getHeader("x-forwarded-for"); + Assert.assertNull("all proxies are trusted, x-forwarded-for must be null", actualXForwardedFor); + + String actualXForwardedBy = actualRequest.getHeader("x-forwarded-by"); + Assert.assertEquals("all proxies are trusted, they must appear in x-forwarded-by", "proxy1,proxy2", + actualXForwardedBy); + + String actualRemoteAddr = actualRequest.getRemoteAddr(); + Assert.assertEquals("remoteAddr", "140.211.11.130", actualRemoteAddr); + + String actualRemoteHost = actualRequest.getRemoteHost(); + Assert.assertEquals("remoteHost", "140.211.11.130", actualRemoteHost); + } + @Test public void testInvokeNotAllowedRemoteAddr() throws Exception { // PREPARE @@ -868,4 +906,38 @@ private void doTestPattern(Pattern pattern, String input, boolean expectedMatch) boolean match = pattern.matcher(input).matches(); Assert.assertEquals(input, Boolean.valueOf(expectedMatch), Boolean.valueOf(match)); } + + @Test + public void testInternalProxiesMasks() { + RemoteIpFilter remoteIpFilter = new RemoteIpFilter(); + List masks = remoteIpFilter.getInternalProxiesMasks(); + + doTestMasks(masks, "8.8.8.8", false); + doTestMasks(masks, "100.62.0.0", false); + doTestMasks(masks, "100.63.255.255", false); + doTestMasks(masks, "100.64.0.0", true); + doTestMasks(masks, "100.65.0.0", true); + doTestMasks(masks, "100.68.0.0", true); + doTestMasks(masks, "100.72.0.0", true); + doTestMasks(masks, "100.88.0.0", true); + doTestMasks(masks, "100.95.0.0", true); + doTestMasks(masks, "100.102.0.0", true); + doTestMasks(masks, "100.110.0.0", true); + doTestMasks(masks, "100.126.0.0", true); + doTestMasks(masks, "100.127.255.255", true); + doTestMasks(masks, "100.128.0.0", false); + doTestMasks(masks, "100.130.0.0", false); + + } + + private void doTestMasks(List masks, String input, boolean expectedMatch) { + boolean match = masks.stream().anyMatch(mask -> { + try { + return mask.matches(InetAddress.getByName(input)); + } catch (UnknownHostException e) { + throw new UncheckedIOException(e); + } + }); + Assert.assertEquals(input, Boolean.valueOf(expectedMatch), Boolean.valueOf(match)); + } } From bf5ae84c4060db55b452cead4fad6d4bef25df69 Mon Sep 17 00:00:00 2001 From: Fabien Chebel Date: Wed, 5 Jul 2023 20:36:26 +0200 Subject: [PATCH 2/2] fix test --- test/org/apache/catalina/filters/TestRemoteIpFilter.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/org/apache/catalina/filters/TestRemoteIpFilter.java b/test/org/apache/catalina/filters/TestRemoteIpFilter.java index 942419ff470e..ced733684a86 100644 --- a/test/org/apache/catalina/filters/TestRemoteIpFilter.java +++ b/test/org/apache/catalina/filters/TestRemoteIpFilter.java @@ -501,7 +501,7 @@ public void testInvokeAllProxiesAreTrustedOrInternal_withNetMasks() throws Excep // PREPARE FilterDef filterDef = new FilterDef(); - filterDef.addInitParameter("internalProxiesMasks", "192.168.1.0/24,192.168.5.0/24"); + filterDef.addInitParameter("internalProxiesMasks", "192.168.0.0/24,192.168.5.0/24"); filterDef.addInitParameter("trustedProxiesMasks", "192.0.2.0/24,198.51.100.0/24"); filterDef.addInitParameter("remoteIpHeader", "x-forwarded-for"); filterDef.addInitParameter("proxiesHeader", "x-forwarded-by"); @@ -510,7 +510,7 @@ public void testInvokeAllProxiesAreTrustedOrInternal_withNetMasks() throws Excep request.setRemoteAddr("192.168.0.10"); request.setRemoteHost("remote-host-original-value"); - request.setHeader("x-forwarded-for", "140.211.11.130, 192.0.2.50, 198.51.100.33, 192.168.1.100, 192.168.5.1"); + request.setHeader("x-forwarded-for", "140.211.11.130, 192.0.2.50, 198.51.100.33, 192.168.0.100, 192.168.5.1"); // TEST HttpServletRequest actualRequest = testRemoteIpFilter(filterDef, request).getRequest(); @@ -520,7 +520,7 @@ public void testInvokeAllProxiesAreTrustedOrInternal_withNetMasks() throws Excep Assert.assertNull("all proxies are trusted, x-forwarded-for must be null", actualXForwardedFor); String actualXForwardedBy = actualRequest.getHeader("x-forwarded-by"); - Assert.assertEquals("all proxies are trusted, they must appear in x-forwarded-by", "proxy1,proxy2", + Assert.assertEquals("all proxies are trusted, they must appear in x-forwarded-by", "192.0.2.50,198.51.100.33", actualXForwardedBy); String actualRemoteAddr = actualRequest.getRemoteAddr();