From 67f23793fcd912ba344cf07c098c1e2f13b7851f Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Sun, 31 Aug 2014 17:44:09 -0400 Subject: [PATCH 1/5] [SECURITY-120] Do not print a warning with stack trace just because we are using a 2.x servlet container. --- .../jenkins/model/JenkinsLocationConfiguration.java | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/core/src/main/java/jenkins/model/JenkinsLocationConfiguration.java b/core/src/main/java/jenkins/model/JenkinsLocationConfiguration.java index 6228c9b251..6836467217 100644 --- a/core/src/main/java/jenkins/model/JenkinsLocationConfiguration.java +++ b/core/src/main/java/jenkins/model/JenkinsLocationConfiguration.java @@ -14,7 +14,6 @@ import javax.mail.internet.InternetAddress; import javax.servlet.ServletContext; import java.io.File; import java.io.IOException; -import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; import java.util.logging.Level; import java.util.logging.Logger; @@ -108,9 +107,14 @@ public class JenkinsLocationConfiguration extends GlobalConfiguration { */ private void updateSecureSessionFlag() { try { - boolean v = fixNull(jenkinsUrl).startsWith("https"); ServletContext context = Jenkins.getInstance().servletContext; - Method m = context.getClass().getMethod("getSessionCookieConfig"); + Method m; + try { + m = context.getClass().getMethod("getSessionCookieConfig"); + } catch (NoSuchMethodException x) { // 3.0+ + LOGGER.log(Level.FINE, "Failed to set secure cookie flag", x); + return; + } Object sessionCookieConfig = m.invoke(context); // not exposing session cookie to JavaScript to mitigate damage caused by XSS @@ -119,9 +123,10 @@ public class JenkinsLocationConfiguration extends GlobalConfiguration { setHttpOnly.invoke(sessionCookieConfig,true); Method setSecure = scc.getMethod("setSecure",boolean.class); + boolean v = fixNull(jenkinsUrl).startsWith("https"); setSecure.invoke(sessionCookieConfig,v); } catch (Exception e) { - LOGGER.log(Level.WARNING, "Failed to set secure cookie flag. Maybe running on Servlet 2.5 and younger?", e); + LOGGER.log(Level.WARNING, "Failed to set secure cookie flag", e); } } From db757116f157e397b33143e81f913dea39b2aff7 Mon Sep 17 00:00:00 2001 From: Kohsuke Kawaguchi Date: Tue, 2 Sep 2014 14:05:13 -0700 Subject: [PATCH 2/5] [FIXED SECURITY-128] It may be that the 'newName' exists and just not visible to the user trying to do a rename --- .../main/java/hudson/model/AbstractItem.java | 33 ++++++++++++++----- core/src/main/java/hudson/security/ACL.java | 16 +++++++++ .../hudson/model/AbstractProjectTest.groovy | 32 ++++++++++++++++++ 3 files changed, 73 insertions(+), 8 deletions(-) diff --git a/core/src/main/java/hudson/model/AbstractItem.java b/core/src/main/java/hudson/model/AbstractItem.java index 6419fc2b45..2aa752f57b 100644 --- a/core/src/main/java/hudson/model/AbstractItem.java +++ b/core/src/main/java/hudson/model/AbstractItem.java @@ -34,6 +34,7 @@ import hudson.cli.declarative.CLIMethod; import hudson.cli.declarative.CLIResolver; import hudson.model.listeners.ItemListener; import hudson.model.listeners.SaveableListener; +import hudson.remoting.Callable; import hudson.security.AccessControlled; import hudson.security.Permission; import hudson.security.ACL; @@ -43,6 +44,7 @@ import hudson.util.AtomicFileWriter; import hudson.util.IOException2; import hudson.util.IOUtils; import jenkins.model.Jenkins; +import org.acegisecurity.Authentication; import org.apache.tools.ant.taskdefs.Copy; import org.apache.tools.ant.types.FileSet; import org.kohsuke.stapler.WebMethod; @@ -199,7 +201,7 @@ public abstract class AbstractItem extends Actionable implements Item, HttpDelet * Not all the Items need to support this operation, but if you decide to do so, * you can use this method. */ - protected void renameTo(String newName) throws IOException { + protected void renameTo(final String newName) throws IOException { // always synchronize from bigger objects first final ItemGroup parent = getParent(); synchronized (parent) { @@ -212,13 +214,28 @@ public abstract class AbstractItem extends Actionable implements Item, HttpDelet if (this.name.equals(newName)) return; - Item existing = parent.getItem(newName); - if (existing != null && existing!=this) - // the look up is case insensitive, so we need "existing!=this" - // to allow people to rename "Foo" to "foo", for example. - // see http://www.nabble.com/error-on-renaming-project-tt18061629.html - throw new IllegalArgumentException("Job " + newName - + " already exists"); + // the test to see if the project already exists or not needs to be done in escalated privilege + // to avoid overwriting + ACL.impersonate(ACL.SYSTEM,new Callable() { + final Authentication user = Jenkins.getAuthentication(); + @Override + public Void call() throws IOException { + Item existing = parent.getItem(newName); + if (existing != null && existing!=AbstractItem.this) { + if (existing.getACL().hasPermission(user,Item.DISCOVER)) + // the look up is case insensitive, so we need "existing!=this" + // to allow people to rename "Foo" to "foo", for example. + // see http://www.nabble.com/error-on-renaming-project-tt18061629.html + throw new IllegalArgumentException("Job " + newName + " already exists"); + else { + // can't think of any real way to hide this, but at least the error message could be vague. + throw new IOException("Unable to rename to " + newName); + } + } + return null; + } + }); + String oldName = this.name; File oldRoot = this.getRootDir(); diff --git a/core/src/main/java/hudson/security/ACL.java b/core/src/main/java/hudson/security/ACL.java index 132344fc52..2ba2e7f322 100644 --- a/core/src/main/java/hudson/security/ACL.java +++ b/core/src/main/java/hudson/security/ACL.java @@ -23,6 +23,7 @@ */ package hudson.security; +import hudson.remoting.Callable; import jenkins.security.NonSerializableSecurityContext; import jenkins.model.Jenkins; import org.acegisecurity.AccessDeniedException; @@ -145,4 +146,19 @@ public abstract class ACL { } } + /** + * Safer variant of {@link #impersonate(Authentication)} that does not require a finally-block. + * @param auth authentication, such as {@link #SYSTEM} + * @param body an action to run with this alternate authentication in effect + * @since 1.581 + */ + public static V impersonate(Authentication auth, Callable body) throws T { + SecurityContext old = impersonate(auth); + try { + return body.call(); + } finally { + SecurityContextHolder.setContext(old); + } + } + } diff --git a/test/src/test/groovy/hudson/model/AbstractProjectTest.groovy b/test/src/test/groovy/hudson/model/AbstractProjectTest.groovy index fba2fa13e7..a17d5965df 100644 --- a/test/src/test/groovy/hudson/model/AbstractProjectTest.groovy +++ b/test/src/test/groovy/hudson/model/AbstractProjectTest.groovy @@ -483,4 +483,36 @@ public class AbstractProjectTest extends HudsonTestCase { def t = j.triggers()[1] assert t == newTrigger } + + public void testRenameToPrivileged() { + def secret = jenkins.createProject(FreeStyleProject.class,"secret"); + def regular = jenkins.createProject(FreeStyleProject.class,"regular") + + jenkins.securityRealm = createDummySecurityRealm(); + def auth = new ProjectMatrixAuthorizationStrategy(); + jenkins.authorizationStrategy = auth; + + auth.add(Jenkins.ADMINISTER, "alice"); + auth.add(Jenkins.READ, "bob"); + + // bob the regular user can only see regular jobs + regular.addProperty(new AuthorizationMatrixProperty([(Job.READ) : ["bob"] as Set])); + + def wc = createWebClient() + wc.login("bob") + wc.executeOnServer { + assert jenkins.getItem("secret")==null; + try { + regular.renameTo("secret") + fail("rename as an overwrite should have failed"); + } catch (Exception e) { + // expected rename to fail in some non-descriptive generic way + e.printStackTrace() + } + } + + // those two jobs should still be there + assert jenkins.getItem("regular")!=null; + assert jenkins.getItem("secret")!=null; + } } From 31df138d696a954b6279cd54c7cf825a78b0576c Mon Sep 17 00:00:00 2001 From: Kohsuke Kawaguchi Date: Tue, 2 Sep 2014 14:16:53 -0700 Subject: [PATCH 3/5] [SECURITY-87] After talking to Jesse, he's OK with me bringing it back to public so long as we don't allow other programmatic dependencies to it. The intention of leaving them mutable is to allow admins to play with this in the groovy script during the initialization and at runtime. Groovy currently ignores the private access modifier anyway, but that is considered as a bug in the upstream (https://jira.codehaus.org/browse/GROOVY-3010) --- core/src/main/java/hudson/model/FullDuplexHttpChannel.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/hudson/model/FullDuplexHttpChannel.java b/core/src/main/java/hudson/model/FullDuplexHttpChannel.java index 8106dbe1b8..6372b60ed2 100644 --- a/core/src/main/java/hudson/model/FullDuplexHttpChannel.java +++ b/core/src/main/java/hudson/model/FullDuplexHttpChannel.java @@ -28,6 +28,8 @@ import hudson.remoting.PingThread; import hudson.remoting.Channel.Mode; import hudson.util.ChunkedOutputStream; import hudson.util.ChunkedInputStream; +import org.kohsuke.accmod.Restricted; +import org.kohsuke.accmod.restrictions.NoExternalUse; import org.kohsuke.stapler.StaplerRequest; import org.kohsuke.stapler.StaplerResponse; @@ -151,10 +153,12 @@ abstract public class FullDuplexHttpChannel { /** * Set to true if the servlet container doesn't support chunked encoding. */ + @Restricted(NoExternalUse.class) public static boolean DIY_CHUNKING = Boolean.getBoolean("hudson.diyChunking"); /** * Controls the time out of waiting for the 2nd HTTP request to arrive. */ - private static long CONNECTION_TIMEOUT = TimeUnit.SECONDS.toMillis(15); + @Restricted(NoExternalUse.class) + public static long CONNECTION_TIMEOUT = TimeUnit.SECONDS.toMillis(15); } From 54d44502bd6ec8df23bf8ef0413bdad650ef321e Mon Sep 17 00:00:00 2001 From: Kohsuke Kawaguchi Date: Tue, 2 Sep 2014 16:12:47 -0700 Subject: [PATCH 4/5] [FIXED SECURITY-127] CONFIGURE permission shouldn't allow the type of the job to be changed. That's more of CREATE+DELETE. In any case, the code doesn't correctly handling submitting config.xml for a different type. --- .../main/java/hudson/model/AbstractItem.java | 8 +++- .../hudson/model/AbstractProjectTest.groovy | 39 ++++++++++++++++++- 2 files changed, 44 insertions(+), 3 deletions(-) diff --git a/core/src/main/java/hudson/model/AbstractItem.java b/core/src/main/java/hudson/model/AbstractItem.java index 2aa752f57b..15de44afa6 100644 --- a/core/src/main/java/hudson/model/AbstractItem.java +++ b/core/src/main/java/hudson/model/AbstractItem.java @@ -603,7 +603,13 @@ public abstract class AbstractItem extends Actionable implements Item, HttpDelet } // try to reflect the changes by reloading - new XmlFile(Items.XSTREAM, out.getTemporaryFile()).unmarshal(this); + Object o = new XmlFile(Items.XSTREAM, out.getTemporaryFile()).unmarshal(this); + if (o!=this) { + // ensure that we've got the same job type. extending this code to support updating + // to different job type requires destroying & creating a new job type + throw new IOException("Expecting "+this.getClass()+" but got "+o.getClass()+" instead"); + } + Items.updatingByXml.set(true); try { onLoad(getParent(), getRootDir().getName()); diff --git a/test/src/test/groovy/hudson/model/AbstractProjectTest.groovy b/test/src/test/groovy/hudson/model/AbstractProjectTest.groovy index a17d5965df..2cb0198a49 100644 --- a/test/src/test/groovy/hudson/model/AbstractProjectTest.groovy +++ b/test/src/test/groovy/hudson/model/AbstractProjectTest.groovy @@ -27,7 +27,8 @@ import com.gargoylesoftware.htmlunit.ElementNotFoundException import com.gargoylesoftware.htmlunit.FailingHttpStatusCodeException; import com.gargoylesoftware.htmlunit.HttpMethod; import com.gargoylesoftware.htmlunit.WebRequestSettings -import com.gargoylesoftware.htmlunit.html.HtmlPage; +import com.gargoylesoftware.htmlunit.html.HtmlPage +import hudson.maven.MavenModuleSet; import hudson.security.*; import hudson.tasks.BuildTrigger; import hudson.tasks.Shell; @@ -44,7 +45,7 @@ import hudson.util.StreamTaskListener; import hudson.util.OneShotEvent import jenkins.model.Jenkins; import org.acegisecurity.context.SecurityContext; -import org.acegisecurity.context.SecurityContextHolder; +import org.acegisecurity.context.SecurityContextHolder import org.jvnet.hudson.test.HudsonTestCase import org.jvnet.hudson.test.Bug; import org.jvnet.hudson.test.MemoryAssert; @@ -515,4 +516,38 @@ public class AbstractProjectTest extends HudsonTestCase { assert jenkins.getItem("regular")!=null; assert jenkins.getItem("secret")!=null; } + + + /** + * Trying to POST to config.xml by a different job type should fail. + */ + public void testConfigDotXmlSubmissionToDifferentType() { + jenkins.crumbIssuer = null + def p = createFreeStyleProject() + + HttpURLConnection con = postConfigDotXml(p, "") + + // this should fail with a type mismatch error + // the error message should report both what was submitted and what was expected + assert con.responseCode == 500 + def msg = con.errorStream.text + println msg + assert msg.contains(FreeStyleProject.class.name) + assert msg.contains(MavenModuleSet.class.name) + + // control. this should work + con = postConfigDotXml(p, "") + assert con.responseCode == 200 + } + + private HttpURLConnection postConfigDotXml(FreeStyleProject p, String xml) { + HttpURLConnection con = new URL(getURL(), "job/${p.name}/config.xml").openConnection() + con.requestMethod = "POST" + con.setRequestProperty("Content-Type", "application/xml") + con.doOutput = true + con.outputStream.withStream { s -> + s.write(xml.bytes) + } + return con + } } From c93635b9e9326586778300e5cbc13c93252e960e Mon Sep 17 00:00:00 2001 From: Kohsuke Kawaguchi Date: Tue, 2 Sep 2014 16:15:22 -0700 Subject: [PATCH 5/5] [SECURITY-128] Plugins that depend on LTS shouldn't be using this API. --- core/src/main/java/hudson/security/ACL.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/hudson/security/ACL.java b/core/src/main/java/hudson/security/ACL.java index 2ba2e7f322..a911e56426 100644 --- a/core/src/main/java/hudson/security/ACL.java +++ b/core/src/main/java/hudson/security/ACL.java @@ -33,6 +33,8 @@ import org.acegisecurity.context.SecurityContextHolder; import org.acegisecurity.providers.UsernamePasswordAuthenticationToken; import org.acegisecurity.acls.sid.PrincipalSid; import org.acegisecurity.acls.sid.Sid; +import org.kohsuke.accmod.Restricted; +import org.kohsuke.accmod.restrictions.NoExternalUse; /** * Gate-keeper that controls access to Hudson's model objects. @@ -150,8 +152,9 @@ public abstract class ACL { * Safer variant of {@link #impersonate(Authentication)} that does not require a finally-block. * @param auth authentication, such as {@link #SYSTEM} * @param body an action to run with this alternate authentication in effect - * @since 1.581 */ + // TODO: this line should be removed in the master branch once this commit is merged to mainline releases + @Restricted(NoExternalUse.class) public static V impersonate(Authentication auth, Callable body) throws T { SecurityContext old = impersonate(auth); try {