Merge pull request #90 from jenkinsci-cert/SECURITY-321

[SECURITY-321] Prevent existing items from being overwritten even if you cannot DISCOVER them

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

@@ -45,7 +45,6 @@
 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;
@@ -76,7 +75,6 @@
 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;
@@ -236,27 +234,10 @@ protected void renameTo(final String newName) throws IOException {
                 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

@@ -262,10 +262,7 @@ public synchronized TopLevelItem createProjectFromXML(String name, InputStream x
         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();
@@ -318,9 +315,7 @@ public synchronized TopLevelItem createProject( TopLevelItemDescriptor type, Str
         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 org.acegisecurity.context.SecurityContext;
+import org.acegisecurity.context.SecurityContextHolder;
 import org.apache.commons.io.FileUtils;
 
 /**
@@ -419,9 +421,7 @@ public static <I extends AbstractItem & TopLevelItem> I move(I item, DirectlyMod
             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);
@@ -434,6 +434,33 @@ public static <I extends AbstractItem & TopLevelItem> I move(I item, DirectlyMod
         return newItem;
     }
 
+    /**
+     * 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

@@ -525,39 +525,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.
      */