From ba33bd67cdaef87aba8a4e95dca8dcf108a7d73f Mon Sep 17 00:00:00 2001
From: Daniel Beck <daniel-beck@users.noreply.github.com>
Date: Thu, 27 Sep 2018 11:38:33 +0200
Subject: [PATCH] [SECURITY-1114]

---
 .../main/java/hudson/model/AbstractItem.java  | 11 +++
 core/src/main/java/hudson/model/Run.java      | 31 +++++-
 core/src/main/java/hudson/model/User.java     | 13 ++-
 .../hudson/AbstractItemSecurity1114Test.java  | 97 +++++++++++++++++++
 4 files changed, 150 insertions(+), 2 deletions(-)
 create mode 100644 test/src/test/java/hudson/AbstractItemSecurity1114Test.java

diff --git a/core/src/main/java/hudson/model/AbstractItem.java b/core/src/main/java/hudson/model/AbstractItem.java
index 9abd3e8708..a9dcfe4137 100644
--- a/core/src/main/java/hudson/model/AbstractItem.java
+++ b/core/src/main/java/hudson/model/AbstractItem.java
@@ -56,6 +56,7 @@ import jenkins.util.xml.XMLUtils;
 
 import org.apache.tools.ant.taskdefs.Copy;
 import org.apache.tools.ant.types.FileSet;
+import org.kohsuke.stapler.HttpResponses;
 import org.kohsuke.stapler.StaplerProxy;
 import org.kohsuke.stapler.WebMethod;
 import org.kohsuke.stapler.export.Exported;
@@ -91,6 +92,8 @@ import javax.xml.transform.stream.StreamSource;
 import static hudson.model.queue.Executables.getParentOf;
 import hudson.model.queue.SubTask;
 import static javax.servlet.http.HttpServletResponse.SC_BAD_REQUEST;
+import static javax.servlet.http.HttpServletResponse.SC_NOT_FOUND;
+
 import org.apache.commons.io.FileUtils;
 import org.kohsuke.accmod.Restricted;
 import org.kohsuke.accmod.restrictions.NoExternalUse;
