Merge branch 'security-stable-2.32' into security-master

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

@@ -44,7 +44,6 @@ import hudson.util.Secret;
 import jenkins.model.DirectlyModifiableTopLevelItemGroup;
 import jenkins.model.Jenkins;
 import jenkins.security.NotReallyRoleSensitiveCallable;
-import org.acegisecurity.Authentication;
 import jenkins.util.xml.XMLUtils;
 
 import org.apache.tools.ant.taskdefs.Copy;
@@ -75,7 +74,6 @@ import org.kohsuke.stapler.interceptor.RequirePOST;
 import org.xml.sax.SAXException;
 
 import javax.servlet.ServletException;
-import javax.servlet.ServletOutputStream;
 import javax.xml.transform.Source;
 import javax.xml.transform.TransformerException;
 import javax.xml.transform.stream.StreamResult;
@@ -235,27 +233,10 @@ public abstract class AbstractItem extends Actionable implements Item, HttpDelet
                 if (this.name.equals(newName))
                     return;
 
-                // the test to see if the project already exists or not needs to be done in escalated privilege
-                // to avoid overwriting
-                ACL.impersonate(ACL.SYSTEM,new NotReallyRoleSensitiveCallable<Void,IOException>() {
-                    final Authentication user = Jenkins.getAuthentication();
-                    @Override
-                    public Void call() throws IOException {
-                        Item existing = parent.getItem(newName);
-                        if (existing != null && existing!=AbstractItem.this) {
-                            if (existing.getACL().hasPermission(user,Item.DISCOVER))
-                                // the look up is case insensitive, so we need "existing!=this"
-                                // to allow people to rename "Foo" to "foo", for example.
-                                // see http://www.nabble.com/error-on-renaming-project-tt18061629.html
-                                throw new IllegalArgumentException("Job " + newName + " already exists");
-                            else {
-                                // can't think of any real way to hide this, but at least the error message could be vague.
-                                throw new IOException("Unable to rename to " + newName);
-                            }
-                        }
-                        return null;
-                    }
-                });
+                // the lookup is case insensitive, so we should not fail if this item was the “existing” one
+                // to allow people to rename "Foo" to "foo", for example.
+                // see http://www.nabble.com/error-on-renaming-project-tt18061629.html
+                Items.verifyItemDoesNotAlreadyExist(parent, newName, this);
 
                 File oldRoot = this.getRootDir();
 

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

