[SECURITY-3373]

jenkinsci · Sep 25, 2024 · dfd7a9c · dfd7a9c
1 parent d0c0d07
commit dfd7a9c
Show file tree

Hide file tree

Showing 5 changed files with 88 additions and 26 deletions.
diff --git a/core/src/main/java/hudson/model/AbstractItem.java b/core/src/main/java/hudson/model/AbstractItem.java
@@ -59,8 +59,6 @@
 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.xml.transform.Source;
 import javax.xml.transform.TransformerException;
 import javax.xml.transform.sax.SAXSource;
@@ -70,6 +68,7 @@
 import jenkins.model.Jenkins;
 import jenkins.model.Loadable;
 import jenkins.model.queue.ItemDeletion;
+import jenkins.security.ExtendedReadRedaction;
 import jenkins.security.NotReallyRoleSensitiveCallable;
 import jenkins.security.stapler.StaplerNotDispatchable;
 import jenkins.util.SystemProperties;
@@ -870,11 +869,11 @@ private void doConfigDotXmlImpl(StaplerRequest2 req, StaplerResponse2 rsp)
         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.
+     * If he lacks {@link #CONFIGURE}, then any {@link Secret}s or other sensitive information detected will be masked out.
+     * @see jenkins.security.ExtendedReadRedaction
      */
 
     @Restricted(NoExternalUse.class)
@@ -886,15 +885,13 @@ public void writeConfigDotXml(OutputStream os) throws IOException {
         } else {
             String encoding = configFile.sniffEncoding();
             String xml = Files.readString(Util.fileToPath(configFile.getFile()), Charset.forName(encoding));
-            Matcher matcher = SECRET_PATTERN.matcher(xml);
-            StringBuilder cleanXml = new StringBuilder();
-            while (matcher.find()) {
-                if (Secret.decrypt(matcher.group(1)) != null) {
-                    matcher.appendReplacement(cleanXml, ">********<");
-                }
+
+            for (ExtendedReadRedaction redaction : ExtendedReadRedaction.all()) {
+                LOGGER.log(Level.FINE, () -> "Applying redaction " + redaction.getClass().getName());
+                xml = redaction.apply(xml);
             }
-            matcher.appendTail(cleanXml);
-            org.apache.commons.io.IOUtils.write(cleanXml.toString(), os, encoding);
+
+            org.apache.commons.io.IOUtils.write(xml, os, encoding);
         }
     }
 

diff --git a/core/src/main/java/hudson/model/ItemGroupMixIn.java b/core/src/main/java/hudson/model/ItemGroupMixIn.java
@@ -31,7 +31,6 @@
 import hudson.security.AccessControlled;
 import hudson.util.CopyOnWriteMap;
 import hudson.util.Function1;
-import hudson.util.Secret;
 import io.jenkins.servlet.ServletExceptionWrapper;
 import jakarta.servlet.ServletException;
 import jakarta.servlet.http.HttpServletResponse;
@@ -44,11 +43,11 @@
 import java.util.Map;
 import java.util.logging.Level;
 import java.util.logging.Logger;
-import java.util.regex.Matcher;
 import javax.xml.transform.TransformerException;
 import javax.xml.transform.stream.StreamResult;
 import javax.xml.transform.stream.StreamSource;
 import jenkins.model.Jenkins;
+import jenkins.security.ExtendedReadRedaction;
 import jenkins.security.NotReallyRoleSensitiveCallable;
 import jenkins.util.xml.XMLUtils;
 import org.kohsuke.stapler.StaplerRequest;
@@ -239,18 +238,17 @@ public synchronized <T extends TopLevelItem> T copy(T src, String name) throws I
         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.getAuthentication2().getName(),
-                                    Item.PERMISSIONS.title,
-                                    Item.EXTENDED_READ.name,
-                                    Item.CONFIGURE.name));
-                }
+            final String originalConfigDotXml = srcConfigFile.asString();
+            final String redactedConfigDotXml = ExtendedReadRedaction.applyAll(originalConfigDotXml);
+            if (!originalConfigDotXml.equals(redactedConfigDotXml)) {
+                // 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.getAuthentication2().getName(),
+                                Item.PERMISSIONS.title,
+                                Item.EXTENDED_READ.name,
+                                Item.CONFIGURE.name));
             }
         }
         src.getDescriptor().checkApplicableIn(parent);

