diff --git a/core/src/main/java/hudson/Util.java b/core/src/main/java/hudson/Util.java index e9f09e8187a0b058e83b605fed262ba69975ecf5..c5bbf5ba88d9f372a17e3ca6d33b3bbf84675abe 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 550ee5ad8069b9f590a86ae361afe5b680082be6..7fce10a0fbb137234bfdd4baf19b7cbb42ae9765 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 e271bb5bd97d741f3dfa37af373ac51c69e1c48a..df04f4bcc76b736ae436d5f0d4bccdab01c426c3 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 8d6fec50dcb6fcfe09fc3d62cffd2d99f895b263..bda002cf106fd51e1c09345ae1b3f160759e3466 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 5be69d004b2c0c367562969bcc6b2dab5b8f58e6..028c9e00684a85583a6227253cc7434c7bdb070e 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 {