@@ -841,6 +844,14 @@ public abstract class AbstractItem extends Actionable implements Item, HttpDelet
     @Restricted(NoExternalUse.class)
     public Object getTarget() {
         if (!SKIP_PERMISSION_CHECK) {
+            if (!getACL().hasPermission(Item.DISCOVER)) {
+                // work around Stapler bug when returning null from getTarget()
+                try {
+                    Stapler.getCurrentResponse().sendError(SC_NOT_FOUND); // send same response body Stapler would send
+                } catch (IOException ex) {
+                    throw HttpResponses.notFound();
+                }
+            }
             getACL().checkPermission(Item.READ);
         }
         return this;
diff --git a/core/src/main/java/hudson/model/Run.java b/core/src/main/java/hudson/model/Run.java
index 84a31086a9..2fb4b7e5f5 100644
--- a/core/src/main/java/hudson/model/Run.java
+++ b/core/src/main/java/hudson/model/Run.java
@@ -93,6 +93,8 @@ import java.util.Map;
 import java.util.Set;
 import java.util.logging.Level;
 import static java.util.logging.Level.*;
+import static javax.servlet.http.HttpServletResponse.SC_NOT_FOUND;
+
 import java.util.logging.Logger;
 import javax.annotation.CheckForNull;
 import javax.annotation.Nonnull;
@@ -120,8 +122,10 @@ import org.apache.commons.lang.ArrayUtils;
 import org.kohsuke.accmod.Restricted;
 import org.kohsuke.accmod.restrictions.NoExternalUse;
 import org.kohsuke.stapler.HttpResponse;
+import org.kohsuke.stapler.HttpResponses;
 import org.kohsuke.stapler.QueryParameter;
 import org.kohsuke.stapler.Stapler;
+import org.kohsuke.stapler.StaplerProxy;
 import org.kohsuke.stapler.StaplerRequest;
 import org.kohsuke.stapler.StaplerResponse;
 import org.kohsuke.stapler.export.Exported;
@@ -141,7 +145,7 @@ import org.kohsuke.stapler.interceptor.RequirePOST;
  */
 @ExportedBean
 public abstract class Run <JobT extends Job<JobT,RunT>,RunT extends Run<JobT,RunT>>
-        extends Actionable implements ExtensionPoint, Comparable<RunT>, AccessControlled, PersistenceRoot, DescriptorByNameOwner, OnMaster {
+        extends Actionable implements ExtensionPoint, Comparable<RunT>, AccessControlled, PersistenceRoot, DescriptorByNameOwner, OnMaster, StaplerProxy {
 
     /**
      * The original {@link Queue.Item#getId()} has not yet been mapped onto the {@link Run} instance.
@@ -2480,6 +2484,31 @@ public abstract class Run <JobT extends Job<JobT,RunT>,RunT extends Run<JobT,Run
         return returnedResult;
     }
 
+    @Override
+    @Restricted(NoExternalUse.class)
+    public Object getTarget() {
+        if (!SKIP_PERMISSION_CHECK) {
+            // This is a bit weird, but while the Run's PermissionScope does not have READ, delegate to the parent
+            if (!getParent().hasPermission(Item.DISCOVER)) {
+                // work around Stapler bug when returning null from getTarget()
+                try {
+                    Stapler.getCurrentResponse().sendError(SC_NOT_FOUND); // send same response body Stapler would send
+                } catch (IOException ex) {
+                    throw HttpResponses.notFound();
+                }
+            }
+            getParent().checkPermission(Item.READ);
+        }
+        return this;
+    }
+
+    /**
+     * Escape hatch for StaplerProxy-based access control
+     */
+    @Restricted(NoExternalUse.class)
+    public static /* Script Console modifiable */ boolean SKIP_PERMISSION_CHECK = Boolean.getBoolean(Run.class.getName() + ".skipPermissionCheck");
+
+
     public static class RedirectUp {
         public void doDynamic(StaplerResponse rsp) throws IOException {
             // Compromise to handle both browsers (auto-redirect) and programmatic access
diff --git a/core/src/main/java/hudson/model/User.java b/core/src/main/java/hudson/model/User.java
index a1e48bbd28..1970545bf9 100644
--- a/core/src/main/java/hudson/model/User.java
+++ b/core/src/main/java/hudson/model/User.java
@@ -97,6 +97,8 @@ import org.apache.commons.lang.StringUtils;
 import org.jenkinsci.Symbol;
 import org.kohsuke.accmod.Restricted;
 import org.kohsuke.accmod.restrictions.NoExternalUse;
+import org.kohsuke.stapler.HttpResponses;
+import org.kohsuke.stapler.Stapler;
 import org.kohsuke.stapler.StaplerProxy;
 import org.kohsuke.stapler.StaplerRequest;
 import org.kohsuke.stapler.StaplerResponse;
@@ -105,6 +107,8 @@ import org.kohsuke.stapler.export.ExportedBean;
 import org.kohsuke.stapler.interceptor.RequirePOST;
 import org.springframework.dao.DataAccessException;
 
+import static javax.servlet.http.HttpServletResponse.SC_NOT_FOUND;
+
 /**
  * Represents a user.
  *
@@ -1065,7 +1069,14 @@ public class User extends AbstractModelObject implements AccessControlled, Descr
     @Restricted(NoExternalUse.class)
     public Object getTarget() {
         if (!SKIP_PERMISSION_CHECK) {
-            Jenkins.getInstance().checkPermission(Jenkins.READ);
+            if (!Jenkins.getInstance().hasPermission(Jenkins.READ)) {
+                // work around Stapler bug when returning null from getTarget()
+                try {
+                    Stapler.getCurrentResponse().sendError(SC_NOT_FOUND); // send same response body Stapler would send
+                } catch (IOException ex) {
+                    throw HttpResponses.notFound();
+                }
+            }
         }
         return this;
     }
diff --git a/test/src/test/java/hudson/AbstractItemSecurity1114Test.java b/test/src/test/java/hudson/AbstractItemSecurity1114Test.java
new file mode 100644
index 0000000000..c6d0e2199e
--- /dev/null
+++ b/test/src/test/java/hudson/AbstractItemSecurity1114Test.java
@@ -0,0 +1,97 @@
+package hudson;
+
+import com.gargoylesoftware.htmlunit.FailingHttpStatusCodeException;
+import hudson.model.AbstractItem;
+import hudson.model.Item;
+import hudson.model.RootAction;
+import hudson.security.ACL;
+import hudson.security.ACLContext;
+import jenkins.model.Jenkins;
+import org.junit.Assert;
+import org.junit.Rule;
+import org.junit.Test;
+import org.jvnet.hudson.test.For;
+import org.jvnet.hudson.test.Issue;
+import org.jvnet.hudson.test.JenkinsRule;
+import org.jvnet.hudson.test.MockAuthorizationStrategy;
+import org.jvnet.hudson.test.TestExtension;
+
+import javax.annotation.CheckForNull;
+
+public class AbstractItemSecurity1114Test {
+    @Rule
+    public JenkinsRule j = new JenkinsRule();
+
+    @Test
+    @Issue("SECURITY-1114")
+    @For(AbstractItem.class)
+    public void testAccess() throws Exception {
+        j.jenkins.setSecurityRealm(j.createDummySecurityRealm());
+        MockAuthorizationStrategy authorizationStrategy = new MockAuthorizationStrategy();
+        authorizationStrategy.grant(Jenkins.READ).onRoot().toEveryone();
+        authorizationStrategy.grant(Item.DISCOVER).everywhere().to("alice");
+        authorizationStrategy.grant(Item.READ).everywhere().to("bob");
+        j.jenkins.setAuthorizationStrategy(authorizationStrategy);
+
+        j.createFreeStyleProject("myproject");
+
+        // alice can discover project
+        JenkinsRule.WebClient wc = j.createWebClient().login("alice");
+        try {
+            wc.goTo("bypass/myproject");
+            Assert.fail("expected exception");
+        } catch (FailingHttpStatusCodeException e) {
+            Assert.assertEquals("alice can discover", 403, e.getStatusCode());
+        }
+
+        // bob can read project
+        wc = j.createWebClient().login("bob");
+        wc.goTo("bypass/myproject"); // success
+
+
+        // carol has no permissions
+        wc = j.createWebClient().login("carol");
+        try {
+            wc.goTo("bypass/nonexisting");
+            Assert.fail("expected exception");
+        } catch (FailingHttpStatusCodeException e) {
+            Assert.assertEquals("carol gets 404 for nonexisting project", 404, e.getStatusCode());
+        }
+        try {
+            wc.goTo("bypass/myproject");
+            Assert.fail("expected exception");
+        } catch (FailingHttpStatusCodeException e) {
+            Assert.assertEquals("carol gets 404 for invisible project", 404, e.getStatusCode());
+        }
+
+    }
+
+    @TestExtension
+    public static class BypassAccess implements RootAction {
+        public Item getDynamic(String name) {
+            Item item;
+            try (ACLContext ctx = ACL.as(ACL.SYSTEM)) {
+                 item = Jenkins.getInstance().getItemByFullName(name);
+            }
+            return item;
+        }
+
+        @CheckForNull
+        @Override
+        public String getIconFileName() {
+            return null;
+        }
+
+        @CheckForNull
+        @Override
+        public String getDisplayName() {
+            return null;
+        }
+
+        @CheckForNull
+        @Override
+        public String getUrlName() {
+            return "bypass";
+        }
+    }
+}