From 2ed0c046dfbb2003a17df27c53777e72c6eaff25 Mon Sep 17 00:00:00 2001 From: Daniel Beck Date: Mon, 25 Apr 2016 19:50:53 +0200 Subject: [PATCH 1/6] [FIX SECURITY-276] Don't allow open redirect using scheme-rel. URL --- core/src/main/java/hudson/Util.java | 14 ++++++++++++++ .../java/hudson/model/DirectoryBrowserSupport.java | 2 +- .../hudson/model/ParametersDefinitionProperty.java | 2 +- .../security/AuthenticationProcessingFilter2.java | 2 +- 4 files changed, 17 insertions(+), 3 deletions(-) diff --git a/core/src/main/java/hudson/Util.java b/core/src/main/java/hudson/Util.java index 3a1ba13864..e9f09e8187 100644 --- a/core/src/main/java/hudson/Util.java +++ b/core/src/main/java/hudson/Util.java @@ -77,6 +77,8 @@ import javax.annotation.CheckForNull; import javax.annotation.Nonnull; import javax.annotation.Nullable; import org.apache.commons.codec.digest.DigestUtils; +import org.kohsuke.accmod.Restricted; +import org.kohsuke.accmod.restrictions.NoExternalUse; /** * Various utility methods that don't have more proper home. @@ -1454,7 +1456,12 @@ public class Util { * The same algorithm can be seen in {@link URI}, but * implementing this by ourselves allow it to be more lenient about * escaping of URI. + * + * @deprecated Use {@code isAbsoluteOrSchemeRelativeUri} instead if your goal is to prevent open redirects */ + @Deprecated + @RestrictedSince("1.651.2 / 2.TODO") + @Restricted(NoExternalUse.class) public static boolean isAbsoluteUri(@Nonnull String uri) { int idx = uri.indexOf(':'); if (idx<0) return false; // no ':'. can't be absolute @@ -1463,6 +1470,13 @@ public class Util { return idx<_indexOf(uri, '#') && idx<_indexOf(uri,'?') && idx<_indexOf(uri,'/'); } + /** + * Return true iff the parameter denotes an absolute URI, or a scheme-relative URI. + */ + public static boolean isAbsoluteOrSchemeRelativeUri(@Nonnull String uri) { + return isAbsoluteUri(uri) || uri.startsWith("//"); + } + /** * Works like {@link String#indexOf(int)} but 'not found' is returned as s.length(), not -1. * This enables more straight-forward comparison. diff --git a/core/src/main/java/hudson/model/DirectoryBrowserSupport.java b/core/src/main/java/hudson/model/DirectoryBrowserSupport.java index 7c562dcb12..550ee5ad80 100644 --- a/core/src/main/java/hudson/model/DirectoryBrowserSupport.java +++ b/core/src/main/java/hudson/model/DirectoryBrowserSupport.java @@ -158,7 +158,7 @@ public final class DirectoryBrowserSupport implements HttpResponse { String pattern = req.getParameter("pattern"); if(pattern==null) pattern = req.getParameter("path"); // compatibility with Hudson<1.129 - if(pattern!=null && !Util.isAbsoluteUri(pattern)) {// avoid open redirect + if(pattern!=null && !Util.isAbsoluteOrSchemeRelativeUri(pattern)) {// avoid open redirect rsp.sendRedirect2(pattern); return; } diff --git a/core/src/main/java/hudson/model/ParametersDefinitionProperty.java b/core/src/main/java/hudson/model/ParametersDefinitionProperty.java index 13a6156b5c..e271bb5bd9 100644 --- a/core/src/main/java/hudson/model/ParametersDefinitionProperty.java +++ b/core/src/main/java/hudson/model/ParametersDefinitionProperty.java @@ -158,7 +158,7 @@ public class ParametersDefinitionProperty extends JobProperty> getJob(), delay.getTime(), new ParametersAction(values), new CauseAction(new Cause.UserIdCause())); if (item!=null) { String url = formData.optString("redirectTo"); - if (url==null || Util.isAbsoluteUri(url)) // avoid open redirect + if (url==null || Util.isAbsoluteOrSchemeRelativeUri(url)) // avoid open redirect url = req.getContextPath()+'/'+item.getUrl(); rsp.sendRedirect(formData.optInt("statusCode",SC_CREATED), url); } else diff --git a/core/src/main/java/hudson/security/AuthenticationProcessingFilter2.java b/core/src/main/java/hudson/security/AuthenticationProcessingFilter2.java index de1a0cb3ce..8d6fec50dc 100644 --- a/core/src/main/java/hudson/security/AuthenticationProcessingFilter2.java +++ b/core/src/main/java/hudson/security/AuthenticationProcessingFilter2.java @@ -53,7 +53,7 @@ public class AuthenticationProcessingFilter2 extends AuthenticationProcessingFil if (targetUrl == null) return getDefaultTargetUrl(); - if (Util.isAbsoluteUri(targetUrl)) + if (Util.isAbsoluteOrSchemeRelativeUri(targetUrl)) return "."; // avoid open redirect // URL returned from determineTargetUrl() is resolved against the context path, From e6efae7d0d742f068498366424b0059e9ed42e4c Mon Sep 17 00:00:00 2001 From: Daniel Beck Date: Wed, 27 Apr 2016 13:51:10 +0200 Subject: [PATCH 2/6] [SECURITY-276] Better method name, add tests --- core/src/main/java/hudson/Util.java | 6 +++--- .../java/hudson/model/DirectoryBrowserSupport.java | 2 +- .../hudson/model/ParametersDefinitionProperty.java | 2 +- .../security/AuthenticationProcessingFilter2.java | 2 +- core/src/test/java/hudson/UtilTest.java | 14 ++++++++++++++ 5 files changed, 20 insertions(+), 6 deletions(-) diff --git a/core/src/main/java/hudson/Util.java b/core/src/main/java/hudson/Util.java index e9f09e8187..c5bbf5ba88 100644 --- a/core/src/main/java/hudson/Util.java +++ b/core/src/main/java/hudson/Util.java @@ -1471,10 +1471,10 @@ public class Util { } /** - * Return true iff the parameter denotes an absolute URI, or a scheme-relative URI. + * Return true iff the parameter does not denote an absolute URI and not a scheme-relative URI. */ - public static boolean isAbsoluteOrSchemeRelativeUri(@Nonnull String uri) { - return isAbsoluteUri(uri) || uri.startsWith("//"); + public static boolean isSafeToRedirectTo(@Nonnull String uri) { + return !isAbsoluteUri(uri) && !uri.startsWith("//"); } /** diff --git a/core/src/main/java/hudson/model/DirectoryBrowserSupport.java b/core/src/main/java/hudson/model/DirectoryBrowserSupport.java index 550ee5ad80..7fce10a0fb 100644 --- a/core/src/main/java/hudson/model/DirectoryBrowserSupport.java +++ b/core/src/main/java/hudson/model/DirectoryBrowserSupport.java @@ -158,7 +158,7 @@ public final class DirectoryBrowserSupport implements HttpResponse { String pattern = req.getParameter("pattern"); if(pattern==null) pattern = req.getParameter("path"); // compatibility with Hudson<1.129 - if(pattern!=null && !Util.isAbsoluteOrSchemeRelativeUri(pattern)) {// avoid open redirect + if(pattern!=null && Util.isSafeToRedirectTo(pattern)) {// avoid open redirect rsp.sendRedirect2(pattern); return; } diff --git a/core/src/main/java/hudson/model/ParametersDefinitionProperty.java b/core/src/main/java/hudson/model/ParametersDefinitionProperty.java index e271bb5bd9..df04f4bcc7 100644 --- a/core/src/main/java/hudson/model/ParametersDefinitionProperty.java +++ b/core/src/main/java/hudson/model/ParametersDefinitionProperty.java @@ -158,7 +158,7 @@ public class ParametersDefinitionProperty extends JobProperty> getJob(), delay.getTime(), new ParametersAction(values), new CauseAction(new Cause.UserIdCause())); if (item!=null) { String url = formData.optString("redirectTo"); - if (url==null || Util.isAbsoluteOrSchemeRelativeUri(url)) // avoid open redirect + if (url==null || !Util.isSafeToRedirectTo(url)) // avoid open redirect url = req.getContextPath()+'/'+item.getUrl(); rsp.sendRedirect(formData.optInt("statusCode",SC_CREATED), url); } else diff --git a/core/src/main/java/hudson/security/AuthenticationProcessingFilter2.java b/core/src/main/java/hudson/security/AuthenticationProcessingFilter2.java index 8d6fec50dc..bda002cf10 100644 --- a/core/src/main/java/hudson/security/AuthenticationProcessingFilter2.java +++ b/core/src/main/java/hudson/security/AuthenticationProcessingFilter2.java @@ -53,7 +53,7 @@ public class AuthenticationProcessingFilter2 extends AuthenticationProcessingFil if (targetUrl == null) return getDefaultTargetUrl(); - if (Util.isAbsoluteOrSchemeRelativeUri(targetUrl)) + if (!Util.isSafeToRedirectTo(targetUrl)) return "."; // avoid open redirect // URL returned from determineTargetUrl() is resolved against the context path, diff --git a/core/src/test/java/hudson/UtilTest.java b/core/src/test/java/hudson/UtilTest.java index 5be69d004b..028c9e0068 100644 --- a/core/src/test/java/hudson/UtilTest.java +++ b/core/src/test/java/hudson/UtilTest.java @@ -344,6 +344,20 @@ public class UtilTest { assertFalse(Util.isAbsoluteUri("foo/bar")); } + @Test + @Issue("SECURITY-276") + public void testIsSafeToRedirectTo() { + assertFalse(Util.isSafeToRedirectTo("http://foobar/")); + assertFalse(Util.isSafeToRedirectTo("mailto:kk@kohsuke.org")); + assertFalse(Util.isSafeToRedirectTo("d123://test/")); + assertFalse(Util.isSafeToRedirectTo("//google.com")); + + assertTrue(Util.isSafeToRedirectTo("foo/bar/abc:def")); + assertTrue(Util.isSafeToRedirectTo("foo?abc:def")); + assertTrue(Util.isSafeToRedirectTo("foo#abc:def")); + assertTrue(Util.isSafeToRedirectTo("foo/bar")); + } + @Test public void loadProperties() throws IOException { From 13e9967e38d3e4a3317e6566727554c86f10264b Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Wed, 27 Apr 2016 18:38:58 -0400 Subject: [PATCH 3/6] [SECURITY-281] Demonstrating that GetNodeCommand and UpdateNodeCommand refuse to operate on Jenkins itself. --- .../java/hudson/cli/GetNodeCommandTest.java | 17 +++++++++++++++-- .../java/hudson/cli/UpdateNodeCommandTest.java | 15 +++++++++++++++ 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/test/src/test/java/hudson/cli/GetNodeCommandTest.java b/test/src/test/java/hudson/cli/GetNodeCommandTest.java index b80b4fb1f3..1706fd3bfc 100644 --- a/test/src/test/java/hudson/cli/GetNodeCommandTest.java +++ b/test/src/test/java/hudson/cli/GetNodeCommandTest.java @@ -30,15 +30,14 @@ import static hudson.cli.CLICommandInvoker.Matcher.hasNoErrorOutput; import static hudson.cli.CLICommandInvoker.Matcher.succeeded; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.containsString; -import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.startsWith; -import static org.hamcrest.text.IsEmptyString.isEmptyString; import hudson.model.Computer; import jenkins.model.Jenkins; import org.junit.Before; import org.junit.Rule; import org.junit.Test; +import org.jvnet.hudson.test.Issue; import org.jvnet.hudson.test.JenkinsRule; public class GetNodeCommandTest { @@ -92,4 +91,18 @@ public class GetNodeCommandTest { assertThat(result, failedWith(-1)); assertThat(result, hasNoStandardOutput()); } + + @Issue("SECURITY-281") + @Test + public void getNodeShouldFailForMaster() throws Exception { + CLICommandInvoker.Result result = command.authorizedTo(Computer.EXTENDED_READ, Jenkins.READ).invokeWithArgs(""); + assertThat(result.stderr(), containsString("No such node ''")); + assertThat(result, failedWith(-1)); + assertThat(result, hasNoStandardOutput()); + result = command.authorizedTo(Computer.EXTENDED_READ, Jenkins.READ).invokeWithArgs("(master)"); + assertThat(result.stderr(), containsString("No such node '(master)'")); + assertThat(result, failedWith(-1)); + assertThat(result, hasNoStandardOutput()); + } + } diff --git a/test/src/test/java/hudson/cli/UpdateNodeCommandTest.java b/test/src/test/java/hudson/cli/UpdateNodeCommandTest.java index b4a977dbe9..4302598d66 100644 --- a/test/src/test/java/hudson/cli/UpdateNodeCommandTest.java +++ b/test/src/test/java/hudson/cli/UpdateNodeCommandTest.java @@ -38,6 +38,7 @@ import jenkins.model.Jenkins; import org.junit.Before; import org.junit.Rule; import org.junit.Test; +import org.jvnet.hudson.test.Issue; import org.jvnet.hudson.test.JenkinsRule; public class UpdateNodeCommandTest { @@ -96,4 +97,18 @@ public class UpdateNodeCommandTest { assertThat(result, failedWith(-1)); assertThat(result, hasNoStandardOutput()); } + + @Issue("SECURITY-281") + @Test + public void getNodeShouldFailForMaster() throws Exception { + CLICommandInvoker.Result result = command.authorizedTo(Computer.CONFIGURE, Jenkins.READ).withStdin(Computer.class.getResourceAsStream("node.xml")).invokeWithArgs(""); + assertThat(result.stderr(), containsString("No such node ''")); + assertThat(result, failedWith(-1)); + assertThat(result, hasNoStandardOutput()); + result = command.authorizedTo(Computer.EXTENDED_READ, Jenkins.READ).withStdin(Computer.class.getResourceAsStream("node.xml")).invokeWithArgs("(master)"); + assertThat(result.stderr(), containsString("No such node '(master)'")); + assertThat(result, failedWith(-1)); + assertThat(result, hasNoStandardOutput()); + } + } From 38f8b43e8d3c324c21bb8b2b3a9da3be93faf354 Mon Sep 17 00:00:00 2001 From: Daniel Beck Date: Thu, 28 Apr 2016 00:45:43 +0200 Subject: [PATCH 4/6] [SECURITY-276] Add more tests for relative URLs --- core/src/test/java/hudson/UtilTest.java | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/core/src/test/java/hudson/UtilTest.java b/core/src/test/java/hudson/UtilTest.java index 028c9e0068..e37ebedaf0 100644 --- a/core/src/test/java/hudson/UtilTest.java +++ b/core/src/test/java/hudson/UtilTest.java @@ -356,6 +356,12 @@ public class UtilTest { assertTrue(Util.isSafeToRedirectTo("foo?abc:def")); assertTrue(Util.isSafeToRedirectTo("foo#abc:def")); assertTrue(Util.isSafeToRedirectTo("foo/bar")); + assertTrue(Util.isSafeToRedirectTo("/")); + assertTrue(Util.isSafeToRedirectTo("/foo")); + assertTrue(Util.isSafeToRedirectTo("..")); + assertTrue(Util.isSafeToRedirectTo("../..")); + assertTrue(Util.isSafeToRedirectTo("/#foo")); + assertTrue(Util.isSafeToRedirectTo("/?foo")); } @Test From d66ad6f3ee46a5c6bb865bb831e8cdfc74cd7eb3 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Wed, 27 Apr 2016 19:19:37 -0400 Subject: [PATCH 5/6] [FIXED SECURITY-281] MasterComputer.doConfigDotXml should be rejected. --- core/src/main/java/jenkins/model/Jenkins.java | 7 +++++++ test/src/test/java/hudson/model/NodeTest.java | 21 +++++++++++++++++++ 2 files changed, 28 insertions(+) diff --git a/core/src/main/java/jenkins/model/Jenkins.java b/core/src/main/java/jenkins/model/Jenkins.java index 4278f9a6c6..2cc4121132 100644 --- a/core/src/main/java/jenkins/model/Jenkins.java +++ b/core/src/main/java/jenkins/model/Jenkins.java @@ -298,6 +298,7 @@ import static hudson.init.InitMilestone.*; import hudson.util.LogTaskListener; import static java.util.logging.Level.*; import static javax.servlet.http.HttpServletResponse.*; +import org.kohsuke.stapler.WebMethod; /** * Root object of the system. @@ -3996,6 +3997,12 @@ public class Jenkins extends AbstractCIBase implements DirectlyModifiableTopLeve Jenkins.getInstance().doConfigExecutorsSubmit(req, rsp); } + @WebMethod(name="config.xml") + @Override + public void doConfigDotXml(StaplerRequest req, StaplerResponse rsp) throws IOException, ServletException { + throw HttpResponses.status(SC_BAD_REQUEST); + } + @Override public boolean hasPermission(Permission permission) { // no one should be allowed to delete the master. diff --git a/test/src/test/java/hudson/model/NodeTest.java b/test/src/test/java/hudson/model/NodeTest.java index d98818698f..40d12165e7 100644 --- a/test/src/test/java/hudson/model/NodeTest.java +++ b/test/src/test/java/hudson/model/NodeTest.java @@ -23,6 +23,10 @@ */ package hudson.model; +import com.gargoylesoftware.htmlunit.FailingHttpStatusCodeException; +import com.gargoylesoftware.htmlunit.HttpMethod; +import com.gargoylesoftware.htmlunit.Page; +import com.gargoylesoftware.htmlunit.WebRequestSettings; import hudson.EnvVars; import hudson.FilePath; import hudson.maven.MavenModuleSet; @@ -41,6 +45,7 @@ import hudson.slaves.OfflineCause; import hudson.slaves.OfflineCause.ByCLI; import hudson.slaves.OfflineCause.UserCause; import hudson.util.TagCloud; +import java.net.HttpURLConnection; import java.util.*; import java.util.concurrent.Callable; @@ -400,6 +405,22 @@ public class NodeTest { assertThatCloudLabelDoesNotContain(cloud, "label1 label2", 0); } + @Issue("SECURITY-281") + @Test + public void masterComputerConfigDotXml() throws Exception { + JenkinsRule.WebClient wc = j.createWebClient(); + wc.assertFails("computer/(master)/config.xml", HttpURLConnection.HTTP_BAD_REQUEST); + WebRequestSettings settings = new WebRequestSettings(wc.createCrumbedUrl("computer/(master)/config.xml")); + settings.setHttpMethod(HttpMethod.POST); + settings.setRequestBody(""); + try { + Page page = wc.getPage(settings); + fail(page.getWebResponse().getContentAsString()); + } catch (FailingHttpStatusCodeException x) { + assertEquals(HttpURLConnection.HTTP_BAD_REQUEST, x.getStatusCode()); + } + } + /** * Assert that a tag cloud contains label name and weight. */ From bfad4c96dc7f0879d964043b296a79b04519a387 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Wed, 27 Apr 2016 19:22:58 -0400 Subject: [PATCH 6/6] Mistyped test name. --- test/src/test/java/hudson/cli/UpdateNodeCommandTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/src/test/java/hudson/cli/UpdateNodeCommandTest.java b/test/src/test/java/hudson/cli/UpdateNodeCommandTest.java index 4302598d66..2472232cd9 100644 --- a/test/src/test/java/hudson/cli/UpdateNodeCommandTest.java +++ b/test/src/test/java/hudson/cli/UpdateNodeCommandTest.java @@ -100,7 +100,7 @@ public class UpdateNodeCommandTest { @Issue("SECURITY-281") @Test - public void getNodeShouldFailForMaster() throws Exception { + public void updateNodeShouldFailForMaster() throws Exception { CLICommandInvoker.Result result = command.authorizedTo(Computer.CONFIGURE, Jenkins.READ).withStdin(Computer.class.getResourceAsStream("node.xml")).invokeWithArgs(""); assertThat(result.stderr(), containsString("No such node ''")); assertThat(result, failedWith(-1));