diff --git a/core/src/main/java/hudson/Functions.java b/core/src/main/java/hudson/Functions.java index 8b3f39f9fe..2fd3428a67 100644 --- a/core/src/main/java/hudson/Functions.java +++ b/core/src/main/java/hudson/Functions.java @@ -1752,7 +1752,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/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 f0f300805e..3dd2b32603 100644 --- a/core/src/main/java/hudson/model/AbstractItem.java +++ b/core/src/main/java/hudson/model/AbstractItem.java @@ -40,6 +40,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; @@ -54,11 +55,14 @@ 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; 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; @@ -71,12 +75,16 @@ 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; 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; /** @@ -600,9 +608,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"); - IOUtils.copy(getConfigFile().getFile(),rsp.getOutputStream()); + writeConfigDotXml(rsp.getOutputStream()); return; } if (req.getMethod().equals("POST")) { @@ -615,6 +622,33 @@ 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}. + * 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()) { + if (Secret.decrypt(matcher.group(1)) != 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/core/src/main/java/hudson/model/Item.java b/core/src/main/java/hudson/model/Item.java index 94ee4181de..834cf97b48 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. @@ -226,6 +227,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 15c1283743..a4921d06ec 100644 --- a/core/src/main/java/hudson/model/ItemGroupMixIn.java +++ b/core/src/main/java/hudson/model/ItemGroupMixIn.java @@ -29,6 +29,7 @@ import hudson.model.listeners.ItemListener; import hudson.security.AccessControlled; 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,7 +48,9 @@ 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.acegisecurity.AccessDeniedException; import org.xml.sax.SAXException; /** @@ -224,13 +227,23 @@ 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) { + // 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)); + } + } + } 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/core/src/main/java/hudson/util/Secret.java b/core/src/main/java/hudson/util/Secret.java index 80be75b8ff..9a4dcb750a 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. @@ -128,6 +131,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/main/resources/hudson/model/Messages.properties b/core/src/main/resources/hudson/model/Messages.properties index 571ec8867f..4df930581a 100644 --- a/core/src/main/resources/hudson/model/Messages.properties +++ b/core/src/main/resources/hudson/model/Messages.properties @@ -168,7 +168,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. 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 36d24a51d4..67963da3ba 100644 --- a/test/src/test/java/lib/form/PasswordTest.java +++ b/test/src/test/java/lib/form/PasswordTest.java @@ -23,13 +23,35 @@ */ package lib.form; +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; +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.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; 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 @@ -51,4 +73,94 @@ public class PasswordTest extends HudsonTestCase implements Describable {} + + @Issue("SECURITY-266") + public void testExposedCiphertext() throws Exception { + boolean saveEnabled = Item.EXTENDED_READ.getEnabled(); + try { + jenkins.setSecurityRealm(createDummySecurityRealm()); + // TODO 1.645+ use MockAuthorizationStrategy + GlobalMatrixAuthorizationStrategy pmas = new GlobalMatrixAuthorizationStrategy(); + pmas.add(Jenkins.ADMINISTER, "admin"); + pmas.add(Jenkins.READ, "dev"); + pmas.add(Item.READ, "dev"); + Item.EXTENDED_READ.setEnabled(true); + pmas.add(Item.EXTENDED_READ, "dev"); + pmas.add(Item.CREATE, "dev"); // so we can show CopyJobCommand would barf; more realistic would be to grant it only in a subfolder + jenkins.setAuthorizationStrategy(pmas); + Secret s = Secret.fromString("s3cr3t"); + String sEnc = s.getEncryptedValue(); + FreeStyleProject p = createFreeStyleProject("p"); + p.setDisplayName("Unicode here ←"); + p.setDescription("This+looks+like+Base64+but+is+not+a+secret"); + p.addProperty(new VulnerableProperty(s)); + WebClient wc = createWebClient(); + // Control case: an administrator can read and write configuration freely. + wc.login("admin"); + HtmlPage configure = wc.getPage(p, "configure"); + assertThat(configure.getWebResponse().getContentAsString(), containsString(sEnc)); + submit(configure.getFormByName("config")); + VulnerableProperty vp = p.getProperty(VulnerableProperty.class); + assertNotNull(vp); + assertEquals(s, vp.secret); + Page configXml = wc.goTo(p.getUrl() + "config.xml", "application/xml"); + String xmlAdmin = configXml.getWebResponse().getContentAsString(); + assertThat(xmlAdmin, containsString("" + 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(); + Authentication adminAuth = User.get("admin").impersonate(); + getJobCommand.setTransportAuth(adminAuth); + ByteArrayOutputStream baos = new ByteArrayOutputStream(); + 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))); + configXml = wc.goTo(p.getUrl() + "config.xml", "application/xml"); + String xmlDev = configXml.getWebResponse().getContentAsString(); + assertThat(xmlDev, not(containsString(sEnc))); + assertEquals(xmlAdmin.replace(sEnc, "********"), xmlDev); + getJobCommand = new GetJobCommand(); + Authentication devAuth = User.get("dev").impersonate(); + getJobCommand.setTransportAuth(devAuth); + baos = new ByteArrayOutputStream(); + getJobCommand.main(Collections.singletonList(pName), Locale.ENGLISH, System.in, new PrintStream(baos), System.err); + assertEquals(xmlDev, baos.toString(configXml.getWebResponse().getContentCharset())); + copyJobCommand = new CopyJobCommand(); + copyJobCommand.setTransportAuth(devAuth); + String pDevName = pName + "-dev"; + assertThat(copyJobCommand.main(Arrays.asList(pName, pDevName), Locale.ENGLISH, System.in, System.out, System.err), not(0)); + assertNull(jenkins.getItemByFullName(pDevName, FreeStyleProject.class)); + + } finally { + Item.EXTENDED_READ.setEnabled(saveEnabled); + } + } + public static class VulnerableProperty extends JobProperty { + 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 @@ + + + + + + + + +