diff --git a/dotCMS/src/integration-test/java/com/dotcms/MainSuite.java b/dotCMS/src/integration-test/java/com/dotcms/MainSuite.java index e4ed07cab8cb..ecd8bb6fbadc 100644 --- a/dotCMS/src/integration-test/java/com/dotcms/MainSuite.java +++ b/dotCMS/src/integration-test/java/com/dotcms/MainSuite.java @@ -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; @@ -494,7 +495,8 @@ Task211101AddContentletAsJsonColumnTest.class, ContentletJsonAPITest.class, ContentletJsonAPITest.class, - Task211103RenameHostNameLabelTest.class + Task211103RenameHostNameLabelTest.class, + XmlToolTest.class }) public class MainSuite { diff --git a/dotCMS/src/integration-test/java/com/dotcms/http/CircuitBreakerUrlTest.java b/dotCMS/src/integration-test/java/com/dotcms/http/CircuitBreakerUrlTest.java index 577cb520542f..34ab8818869b 100644 --- a/dotCMS/src/integration-test/java/com/dotcms/http/CircuitBreakerUrlTest.java +++ b/dotCMS/src/integration-test/java/com/dotcms/http/CircuitBreakerUrlTest.java @@ -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; @@ -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()); } /* @@ -86,7 +112,7 @@ public void testBadBreaker() throws ExecutionException { //@Test - public void testHeaders() throws ExecutionException, CircuitBreakerOpenException, IOException { + public void testHeaders() throws CircuitBreakerOpenException, IOException { Map headers = ImmutableMap.of(HEADER, HEADER_VALUE); CircuitBreakerUrl cburl = CircuitBreakerUrl.builder().setMethod(Method.GET).setUrl("http://localhost/get").setTimeout(1000) @@ -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 params = ImmutableMap.of(PARAM, PARAM_VALUE); CircuitBreakerUrl cburl = CircuitBreakerUrl.builder().setMethod(Method.POST).setUrl("http://localhost/post").setTimeout(1000) @@ -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 diff --git a/dotCMS/src/integration-test/java/com/dotcms/rendering/velocity/viewtools/XmlToolTest.java b/dotCMS/src/integration-test/java/com/dotcms/rendering/velocity/viewtools/XmlToolTest.java new file mode 100644 index 000000000000..2c9cf41c6f76 --- /dev/null +++ b/dotCMS/src/integration-test/java/com/dotcms/rendering/velocity/viewtools/XmlToolTest.java @@ -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")); + } + } +} diff --git a/dotCMS/src/main/java/com/dotcms/http/CircuitBreakerUrl.java b/dotCMS/src/main/java/com/dotcms/http/CircuitBreakerUrl.java index 109df941f879..74f1fc4e888b 100644 --- a/dotCMS/src/main/java/com/dotcms/http/CircuitBreakerUrl.java +++ b/dotCMS/src/main/java/com/dotcms/http/CircuitBreakerUrl.java @@ -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; @@ -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(); diff --git a/dotCMS/src/main/java/com/dotcms/rendering/velocity/viewtools/XmlTool.java b/dotCMS/src/main/java/com/dotcms/rendering/velocity/viewtools/XmlTool.java index 14f278d15a88..2f8ff87efee6 100644 --- a/dotCMS/src/main/java/com/dotcms/rendering/velocity/viewtools/XmlTool.java +++ b/dotCMS/src/main/java/com/dotcms/rendering/velocity/viewtools/XmlTool.java @@ -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; @@ -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)); } /** diff --git a/dotCMS/src/main/java/com/dotcms/util/network/IPUtils.java b/dotCMS/src/main/java/com/dotcms/util/network/IPUtils.java index e85b6ebb0614..a8e1b6b4e94c 100644 --- a/dotCMS/src/main/java/com/dotcms/util/network/IPUtils.java +++ b/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 { @@ -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 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 @@ -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; } diff --git a/dotCMS/src/test/java/com/dotcms/util/network/IPUtilsTest.java b/dotCMS/src/test/java/com/dotcms/util/network/IPUtilsTest.java index 5fa47e3ac888..33877e6987b1 100644 --- a/dotCMS/src/test/java/com/dotcms/util/network/IPUtilsTest.java +++ b/dotCMS/src/test/java/com/dotcms/util/network/IPUtilsTest.java @@ -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= {