From c826a1b8276879c6e3de384b697d42eadfcdf127 Mon Sep 17 00:00:00 2001 From: Kohsuke Kawaguchi Date: Sat, 7 Mar 2015 09:35:28 -0800 Subject: [PATCH 1/8] [FIXED SECURITY-180] --- core/src/main/java/jenkins/security/ApiTokenProperty.java | 1 + 1 file changed, 1 insertion(+) diff --git a/core/src/main/java/jenkins/security/ApiTokenProperty.java b/core/src/main/java/jenkins/security/ApiTokenProperty.java index caa9ed8853..124d8bd78f 100644 --- a/core/src/main/java/jenkins/security/ApiTokenProperty.java +++ b/core/src/main/java/jenkins/security/ApiTokenProperty.java @@ -81,6 +81,7 @@ public class ApiTokenProperty extends UserProperty { } public void changeApiToken() throws IOException { + user.checkPermission(Jenkins.ADMINISTER); _changeApiToken(); if (user!=null) user.save(); From 1699e2c1f36a22d6a149e71c80937e9972ef6224 Mon Sep 17 00:00:00 2001 From: Kohsuke Kawaguchi Date: Sat, 7 Mar 2015 10:22:24 -0800 Subject: [PATCH 2/8] [FIXED SECURITY-177] Add nosniff header liberally to every request we serve. --- core/src/main/java/hudson/security/HudsonFilter.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/hudson/security/HudsonFilter.java b/core/src/main/java/hudson/security/HudsonFilter.java index 40c7e7c69d..e59e9d4e7e 100644 --- a/core/src/main/java/hudson/security/HudsonFilter.java +++ b/core/src/main/java/hudson/security/HudsonFilter.java @@ -36,6 +36,7 @@ import javax.servlet.ServletContext; import javax.servlet.ServletException; import javax.servlet.ServletRequest; import javax.servlet.ServletResponse; +import javax.servlet.http.HttpServletResponse; import org.acegisecurity.AuthenticationManager; import org.acegisecurity.ui.rememberme.RememberMeServices; @@ -153,7 +154,10 @@ public class HudsonFilter implements Filter { public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, ServletException { LOGGER.entering(HudsonFilter.class.getName(), "doFilter"); - + + // this is not the best place to do it, but doing it here makes the patch smaller. + ((HttpServletResponse)response).setHeader("X-Content-Type-Options", "nosniff"); + // to deal with concurrency, we need to capture the object. Filter f = filter; From feb2703adb0c121705e5c8e9ddf8f663b6481cbd Mon Sep 17 00:00:00 2001 From: Kohsuke Kawaguchi Date: Sat, 7 Mar 2015 10:28:40 -0800 Subject: [PATCH 3/8] [FIXED SECURITY-171] --- core/src/main/java/hudson/util/FormValidation.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/java/hudson/util/FormValidation.java b/core/src/main/java/hudson/util/FormValidation.java index 8190dab1f1..396fe79649 100644 --- a/core/src/main/java/hudson/util/FormValidation.java +++ b/core/src/main/java/hudson/util/FormValidation.java @@ -196,7 +196,7 @@ public abstract class FormValidation extends IOException implements HttpResponse " " + Messages.FormValidation_Error_Details() + "
"
-            + Functions.printThrowable(e) +
+            + Util.escape(Functions.printThrowable(e)) +
             "
",kind ); } From 25bf232c0d1abf20ab5a27c6cc808c53d46ea1c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oliver=20Gond=C5=BEa?= Date: Sat, 7 Mar 2015 20:02:02 +0100 Subject: [PATCH 4/8] [SECURITY-180] Test --- test/src/test/java/hudson/model/UserTest.java | 44 +++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/test/src/test/java/hudson/model/UserTest.java b/test/src/test/java/hudson/model/UserTest.java index a8f37057a4..65dda3e978 100644 --- a/test/src/test/java/hudson/model/UserTest.java +++ b/test/src/test/java/hudson/model/UserTest.java @@ -28,19 +28,27 @@ import com.gargoylesoftware.htmlunit.FailingHttpStatusCodeException; import com.gargoylesoftware.htmlunit.WebAssert; import com.gargoylesoftware.htmlunit.html.HtmlForm; import com.gargoylesoftware.htmlunit.html.HtmlPage; + import hudson.security.AccessDeniedException2; import hudson.security.GlobalMatrixAuthorizationStrategy; import hudson.security.HudsonPrivateSecurityRealm; import hudson.security.Permission; import hudson.tasks.MailAddressResolver; + import java.io.IOException; import java.io.PrintStream; import java.util.Collections; + import jenkins.model.Jenkins; +import jenkins.security.ApiTokenProperty; + +import org.acegisecurity.AccessDeniedException; import org.acegisecurity.Authentication; import org.acegisecurity.context.SecurityContext; import org.acegisecurity.context.SecurityContextHolder; + import static org.junit.Assert.*; + import org.junit.Rule; import org.junit.Test; import org.jvnet.hudson.test.Bug; @@ -419,6 +427,42 @@ public class UserTest { assertTrue("User should be able to delete.", user.canDelete()); } + @Test + // @Issue("SECURITY-180") + public void security180() throws Exception { + final GlobalMatrixAuthorizationStrategy auth = new GlobalMatrixAuthorizationStrategy(); + j.jenkins.setAuthorizationStrategy(auth); + j.jenkins.setSecurityRealm(new HudsonPrivateSecurityRealm(false)); + + User alice = User.get("alice"); + User bob = User.get("bob"); + User anonymous = User.get("anonymous"); + User admin = User.get("admin"); + + auth.add(Jenkins.READ, alice.getId()); + auth.add(Jenkins.READ, bob.getId()); + auth.add(Jenkins.ADMINISTER, admin.getId()); + + SecurityContextHolder.getContext().setAuthentication(admin.impersonate()); + // Change token by admin + admin.getProperty(ApiTokenProperty.class).changeApiToken(); + alice.getProperty(ApiTokenProperty.class).changeApiToken(); + + SecurityContextHolder.getContext().setAuthentication(bob.impersonate()); + // Change own token + bob.getProperty(ApiTokenProperty.class).changeApiToken(); + + try { + alice.getProperty(ApiTokenProperty.class).changeApiToken(); + fail("Bob should not be authorized to change alice's token"); + } catch (AccessDeniedException expected) { } + + try { + anonymous.getProperty(ApiTokenProperty.class).changeApiToken(); + fail("Anonymous should not be authorized to change alice's token"); + } catch (AccessDeniedException expected) { } + } + @Test public void testGetDynamic() { From f880d8d2cd9d46987ee3630fa04f77b17784f4e8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oliver=20Gond=C5=BEa?= Date: Sat, 7 Mar 2015 20:32:17 +0100 Subject: [PATCH 5/8] [SECURITY-171] test --- core/src/test/java/hudson/util/FormValidationTest.java | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/core/src/test/java/hudson/util/FormValidationTest.java b/core/src/test/java/hudson/util/FormValidationTest.java index 281a11f152..1840a54e3f 100644 --- a/core/src/test/java/hudson/util/FormValidationTest.java +++ b/core/src/test/java/hudson/util/FormValidationTest.java @@ -23,6 +23,10 @@ */ package hudson.util; +import static org.hamcrest.CoreMatchers.containsString; +import static org.hamcrest.CoreMatchers.not; +import static org.hamcrest.MatcherAssert.assertThat; + import junit.framework.TestCase; /** @@ -51,4 +55,9 @@ public class FormValidationTest extends TestCase { public void testMessage() { assertEquals("test msg", FormValidation.errorWithMarkup("test msg").getMessage()); } + + public void testFormValidationException() { + FormValidation fv = FormValidation.error(new Exception(" Date: Mon, 16 Mar 2015 15:20:38 -0700 Subject: [PATCH 6/8] [SECURITY-177] Added a test case to cover bases --- .../jenkins/security/Security177Test.java | 37 +++++++++++++++++++ 1 file changed, 37 insertions(+) create mode 100644 test/src/test/java/jenkins/security/Security177Test.java diff --git a/test/src/test/java/jenkins/security/Security177Test.java b/test/src/test/java/jenkins/security/Security177Test.java new file mode 100644 index 0000000000..4038c92d81 --- /dev/null +++ b/test/src/test/java/jenkins/security/Security177Test.java @@ -0,0 +1,37 @@ +package jenkins.security; + +import com.gargoylesoftware.htmlunit.Page; +import org.junit.Rule; +import org.junit.Test; +import org.jvnet.hudson.test.JenkinsRule; +import org.jvnet.hudson.test.JenkinsRule.WebClient; + +import java.net.URL; + +import static org.junit.Assert.assertEquals; + +/** + * @author Kohsuke Kawaguchi + */ +// @Issue("SECURITY-177") +public class Security177Test { + @Rule + public JenkinsRule jenkins = new JenkinsRule(); + + @Test + public void nosniff() throws Exception { + WebClient wc = jenkins.createWebClient(); + wc.setThrowExceptionOnFailingStatusCode(false); + + URL u = jenkins.getURL(); + verifyNoSniff(wc.getPage(new URL(u, "adjuncts/507db12b/nosuch/adjunct.js"))); + verifyNoSniff(wc.getPage(new URL(u, "no-such-page"))); + verifyNoSniff(wc.getPage(new URL(u, "images/title.svg"))); + verifyNoSniff(wc.getPage(u)); + } + + private void verifyNoSniff(Page p) { + String v = p.getWebResponse().getResponseHeaderValue("X-Content-Type-Options"); + assertEquals(v,"nosniff"); + } +} From b44a2fb1bc184077d324a31feee9835fd3a7cc6e Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Tue, 17 Mar 2015 09:45:19 -0400 Subject: [PATCH 7/8] Can uncomment @Issue now. --- test/src/test/java/jenkins/security/Security177Test.java | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/test/src/test/java/jenkins/security/Security177Test.java b/test/src/test/java/jenkins/security/Security177Test.java index 4038c92d81..c212cba2d2 100644 --- a/test/src/test/java/jenkins/security/Security177Test.java +++ b/test/src/test/java/jenkins/security/Security177Test.java @@ -1,19 +1,18 @@ package jenkins.security; import com.gargoylesoftware.htmlunit.Page; +import java.net.URL; +import static org.junit.Assert.assertEquals; import org.junit.Rule; import org.junit.Test; +import org.jvnet.hudson.test.Issue; import org.jvnet.hudson.test.JenkinsRule; import org.jvnet.hudson.test.JenkinsRule.WebClient; -import java.net.URL; - -import static org.junit.Assert.assertEquals; - /** * @author Kohsuke Kawaguchi */ -// @Issue("SECURITY-177") +@Issue("SECURITY-177") public class Security177Test { @Rule public JenkinsRule jenkins = new JenkinsRule(); From 2cc3afdebedbe356b7451e382e62a4d62a521208 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oliver=20Gond=C5=BEa?= Date: Tue, 17 Mar 2015 18:01:49 +0100 Subject: [PATCH 8/8] [SECURITY-180] fix test --- test/src/test/java/hudson/model/UserTest.java | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/test/src/test/java/hudson/model/UserTest.java b/test/src/test/java/hudson/model/UserTest.java index 65dda3e978..ec1be9c0a2 100644 --- a/test/src/test/java/hudson/model/UserTest.java +++ b/test/src/test/java/hudson/model/UserTest.java @@ -432,33 +432,33 @@ public class UserTest { public void security180() throws Exception { final GlobalMatrixAuthorizationStrategy auth = new GlobalMatrixAuthorizationStrategy(); j.jenkins.setAuthorizationStrategy(auth); - j.jenkins.setSecurityRealm(new HudsonPrivateSecurityRealm(false)); + j.jenkins.setSecurityRealm(j.createDummySecurityRealm()); User alice = User.get("alice"); User bob = User.get("bob"); - User anonymous = User.get("anonymous"); User admin = User.get("admin"); auth.add(Jenkins.READ, alice.getId()); auth.add(Jenkins.READ, bob.getId()); auth.add(Jenkins.ADMINISTER, admin.getId()); + // Admin can change everyone's token SecurityContextHolder.getContext().setAuthentication(admin.impersonate()); - // Change token by admin admin.getProperty(ApiTokenProperty.class).changeApiToken(); alice.getProperty(ApiTokenProperty.class).changeApiToken(); + // User can change only own token SecurityContextHolder.getContext().setAuthentication(bob.impersonate()); - // Change own token bob.getProperty(ApiTokenProperty.class).changeApiToken(); - try { alice.getProperty(ApiTokenProperty.class).changeApiToken(); fail("Bob should not be authorized to change alice's token"); } catch (AccessDeniedException expected) { } + // ANONYMOUS can not change any token + SecurityContextHolder.getContext().setAuthentication(Jenkins.ANONYMOUS); try { - anonymous.getProperty(ApiTokenProperty.class).changeApiToken(); + alice.getProperty(ApiTokenProperty.class).changeApiToken(); fail("Anonymous should not be authorized to change alice's token"); } catch (AccessDeniedException expected) { } }