Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
#21415 blocking remote calls to private subnets (#21427)
* #21415 blocking remote calls to private subnets

* #21415 Implementing ITs

Co-authored-by: nollymar <nollymarlonga@Nollymars-MacBook-Pro-2.local>
  • Loading branch information
wezell and nollymar committed Jan 7, 2022
1 parent dff5e0b commit 0b62547
Show file tree
Hide file tree
Showing 7 changed files with 149 additions and 75 deletions.
4 changes: 3 additions & 1 deletion dotCMS/src/integration-test/java/com/dotcms/MainSuite.java
Expand Up @@ -54,6 +54,7 @@
import com.dotcms.rendering.velocity.viewtools.DotTemplateToolTest;
import com.dotcms.rendering.velocity.viewtools.FileToolTest;
import com.dotcms.rendering.velocity.viewtools.JSONToolTest;
import com.dotcms.rendering.velocity.viewtools.XmlToolTest;
import com.dotcms.rest.BundlePublisherResourceIntegrationTest;
import com.dotcms.rest.BundleResourceTest;
import com.dotcms.rest.IntegrityResourceIntegrationTest;
Expand Down Expand Up @@ -494,7 +495,8 @@
Task211101AddContentletAsJsonColumnTest.class,
ContentletJsonAPITest.class,
ContentletJsonAPITest.class,
Task211103RenameHostNameLabelTest.class
Task211103RenameHostNameLabelTest.class,
XmlToolTest.class
})
public class MainSuite {

Expand Down
@@ -1,6 +1,8 @@
package com.dotcms.http;

import com.dotcms.rest.exception.BadRequestException;
import com.dotmarketing.exception.DotRuntimeException;
import com.dotmarketing.util.Config;
import java.io.IOException;
import java.io.OutputStream;
import java.util.Map;
Expand Down Expand Up @@ -54,27 +56,51 @@ public void testGoodBreaker() throws Exception {


@Test
public void testBadBreaker() throws ExecutionException {
public void testBadBreaker() {

try {
final NullOutputStream nos = new NullOutputStream();

final NullOutputStream nos = new NullOutputStream();
final String key = "testBadBreaker";
final int timeout = 2000;

final String key = "testBadBreaker";
final int timeout = 2000;
CircuitBreaker breaker = CurcuitBreakerPool.getBreaker(key);
assert (breaker.isClosed());

CircuitBreaker breaker = CurcuitBreakerPool.getBreaker(key);
assert (breaker.isClosed());
Config.setProperty("ALLOW_ACCESS_TO_PRIVATE_SUBNETS", true);

for (int i = 0; i < 10; i++) {
try {
new CircuitBreakerUrl(badUrl, timeout, breaker).doOut(nos);
} catch (Exception e) {
assert (e instanceof CircuitBreakerOpenException);
}
}
breaker = CurcuitBreakerPool.getBreaker(key);

assert (breaker.isOpen());
}finally {
Config.setProperty("ALLOW_ACCESS_TO_PRIVATE_SUBNETS", false);
}
}

@Test
public void test_breaker_url_using_private_ip_throws_an_exception() {
final NullOutputStream nos = new NullOutputStream();

final String key = "testPrivateIP";
final int timeout = 2000;

CircuitBreaker breaker = CurcuitBreakerPool.getBreaker(key);
assert (breaker.isClosed());

for (int i = 0; i < 10; i++) {
try {
new CircuitBreakerUrl(badUrl, timeout, breaker).doOut(nos);
} catch (Exception e) {
assert (e instanceof CircuitBreakerOpenException);
assert (e instanceof DotRuntimeException);
assert (e.getMessage().contains("Remote HttpRequests cannot access private subnets"));
}
}
breaker = CurcuitBreakerPool.getBreaker(key);

assert (breaker.isOpen());
}

/*
Expand All @@ -86,7 +112,7 @@ public void testBadBreaker() throws ExecutionException {


//@Test
public void testHeaders() throws ExecutionException, CircuitBreakerOpenException, IOException {
public void testHeaders() throws CircuitBreakerOpenException, IOException {
Map<String, String> headers = ImmutableMap.of(HEADER, HEADER_VALUE);

CircuitBreakerUrl cburl = CircuitBreakerUrl.builder().setMethod(Method.GET).setUrl("http://localhost/get").setTimeout(1000)
Expand All @@ -108,7 +134,7 @@ public void testHeaders() throws ExecutionException, CircuitBreakerOpenException
* docker run -p 80:80 kennethreitz/httpbin
*/
//@Test
public void testPost() throws ExecutionException, CircuitBreakerOpenException, IOException {
public void testPost() throws CircuitBreakerOpenException, IOException {
Map<String, String> params = ImmutableMap.of(PARAM, PARAM_VALUE);

CircuitBreakerUrl cburl = CircuitBreakerUrl.builder().setMethod(Method.POST).setUrl("http://localhost/post").setTimeout(1000)
Expand All @@ -127,66 +153,68 @@ public void testPost() throws ExecutionException, CircuitBreakerOpenException, I
*/
@Test
public void testRecovery() throws InterruptedException, IOException {
Config.setProperty("ALLOW_ACCESS_TO_PRIVATE_SUBNETS", true);

try {
final NullOutputStream nos = new NullOutputStream();

final String key = "testRecoveryBreaker";
final int timeout = 2000;

CircuitBreaker breaker = CurcuitBreakerPool.getBreaker(key);
breaker.withDelay(5, TimeUnit.SECONDS);
assert (breaker.isClosed());

for (int i = 0; i < breaker.getSuccessThreshold().denominator; i++) {
try {
new CircuitBreakerUrl(goodUrl, timeout, breaker).doOut(nos);
} catch (Exception e) {
// shoud not be here
assert (false);
}
}

final NullOutputStream nos = new NullOutputStream();


final String key = "testRecoveryBreaker";
final int timeout = 2000;

CircuitBreaker breaker = CurcuitBreakerPool.getBreaker(key);
breaker.withDelay(5, TimeUnit.SECONDS);
assert (breaker.isClosed());
assert (breaker.isClosed());

for (int i = 0; i < breaker.getFailureThreshold().denominator; i++) {
try {
new CircuitBreakerUrl(badUrl, timeout, breaker).doOut(nos);
} catch (Exception e) {
assert (e instanceof CircuitBreakerOpenException);
}
}
assert (breaker.isOpen());
for (int i = 0; i < breaker.getFailureThreshold().denominator; i++) {
try {
new CircuitBreakerUrl(badUrl, timeout, breaker).doOut(nos);
} catch (CircuitBreakerOpenException e) {
assert (e instanceof CircuitBreakerOpenException);
}
}
Thread.sleep(breaker.getDelay().toMillis() + 1000);

for (int i = 0; i < breaker.getSuccessThreshold().denominator; i++) {
try {
new CircuitBreakerUrl(goodUrl, timeout, breaker).doOut(nos);
} catch (Exception e) {
// shoud not be here
assert (false);
}
}

assert (breaker.isClosed());
assert (breaker.isHalfOpen());

for (int i = 0; i < breaker.getFailureThreshold().denominator; i++) {
try {
new CircuitBreakerUrl(badUrl, timeout, breaker).doOut(nos);
} catch (Exception e) {
assert (e instanceof CircuitBreakerOpenException);
for (int i = 0; i < breaker.getSuccessThreshold().denominator; i++) {
try {
new CircuitBreakerUrl(goodUrl, timeout, breaker).doOut(nos);
} catch (Exception e) {
// shoud not be here
assert (false);
}
}
}
assert (breaker.isOpen());
for (int i = 0; i < breaker.getFailureThreshold().denominator; i++) {
try {
new CircuitBreakerUrl(badUrl, timeout, breaker).doOut(nos);
} catch (CircuitBreakerOpenException e) {
assert (e instanceof CircuitBreakerOpenException);
}
}
Thread.sleep(breaker.getDelay().toMillis() + 1000);

try {
new CircuitBreakerUrl(goodUrl, timeout, breaker).doOut(nos);
} catch (Exception e) {
// shoud not be here
assert (false);
assert (breaker.isClosed());
}finally {
Config.setProperty("ALLOW_ACCESS_TO_PRIVATE_SUBNETS", false);
}

assert (breaker.isHalfOpen());

for (int i = 0; i < breaker.getSuccessThreshold().denominator; i++) {
try {
new CircuitBreakerUrl(goodUrl, timeout, breaker).doOut(nos);
} catch (Exception e) {
// shoud not be here
assert (false);
}
}

assert (breaker.isClosed());
}

@Test
Expand Down
@@ -0,0 +1,26 @@
package com.dotcms.rendering.velocity.viewtools;

import com.dotcms.util.IntegrationTestInitService;
import com.dotmarketing.exception.DotRuntimeException;
import java.net.URL;
import org.junit.BeforeClass;
import org.junit.Test;

public class XmlToolTest {
@BeforeClass
public static void prepare() throws Exception{
//Setting web app environment
IntegrationTestInitService.getInstance().init();
}

@Test
public void test_xmltool_using_private_ip_should_throw_an_exception() {
XmlTool xmlTool = new XmlTool();
try {
xmlTool.read(new URL("https://localhost:9999/test"));
} catch (Exception e){
assert (e instanceof DotRuntimeException);
assert (e.getMessage().contains("XMLTool Cannot access private subnets"));
}
}
}
6 changes: 6 additions & 0 deletions dotCMS/src/main/java/com/dotcms/http/CircuitBreakerUrl.java
Expand Up @@ -3,6 +3,7 @@
import com.dotcms.rest.EmptyHttpResponse;
import com.dotcms.rest.api.v1.DotObjectMapperProvider;
import com.dotcms.rest.exception.BadRequestException;
import com.dotcms.util.network.IPUtils;
import com.dotmarketing.business.DotStateException;
import com.dotmarketing.exception.DotRuntimeException;
import com.dotmarketing.util.Config;
Expand Down Expand Up @@ -193,6 +194,11 @@ public void doOut(final HttpServletResponse response) throws IOException {
.setConnectionRequestTimeout(Math.toIntExact(this.timeoutMs))
.setSocketTimeout(Math.toIntExact(this.timeoutMs)).build();
try (CloseableHttpClient httpclient = HttpClientBuilder.create().setDefaultRequestConfig(config).build()) {

if(IPUtils.isIpPrivateSubnet(this.request.getURI().getHost()) && !Config.getBooleanProperty("ALLOW_ACCESS_TO_PRIVATE_SUBNETS", false)){
throw new DotRuntimeException("Remote HttpRequests cannot access private subnets. Set ALLOW_ACCESS_TO_PRIVATE_SUBNETS=true to allow");
}

HttpResponse innerResponse = httpclient.execute(this.request);

this.responseHeaders = innerResponse.getAllHeaders();
Expand Down
Expand Up @@ -22,6 +22,9 @@
import com.dotcms.rendering.velocity.viewtools.bean.XmlToolDoc;
import com.dotcms.rendering.velocity.viewtools.cache.XmlToolCache;
import com.dotcms.rendering.velocity.viewtools.util.ConversionUtils;
import com.dotcms.util.network.IPUtils;
import com.dotmarketing.exception.DotRuntimeException;
import com.dotmarketing.util.Config;
import java.net.URL;
import java.util.ArrayList;
import java.util.Collections;
Expand Down Expand Up @@ -146,8 +149,17 @@ protected void read(String file) throws Exception {
* this instance.
*/
protected void read(URL url) throws Exception {
SAXReader reader = new SAXReader();
setRoot(reader.read(url));


if(IPUtils.isIpPrivateSubnet(url.getHost()) && !Config.getBooleanProperty("ALLOW_ACCESS_TO_PRIVATE_SUBNETS", false)){
throw new DotRuntimeException("XMLTool Cannot access private subnets. Set ALLOW_ACCESS_TO_PRIVATE_SUBNETS=true to allow");
}

SAXReader reader = new SAXReader();
reader.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
reader.setFeature("http://xml.org/sax/features/external-general-entities", false);
reader.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
setRoot(reader.read(url));
}

/**
Expand Down
25 changes: 12 additions & 13 deletions dotCMS/src/main/java/com/dotcms/util/network/IPUtils.java
@@ -1,12 +1,12 @@
package com.dotcms.util.network;

import java.net.InetAddress;
import java.util.Objects;
import org.xbill.DNS.Address;
import com.dotcms.repackage.org.apache.commons.net.util.SubnetUtils;
import com.dotmarketing.exception.DotRuntimeException;
import com.dotmarketing.util.Config;
import com.dotmarketing.util.Logger;
import com.dotmarketing.util.UtilMethods;
import io.vavr.Lazy;
import io.vavr.control.Try;

public class IPUtils {
Expand Down Expand Up @@ -43,9 +43,14 @@ public static boolean isIpInCIDR(final String ip, final String CIDR) {

}

final static private String[] privateSubnets = {"10.0.0.0/8","172.16.0.0/12", "192.168.0.0/16"};
final static private String[] REMOTE_CALL_SUBNET_BLACKLIST_DEFAULT = {"127.0.0.1/32","10.0.0.0/8","172.16.0.0/12", "192.168.0.0/16"};


final static Lazy<String[]> disallowedSubnets = Lazy.of(() ->
Try.of(() -> Config.getStringArrayProperty("REMOTE_CALL_SUBNET_BLACKLIST", REMOTE_CALL_SUBNET_BLACKLIST_DEFAULT))
.getOrElse(REMOTE_CALL_SUBNET_BLACKLIST_DEFAULT));



/**
* It is important when we allow calling to remote endpoints that we verify
Expand All @@ -57,24 +62,18 @@ public static boolean isIpInCIDR(final String ip, final String CIDR) {
*/
public static boolean isIpPrivateSubnet(final String ipOrHostName) {



if (ipOrHostName == null) {
return true;
}

try {
InetAddress addr = Address.getByName(ipOrHostName);

final String ip = addr.getHostAddress();
final String ip = "localhost".equals(ipOrHostName) ? "127.0.0.1" : Address.getByName(ipOrHostName).getHostAddress();

if ("127.0.0.1".equals(ip)) {
return true;
}

if ("localhost".equals(ip)) {
return true;
}

for (String subnet : privateSubnets) {
for (String subnet : disallowedSubnets.get()) {
if (isIpInCIDR(ip, subnet)) {
return true;
}
Expand Down
Expand Up @@ -64,7 +64,8 @@ public void test_false_cases() {
"127.0.0.1",
"172.16.3.5",
"172.16.3.0",
"localhost"
"localhost",
"127.0.0.1"
};

final static String[] ipsOnPublicSubnets= {
Expand Down

0 comments on commit 0b62547

Please sign in to comment.