Improper permission checks allow canceling queue items and aborting builds in Jenkins

@@ -872,8 +872,16 @@ public HttpResponse doStopBuild(@CheckForNull @QueryParameter(fixEmpty = true) S
         try {
             if (executable != null) {
                 if (runExtId == null || runExtId.isEmpty() || ! (executable instanceof Run)
-                        || (executable instanceof Run && runExtId.equals(((Run<?,?>) executable).getExternalizableId()))) {
-                    getParentOf(executable).getOwnerTask().checkAbortPermission();
+                        || (runExtId.equals(((Run<?,?>) executable).getExternalizableId()))) {
+                    final Queue.Task ownerTask = getParentOf(executable).getOwnerTask();
+                    boolean canAbort = ownerTask.hasAbortPermission();
+                    if (canAbort && ownerTask instanceof AccessControlled) {
+                        if (!((AccessControlled) ownerTask).hasPermission(Item.READ)) {
+                            // pretend the build does not exist
+                            return HttpResponses.forwardToPreviousPage();
+                        }
+                    }
+                    ownerTask.checkAbortPermission();
                     interrupt();
                 }
             }

@@ -747,6 +747,9 @@ public boolean cancel(Item item) {
     @RequirePOST
     public HttpResponse doCancelItem(@QueryParameter long id) throws IOException, ServletException {
         Item item = getItem(id);
+        if (item != null && !hasReadPermission(item, true)) {
+            item = null;
+        }
         if (item != null) {
             if(item.hasCancelPermission()){
                 if(cancel(item)) {
@@ -798,14 +801,27 @@ public Item[] getItems() {
     }
  
     private List<Item> checkPermissionsAndAddToList(List<Item> r, Item t) {
-        if (t.task instanceof AccessControlled) {
-            AccessControlled taskAC = (AccessControlled) t.task;
+        // TODO Changing the second arg to 'true' should reveal some tasks currently hidden for no obvious reason
+        if (hasReadPermission(t.task, false)) {
+            r.add(t);
+        }
+        return r;
+    }
+ 
+    private static boolean hasReadPermission(Item t, boolean valueIfNotAccessControlled) {
+        return hasReadPermission(t.task, valueIfNotAccessControlled);
+    }
+ 
+    private static boolean hasReadPermission(Queue.Task t, boolean valueIfNotAccessControlled) {
+        if (t instanceof AccessControlled) {
+            AccessControlled taskAC = (AccessControlled) t;
             if (taskAC.hasPermission(hudson.model.Item.READ)
-                    || taskAC.hasPermission(Permission.READ)) {
-                r.add(t);
+                    || taskAC.hasPermission(Permission.READ)) { // TODO should be unnecessary given the 'implies' relationship
+                return true;
             }
+            return false;
         }
-        return r;
+        return valueIfNotAccessControlled;
     }
  
     /**

@@ -122,7 +122,7 @@ THE SOFTWARE.
                 </td>
                 </st:include>
                 <td class="pane" align="center" valign="middle">
-                  <j:if test="${e.hasStopPermission()}">
+                  <j:if test="${h.hasPermission(exeparent,exeparent.READ) and e.hasStopPermission()}">
                     <l:stopButton href="${rootURL}/${c.url}${url}/stopBuild?runExtId=${h.urlEncode(exe.externalizableId)}" confirm="${%confirm(exe.fullDisplayName)}" alt="${%terminate this build}"  />
                   </j:if>
                 </td>

@@ -1116,7 +1116,7 @@ private void checkCancelOperationUsingUrl(Function<Queue.Item, String> urlProvid
         r.jenkins.setCrumbIssuer(null);
         r.jenkins.setSecurityRealm(r.createDummySecurityRealm());
         r.jenkins.setAuthorizationStrategy(new MockAuthorizationStrategy()
-                .grant(Jenkins.READ, Item.CANCEL).everywhere().to("admin")
+                .grant(Jenkins.READ, Item.READ, Item.CANCEL).everywhere().to("admin")
                 .grant(Jenkins.READ).everywhere().to("user")
         );
  

@@ -0,0 +1,146 @@
+package jenkins.security;
+ 
+import com.gargoylesoftware.htmlunit.HttpMethod;
+import com.gargoylesoftware.htmlunit.Page;
+import com.gargoylesoftware.htmlunit.WebRequest;
+import com.gargoylesoftware.htmlunit.html.HtmlPage;
+import hudson.model.Cause;
+import hudson.model.Computer;
+import hudson.model.Executor;
+import hudson.model.FreeStyleBuild;
+import hudson.model.FreeStyleProject;
+import hudson.model.Item;
+import hudson.model.Queue;
+import jenkins.model.Jenkins;
+import org.junit.AfterClass;
+import org.junit.Assert;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.Test;
+import org.jvnet.hudson.test.JenkinsRule;
+import org.jvnet.hudson.test.MockAuthorizationStrategy;
+import org.jvnet.hudson.test.MockFolder;
+import org.jvnet.hudson.test.SleepBuilder;
+ 
+import java.net.URL;
+import java.util.List;
+import java.util.Objects;
+import java.util.concurrent.TimeUnit;
+ 
+import static org.hamcrest.CoreMatchers.containsString;
+import static org.hamcrest.CoreMatchers.not;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+ 
+public class Security2278Test {
+ 
+    @ClassRule
+    public static JenkinsRule j = new JenkinsRule();
+    private static FreeStyleProject project;
+ 
+    @BeforeClass
+    public static void setUp() throws Exception {
+        j.jenkins.setSecurityRealm(j.createDummySecurityRealm());
+ 
+        final MockFolder folder = j.createFolder("foo");
+        project = folder.createProject(FreeStyleProject.class, "bar");
+        project.getBuildersList().add(new SleepBuilder(TimeUnit.SECONDS.toMillis(600)));
+        project.save();
+ 
+        // can cancel but not read the project, plus it's in a subfolder (unsure whether that is needed)
+        j.jenkins.setAuthorizationStrategy(new MockAuthorizationStrategy()
+                .grant(Jenkins.READ).everywhere().toEveryone()
+                .grant(Item.CANCEL).onItems(project).toEveryone()
+                .grant(Jenkins.ADMINISTER).everywhere().toAuthenticated());
+ 
+        // one build in progress
+        project.scheduleBuild2(0, new Cause.UserIdCause("alice")).waitForStart();
+ 
+        // one waiting in queue
+        project.scheduleBuild2(0, new Cause.UserIdCause("alice"));
+    }
+ 
+    @Test
+    public void testUi() throws Exception {
+        final JenkinsRule.WebClient webClient = j.createWebClient();
+        final HtmlPage topPage = webClient.goTo("");
+        final String contentAsString = topPage.getWebResponse().getContentAsString();
+        assertThat(contentAsString, containsString("Build Executor Status"));
+        assertThat(contentAsString, containsString("Unknown Task"));
+        assertThat(contentAsString, not(containsString("job/foo/job/bar")));
+        assertThat(contentAsString, not(containsString("icon-stop")));
+    }
+ 
+    @Test
+    public void testUiWithPermission() throws Exception {
+        final JenkinsRule.WebClient webClient = j.createWebClient().login("alice");
+        final HtmlPage topPage = webClient.goTo("");
+        final String contentAsString = topPage.getWebResponse().getContentAsString();
+        assertThat(contentAsString, containsString("Build Executor Status"));
+        assertThat(contentAsString, not(containsString("Unknown Task")));
+        assertThat(contentAsString, containsString("job/foo/job/bar"));
+        assertThat(contentAsString, containsString("icon-stop"));
+    }
+ 
+    @Test
+    public void testQueueCancelWithoutPermission() throws Exception {
+        final Queue.Item[] items = j.jenkins.getQueue().getItems();
+        Assert.assertEquals(1, items.length);
+        final long id = items[0].getId();
+ 
+        final JenkinsRule.WebClient webClient = j.createWebClient();
+        webClient.setThrowExceptionOnFailingStatusCode(false);
+        webClient.setRedirectEnabled(false);
+ 
+        final Page stopResponse = webClient.getPage(addReferer(webClient.addCrumb(new WebRequest(new URL(j.jenkins.getRootUrl() + "/queue/cancelItem?id=" + id), HttpMethod.POST)), j.jenkins.getRootUrl()));
+        assertEquals(404, stopResponse.getWebResponse().getStatusCode());
+        Assert.assertEquals(1, j.jenkins.getQueue().getItems().length);
+    }
+ 
+    @Test
+    public void testWebMethodWithoutPermission() throws Exception {
+        final JenkinsRule.WebClient webClient = j.createWebClient();
+        webClient.setThrowExceptionOnFailingStatusCode(false);
+        webClient.setRedirectEnabled(false);
+ 
+        final Executor busyExecutor = project.getBuilds().stream().findFirst().orElseThrow(() -> new IllegalStateException("expected build")).getExecutor();
+        final Computer computer = j.jenkins.toComputer();
+        assertNotNull(computer);
+        final List<Executor> executors = computer.getExecutors();
+        int found = -1;
+        for (int i = 0; i < computer.getNumExecutors(); i ++) {
+            if (executors.get(i) == busyExecutor) {
+                found = i;
+                break;
+            }
+        }
+        if (found < 0) {
+            throw new IllegalStateException("didn't find executor");
+        }
+        final Page stopResponse = webClient.getPage(addReferer(webClient.addCrumb(new WebRequest(new URL(j.jenkins.getRootUrl() + "/computer/(master)/executors/" + found + "/stop/"), HttpMethod.POST)), j.jenkins.getRootUrl()));
+        assertEquals(302, stopResponse.getWebResponse().getStatusCode());
+ 
+        final FreeStyleBuild build = project.getBuildByNumber(1);
+        assertTrue(build.isBuilding());
+        assertFalse(Objects.requireNonNull(build.getExecutor()).isInterrupted());
+    }
+ 
+    private WebRequest addReferer(WebRequest request, String referer) {
+        request.setAdditionalHeader("Referer", referer);
+        return request;
+    }
+ 
+    @AfterClass
+    public static void tearDown() throws Exception {
+        // clear out queue
+        j.jenkins.getQueue().clear();
+ 
+        // wait for build to finish aborting
+        final FreeStyleBuild freeStyleBuild = project.getBuilds().stream().findFirst().orElseThrow(() -> new IllegalStateException("Didn't find build"));
+        freeStyleBuild.doStop();
+        j.jenkins.getQueue().getItem(freeStyleBuild.getQueueId()).getFuture().get();
+    }
+}