@@ -260,10 +260,7 @@ public abstract class ItemGroupMixIn {
         acl.checkPermission(Item.CREATE);
 
         Jenkins.getInstance().getProjectNamingStrategy().checkName(name);
-        if (parent.getItem(name) != null) {
-            throw new IllegalArgumentException(parent.getDisplayName() + " already contains an item '" + name + "'");
-        }
-        // TODO what if we have no DISCOVER permission on the existing job?
+        Items.verifyItemDoesNotAlreadyExist(parent, name, null);
 
         // place it as config.xml
         File configXml = Items.getConfigFile(getRootDirFor(name)).getFile();
@@ -316,9 +313,7 @@ public abstract class ItemGroupMixIn {
         acl.getACL().checkCreatePermission(parent, type);
 
         Jenkins.getInstance().getProjectNamingStrategy().checkName(name);
-        if(parent.getItem(name)!=null)
-            throw new IllegalArgumentException("Project of the name "+name+" already exists");
-        // TODO problem with DISCOVER as noted above
+        Items.verifyItemDoesNotAlreadyExist(parent, name, null);
 
         TopLevelItem item = type.newInstance(parent, name);
         try {

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

@@ -52,6 +52,8 @@ import javax.annotation.Nonnull;
 import jenkins.model.DirectlyModifiableTopLevelItemGroup;
 import jenkins.model.Jenkins;
 import org.acegisecurity.Authentication;
+import org.acegisecurity.context.SecurityContext;
+import org.acegisecurity.context.SecurityContextHolder;
 import org.apache.commons.io.FileUtils;
 import org.apache.commons.lang.StringUtils;
 
@@ -493,9 +495,7 @@ public class Items {
             throw new IllegalArgumentException();
         }
         String name = item.getName();
-        if (destination.getItem(name) != null) {
-            throw new IllegalArgumentException(name + " already exists");
-        }
+        verifyItemDoesNotAlreadyExist(destination, name, null);
         String oldFullName = item.getFullName();
         // TODO AbstractItem.renameTo has a more baroque implementation; factor it out into a utility method perhaps?
         File destDir = destination.getRootDirFor(item);
@@ -623,6 +623,33 @@ public class Items {
         }
     }
 
+    /**
+     * Securely check for the existence of an item before trying to create one with the same name.
+     * @param parent the folder where we are about to create/rename/move an item
+     * @param newName the proposed new name
+     * @param variant if not null, an existing item which we accept could be there
+     * @throws IllegalArgumentException if there is already something there, which you were supposed to know about
+     * @throws Failure if there is already something there but you should not be told details
+     */
+    static void verifyItemDoesNotAlreadyExist(@Nonnull ItemGroup<?> parent, @Nonnull String newName, @CheckForNull Item variant) throws IllegalArgumentException, Failure {
+        Item existing;
+        SecurityContext orig = ACL.impersonate(ACL.SYSTEM);
+        try {
+            existing = parent.getItem(newName);
+        } finally {
+            SecurityContextHolder.setContext(orig);
+        }
+        if (existing != null && existing != variant) {
+            if (existing.hasPermission(Item.DISCOVER)) {
+                String prefix = parent.getFullName();
+                throw new IllegalArgumentException((prefix.isEmpty() ? "" : prefix + "/") + newName + " already exists");
+            } else {
+                // Cannot hide its existence, so at least be as vague as possible.
+                throw new Failure("");
+            }
+        }
+    }
+
     /**
      * Used to load/save job configuration.
      *

test/src/test/groovy/hudson/model/AbstractProjectTest.groovy

@@ -519,39 +519,6 @@ public class AbstractProjectTest extends HudsonTestCase {
         done.signal()
     }
 
-    public void testRenameToPrivileged() {
-        def secret = jenkins.createProject(FreeStyleProject.class,"secret");
-        def regular = jenkins.createProject(FreeStyleProject.class,"regular")
-
-        jenkins.securityRealm = createDummySecurityRealm();
-        def auth = new ProjectMatrixAuthorizationStrategy();
-        jenkins.authorizationStrategy = auth;
-
-        auth.add(Jenkins.ADMINISTER, "alice");
-        auth.add(Jenkins.READ, "bob");
-
-        // bob the regular user can only see regular jobs
-        regular.addProperty(new AuthorizationMatrixProperty([(Job.READ) : ["bob"] as Set]));
-
-        def wc = createWebClient()
-        wc.login("bob")
-        wc.executeOnServer {
-            assert jenkins.getItem("secret")==null;
-            try {
-                regular.renameTo("secret")
-                fail("rename as an overwrite should have failed");
-            } catch (Exception e) {
-                // expected rename to fail in some non-descriptive generic way
-                e.printStackTrace()
-            }
-        }
-
-        // those two jobs should still be there
-        assert jenkins.getItem("regular")!=null;
-        assert jenkins.getItem("secret")!=null;
-    }
-
-
     /**
      * Trying to POST to config.xml by a different job type should fail.
      */

test/src/test/java/hudson/model/ItemsTest.java

@@ -24,8 +24,26 @@
 
 package hudson.model;
 
+import com.gargoylesoftware.htmlunit.FailingHttpStatusCodeException;
+import com.gargoylesoftware.htmlunit.HttpMethod;
+import com.gargoylesoftware.htmlunit.WebRequest;
+import com.gargoylesoftware.htmlunit.WebResponse;
+import hudson.AbortException;
+import hudson.cli.CLICommand;
+import hudson.cli.CLICommandInvoker;
+import hudson.cli.CopyJobCommand;
+import hudson.cli.CreateJobCommand;
+import hudson.security.ACL;
+import hudson.security.csrf.CrumbIssuer;
+import java.io.ByteArrayInputStream;
 import java.io.File;
+import java.io.IOException;
+import java.net.URL;
 import java.util.Arrays;
+import jenkins.model.Jenkins;
+import org.acegisecurity.context.SecurityContext;
+import org.acegisecurity.context.SecurityContextHolder;
+import org.apache.commons.httpclient.HttpStatus;
 
 import org.junit.Test;
 
@@ -35,6 +53,7 @@ import org.junit.Rule;
 import org.junit.rules.TemporaryFolder;
 import org.jvnet.hudson.test.Issue;
 import org.jvnet.hudson.test.JenkinsRule;
+import org.jvnet.hudson.test.MockAuthorizationStrategy;
 import org.jvnet.hudson.test.MockFolder;
 
 public class ItemsTest {
@@ -100,5 +119,171 @@ public class ItemsTest {
         assertFalse(new File(tmp, "foo/test/1").exists());
         assertTrue(new File(tmp, "bar/test/1").exists());
     }
+    
+    // TODO would be more efficient to run these all as a single test case, but after a few Jetty seems to stop serving new content and new requests just hang.
+
+    private void overwriteTargetSetUp() throws Exception {
+        // A fully visible item:
+        r.createFreeStyleProject("visible").setDescription("visible");
+        // An item known to exist but not visible:
+        r.createFreeStyleProject("known").setDescription("known");
+        // An item not even known to exist:
+        r.createFreeStyleProject("secret").setDescription("secret");
+        // A folder from which to launch move attacks:
+        r.createFolder("d");
+        r.jenkins.setSecurityRealm(r.createDummySecurityRealm());
+        r.jenkins.setAuthorizationStrategy(new MockAuthorizationStrategy().
+            grant(Jenkins.READ).everywhere().to("attacker").
+            grant(Item.READ, Item.CONFIGURE, Item.CREATE, Item.DELETE).onPaths("(?!known|secret).*").to("attacker").
+            grant(Item.DISCOVER).onPaths("known").to("attacker"));
+    }
+
+    /** Control cases: if there is no such item yet, nothing is stopping you. */
+    @Test public void overwriteNonexistentTarget() throws Exception {
+        overwriteTargetSetUp();
+        for (OverwriteTactic tactic : OverwriteTactic.values()) {
+            tactic.run(r, "nonexistent");
+            System.out.println(tactic + " worked as expected on a nonexistent target");
+            r.jenkins.getItem("nonexistent").delete();
+        }
+    }
+
+    private void cannotOverwrite(String target) throws Exception {
+        overwriteTargetSetUp();
+        for (OverwriteTactic tactic : OverwriteTactic.values()) {
+            try {
+                tactic.run(r, target);
+                fail(tactic + " was not supposed to work against " + target);
+            } catch (Exception x) {
+                System.out.println("good, " + tactic + " failed on " + target + ": " + x);
+                assertEquals(tactic + " still overwrote " + target, target, r.jenkins.getItemByFullName(target, FreeStyleProject.class).getDescription());
+            }
+        }
+    }
+
+    /** More control cases: for non-security-sensitive scenarios, we prevent you from overwriting existing items. */
+    @Test public void overwriteVisibleTarget() throws Exception {
+        cannotOverwrite("visible");
+    }
+
+    /** You may not overwrite an item you know is there even if you cannot see it. */
+    @Test public void overwriteKnownTarget() throws Exception {
+        cannotOverwrite("known");
+    }
+
+    /** You are somehow prevented from overwriting an item even if you did not previously know it was there. */
+    @Issue("SECURITY-321")
+    @Test public void overwriteHiddenTarget() throws Exception {
+        cannotOverwrite("secret");
+    }
+
+    /** All known means of creating an item under a new name. */
+    private enum OverwriteTactic {
+        /** Use the REST command to create an empty project (normally used only from the UI in the New Item dialog). */
+        REST_EMPTY {
+            @Override void run(JenkinsRule r, String target) throws Exception {
+                JenkinsRule.WebClient wc = wc(r);
+                wc.getOptions().setRedirectEnabled(false);
+                wc.getOptions().setThrowExceptionOnFailingStatusCode(false); // redirect perversely counts as a failure
+                WebResponse webResponse = wc.getPage(new WebRequest(createCrumbedUrl(r, wc, "createItem?name=" + target + "&mode=hudson.model.FreeStyleProject"), HttpMethod.POST)).getWebResponse();
+                if (webResponse.getStatusCode() != HttpStatus.SC_MOVED_TEMPORARILY) {
+                    throw new FailingHttpStatusCodeException(webResponse);
+                }
+            }
+        },
+        /** Use the REST command to copy an existing project (normally used from the UI in the New Item dialog). */
+        REST_COPY {
+            @Override void run(JenkinsRule r, String target) throws Exception {
+                r.createFreeStyleProject("dupe");
+                JenkinsRule.WebClient wc = wc(r);
+                wc.getOptions().setRedirectEnabled(false);
+                wc.getOptions().setThrowExceptionOnFailingStatusCode(false);
+                WebResponse webResponse = wc.getPage(new WebRequest(createCrumbedUrl(r, wc, "createItem?name=" + target + "&mode=copy&from=dupe"), HttpMethod.POST)).getWebResponse();
+                r.jenkins.getItem("dupe").delete();
+                if (webResponse.getStatusCode() != HttpStatus.SC_MOVED_TEMPORARILY) {
+                    throw new FailingHttpStatusCodeException(webResponse);
+                }
+            }
+        },
+        /** Overwrite target using REST command to create a project from XML submission. */
+        REST_CREATE {
+            @Override void run(JenkinsRule r, String target) throws Exception {
+                JenkinsRule.WebClient wc = wc(r);
+                WebRequest req = new WebRequest(createCrumbedUrl(r, wc, "createItem?name=" + target), HttpMethod.POST);
+                req.setAdditionalHeader("Content-Type", "application/xml");
+                req.setRequestBody("<project/>");
+                wc.getPage(req);
+            }
+        },
+        /** Overwrite target using REST command to rename an existing project (normally used from the UI in the Configure screen). */
+        REST_RENAME {
+            @Override void run(JenkinsRule r, String target) throws Exception {
+                r.createFreeStyleProject("dupe");
+                JenkinsRule.WebClient wc = wc(r);
+                wc.getOptions().setRedirectEnabled(false);
+                wc.getOptions().setThrowExceptionOnFailingStatusCode(false);
+                WebResponse webResponse = wc.getPage(new WebRequest(createCrumbedUrl(r, wc, "job/dupe/doRename?newName=" + target), HttpMethod.POST)).getWebResponse();
+                if (webResponse.getStatusCode() != HttpStatus.SC_MOVED_TEMPORARILY) {
+                    r.jenkins.getItem("dupe").delete();
+                    throw new FailingHttpStatusCodeException(webResponse);
+                }
+                assertNull(r.jenkins.getItem("dupe"));
+            }
+        },
+        /** Overwrite target using the CLI {@code create-job} command. */
+        CLI_CREATE {
+            @Override void run(JenkinsRule r, String target) throws Exception {
+                CLICommand cmd = new CreateJobCommand();
+                CLICommandInvoker invoker = new CLICommandInvoker(r, cmd);
+                cmd.setTransportAuth(User.get("attacker").impersonate());
+                int status = invoker.withStdin(new ByteArrayInputStream("<project/>".getBytes("US-ASCII"))).invokeWithArgs(target).returnCode();
+                if (status != 0) {
+                    throw new AbortException("CLI command failed with status " + status);
+                }
+            }
+        },
+        /** Overwrite target using the CLI {@code copy-job} command. */
+        CLI_COPY {
+            @Override void run(JenkinsRule r, String target) throws Exception {
+                r.createFreeStyleProject("dupe");
+                CLICommand cmd = new CopyJobCommand();
+                CLICommandInvoker invoker = new CLICommandInvoker(r, cmd);
+                cmd.setTransportAuth(User.get("attacker").impersonate());
+                int status = invoker.invokeWithArgs("dupe", target).returnCode();
+                r.jenkins.getItem("dupe").delete();
+                if (status != 0) {
+                    throw new AbortException("CLI command failed with status " + status);
+                }
+            }
+        },
+        /** Overwrite target using a move function normally called from {@code cloudbees-folder} via a {@code move} action. */
+        MOVE {
+            @Override void run(JenkinsRule r, String target) throws Exception {
+                try {
+                    SecurityContext orig = ACL.impersonate(User.get("attacker").impersonate());
+                    try {
+                        Items.move(r.jenkins.getItemByFullName("d", MockFolder.class).createProject(FreeStyleProject.class, target), r.jenkins);
+                    } finally {
+                        SecurityContextHolder.setContext(orig);
+                    }
+                    assertNull(r.jenkins.getItemByFullName("d/" + target));
+                } catch (Exception x) {
+                    r.jenkins.getItemByFullName("d/" + target).delete();
+                    throw x;
+                }
+            }
+        };
+        abstract void run(JenkinsRule r, String target) throws Exception;
+        private static final JenkinsRule.WebClient wc(JenkinsRule r) throws Exception {
+            return r.createWebClient().login("attacker");
+        }
+        // TODO replace with standard version once it is fixed to detect an existing query string
+        private static URL createCrumbedUrl(JenkinsRule r, JenkinsRule.WebClient wc, String relativePath) throws IOException {
+            CrumbIssuer issuer = r.jenkins.getCrumbIssuer();
+            String crumbName = issuer.getDescriptor().getCrumbRequestField();
+            String crumb = issuer.getCrumb(null);
+            return new URL(wc.getContextPath() + relativePath + (relativePath.contains("?") ? "&" : "?") + crumbName + "=" + crumb);
+        }
+    }
 
 }