Merge branch 'security-stable-1.609' into security-stable-1.625

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.
@@ -1456,7 +1458,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
@@ -1465,6 +1472,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.

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;
         }

core/src/main/java/hudson/model/ParametersDefinitionProperty.java

@@ -158,7 +158,7 @@ public class ParametersDefinitionProperty extends JobProperty<Job<?, ?>>
                 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

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,

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.
@@ -4026,6 +4027,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.

core/src/test/java/hudson/UtilTest.java

@@ -344,6 +344,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 {
 

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());
+    }
+
 }

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 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());
+    }
+
 }

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("<hudson/>");
+        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.
      */