diff --git a/core/src/main/java/hudson/Util.java b/core/src/main/java/hudson/Util.java index 8403a8fd2e..4e2c30436e 100644 --- a/core/src/main/java/hudson/Util.java +++ b/core/src/main/java/hudson/Util.java @@ -91,6 +91,8 @@ 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. @@ -1421,7 +1423,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 @@ -1430,6 +1437,13 @@ public class Util { return idx<_indexOf(uri, '#') && idx<_indexOf(uri,'?') && idx<_indexOf(uri,'/'); } + /** + * Return true iff the parameter does not denote an absolute URI and not a scheme-relative URI. + */ + public static boolean isSafeToRedirectTo(@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 8be60142d0..29cffef47f 100644 --- a/core/src/main/java/hudson/model/DirectoryBrowserSupport.java +++ b/core/src/main/java/hudson/model/DirectoryBrowserSupport.java @@ -160,7 +160,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.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 65c948c419..d4fcd0e516 100644 --- a/core/src/main/java/hudson/model/ParametersDefinitionProperty.java +++ b/core/src/main/java/hudson/model/ParametersDefinitionProperty.java @@ -163,7 +163,7 @@ public class ParametersDefinitionProperty extends OptionalJobProperty> 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.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 de1a0cb3ce..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.isAbsoluteUri(targetUrl)) + if (!Util.isSafeToRedirectTo(targetUrl)) return "."; // avoid open redirect // URL returned from determineTargetUrl() is resolved against the context path, diff --git a/core/src/main/java/jenkins/model/Jenkins.java b/core/src/main/java/jenkins/model/Jenkins.java index 3e37545782..c5134d8ac6 100644 --- a/core/src/main/java/jenkins/model/Jenkins.java +++ b/core/src/main/java/jenkins/model/Jenkins.java @@ -310,6 +310,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. @@ -4492,6 +4493,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/core/src/test/java/hudson/UtilTest.java b/core/src/test/java/hudson/UtilTest.java index f413ffe9d9..b1df3da148 100644 --- a/core/src/test/java/hudson/UtilTest.java +++ b/core/src/test/java/hudson/UtilTest.java @@ -457,6 +457,26 @@ 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")); + assertTrue(Util.isSafeToRedirectTo("/")); + assertTrue(Util.isSafeToRedirectTo("/foo")); + assertTrue(Util.isSafeToRedirectTo("..")); + assertTrue(Util.isSafeToRedirectTo("../..")); + assertTrue(Util.isSafeToRedirectTo("/#foo")); + assertTrue(Util.isSafeToRedirectTo("/?foo")); + } + @Test public void loadProperties() throws IOException { diff --git a/test/src/test/java/hudson/cli/GetNodeCommandTest.java b/test/src/test/java/hudson/cli/GetNodeCommandTest.java index b6f7c575da..3c98d310bf 100644 --- a/test/src/test/java/hudson/cli/GetNodeCommandTest.java +++ b/test/src/test/java/hudson/cli/GetNodeCommandTest.java @@ -37,6 +37,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 GetNodeCommandTest { @@ -90,4 +91,18 @@ public class GetNodeCommandTest { assertThat(result, failedWith(3)); 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 f03b7d2a5f..23b5de2568 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(3)); assertThat(result, hasNoStandardOutput()); } + + @Issue("SECURITY-281") + @Test + 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)); + 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()); + } + } diff --git a/test/src/test/java/hudson/model/NodeTest.java b/test/src/test/java/hudson/model/NodeTest.java index 093e06b2c0..de1dce66b7 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; @@ -406,6 +411,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. */