Merge branch 'security-stable-1.609' into security-stable-1.625

core/src/main/java/hudson/Functions.java

@@ -1728,7 +1728,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");
         }

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;
     }
 }

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;
@@ -55,11 +56,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;
@@ -72,12 +76,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;
 
 /**
@@ -601,9 +609,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")) {
@@ -616,6 +623,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)}

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);

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;
 
 /**
@@ -221,13 +224,23 @@ public abstract class ItemGroupMixIn {
     public synchronized <T extends TopLevelItem> 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();

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.

core/src/main/resources/hudson/model/Messages.properties

@@ -169,7 +169,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.

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"))

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
@@ -55,4 +77,94 @@ public class PasswordTest extends HudsonTestCase implements Describable<Password
             return null;
         }
     }
+
+    @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("<secret>" + sEnc + "</secret>"));
+            assertThat(xmlAdmin, containsString("<displayName>" + p.getDisplayName() + "</displayName>"));
+            assertThat(xmlAdmin, containsString("<description>" + p.getDescription() + "</description>"));
+            // 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<FreeStyleProject> {
+        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";
+            }
+        }
+    }
+
 }

test/src/test/resources/lib/form/PasswordTest/VulnerableProperty/config.jelly

+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+The MIT License
+
+Copyright 2016 CloudBees, Inc.
+
+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.
+-->
+
+<?jelly escape-by-default='true'?>
+<j:jelly xmlns:j="jelly:core" xmlns:f="/lib/form">
+    <f:entry field="secret" title="secret">
+        <f:password/>
+    </f:entry>
+</j:jelly>