Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for CIDR notation in RemoteIpFilter #632

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add support for CIDR notation in RemoteIpFilter #632

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
da8c04c
Add support for CIDR notation in `RemoteIpFilter`
fabien-chebel Jul 4, 2023
bf5ae84
fix test
fabien-chebel Jul 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
109 changes: 105 additions & 4 deletions java/org/apache/catalina/filters/RemoteIpFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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);
Expand All @@ -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";

/**
Expand Down Expand Up @@ -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<NetMask> 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)
*/
Expand Down Expand Up @@ -760,14 +771,19 @@ protected static String[] commaDelimitedListToStringArray(String commaDelimitedS
*/
private Pattern trustedProxies = null;

/**
* @see #setTrustedProxiesMasks(String)
*/
private List<NetMask> 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<String> proxiesHeaderValue = new ArrayDeque<>();
StringBuilder concatRemoteIpHeaderValue = new StringBuilder();
Expand All @@ -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
Expand Down Expand Up @@ -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<NetMask> 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}.
*/
Expand Down Expand Up @@ -975,6 +1016,10 @@ public Pattern getInternalProxies() {
return internalProxies;
}

public List<NetMask> getInternalProxiesMasks() {
return internalProxiesMasks;
}

public String getProtocolHeader() {
return protocolHeader;
}
Expand Down Expand Up @@ -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));
}
Expand Down Expand Up @@ -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)));
Expand Down Expand Up @@ -1157,6 +1212,35 @@ public void setInternalProxies(String internalProxies) {
}
}

/**
* <p>
* List of network masks defining the internal proxies.
* This value will be used if internalProxies is not defined
* </p>
* <p>
* Default value:
* <ul>
* <li>10.0.0.0/8</li>
* <li>192.168.0.0/16</li>
* <li>169.254.0.0/16</li>
* <li>127.0.0.0/8</li>
* <li>100.64.0.0/10</li>
* <li>172.16.0.0/12</li>
* <li>::1/128</li>
* </ul>
* </p>
* @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();
}
}

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

/**
* <p>
* List of network masks defining the trusted proxies.
* This value will be used if trustedProxies is not defined
* </p>
* @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;
}
Expand Down
72 changes: 72 additions & 0 deletions test/org/apache/catalina/filters/TestRemoteIpFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;

Expand Down Expand Up @@ -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.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");

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.0.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", "192.0.2.50,198.51.100.33",
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
Expand Down Expand Up @@ -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<NetMask> 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<NetMask> 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));
}
}