Skip to content

Commit

Permalink
[SECURITY-2278]
Browse files Browse the repository at this point in the history
  • Loading branch information
daniel-beck authored and jenkinsci-cert-ci committed Jun 16, 2021
1 parent 25a42f3 commit 86b7d7e
Show file tree
Hide file tree
Showing 5 changed files with 179 additions and 9 deletions.
12 changes: 10 additions & 2 deletions core/src/main/java/hudson/model/Executor.java
Expand Up @@ -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();
}
}
Expand Down
26 changes: 21 additions & 5 deletions core/src/main/java/hudson/model/Queue.java
Expand Up @@ -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)) {
Expand Down Expand Up @@ -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;
}

/**
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/resources/lib/hudson/executors.jelly
Expand Up @@ -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>
Expand Down
2 changes: 1 addition & 1 deletion test/src/test/java/hudson/model/QueueTest.java
Expand Up @@ -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")
);

Expand Down
146 changes: 146 additions & 0 deletions test/src/test/java/jenkins/security/Security2278Test.java
@@ -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();
}
}

0 comments on commit 86b7d7e

Please sign in to comment.