From 82b42359797e42fa4b2428d5e390e7ce91b74998 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Tue, 19 Apr 2016 17:44:31 -0400 Subject: [PATCH 1/5] [SECURITY-266] Conceal ciphertext from the web UI for a user who has no Item.CONFIGURE permission. --- core/src/main/java/hudson/Functions.java | 11 +++- test/src/test/java/lib/form/PasswordTest.java | 59 +++++++++++++++++++ .../VulnerableProperty/config.jelly | 31 ++++++++++ 3 files changed, 100 insertions(+), 1 deletion(-) create mode 100644 test/src/test/resources/lib/form/PasswordTest/VulnerableProperty/config.jelly diff --git a/core/src/main/java/hudson/Functions.java b/core/src/main/java/hudson/Functions.java index 6c4377652a..88c21defed 100644 --- a/core/src/main/java/hudson/Functions.java +++ b/core/src/main/java/hudson/Functions.java @@ -1715,7 +1715,16 @@ public class Functions { */ public String getPasswordValue(Object o) { if (o==null) return null; - if (o instanceof Secret) return ((Secret)o).getEncryptedValue(); + if (o instanceof Secret) { + StaplerRequest req = Stapler.getCurrentRequest(); + if (req != null) { + Item item = req.findAncestorObject(Item.class); + if (item != null && !item.hasPermission(Item.CONFIGURE)) { + return ""; + } + } + return ((Secret) o).getEncryptedValue(); + } if (getIsUnitTest()) { throw new SecurityException("attempted to render plaintext ‘" + o + "’ in password field; use a getter of type Secret instead"); } diff --git a/test/src/test/java/lib/form/PasswordTest.java b/test/src/test/java/lib/form/PasswordTest.java index 030c38f504..e4e103cc4b 100644 --- a/test/src/test/java/lib/form/PasswordTest.java +++ b/test/src/test/java/lib/form/PasswordTest.java @@ -28,8 +28,20 @@ import com.gargoylesoftware.htmlunit.html.HtmlPage; import hudson.Extension; import hudson.model.Describable; import hudson.model.Descriptor; +import hudson.model.FreeStyleProject; +import hudson.model.Item; +import hudson.model.JobProperty; +import hudson.model.JobPropertyDescriptor; +import hudson.security.GlobalMatrixAuthorizationStrategy; import hudson.util.Secret; +import jenkins.model.Jenkins; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.not; +import static org.junit.Assert.assertThat; import org.jvnet.hudson.test.HudsonTestCase; +import org.jvnet.hudson.test.Issue; +import org.jvnet.hudson.test.TestExtension; +import org.kohsuke.stapler.DataBoundConstructor; /** * @author Kohsuke Kawaguchi @@ -55,4 +67,51 @@ public class PasswordTest extends HudsonTestCase implements Describable { + public final Secret secret; + @DataBoundConstructor + public VulnerableProperty(Secret secret) { + this.secret = secret; + } + @TestExtension("testExposedCiphertext") + public static class DescriptorImpl extends JobPropertyDescriptor { + @Override // TODO delete in 1.635+ + public String getDisplayName() { + return "VulnerableProperty"; + } + } + } + } diff --git a/test/src/test/resources/lib/form/PasswordTest/VulnerableProperty/config.jelly b/test/src/test/resources/lib/form/PasswordTest/VulnerableProperty/config.jelly new file mode 100644 index 0000000000..0de0f24317 --- /dev/null +++ b/test/src/test/resources/lib/form/PasswordTest/VulnerableProperty/config.jelly @@ -0,0 +1,31 @@ + + + + + + + + + From e8f6410793b418be3786b38dbe4fddfbb23db4cf Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Wed, 20 Apr 2016 09:14:42 -0400 Subject: [PATCH 2/5] [SECURITY-266] Conceal ciphertext from REST config.xml. --- core/src/main/java/hudson/Functions.java | 2 +- .../main/java/hudson/model/AbstractItem.java | 23 ++++++++++++++++++- core/src/main/java/hudson/util/Secret.java | 11 +++++++++ .../test/groovy/hudson/util/SecretTest.groovy | 13 +++++++++++ test/src/test/java/lib/form/PasswordTest.java | 23 +++++++++++++++---- 5 files changed, 65 insertions(+), 7 deletions(-) diff --git a/core/src/main/java/hudson/Functions.java b/core/src/main/java/hudson/Functions.java index 88c21defed..fcfae2c3cf 100644 --- a/core/src/main/java/hudson/Functions.java +++ b/core/src/main/java/hudson/Functions.java @@ -1720,7 +1720,7 @@ public class Functions { if (req != null) { Item item = req.findAncestorObject(Item.class); if (item != null && !item.hasPermission(Item.CONFIGURE)) { - return ""; + return "(some secret)"; } } return ((Secret) o).getEncryptedValue(); diff --git a/core/src/main/java/hudson/model/AbstractItem.java b/core/src/main/java/hudson/model/AbstractItem.java index 980da8ed44..bea1a07c9e 100644 --- a/core/src/main/java/hudson/model/AbstractItem.java +++ b/core/src/main/java/hudson/model/AbstractItem.java @@ -41,6 +41,7 @@ import hudson.util.AlternativeUiTextProvider; import hudson.util.AlternativeUiTextProvider.Message; import hudson.util.AtomicFileWriter; import hudson.util.IOUtils; +import hudson.util.Secret; import jenkins.model.DirectlyModifiableTopLevelItemGroup; import jenkins.model.Jenkins; import jenkins.security.NotReallyRoleSensitiveCallable; @@ -60,6 +61,8 @@ import java.util.List; import java.util.ListIterator; import java.util.logging.Level; import java.util.logging.Logger; +import java.util.regex.Matcher; +import java.util.regex.Pattern; import javax.annotation.Nonnull; import org.kohsuke.stapler.StaplerRequest; @@ -78,6 +81,7 @@ import javax.xml.transform.stream.StreamResult; import javax.xml.transform.stream.StreamSource; import static javax.servlet.http.HttpServletResponse.SC_BAD_REQUEST; +import org.apache.commons.io.FileUtils; import org.kohsuke.stapler.Ancestor; /** @@ -594,6 +598,7 @@ public abstract class AbstractItem extends Actionable implements Item, HttpDelet Util.deleteRecursive(getRootDir()); } + private static final Pattern SECRET_PATTERN = Pattern.compile(">(" + Secret.ENCRYPTED_VALUE_PATTERN + ")<"); /** * Accepts config.xml submission, as well as serve it. */ @@ -604,7 +609,23 @@ public abstract class AbstractItem extends Actionable implements Item, HttpDelet // read checkPermission(EXTENDED_READ); rsp.setContentType("application/xml"); - IOUtils.copy(getConfigFile().getFile(),rsp.getOutputStream()); + XmlFile configFile = getConfigFile(); + if (hasPermission(CONFIGURE)) { + IOUtils.copy(configFile.getFile(), rsp.getOutputStream()); + } else { + String encoding = configFile.sniffEncoding(); + String xml = FileUtils.readFileToString(configFile.getFile(), encoding); + Matcher matcher = SECRET_PATTERN.matcher(xml); + StringBuffer cleanXml = new StringBuffer(); + while (matcher.find()) { + String text = matcher.group(1); + if (Secret.decrypt(text) != null) { + matcher.appendReplacement(cleanXml, ">(some secret)<"); + } + } + matcher.appendTail(cleanXml); + org.apache.commons.io.IOUtils.write(cleanXml.toString(), rsp.getOutputStream(), encoding); + } return; } if (req.getMethod().equals("POST")) { diff --git a/core/src/main/java/hudson/util/Secret.java b/core/src/main/java/hudson/util/Secret.java index 90144ed6af..2effd821e2 100644 --- a/core/src/main/java/hudson/util/Secret.java +++ b/core/src/main/java/hudson/util/Secret.java @@ -40,6 +40,9 @@ import java.io.Serializable; import java.io.UnsupportedEncodingException; import java.io.IOException; import java.security.GeneralSecurityException; +import java.util.regex.Pattern; +import org.kohsuke.accmod.Restricted; +import org.kohsuke.accmod.restrictions.NoExternalUse; /** * Glorified {@link String} that uses encryption in the persisted form, to avoid accidental exposure of a secret. @@ -126,6 +129,14 @@ public final class Secret implements Serializable { } } + /** + * Pattern matching a possible output of {@link #getEncryptedValue}. + * Basically, any Base64-encoded value. + * You must then call {@link #decrypt} to eliminate false positives. + */ + @Restricted(NoExternalUse.class) + public static final Pattern ENCRYPTED_VALUE_PATTERN = Pattern.compile("[A-Za-z0-9+/]+={0,2}"); + /** * Reverse operation of {@link #getEncryptedValue()}. Returns null * if the given cipher text was invalid. diff --git a/core/src/test/groovy/hudson/util/SecretTest.groovy b/core/src/test/groovy/hudson/util/SecretTest.groovy index e6532dd8f9..f7a0ba3ad0 100644 --- a/core/src/test/groovy/hudson/util/SecretTest.groovy +++ b/core/src/test/groovy/hudson/util/SecretTest.groovy @@ -26,9 +26,11 @@ package hudson.util import com.trilead.ssh2.crypto.Base64; import jenkins.model.Jenkins import jenkins.security.ConfidentialStoreRule; +import org.apache.commons.lang.RandomStringUtils; import org.junit.Rule import org.junit.Test +import java.util.Random; import javax.crypto.Cipher; /** @@ -54,6 +56,17 @@ public class SecretTest { assert secret==Secret.fromString(secret.encryptedValue); } + @Test + void testEncryptedValuePattern() { + for (int i = 1; i < 100; i++) { + String plaintext = RandomStringUtils.random(new Random().nextInt(i)); + String ciphertext = Secret.fromString(plaintext).getEncryptedValue(); + //println "${plaintext} → ${ciphertext}" + assert Secret.ENCRYPTED_VALUE_PATTERN.matcher(ciphertext).matches(); + } + assert !Secret.ENCRYPTED_VALUE_PATTERN.matcher("hello world").matches(); + } + @Test void testDecrypt() { assert "abc"==Secret.toString(Secret.fromString("abc")) diff --git a/test/src/test/java/lib/form/PasswordTest.java b/test/src/test/java/lib/form/PasswordTest.java index e4e103cc4b..e5a3ebcee6 100644 --- a/test/src/test/java/lib/form/PasswordTest.java +++ b/test/src/test/java/lib/form/PasswordTest.java @@ -23,6 +23,7 @@ */ package lib.form; +import com.gargoylesoftware.htmlunit.Page; import com.gargoylesoftware.htmlunit.html.HtmlInput; import com.gargoylesoftware.htmlunit.html.HtmlPage; import hudson.Extension; @@ -81,20 +82,32 @@ public class PasswordTest extends HudsonTestCase implements Describable" + sEnc + "")); + assertThat(xmlAdmin, containsString("" + p.getDisplayName() + "")); + assertThat(xmlAdmin, containsString("" + p.getDescription() + "")); wc.login("dev"); configure = wc.getPage(p, "configure"); - assertThat(configure.getWebResponse().getContentAsString(), not(containsString(secret.getEncryptedValue()))); + assertThat(configure.getWebResponse().getContentAsString(), not(containsString(sEnc))); + configXml = wc.goTo(p.getUrl() + "config.xml", "application/xml"); + String xmlDev = configXml.getWebResponse().getContentAsString(); + assertThat(xmlDev, not(containsString(sEnc))); + assertEquals(xmlAdmin.replace(sEnc, "(some secret)"), xmlDev); } finally { Item.EXTENDED_READ.setEnabled(saveEnabled); } From dc2aa07e582a3ff5d427c3546634c70686268714 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Wed, 20 Apr 2016 09:44:02 -0400 Subject: [PATCH 3/5] [SECURITY-266] Protecting GetJobCommand. --- core/src/main/java/hudson/Functions.java | 2 +- .../main/java/hudson/cli/GetJobCommand.java | 7 +-- .../main/java/hudson/model/AbstractItem.java | 50 ++++++++++++------- test/src/test/java/lib/form/PasswordTest.java | 19 ++++++- 4 files changed, 52 insertions(+), 26 deletions(-) diff --git a/core/src/main/java/hudson/Functions.java b/core/src/main/java/hudson/Functions.java index fcfae2c3cf..653e75f3e5 100644 --- a/core/src/main/java/hudson/Functions.java +++ b/core/src/main/java/hudson/Functions.java @@ -1720,7 +1720,7 @@ public class Functions { if (req != null) { Item item = req.findAncestorObject(Item.class); if (item != null && !item.hasPermission(Item.CONFIGURE)) { - return "(some secret)"; + return "********"; } } return ((Secret) o).getEncryptedValue(); diff --git a/core/src/main/java/hudson/cli/GetJobCommand.java b/core/src/main/java/hudson/cli/GetJobCommand.java index efc1e90836..051e75f6c0 100644 --- a/core/src/main/java/hudson/cli/GetJobCommand.java +++ b/core/src/main/java/hudson/cli/GetJobCommand.java @@ -25,8 +25,6 @@ package hudson.cli; import hudson.Extension; import hudson.model.AbstractItem; -import hudson.model.Item; -import hudson.util.IOUtils; import org.kohsuke.args4j.Argument; /** @@ -43,10 +41,7 @@ public class GetJobCommand extends CLICommand { } protected int run() throws Exception { - job.checkPermission(Item.EXTENDED_READ); - IOUtils.copy( - job.getConfigFile().getFile(), - stdout); + job.writeConfigDotXml(stdout); return 0; } } diff --git a/core/src/main/java/hudson/model/AbstractItem.java b/core/src/main/java/hudson/model/AbstractItem.java index bea1a07c9e..f3802e1df0 100644 --- a/core/src/main/java/hudson/model/AbstractItem.java +++ b/core/src/main/java/hudson/model/AbstractItem.java @@ -56,6 +56,7 @@ import org.kohsuke.stapler.export.ExportedBean; import java.io.File; import java.io.IOException; +import java.io.OutputStream; import java.util.Collection; import java.util.List; import java.util.ListIterator; @@ -75,6 +76,7 @@ import org.kohsuke.stapler.interceptor.RequirePOST; import org.xml.sax.SAXException; import javax.servlet.ServletException; +import javax.servlet.ServletOutputStream; import javax.xml.transform.Source; import javax.xml.transform.TransformerException; import javax.xml.transform.stream.StreamResult; @@ -82,6 +84,8 @@ import javax.xml.transform.stream.StreamSource; import static javax.servlet.http.HttpServletResponse.SC_BAD_REQUEST; import org.apache.commons.io.FileUtils; +import org.kohsuke.accmod.Restricted; +import org.kohsuke.accmod.restrictions.NoExternalUse; import org.kohsuke.stapler.Ancestor; /** @@ -607,25 +611,8 @@ public abstract class AbstractItem extends Actionable implements Item, HttpDelet throws IOException { if (req.getMethod().equals("GET")) { // read - checkPermission(EXTENDED_READ); rsp.setContentType("application/xml"); - XmlFile configFile = getConfigFile(); - if (hasPermission(CONFIGURE)) { - IOUtils.copy(configFile.getFile(), rsp.getOutputStream()); - } else { - String encoding = configFile.sniffEncoding(); - String xml = FileUtils.readFileToString(configFile.getFile(), encoding); - Matcher matcher = SECRET_PATTERN.matcher(xml); - StringBuffer cleanXml = new StringBuffer(); - while (matcher.find()) { - String text = matcher.group(1); - if (Secret.decrypt(text) != null) { - matcher.appendReplacement(cleanXml, ">(some secret)<"); - } - } - matcher.appendTail(cleanXml); - org.apache.commons.io.IOUtils.write(cleanXml.toString(), rsp.getOutputStream(), encoding); - } + writeConfigDotXml(rsp.getOutputStream()); return; } if (req.getMethod().equals("POST")) { @@ -638,6 +625,33 @@ public abstract class AbstractItem extends Actionable implements Item, HttpDelet rsp.sendError(SC_BAD_REQUEST); } + /** + * Writes {@code config.xml} to the specified output stream. + * The user must have at least {@link #EXTENDED_READ}. + * If he lacks {@link #CONFIGURE}, then any {@link Secret}s detected will be masked out. + */ + @Restricted(NoExternalUse.class) + public void writeConfigDotXml(OutputStream os) throws IOException { + checkPermission(EXTENDED_READ); + XmlFile configFile = getConfigFile(); + if (hasPermission(CONFIGURE)) { + IOUtils.copy(configFile.getFile(), os); + } else { + String encoding = configFile.sniffEncoding(); + String xml = FileUtils.readFileToString(configFile.getFile(), encoding); + Matcher matcher = SECRET_PATTERN.matcher(xml); + StringBuffer cleanXml = new StringBuffer(); + while (matcher.find()) { + String text = matcher.group(1); + if (Secret.decrypt(text) != null) { + matcher.appendReplacement(cleanXml, ">********<"); + } + } + matcher.appendTail(cleanXml); + org.apache.commons.io.IOUtils.write(cleanXml.toString(), os, encoding); + } + } + /** * @deprecated as of 1.473 * Use {@link #updateByXml(Source)} diff --git a/test/src/test/java/lib/form/PasswordTest.java b/test/src/test/java/lib/form/PasswordTest.java index e5a3ebcee6..52ad7ca6b0 100644 --- a/test/src/test/java/lib/form/PasswordTest.java +++ b/test/src/test/java/lib/form/PasswordTest.java @@ -27,14 +27,20 @@ import com.gargoylesoftware.htmlunit.Page; import com.gargoylesoftware.htmlunit.html.HtmlInput; import com.gargoylesoftware.htmlunit.html.HtmlPage; import hudson.Extension; +import hudson.cli.GetJobCommand; import hudson.model.Describable; import hudson.model.Descriptor; import hudson.model.FreeStyleProject; import hudson.model.Item; import hudson.model.JobProperty; import hudson.model.JobPropertyDescriptor; +import hudson.model.User; import hudson.security.GlobalMatrixAuthorizationStrategy; import hudson.util.Secret; +import java.io.ByteArrayOutputStream; +import java.io.PrintStream; +import java.util.Collections; +import java.util.Locale; import jenkins.model.Jenkins; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.not; @@ -101,13 +107,24 @@ public class PasswordTest extends HudsonTestCase implements Describable" + sEnc + "")); assertThat(xmlAdmin, containsString("" + p.getDisplayName() + "")); assertThat(xmlAdmin, containsString("" + p.getDescription() + "")); + // CLICommandInvoker does not work here, as it sets up its own SecurityRealm + AuthorizationStrategy. + GetJobCommand getJobCommand = new GetJobCommand(); + getJobCommand.setTransportAuth(User.get("admin").impersonate()); + ByteArrayOutputStream baos = new ByteArrayOutputStream(); + getJobCommand.main(Collections.singletonList(p.getFullName()), Locale.ENGLISH, System.in, new PrintStream(baos), System.err); + assertEquals(xmlAdmin, baos.toString(configXml.getWebResponse().getContentCharset())); wc.login("dev"); configure = wc.getPage(p, "configure"); assertThat(configure.getWebResponse().getContentAsString(), not(containsString(sEnc))); configXml = wc.goTo(p.getUrl() + "config.xml", "application/xml"); String xmlDev = configXml.getWebResponse().getContentAsString(); assertThat(xmlDev, not(containsString(sEnc))); - assertEquals(xmlAdmin.replace(sEnc, "(some secret)"), xmlDev); + assertEquals(xmlAdmin.replace(sEnc, "********"), xmlDev); + getJobCommand = new GetJobCommand(); + getJobCommand.setTransportAuth(User.get("dev").impersonate()); + baos = new ByteArrayOutputStream(); + getJobCommand.main(Collections.singletonList(p.getFullName()), Locale.ENGLISH, System.in, new PrintStream(baos), System.err); + assertEquals(xmlDev, baos.toString(configXml.getWebResponse().getContentCharset())); } finally { Item.EXTENDED_READ.setEnabled(saveEnabled); } From 3d655938189af46d40e55268c280b6dcaa3770cd Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Wed, 20 Apr 2016 10:09:41 -0400 Subject: [PATCH 4/5] [SECURITY-266] Protecting ItemGroupMixIn.copy (used by both CLI and REST). --- .../main/java/hudson/model/AbstractItem.java | 5 ++- core/src/main/java/hudson/model/Item.java | 6 ++++ .../java/hudson/model/ItemGroupMixIn.java | 14 ++++++++- test/src/test/java/lib/form/PasswordTest.java | 31 ++++++++++++++++--- 4 files changed, 48 insertions(+), 8 deletions(-) diff --git a/core/src/main/java/hudson/model/AbstractItem.java b/core/src/main/java/hudson/model/AbstractItem.java index f3802e1df0..cc07db4e97 100644 --- a/core/src/main/java/hudson/model/AbstractItem.java +++ b/core/src/main/java/hudson/model/AbstractItem.java @@ -602,7 +602,6 @@ public abstract class AbstractItem extends Actionable implements Item, HttpDelet Util.deleteRecursive(getRootDir()); } - private static final Pattern SECRET_PATTERN = Pattern.compile(">(" + Secret.ENCRYPTED_VALUE_PATTERN + ")<"); /** * Accepts config.xml submission, as well as serve it. */ @@ -625,6 +624,7 @@ public abstract class AbstractItem extends Actionable implements Item, HttpDelet rsp.sendError(SC_BAD_REQUEST); } + static final Pattern SECRET_PATTERN = Pattern.compile(">(" + Secret.ENCRYPTED_VALUE_PATTERN + ")<"); /** * Writes {@code config.xml} to the specified output stream. * The user must have at least {@link #EXTENDED_READ}. @@ -642,8 +642,7 @@ public abstract class AbstractItem extends Actionable implements Item, HttpDelet Matcher matcher = SECRET_PATTERN.matcher(xml); StringBuffer cleanXml = new StringBuffer(); while (matcher.find()) { - String text = matcher.group(1); - if (Secret.decrypt(text) != null) { + if (Secret.decrypt(matcher.group(1)) != null) { matcher.appendReplacement(cleanXml, ">********<"); } } diff --git a/core/src/main/java/hudson/model/Item.java b/core/src/main/java/hudson/model/Item.java index 726796fa8e..3dadf08163 100644 --- a/core/src/main/java/hudson/model/Item.java +++ b/core/src/main/java/hudson/model/Item.java @@ -35,6 +35,7 @@ import hudson.search.SearchableModelObject; import hudson.security.Permission; import hudson.security.PermissionGroup; import hudson.security.AccessControlled; +import hudson.util.Secret; /** * Basic configuration unit in Hudson. @@ -225,6 +226,11 @@ public interface Item extends PersistenceRoot, SearchableModelObject, AccessCont Permission CONFIGURE = new Permission(PERMISSIONS, "Configure", Messages._Item_CONFIGURE_description(), Permission.CONFIGURE, PermissionScope.ITEM); Permission READ = new Permission(PERMISSIONS, "Read", Messages._Item_READ_description(), Permission.READ, PermissionScope.ITEM); Permission DISCOVER = new Permission(PERMISSIONS, "Discover", Messages._AbstractProject_DiscoverPermission_Description(), READ, PermissionScope.ITEM); + /** + * Ability to view configuration details. + * If the user lacks {@link CONFIGURE} then any {@link Secret}s must be masked out, even in encrypted form. + * @see Secret#ENCRYPTED_VALUE_PATTERN + */ Permission EXTENDED_READ = new Permission(PERMISSIONS,"ExtendedRead", Messages._AbstractProject_ExtendedReadPermission_Description(), CONFIGURE, Boolean.getBoolean("hudson.security.ExtendedReadPermission"), new PermissionScope[]{PermissionScope.ITEM}); // TODO the following really belong in Job, not Item, but too late to move since the owner.name is encoded in the ID: Permission BUILD = new Permission(PERMISSIONS, "Build", Messages._AbstractProject_BuildPermission_Description(), Permission.UPDATE, PermissionScope.ITEM); diff --git a/core/src/main/java/hudson/model/ItemGroupMixIn.java b/core/src/main/java/hudson/model/ItemGroupMixIn.java index db4fb0aa8a..f41616aff4 100644 --- a/core/src/main/java/hudson/model/ItemGroupMixIn.java +++ b/core/src/main/java/hudson/model/ItemGroupMixIn.java @@ -27,8 +27,10 @@ import hudson.Util; import hudson.XmlFile; import hudson.model.listeners.ItemListener; import hudson.security.AccessControlled; +import hudson.security.AccessDeniedException2; import hudson.util.CopyOnWriteMap; import hudson.util.Function1; +import hudson.util.Secret; import jenkins.model.Jenkins; import jenkins.util.xml.XMLUtils; import org.kohsuke.stapler.StaplerRequest; @@ -47,6 +49,7 @@ import java.io.InputStream; import java.util.Map; import java.util.logging.Level; import java.util.logging.Logger; +import java.util.regex.Matcher; import jenkins.security.NotReallyRoleSensitiveCallable; import org.xml.sax.SAXException; @@ -221,13 +224,22 @@ public abstract class ItemGroupMixIn { public synchronized T copy(T src, String name) throws IOException { acl.checkPermission(Item.CREATE); src.checkPermission(Item.EXTENDED_READ); + XmlFile srcConfigFile = Items.getConfigFile(src); + if (!src.hasPermission(Item.CONFIGURE)) { + Matcher matcher = AbstractItem.SECRET_PATTERN.matcher(srcConfigFile.asString()); + while (matcher.find()) { + if (Secret.decrypt(matcher.group(1)) != null) { + throw new AccessDeniedException2(Jenkins.getAuthentication(), Item.CONFIGURE); + } + } + } src.getDescriptor().checkApplicableIn(parent); acl.getACL().checkCreatePermission(parent, src.getDescriptor()); T result = (T)createProject(src.getDescriptor(),name,false); // copy config - Util.copyFile(Items.getConfigFile(src).getFile(),Items.getConfigFile(result).getFile()); + Util.copyFile(srcConfigFile.getFile(), Items.getConfigFile(result).getFile()); // reload from the new config final File rootDir = result.getRootDir(); diff --git a/test/src/test/java/lib/form/PasswordTest.java b/test/src/test/java/lib/form/PasswordTest.java index 52ad7ca6b0..595b0e7e85 100644 --- a/test/src/test/java/lib/form/PasswordTest.java +++ b/test/src/test/java/lib/form/PasswordTest.java @@ -27,6 +27,7 @@ import com.gargoylesoftware.htmlunit.Page; import com.gargoylesoftware.htmlunit.html.HtmlInput; import com.gargoylesoftware.htmlunit.html.HtmlPage; import hudson.Extension; +import hudson.cli.CopyJobCommand; import hudson.cli.GetJobCommand; import hudson.model.Describable; import hudson.model.Descriptor; @@ -39,9 +40,11 @@ import hudson.security.GlobalMatrixAuthorizationStrategy; import hudson.util.Secret; import java.io.ByteArrayOutputStream; import java.io.PrintStream; +import java.util.Arrays; import java.util.Collections; import java.util.Locale; import jenkins.model.Jenkins; +import org.acegisecurity.Authentication; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.not; import static org.junit.Assert.assertThat; @@ -87,6 +90,7 @@ public class PasswordTest extends HudsonTestCase implements Describable" + p.getDescription() + "")); // CLICommandInvoker does not work here, as it sets up its own SecurityRealm + AuthorizationStrategy. GetJobCommand getJobCommand = new GetJobCommand(); - getJobCommand.setTransportAuth(User.get("admin").impersonate()); + Authentication adminAuth = User.get("admin").impersonate(); + getJobCommand.setTransportAuth(adminAuth); ByteArrayOutputStream baos = new ByteArrayOutputStream(); - getJobCommand.main(Collections.singletonList(p.getFullName()), Locale.ENGLISH, System.in, new PrintStream(baos), System.err); + String pName = p.getFullName(); + getJobCommand.main(Collections.singletonList(pName), Locale.ENGLISH, System.in, new PrintStream(baos), System.err); assertEquals(xmlAdmin, baos.toString(configXml.getWebResponse().getContentCharset())); + CopyJobCommand copyJobCommand = new CopyJobCommand(); + copyJobCommand.setTransportAuth(adminAuth); + String pAdminName = pName + "-admin"; + assertEquals(0, copyJobCommand.main(Arrays.asList(pName, pAdminName), Locale.ENGLISH, System.in, System.out, System.err)); + FreeStyleProject pAdmin = jenkins.getItemByFullName(pAdminName, FreeStyleProject.class); + assertNotNull(pAdmin); + pAdmin.setDisplayName(p.getDisplayName()); // counteract DisplayNameListener + assertEquals(p.getConfigFile().asString(), pAdmin.getConfigFile().asString()); + // Test case: another user with EXTENDED_READ but not CONFIGURE should not get access even to encrypted secrets. wc.login("dev"); configure = wc.getPage(p, "configure"); assertThat(configure.getWebResponse().getContentAsString(), not(containsString(sEnc))); @@ -121,10 +137,17 @@ public class PasswordTest extends HudsonTestCase implements Describable Date: Fri, 29 Apr 2016 08:18:13 -0400 Subject: [PATCH 5/5] More informative error when refusing to copy. --- core/src/main/java/hudson/model/ItemGroupMixIn.java | 5 +++-- core/src/main/resources/hudson/model/Messages.properties | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/core/src/main/java/hudson/model/ItemGroupMixIn.java b/core/src/main/java/hudson/model/ItemGroupMixIn.java index f41616aff4..e973a52e69 100644 --- a/core/src/main/java/hudson/model/ItemGroupMixIn.java +++ b/core/src/main/java/hudson/model/ItemGroupMixIn.java @@ -27,7 +27,6 @@ import hudson.Util; import hudson.XmlFile; import hudson.model.listeners.ItemListener; import hudson.security.AccessControlled; -import hudson.security.AccessDeniedException2; import hudson.util.CopyOnWriteMap; import hudson.util.Function1; import hudson.util.Secret; @@ -51,6 +50,7 @@ import java.util.logging.Level; import java.util.logging.Logger; import java.util.regex.Matcher; import jenkins.security.NotReallyRoleSensitiveCallable; +import org.acegisecurity.AccessDeniedException; import org.xml.sax.SAXException; /** @@ -229,7 +229,8 @@ public abstract class ItemGroupMixIn { Matcher matcher = AbstractItem.SECRET_PATTERN.matcher(srcConfigFile.asString()); while (matcher.find()) { if (Secret.decrypt(matcher.group(1)) != null) { - throw new AccessDeniedException2(Jenkins.getAuthentication(), Item.CONFIGURE); + // AccessDeniedException2 does not permit a custom message, and anyway redirecting the user to the login screen is obviously pointless. + throw new AccessDeniedException(Messages.ItemGroupMixIn_may_not_copy_as_it_contains_secrets_and_(src.getFullName(), Jenkins.getAuthentication().getName(), Item.PERMISSIONS.title, Item.EXTENDED_READ.name, Item.CONFIGURE.name)); } } } diff --git a/core/src/main/resources/hudson/model/Messages.properties b/core/src/main/resources/hudson/model/Messages.properties index c9b024bca3..ed8f1e6be0 100644 --- a/core/src/main/resources/hudson/model/Messages.properties +++ b/core/src/main/resources/hudson/model/Messages.properties @@ -175,7 +175,7 @@ Item.CREATE.description=Create a new job. Item.DELETE.description=Delete a job. Item.CONFIGURE.description=Change the configuration of a job. Item.READ.description=See a job. (You may deny this permission but allow Discover to force an anonymous user to log in to see the job.) - +ItemGroupMixIn.may_not_copy_as_it_contains_secrets_and_=May not copy {0} as it contains secrets and {1} has {2}/{3} but not /{4} Job.AllRecentBuildFailed=All recent builds failed. Job.BuildStability=Build stability: {0} Job.NOfMFailed={0} out of the last {1} builds failed.