diff --git a/core/pom.xml b/core/pom.xml index 3a30aa95542869a9f28e24225c6f11bd4802b64d..584ab57f03539c96789ebe322c43f40737a3f161 100644 --- a/core/pom.xml +++ b/core/pom.xml @@ -156,7 +156,7 @@ THE SOFTWARE. org.kohsuke.stapler stapler-adjunct-zeroclipboard - 1.1.7-1 + 1.3.5-1 org.kohsuke.stapler diff --git a/core/src/main/java/hudson/Plugin.java b/core/src/main/java/hudson/Plugin.java index e2a061fbda6791bfd97679e95daa84117e0aba54..1e11d23bcd848cc1625542e564cbc7833228fa05 100644 --- a/core/src/main/java/hudson/Plugin.java +++ b/core/src/main/java/hudson/Plugin.java @@ -30,19 +30,19 @@ import hudson.model.Saveable; import hudson.model.listeners.ItemListener; import hudson.model.listeners.SaveableListener; import hudson.model.Descriptor.FormException; -import org.kohsuke.stapler.MetaClass; import org.kohsuke.stapler.StaplerRequest; import org.kohsuke.stapler.StaplerResponse; import javax.servlet.ServletContext; import javax.servlet.ServletException; -import javax.servlet.http.HttpServletResponse; import java.io.IOException; import java.io.File; -import java.net.URL; import net.sf.json.JSONObject; import com.thoughtworks.xstream.XStream; +import java.net.URI; +import java.net.URISyntaxException; +import org.kohsuke.stapler.HttpResponses; /** * Base class of Hudson plugin. @@ -204,15 +204,13 @@ public abstract class Plugin implements Saveable { public void doDynamic(StaplerRequest req, StaplerResponse rsp) throws IOException, ServletException { String path = req.getRestOfPath(); + if (path.startsWith("/META-INF/") || path.startsWith("/WEB-INF/")) { + throw HttpResponses.notFound(); + } + if(path.length()==0) path = "/"; - if(path.indexOf("..")!=-1 || path.length()<1) { - // don't serve anything other than files in the sub directory. - rsp.sendError(HttpServletResponse.SC_BAD_REQUEST); - return; - } - // Stapler routes requests like the "/static/.../foo/bar/zot" to be treated like "/foo/bar/zot" // and this is used to serve long expiration header, by using Jenkins.VERSION_HASH as "..." // to create unique URLs. Recognize that and set a long expiration header. @@ -222,7 +220,11 @@ public abstract class Plugin implements Saveable { long expires = staticLink ? TimeUnit2.DAYS.toMillis(365) : -1; // use serveLocalizedFile to support automatic locale selection - rsp.serveLocalizedFile(req, new URL(wrapper.baseResourceURL,'.'+path),expires); + try { + rsp.serveLocalizedFile(req, wrapper.baseResourceURL.toURI().resolve(new URI(null, '.' + path, null)).toURL(), expires); + } catch (URISyntaxException x) { + throw new IOException(x); + } } // diff --git a/core/src/main/java/hudson/cli/CLICommand.java b/core/src/main/java/hudson/cli/CLICommand.java index a25872d9298402bf2d88e79ff1f453d7f080f652..410605951a83e0c542644a456b3b359c128f40c1 100644 --- a/core/src/main/java/hudson/cli/CLICommand.java +++ b/core/src/main/java/hudson/cli/CLICommand.java @@ -37,6 +37,7 @@ import hudson.remoting.ChannelProperty; import hudson.security.CliAuthenticator; import hudson.security.SecurityRealm; import org.acegisecurity.Authentication; +import org.acegisecurity.BadCredentialsException; import org.acegisecurity.context.SecurityContext; import org.acegisecurity.context.SecurityContextHolder; import org.apache.commons.discovery.ResourceClassIterator; @@ -63,6 +64,7 @@ import java.nio.charset.Charset; import java.nio.charset.UnsupportedCharsetException; import java.util.List; import java.util.Locale; +import java.util.UUID; import java.util.logging.Level; import java.util.logging.Logger; @@ -240,6 +242,13 @@ public abstract class CLICommand implements ExtensionPoint, Cloneable { // signals an error without stack trace stderr.println(e.getMessage()); return -1; + } catch (BadCredentialsException e) { + // to the caller, we can't reveal whether the user didn't exist or the password didn't match. + // do that to the server log instead + String id = UUID.randomUUID().toString(); + LOGGER.log(Level.INFO, "CLI login attempt failed: "+id, e); + stderr.println("Bad Credentials. Search the server log for "+id+" for more details."); + return -1; } catch (Exception e) { e.printStackTrace(stderr); return -1; diff --git a/core/src/main/java/hudson/model/AbstractItem.java b/core/src/main/java/hudson/model/AbstractItem.java index 115a01e63597ffbd3b9236f39732e71b386bf2c1..9d8f3ec1f53cbaac5496b8201b4da473747def2a 100644 --- a/core/src/main/java/hudson/model/AbstractItem.java +++ b/core/src/main/java/hudson/model/AbstractItem.java @@ -43,6 +43,7 @@ import hudson.util.AlternativeUiTextProvider.Message; import hudson.util.AtomicFileWriter; 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; @@ -210,7 +211,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) { @@ -223,13 +224,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; String oldFullName = getFullName(); @@ -634,7 +650,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.whileUpdatingByXml(new Callable() { @Override public Void call() throws IOException { onLoad(getParent(), getRootDir().getName()); diff --git a/core/src/main/java/hudson/model/FullDuplexHttpChannel.java b/core/src/main/java/hudson/model/FullDuplexHttpChannel.java index c75b30d5389f1392c668041cf1671f6f277f72c9..6372b60ed21b917ec65346f035e188f8f5078e87 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; @@ -36,6 +38,8 @@ import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; import java.util.UUID; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; import java.util.logging.Level; import java.util.logging.Logger; @@ -78,9 +82,14 @@ abstract public class FullDuplexHttpChannel { out.write("Starting HTTP duplex channel".getBytes()); out.flush(); - // wait until we have the other channel - while(upload==null) - wait(); + {// wait until we have the other channel + long end = System.currentTimeMillis() + CONNECTION_TIMEOUT; + while (upload == null && System.currentTimeMillis() V impersonate(Authentication auth, Callable body) throws T { + SecurityContext old = impersonate(auth); + try { + return body.call(); + } finally { + SecurityContextHolder.setContext(old); + } + } + } diff --git a/core/src/main/java/jenkins/model/JenkinsLocationConfiguration.java b/core/src/main/java/jenkins/model/JenkinsLocationConfiguration.java index 4f6ab1ce873e23dcb4e46c6214511813f8b84e89..6836467217f28345b9d286caa312199d10d5a5a8 100644 --- a/core/src/main/java/jenkins/model/JenkinsLocationConfiguration.java +++ b/core/src/main/java/jenkins/model/JenkinsLocationConfiguration.java @@ -11,11 +11,15 @@ import org.kohsuke.stapler.StaplerRequest; import javax.mail.internet.AddressException; import javax.mail.internet.InternetAddress; +import javax.servlet.ServletContext; import java.io.File; import java.io.IOException; +import java.lang.reflect.Method; import java.util.logging.Level; import java.util.logging.Logger; +import static hudson.Util.fixNull; + /** * Stores the location of Jenkins (e-mail address and the HTTP URL.) * @@ -63,6 +67,8 @@ public class JenkinsLocationConfiguration extends GlobalConfiguration { } else { super.load(); } + + updateSecureSessionFlag(); } public String getAdminAddress() { @@ -91,6 +97,37 @@ public class JenkinsLocationConfiguration extends GlobalConfiguration { url += '/'; this.jenkinsUrl = url; save(); + updateSecureSessionFlag(); + } + + /** + * If the Jenkins URL starts from "https", force the secure session flag + * + * @see discussion of this topic in OWASP + */ + private void updateSecureSessionFlag() { + try { + ServletContext context = Jenkins.getInstance().servletContext; + 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 + Class scc = Class.forName("javax.servlet.SessionCookieConfig"); + Method setHttpOnly = scc.getMethod("setHttpOnly",boolean.class); + 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", e); + } } @Override diff --git a/core/src/main/resources/hudson/model/LoadStatistics/main.jelly b/core/src/main/resources/hudson/model/LoadStatistics/main.jelly index fc241545dbf32a207bc3f8e451afec9b15468640..6030347bdb19f107c22053bbc4cdc44d35bcb44e 100644 --- a/core/src/main/resources/hudson/model/LoadStatistics/main.jelly +++ b/core/src/main/resources/hudson/model/LoadStatistics/main.jelly @@ -29,7 +29,16 @@ THE SOFTWARE. ${%title(it.displayName)} - + + + + + + + + + +
${%Timespan}: diff --git a/core/src/main/resources/hudson/model/PasswordParameterDefinition/index.jelly b/core/src/main/resources/hudson/model/PasswordParameterDefinition/index.jelly index a93046e764ae89db6cd04f865e7e0829a59dfc35..9bac23608cc5a435122249e64f2429574c9cbe8b 100644 --- a/core/src/main/resources/hudson/model/PasswordParameterDefinition/index.jelly +++ b/core/src/main/resources/hudson/model/PasswordParameterDefinition/index.jelly @@ -29,7 +29,7 @@ THE SOFTWARE.
- +
\ No newline at end of file diff --git a/core/src/main/resources/lib/layout/copyButton/copyButton.css b/core/src/main/resources/lib/layout/copyButton/copyButton.css index 5925034b9c004d8f5b5c4ad5b2cd367e14d5e2bf..0c9f98058e3d1d72c4284b4300bed5ffd64a2366 100644 --- a/core/src/main/resources/lib/layout/copyButton/copyButton.css +++ b/core/src/main/resources/lib/layout/copyButton/copyButton.css @@ -1,3 +1,6 @@ +.copy-button { + display: inline-block; +} .copy-button BUTTON { background: url('clipboard.png') center center no-repeat; width: 20px; diff --git a/core/src/main/resources/lib/layout/copyButton/copyButton.js b/core/src/main/resources/lib/layout/copyButton/copyButton.js index 6e5283952a227f50f7ed1661fac38b8520d01666..aa9e557a5d8493766fe342855e85b560cdb93a3e 100644 --- a/core/src/main/resources/lib/layout/copyButton/copyButton.js +++ b/core/src/main/resources/lib/layout/copyButton/copyButton.js @@ -5,8 +5,7 @@ Behaviour.specify("span.copy-button", 'copyButton', 0, function(e) { var id = "copy-button"+(iota++); btn.id = id; - var clip = new ZeroClipboard.Client(); - clip.setText(e.getAttribute("text")); + var clip = new ZeroClipboard(e); makeButton(btn); clip.setHandCursor(true); @@ -14,24 +13,24 @@ Behaviour.specify("span.copy-button", 'copyButton', 0, function(e) { if (container) { container = $(e).up(container); container.style.position = "relative"; - clip.glue(id,container); - } else { - clip.glue(id); } - clip.addEventListener('onComplete',function() { + clip.on('datarequested',function() { + clip.setText(e.getAttribute("text")); + }); + clip.on('complete',function() { notificationBar.show(e.getAttribute("message")); }); - clip.addEventListener('onMouseOver',function() { + clip.on('mouseOver',function() { $(id).addClassName('yui-button-hover') }); - clip.addEventListener('onMouseOut',function() { + clip.on('mouseOut',function() { $(id).removeClassName('yui-button-hover') }); - clip.addEventListener('onMouseDown',function() { + clip.on('mouseDown',function() { $(id).addClassName('yui-button-active') }); - clip.addEventListener('onMouseUp',function() { + clip.on('mouseUp',function() { $(id).removeClassName('yui-button-active') }); }); diff --git a/test/src/test/groovy/hudson/model/AbstractProjectTest.groovy b/test/src/test/groovy/hudson/model/AbstractProjectTest.groovy index ea4b0e064960ddd6149083bbea3445687e9e00e1..56e8696754fd423a71a9f5d8c590822a8ba5aeb8 100644 --- a/test/src/test/groovy/hudson/model/AbstractProjectTest.groovy +++ b/test/src/test/groovy/hudson/model/AbstractProjectTest.groovy @@ -33,6 +33,10 @@ import hudson.tasks.BuildStepMonitor; import hudson.tasks.BuildTrigger import hudson.tasks.Publisher import hudson.tasks.Recorder; +import com.gargoylesoftware.htmlunit.html.HtmlPage +import hudson.maven.MavenModuleSet; +import hudson.security.*; +import hudson.tasks.BuildTrigger; import hudson.tasks.Shell; import hudson.scm.NullSCM; import hudson.scm.SCM @@ -49,7 +53,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 @@ -524,4 +528,70 @@ public class AbstractProjectTest extends HudsonTestCase { done.signal() } + + 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; + } + + + /** + * 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 + } } diff --git a/test/src/test/java/hudson/PluginTest.java b/test/src/test/java/hudson/PluginTest.java new file mode 100644 index 0000000000000000000000000000000000000000..05e7156dfcb86f91ed55b20413cf72920af080e3 --- /dev/null +++ b/test/src/test/java/hudson/PluginTest.java @@ -0,0 +1,51 @@ +/* + * The MIT License + * + * Copyright 2014 Jesse Glick. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ + +package hudson; + +import javax.servlet.http.HttpServletResponse; +import org.junit.Test; +import org.junit.Rule; +import org.jvnet.hudson.test.Issue; +import org.jvnet.hudson.test.JenkinsRule; + +public class PluginTest { + + @Rule public JenkinsRule r = new JenkinsRule(); + + @Issue("SECURITY-131") // TODO test-annotations 1.2+: @Issue({"SECURITY-131", "SECURITY-155"}) + @Test public void doDynamic() throws Exception { + r.createWebClient().goTo("plugin/credentials/images/24x24/credentials.png", "image/png"); + /* Collapsed somewhere before it winds up in restOfPath: + r.createWebClient().assertFails("plugin/credentials/images/../images/24x24/credentials.png", HttpServletResponse.SC_BAD_REQUEST); + */ + r.createWebClient().assertFails("plugin/credentials/images/%2E%2E/images/24x24/credentials.png", HttpServletResponse.SC_INTERNAL_SERVER_ERROR); // IAE from TokenList. + r.createWebClient().assertFails("plugin/credentials/images/%252E%252E/images/24x24/credentials.png", HttpServletResponse.SC_NOT_FOUND); // SECURITY-131 + r.createWebClient().assertFails("plugin/credentials/images/%25252E%25252E/images/24x24/credentials.png", HttpServletResponse.SC_NOT_FOUND); // just checking + // SECURITY-155: + r.createWebClient().assertFails("plugin/credentials/WEB-INF/licenses.xml", HttpServletResponse.SC_NOT_FOUND); + r.createWebClient().assertFails("plugin/credentials/META-INF/MANIFEST.MF", HttpServletResponse.SC_NOT_FOUND); + } + +} diff --git a/test/src/test/java/hudson/security/CliAuthenticationTest.java b/test/src/test/java/hudson/security/CliAuthenticationTest.java index 81b829fc02e464525ad3e1f970fa9b10003ba075..aca444fe178efa9528c094f59eb971fedce58e0b 100644 --- a/test/src/test/java/hudson/security/CliAuthenticationTest.java +++ b/test/src/test/java/hudson/security/CliAuthenticationTest.java @@ -7,11 +7,15 @@ import hudson.cli.LoginCommand; import hudson.cli.LogoutCommand; import jenkins.model.Jenkins; import org.acegisecurity.Authentication; +import org.apache.commons.io.input.NullInputStream; import org.junit.Assert; import org.jvnet.hudson.test.For; import org.jvnet.hudson.test.HudsonTestCase; import org.jvnet.hudson.test.TestExtension; +import java.io.ByteArrayOutputStream; +import java.util.Arrays; + /** * @author Kohsuke Kawaguchi */ @@ -36,6 +40,17 @@ public class CliAuthenticationTest extends HudsonTestCase { } } + private String commandAndOutput(String... args) throws Exception { + CLI cli = new CLI(getURL()); + try { + ByteArrayOutputStream baos = new ByteArrayOutputStream(); + cli.execute(Arrays.asList(args), new NullInputStream(0), baos, baos); + return baos.toString(); + } finally { + cli.close(); + } + } + @TestExtension public static class TestCommand extends CLICommand { @Override @@ -76,4 +91,20 @@ public class CliAuthenticationTest extends HudsonTestCase { successfulCommand("logout"); successfulCommand("anonymous"); // now we should run as anonymous } + + /** + * Login failure shouldn't reveal information about the existence of user + */ + public void testSecurity110() throws Exception { + HudsonPrivateSecurityRealm realm = new HudsonPrivateSecurityRealm(false,false,null); + jenkins.setSecurityRealm(realm); + jenkins.setAuthorizationStrategy(new FullControlOnceLoggedInAuthorizationStrategy()); + realm.createAccount("alice","alice"); + + String out1 = commandAndOutput("help", "--username", "alice", "--password", "bogus"); + String out2 = commandAndOutput("help", "--username", "bob", "--password", "bogus"); + + assertTrue(out1.contains("Bad Credentials. Search the server log for")); + assertTrue(out2.contains("Bad Credentials. Search the server log for")); + } }