From d3fb2c09f29007dce84a213ae8323df1105dcc30 Mon Sep 17 00:00:00 2001 From: Daniel Beck Date: Sun, 15 Nov 2015 19:52:25 +0100 Subject: [PATCH 1/5] [FIX SECURITY-95] Add CSP headers to files served by Jenkins --- .../hudson/model/DirectoryBrowserSupport.java | 16 ++++++++++ .../model/DirectoryBrowserSupportTest.java | 30 +++++++++++++++++++ 2 files changed, 46 insertions(+) diff --git a/core/src/main/java/hudson/model/DirectoryBrowserSupport.java b/core/src/main/java/hudson/model/DirectoryBrowserSupport.java index cf803fcdc5..7c562dcb12 100644 --- a/core/src/main/java/hudson/model/DirectoryBrowserSupport.java +++ b/core/src/main/java/hudson/model/DirectoryBrowserSupport.java @@ -47,6 +47,8 @@ import jenkins.util.VirtualFile; import org.apache.commons.io.IOUtils; import org.apache.tools.zip.ZipEntry; import org.apache.tools.zip.ZipOutputStream; +import org.kohsuke.accmod.Restricted; +import org.kohsuke.accmod.restrictions.NoExternalUse; import org.kohsuke.stapler.HttpResponse; import org.kohsuke.stapler.StaplerRequest; import org.kohsuke.stapler.StaplerResponse; @@ -307,6 +309,17 @@ public final class DirectoryBrowserSupport implements HttpResponse { // pseudo file name to let the Stapler set text/plain rsp.serveFile(req, in, lastModified, -1, length, "plain.txt"); } else { + String csp = System.getProperty(DirectoryBrowserSupport.class.getName() + ".CSP"); + if (csp == null) { + // default value unless overridden with system property + csp = DEFAULT_CSP_VALUE; + } + if (!csp.trim().equals("")) { + // allow users to prevent sending this header by setting empty system property + for (String header : new String[]{"Content-Security-Policy", "X-WebKit-CSP", "X-Content-Security-Policy"}) { + rsp.setHeader(header, csp); + } + } rsp.serveFile(req, in, lastModified, -1, length, baseFile.getName() ); } } @@ -574,4 +587,7 @@ public final class DirectoryBrowserSupport implements HttpResponse { private static final Logger LOGGER = Logger.getLogger(DirectoryBrowserSupport.class.getName()); + + @Restricted(NoExternalUse.class) + public static final String DEFAULT_CSP_VALUE = "sandbox; default-src 'none'; img-src 'self'; style-src 'self';"; } diff --git a/test/src/test/java/hudson/model/DirectoryBrowserSupportTest.java b/test/src/test/java/hudson/model/DirectoryBrowserSupportTest.java index fc07a335b0..06473ed4e4 100644 --- a/test/src/test/java/hudson/model/DirectoryBrowserSupportTest.java +++ b/test/src/test/java/hudson/model/DirectoryBrowserSupportTest.java @@ -169,6 +169,36 @@ public class DirectoryBrowserSupportTest { zipfile.delete(); } + @Issue("SECURITY-95") + @Test + public void contentSecurityPolicy() throws Exception { + FreeStyleProject p = j.createFreeStyleProject(); + p.setScm(new SingleFileSCM("test.html", "

Hello world!

")); + p.getPublishersList().add(new ArtifactArchiver("*", "", true)); + assertEquals(Result.SUCCESS, p.scheduleBuild2(0).get().getResult()); + + HtmlPage page = j.createWebClient().goTo("job/" + p.getName() + "/lastSuccessfulBuild/artifact/test.html"); + for (String header : new String[]{"Content-Security-Policy", "X-WebKit-CSP", "X-Content-Security-Policy"}) { + assertEquals("Header set: " + header, page.getWebResponse().getResponseHeaderValue(header), DirectoryBrowserSupport.DEFAULT_CSP_VALUE); + } + + String propName = DirectoryBrowserSupport.class.getName() + ".CSP"; + String initialValue = System.getProperty(propName); + try { + System.setProperty(propName, ""); + page = j.createWebClient().goTo("job/" + p.getName() + "/lastSuccessfulBuild/artifact/test.html"); + for (String header : new String[]{"Content-Security-Policy", "X-WebKit-CSP", "X-Content-Security-Policy"}) { + assertFalse("Header not set: " + header, page.getWebResponse().getResponseHeaders().contains(header)); + } + } finally { + if (initialValue == null) { + System.clearProperty(DirectoryBrowserSupport.class.getName() + ".CSP"); + } else { + System.setProperty(DirectoryBrowserSupport.class.getName() + ".CSP", initialValue); + } + } + } + private File download(UnexpectedPage page) throws IOException { File file = File.createTempFile("DirectoryBrowserSupport", "zipDownload"); From 40a28999e221a209212c30586be9c39049510bd1 Mon Sep 17 00:00:00 2001 From: Daniel Beck Date: Sun, 15 Nov 2015 21:07:06 +0100 Subject: [PATCH 2/5] [FIX SECURITY-225] Add RequirePOST annotations --- core/src/main/java/hudson/PluginManager.java | 3 +++ core/src/main/java/hudson/logging/LogRecorderManager.java | 2 ++ core/src/main/java/hudson/model/AbstractProject.java | 1 + 3 files changed, 6 insertions(+) diff --git a/core/src/main/java/hudson/PluginManager.java b/core/src/main/java/hudson/PluginManager.java index 35f1d9ebbb..b7cb78fedf 100644 --- a/core/src/main/java/hudson/PluginManager.java +++ b/core/src/main/java/hudson/PluginManager.java @@ -809,6 +809,7 @@ public abstract class PluginManager extends AbstractModelObject implements OnMas /** * Bare-minimum configuration mechanism to change the update center. */ + @RequirePOST public HttpResponse doSiteConfigure(@QueryParameter String site) throws IOException { Jenkins hudson = Jenkins.getInstance(); hudson.checkPermission(CONFIGURE_UPDATECENTER); @@ -824,6 +825,7 @@ public abstract class PluginManager extends AbstractModelObject implements OnMas } + @RequirePOST public HttpResponse doProxyConfigure(StaplerRequest req) throws IOException, ServletException { Jenkins jenkins = Jenkins.getInstance(); jenkins.checkPermission(CONFIGURE_UPDATECENTER); @@ -842,6 +844,7 @@ public abstract class PluginManager extends AbstractModelObject implements OnMas /** * Uploads a plugin. */ + @RequirePOST public HttpResponse doUploadPlugin(StaplerRequest req) throws IOException, ServletException { try { Jenkins.getInstance().checkPermission(UPLOAD_PLUGINS); diff --git a/core/src/main/java/hudson/logging/LogRecorderManager.java b/core/src/main/java/hudson/logging/LogRecorderManager.java index d16ece6c2b..e35a098d4b 100644 --- a/core/src/main/java/hudson/logging/LogRecorderManager.java +++ b/core/src/main/java/hudson/logging/LogRecorderManager.java @@ -40,6 +40,7 @@ import org.kohsuke.stapler.StaplerRequest; import org.kohsuke.stapler.StaplerResponse; import org.kohsuke.stapler.HttpResponse; import org.kohsuke.stapler.HttpRedirect; +import org.kohsuke.stapler.interceptor.RequirePOST; import javax.servlet.ServletException; import java.io.File; @@ -106,6 +107,7 @@ public class LogRecorderManager extends AbstractModelObject implements ModelObje /** * Creates a new log recorder. */ + @RequirePOST public HttpResponse doNewLogRecorder(@QueryParameter String name) { Jenkins.checkGoodName(name); diff --git a/core/src/main/java/hudson/model/AbstractProject.java b/core/src/main/java/hudson/model/AbstractProject.java index dab18dc8dd..6ea82bc165 100644 --- a/core/src/main/java/hudson/model/AbstractProject.java +++ b/core/src/main/java/hudson/model/AbstractProject.java @@ -1924,6 +1924,7 @@ public abstract class AbstractProject

,R extends A /** * Wipes out the workspace. */ + @RequirePOST public HttpResponse doDoWipeOutWorkspace() throws IOException, ServletException, InterruptedException { checkPermission(Functions.isWipeOutPermissionEnabled() ? WIPEOUT : BUILD); R b = getSomeBuildWithWorkspace(); From 83f18a8e6a03f8dabdefb234ea62aa18144b7828 Mon Sep 17 00:00:00 2001 From: Daniel Beck Date: Mon, 16 Nov 2015 01:03:08 +0100 Subject: [PATCH 3/5] [SECURITY-225] Fix failing tests --- .../test/groovy/hudson/model/AbstractProjectTest.groovy | 8 +++++++- test/src/test/java/hudson/model/ProjectTest.java | 8 +++++++- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/test/src/test/groovy/hudson/model/AbstractProjectTest.groovy b/test/src/test/groovy/hudson/model/AbstractProjectTest.groovy index f40a32ed3a..d2d54595f4 100644 --- a/test/src/test/groovy/hudson/model/AbstractProjectTest.groovy +++ b/test/src/test/groovy/hudson/model/AbstractProjectTest.groovy @@ -109,7 +109,13 @@ public class AbstractProjectTest extends HudsonTestCase { assert b.getWorkspace().exists(): "Workspace should exist by now"; // make sure that the action link is protected - createWebClient().assertFails(project.getUrl() + "doWipeOutWorkspace", HttpURLConnection.HTTP_FORBIDDEN); + com.gargoylesoftware.htmlunit.WebClient wc = createWebClient(); + try { + wc.getPage(new WebRequestSettings(new URL(wc.getContextPath() + project.getUrl() + "doWipeOutWorkspace"), HttpMethod.POST)); + fail("Expected HTTP status code 403") + } catch (FailingHttpStatusCodeException e) { + assertEquals(HttpURLConnection.HTTP_FORBIDDEN, e.getStatusCode()); + } } /** diff --git a/test/src/test/java/hudson/model/ProjectTest.java b/test/src/test/java/hudson/model/ProjectTest.java index b7faf8bff9..effa4a8d42 100644 --- a/test/src/test/java/hudson/model/ProjectTest.java +++ b/test/src/test/java/hudson/model/ProjectTest.java @@ -23,11 +23,15 @@ */ package hudson.model; +import com.gargoylesoftware.htmlunit.HttpMethod; +import com.gargoylesoftware.htmlunit.WebRequestSettings; import hudson.model.queue.QueueTaskFuture; import hudson.security.AccessDeniedException2; import org.acegisecurity.context.SecurityContextHolder; import hudson.security.HudsonPrivateSecurityRealm; import hudson.security.GlobalMatrixAuthorizationStrategy; + +import java.net.URL; import java.util.Collections; import org.jvnet.hudson.reactor.ReactorException; @@ -605,7 +609,9 @@ public class ProjectTest { project.setAssignedLabel(slave.getSelfLabel()); project.getBuildersList().add(new Shell("echo hello > change.log")); j.buildAndAssertSuccess(project); - HtmlPage page = j.createWebClient().login(user.getId(), "password").goTo(project.getUrl() + "doWipeOutWorkspace"); + JenkinsRule.WebClient wc = j.createWebClient().login(user.getId(), "password"); + WebRequestSettings request = new WebRequestSettings(new URL(wc.getContextPath() + project.getUrl() + "doWipeOutWorkspace"), HttpMethod.POST); + HtmlPage p = wc.getPage(request); Thread.sleep(500); assertFalse("Workspace should not exist.", project.getSomeWorkspace().exists()); } From ef2c0dc163695af3a57ad7a45571293377ff679b Mon Sep 17 00:00:00 2001 From: Daniel Beck Date: Thu, 19 Nov 2015 17:42:14 +0100 Subject: [PATCH 4/5] [FIX SECURITY-233] Remove multipart form exception from crumb filter --- core/src/main/java/hudson/security/csrf/CrumbFilter.java | 4 ++-- war/src/main/webapp/scripts/hudson-behavior.js | 5 +++++ 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/hudson/security/csrf/CrumbFilter.java b/core/src/main/java/hudson/security/csrf/CrumbFilter.java index 788eaa50ca..6898ddc019 100644 --- a/core/src/main/java/hudson/security/csrf/CrumbFilter.java +++ b/core/src/main/java/hudson/security/csrf/CrumbFilter.java @@ -80,8 +80,8 @@ public class CrumbFilter implements Filter { LOGGER.log(Level.WARNING, "Found invalid crumb {0}. Will check remaining parameters for a valid one...", crumb); } } - // Multipart requests need to be handled by each handler. - if (valid || isMultipart(httpRequest)) { + + if (valid) { chain.doFilter(request, response); } else { LOGGER.log(Level.WARNING, "No valid crumb was included in request for {0}. Returning {1}.", new Object[] {httpRequest.getRequestURI(), HttpServletResponse.SC_FORBIDDEN}); diff --git a/war/src/main/webapp/scripts/hudson-behavior.js b/war/src/main/webapp/scripts/hudson-behavior.js index 270ab19cbc..d369b15069 100644 --- a/war/src/main/webapp/scripts/hudson-behavior.js +++ b/war/src/main/webapp/scripts/hudson-behavior.js @@ -117,6 +117,11 @@ var crumb = { var div = document.createElement("div"); div.innerHTML = ""; form.appendChild(div); + if (form.action.indexOf("?") != -1) { + form.action = form.action+"&"+this.fieldName+"="+this.value; + } else { + form.action = form.action+"?"+this.fieldName+"="+this.value; + } } } From ba747888108d0db90d469c6d210b1df465d8fac1 Mon Sep 17 00:00:00 2001 From: Daniel Beck Date: Thu, 19 Nov 2015 19:40:16 +0100 Subject: [PATCH 5/5] [SECURITY-233] Only append crumb to URL in multipart forms --- war/src/main/webapp/scripts/hudson-behavior.js | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/war/src/main/webapp/scripts/hudson-behavior.js b/war/src/main/webapp/scripts/hudson-behavior.js index d369b15069..1bbab80090 100644 --- a/war/src/main/webapp/scripts/hudson-behavior.js +++ b/war/src/main/webapp/scripts/hudson-behavior.js @@ -117,10 +117,12 @@ var crumb = { var div = document.createElement("div"); div.innerHTML = ""; form.appendChild(div); - if (form.action.indexOf("?") != -1) { - form.action = form.action+"&"+this.fieldName+"="+this.value; - } else { - form.action = form.action+"?"+this.fieldName+"="+this.value; + if (form.enctype == "multipart/form-data") { + if (form.action.indexOf("?") != -1) { + form.action = form.action+"&"+this.fieldName+"="+this.value; + } else { + form.action = form.action+"?"+this.fieldName+"="+this.value; + } } } } @@ -2424,6 +2426,7 @@ function buildFormTree(form) { // switch to multipart/form-data to support file submission // @enctype is the standard, but IE needs @encoding. form.enctype = form.encoding = "multipart/form-data"; + crumb.appendToForm(form); break; case "radio": if(!e.checked) break;