From e8f6410793b418be3786b38dbe4fddfbb23db4cf Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Wed, 20 Apr 2016 09:14:42 -0400 Subject: [PATCH] [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); }