Skip to content

Commit

Permalink
[SECURITY-3315]
Browse files Browse the repository at this point in the history
  • Loading branch information
daniel-beck authored and jenkinsci-cert-ci committed Jan 16, 2024
1 parent 86d39dd commit de45096
Show file tree
Hide file tree
Showing 2 changed files with 82 additions and 1 deletion.
21 changes: 20 additions & 1 deletion core/src/main/java/hudson/cli/CLIAction.java
Expand Up @@ -49,8 +49,10 @@
import javax.servlet.http.HttpServletResponse;
import jenkins.model.Jenkins;
import jenkins.util.FullDuplexHttpService;
import jenkins.util.SystemProperties;
import jenkins.websocket.WebSocketSession;
import jenkins.websocket.WebSockets;
import org.apache.commons.lang.StringUtils;
import org.jenkinsci.Symbol;
import org.kohsuke.accmod.Restricted;
import org.kohsuke.accmod.restrictions.NoExternalUse;
Expand All @@ -73,6 +75,12 @@ public class CLIAction implements UnprotectedRootAction, StaplerProxy {

private static final Logger LOGGER = Logger.getLogger(CLIAction.class.getName());

/**
* Boolean values map to allowing/disallowing WS CLI endpoint always, {@code null} is the default of doing an {@code Origin} check.
* {@code true} is only advisable if anonymous users have no permissions, and Jenkins sends SameSite=Lax cookies (or browsers use that as the implicit default).
*/
/* package-private for testing */ static /* non-final for Script Console */ Boolean ALLOW_WEBSOCKET = SystemProperties.optBoolean(CLIAction.class.getName() + ".ALLOW_WEBSOCKET");

private final transient Map<UUID, FullDuplexHttpService> duplexServices = new HashMap<>();

@Override
Expand Down Expand Up @@ -114,10 +122,21 @@ public boolean isWebSocketSupported() {
/**
* WebSocket endpoint.
*/
public HttpResponse doWs() {
public HttpResponse doWs(StaplerRequest req) {
if (!WebSockets.isSupported()) {
return HttpResponses.notFound();
}
if (ALLOW_WEBSOCKET == null) {
final String actualOrigin = req.getHeader("Origin");
final String expectedOrigin = StringUtils.removeEnd(StringUtils.removeEnd(Jenkins.get().getRootUrlFromRequest(), "/"), req.getContextPath());

if (actualOrigin == null || !actualOrigin.equals(expectedOrigin)) {
LOGGER.log(Level.FINE, () -> "Rejecting origin: " + actualOrigin + "; expected was from request: " + expectedOrigin);
return HttpResponses.forbidden();
}
} else if (!ALLOW_WEBSOCKET) {
return HttpResponses.forbidden();
}
Authentication authentication = Jenkins.getAuthentication2();
return WebSockets.upgrade(new WebSocketSession() {
ServerSideImpl connection;
Expand Down
62 changes: 62 additions & 0 deletions test/src/test/java/hudson/cli/Security3315Test.java
@@ -0,0 +1,62 @@
package hudson.cli;

import static org.hamcrest.CoreMatchers.is;
import static org.hamcrest.MatcherAssert.assertThat;

import java.io.IOException;
import java.net.URL;
import java.util.Arrays;
import java.util.List;
import org.htmlunit.HttpMethod;
import org.htmlunit.Page;
import org.htmlunit.WebRequest;
import org.junit.Rule;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
import org.jvnet.hudson.test.FlagRule;
import org.jvnet.hudson.test.JenkinsRule;

@RunWith(Parameterized.class)
public class Security3315Test {
@Rule
public JenkinsRule j = new JenkinsRule();

@Rule
public FlagRule<Boolean> escapeHatch;

private final Boolean allowWs;

@Parameterized.Parameters
public static List<String> escapeHatchValues() {
return Arrays.asList(null, "true", "false");
}

public Security3315Test(String allowWs) {
this.allowWs = allowWs == null ? null : Boolean.valueOf(allowWs);
this.escapeHatch = new FlagRule<>(() -> CLIAction.ALLOW_WEBSOCKET, v -> { CLIAction.ALLOW_WEBSOCKET = v; }, this.allowWs);
}

@Test
public void test() throws IOException {
try (JenkinsRule.WebClient wc = j.createWebClient().withThrowExceptionOnFailingStatusCode(false)) {
// HTTP 400 is WebSocket "success" (HTMLUnit doesn't support it)
final URL jenkinsUrl = j.getURL();
WebRequest request = new WebRequest(new URL(jenkinsUrl.toString() + "cli/ws"), HttpMethod.GET);
Page page = wc.getPage(request);
assertThat(page.getWebResponse().getStatusCode(), is(allowWs == Boolean.TRUE ? 400 : 403)); // no Origin header

request.setAdditionalHeader("Origin", jenkinsUrl.getProtocol() + "://example.org:" + jenkinsUrl.getPort());
page = wc.getPage(request);
assertThat(page.getWebResponse().getStatusCode(), is(allowWs == Boolean.TRUE ? 400 : 403)); // Wrong Origin host

request.setAdditionalHeader("Origin", jenkinsUrl.getProtocol() + "://" + jenkinsUrl.getHost());
page = wc.getPage(request);
assertThat(page.getWebResponse().getStatusCode(), is(allowWs == Boolean.TRUE ? 400 : 403)); // Wrong Origin port

request.setAdditionalHeader("Origin", jenkinsUrl.getProtocol() + "://" + jenkinsUrl.getHost() + ":" + jenkinsUrl.getPort());
page = wc.getPage(request);
assertThat(page.getWebResponse().getStatusCode(), is(allowWs == Boolean.FALSE ? 403 : 400)); // Reject correct Origin if ALLOW_WS is explicitly false
}
}
}

0 comments on commit de45096

Please sign in to comment.