[SECURITY-3371]

src/main/java/org/jenkinsci/plugins/structs/describable/DescribableModel.java

@@ -13,6 +13,7 @@
 import hudson.model.Result;
 import hudson.model.TaskListener;
 import hudson.util.LogTaskListener;
+import hudson.util.Secret;
 import jenkins.model.Jenkins;
 import org.apache.commons.io.IOUtils;
 import org.apache.commons.lang.ClassUtils;
@@ -331,6 +332,11 @@ public T instantiate(Map<String,?> arguments, @CheckForNull TaskListener listene
             injectSetters(o, arguments, listener);
             return o;
         } catch (Exception x) {
+            if (arguments.values().stream().anyMatch(o -> o instanceof Secret)) {
+                LOGGER.log(Level.FINE, "Could not instantiate " + arguments + " for " + this.type.getName() + ": " + x);
+                throw new IllegalArgumentException("Could not instantiate arguments for " + this.type.getName()
+                        + ". Secrets are involved, so details are available on more verbose logging levels.");
+            }
             throw new IllegalArgumentException("Could not instantiate " + arguments + " for " + this.type.getName() + ": " + x, x);
         }
     }
@@ -672,7 +678,12 @@ public UninstantiatedDescribable uninstantiate2(T o) throws UnsupportedOperation
         try {
             control = instantiate(constructorOnlyDataBoundProps, null);
         } catch (Exception x) {
-            LOGGER.log(Level.WARNING, "Cannot create control version of " + type + " using " + constructorOnlyDataBoundProps, x);
+            if (constructorOnlyDataBoundProps.values().stream().anyMatch(obj -> obj instanceof Secret)) {
+                LOGGER.log(Level.FINE, "Cannot create control version of " + type + " using " + constructorOnlyDataBoundProps, x);
+                LOGGER.log(Level.WARNING, "Cannot create control version of " + type + ". Secrets are involved, so details are available on more verbose logging levels.", x);
+            } else {
+                LOGGER.log(Level.WARNING, "Cannot create control version of " + type + " using " + constructorOnlyDataBoundProps, x);
+            }
         }
 
         if (control!=null) {
@@ -696,8 +707,14 @@ public UninstantiatedDescribable uninstantiate2(T o) throws UnsupportedOperation
             try {
                 control = instantiate(nonDeprecatedDataBoundProps, null);
             } catch (Exception x) {
-                LOGGER.log(Level.WARNING,
-                        "Cannot create control version of " + type + " using " + nonDeprecatedDataBoundProps, x);
+                if (nonDeprecatedDataBoundProps.values().stream().anyMatch(obj -> obj instanceof Secret)) {
+                    LOGGER.log(Level.FINE,
+                            "Cannot create control version of " + type + " using " + nonDeprecatedDataBoundProps, x);
+                    LOGGER.log(Level.WARNING, "Cannot create control version of " + type + ". Secrets are involved, so details are available on more verbose logging levels.", x);
+                } else {
+                    LOGGER.log(Level.WARNING,
+                            "Cannot create control version of " + type + " using " + nonDeprecatedDataBoundProps, x);
+                }
             }
 
             if (control != null) {

src/main/java/org/jenkinsci/plugins/structs/describable/UninstantiatedDescribable.java

@@ -2,6 +2,7 @@
 
 import hudson.model.Describable;
 import hudson.model.TaskListener;
+import hudson.util.Secret;
 import org.jenkinsci.Symbol;
 
 import edu.umd.cs.findbugs.annotations.Nullable;
@@ -248,9 +249,14 @@ public String toString() {
         b.append('(');
         boolean first = true;
         for (Entry<String,?> e : arguments.entrySet()) {
-            if (first)  first = false;
-            else        b.append(',');
-            b.append(e.getKey()).append('=').append(e.getValue());
+            if (first)
+                first = false;
+            else
+                b.append(',');
+            if (e.getValue() instanceof Secret)
+                b.append(e.getKey()).append('=').append("****");
+            else
+                b.append(e.getKey()).append('=').append(e.getValue());
         }
         b.append(')');
         return b.toString();

src/test/java/org/jenkinsci/plugins/structs/describable/Security3371Test.java

@@ -0,0 +1,104 @@
+package org.jenkinsci.plugins.structs.describable;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.not;
+
+import hudson.Extension;
+import hudson.model.AbstractDescribableImpl;
+import hudson.model.Descriptor;
+import hudson.util.Secret;
+import java.io.Serializable;
+import java.util.Map;
+import java.util.logging.Level;
+import org.junit.Rule;
+import org.junit.Test;
+import org.jvnet.hudson.test.JenkinsRule;
+import org.jvnet.hudson.test.LoggerRule;
+import org.kohsuke.stapler.DataBoundConstructor;
+import org.kohsuke.stapler.DataBoundSetter;
+
+public class Security3371Test {
+
+    @Rule
+    public JenkinsRule r = new JenkinsRule();
+
+    @Rule
+    public LoggerRule warningLogger = new LoggerRule().record(DescribableModel.class, Level.WARNING).capture(10);
+
+    @Test
+    public void secretsAreNotLoggedWhenInstantiationFails() {
+        String password = "password";
+        try {
+            new DescribableModel<>(Cred.class).instantiate(Map.of("username", "username", "pwd", Secret.fromString(password)), null);
+        } catch (Exception e) {
+            assertThat(e.getMessage(), containsString("Secrets are involved, so details are available on more verbose logging levels."));
+            assertThat(e.getMessage(), not(containsString("pwd=" + password)));
+        }
+    }
+
+    @Test
+    public void secretsAreNotLoggedInUninstantiatedDescribable() {
+        String password = "password";
+        ServerModel serverModel = new ServerModel("http://localhost", new Cred("username", password));
+        new DescribableModel<>(ServerModel.class).uninstantiate2(serverModel);
+        assertThat(warningLogger.toString(), not(containsString("pwd=" + password)));
+    }
+
+    static final class Cred extends AbstractDescribableImpl<Cred> implements Serializable {
+        String username;
+        Secret pwd;
+        String deprecated;
+
+        @DataBoundConstructor
+        public Cred(String username, String pwd) {
+            this.username = username;
+            this.pwd = Secret.fromString(pwd);
+        }
+
+        public String getUsername() {
+            return username;
+        }
+
+        public Secret getPwd() {
+            return pwd;
+        }
+
+        public String getDeprecated() {
+            return deprecated;
+        }
+
+        @DataBoundSetter
+        @Deprecated
+        public void setDeprecated(String deprecated) {
+            this.deprecated = deprecated;
+        }
+
+        @Extension
+        public static class DescriptorImpl extends Descriptor<Cred> {
+        }
+    }
+
+    static final class ServerModel extends AbstractDescribableImpl<ServerModel> implements Serializable {
+        private String host;
+        private Cred cred;
+
+        @DataBoundConstructor
+        public ServerModel(String host, Cred cred) {
+            this.host = host;
+            this.cred = cred;
+        }
+
+        public String getHost() {
+            return host;
+        }
+
+        public Cred getCred() {
+            return cred;
+        }
+
+        @Extension
+        public static class DescriptorImpl extends Descriptor<ServerModel> {
+        }
+    }
+}