diff --git a/core/src/main/java/jenkins/security/ExtendedReadRedaction.java b/core/src/main/java/jenkins/security/ExtendedReadRedaction.java
@@ -0,0 +1,36 @@
+package jenkins.security;
+
+import hudson.ExtensionList;
+import hudson.ExtensionPoint;
+
+/**
+ * Redact {@code config.xml} contents for users with ExtendedRead permission
+ * while lacking the required Configure permission to see the full unredacted
+ * configuration.
+ *
+ * @see <a href="https://issues.jenkins.io/browse/SECURITY-266">SECURITY-266</a>
+ * @see <a href="https://www.jenkins.io/security/advisory/2016-05-11/">Jenkins Security Advisory 2016-05-11</a>
+ * @since TODO
+ */
+public interface ExtendedReadRedaction extends ExtensionPoint {
+    /**
+     * Redacts sensitive information from the provided {@code config.xml} file content.
+     * Input may already have redactions applied; output may be passed through further redactions.
+     * These methods are expected to retain the basic structure of the XML document contained in input/output strings.
+     *
+     * @param configDotXml String representation of (potentially already redacted) config.xml file
+     * @return Redacted config.xml file content
+     */
+    String apply(String configDotXml);
+
+    static ExtensionList<ExtendedReadRedaction> all() {
+        return ExtensionList.lookup(ExtendedReadRedaction.class);
+    }
+
+    static String applyAll(String configDotXml) {
+        for (ExtendedReadRedaction redaction : all()) {
+            configDotXml = redaction.apply(configDotXml);
+        }
+        return configDotXml;
+    }
+}
diff --git a/core/src/main/java/jenkins/security/ExtendedReadSecretRedaction.java b/core/src/main/java/jenkins/security/ExtendedReadSecretRedaction.java
@@ -0,0 +1,28 @@
+package jenkins.security;
+
+import hudson.Extension;
+import hudson.util.Secret;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+import org.kohsuke.accmod.Restricted;
+import org.kohsuke.accmod.restrictions.NoExternalUse;
+
+@Restricted(NoExternalUse.class)
+@Extension
+public class ExtendedReadSecretRedaction implements ExtendedReadRedaction {
+
+    private static final Pattern SECRET_PATTERN = Pattern.compile(">(" + Secret.ENCRYPTED_VALUE_PATTERN + ")<");
+
+    @Override
+    public String apply(String configDotXml) {
+        Matcher matcher = SECRET_PATTERN.matcher(configDotXml);
+        StringBuilder cleanXml = new StringBuilder();
+        while (matcher.find()) {
+            if (Secret.decrypt(matcher.group(1)) != null) {
+                matcher.appendReplacement(cleanXml, ">********<");
+            }
+        }
+        matcher.appendTail(cleanXml);
+        return cleanXml.toString();
+    }
+}
diff --git a/test/src/test/java/lib/form/PasswordTest.java b/test/src/test/java/lib/form/PasswordTest.java
@@ -67,6 +67,7 @@
 import jenkins.model.GlobalConfiguration;
 import jenkins.model.Jenkins;
 import jenkins.model.TransientActionFactory;
+import jenkins.security.ExtendedReadSecretRedaction;
 import jenkins.tasks.SimpleBuildStep;
 import org.htmlunit.Page;
 import org.htmlunit.html.DomElement;
@@ -76,6 +77,7 @@
 import org.htmlunit.html.HtmlTextInput;
 import org.junit.Rule;
 import org.junit.Test;
+import org.jvnet.hudson.test.For;
 import org.jvnet.hudson.test.Issue;
 import org.jvnet.hudson.test.JenkinsRule;
 import org.jvnet.hudson.test.MockAuthorizationStrategy;
@@ -124,6 +126,7 @@ public String getUrlName() {
 
     @Issue({"SECURITY-266", "SECURITY-304"})
     @Test
+    @For(ExtendedReadSecretRedaction.class)
     public void testExposedCiphertext() throws Exception {
         boolean saveEnabled = Item.EXTENDED_READ.getEnabled();
         Item.EXTENDED_READ.setEnabled(true);