[SECURITY-440]

Wadeck · daniel-beck · commit 18b3121fa94a · 2018-06-15T13:01:13.000+02:00
diff --git a/pom.xml b/pom.xml
@@ -66,8 +66,8 @@
   </scm>
 
   <properties>
-    <jenkins.version>1.609</jenkins.version>
-    <java.level>6</java.level>
+    <jenkins.version>1.625</jenkins.version>
+    <java.level>7</java.level>
   </properties>
 
   <repositories>
@@ -95,10 +95,16 @@
     <dependency>
       <groupId>org.jenkins-ci.plugins</groupId>
       <artifactId>credentials</artifactId>
-      <version>2.1.0</version>
+      <version>2.1.17</version>
     </dependency>
     <!-- jenkins dependencies -->
     <!-- test dependencies -->
+    <dependency>
+      <groupId>org.jenkins-ci.plugins</groupId>
+      <artifactId>cloudbees-folder</artifactId>
+      <version>6.4</version>
+      <scope>test</scope>
+    </dependency>
   </dependencies>
 
 </project>
diff --git a/src/main/java/com/cloudbees/jenkins/plugins/sshcredentials/impl/BasicSSHUserPrivateKey.java b/src/main/java/com/cloudbees/jenkins/plugins/sshcredentials/impl/BasicSSHUserPrivateKey.java
@@ -26,27 +26,31 @@
 import com.cloudbees.jenkins.plugins.sshcredentials.SSHUserPrivateKey;
 import com.cloudbees.plugins.credentials.CredentialsProvider;
 import com.cloudbees.plugins.credentials.CredentialsScope;
-import com.cloudbees.plugins.credentials.CredentialsSnapshotTaker;
 import edu.umd.cs.findbugs.annotations.CheckForNull;
 import edu.umd.cs.findbugs.annotations.NonNull;
 import hudson.DescriptorExtensionList;
 import hudson.Extension;
 import hudson.model.AbstractDescribableImpl;
 import hudson.model.Descriptor;
+import hudson.model.Items;
 import hudson.remoting.Channel;
 import hudson.util.Secret;
 import java.io.File;
 import java.io.IOException;
 import java.io.ObjectStreamException;
 import java.io.Serializable;
 import java.io.StringReader;
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
 import java.util.concurrent.TimeUnit;
 import java.util.logging.Level;
 import java.util.logging.Logger;
+
+import hudson.util.XStream2;
 import jenkins.model.Jenkins;
 import net.jcip.annotations.GuardedBy;
 import org.apache.commons.io.FileUtils;
@@ -148,16 +152,6 @@ private synchronized Object readResolve() throws ObjectStreamException {
         return this;
     }
 
-    private Object writeReplace() {
-        if (/* XStream */Channel.current() == null) {
-            return this;
-        }
-        if (privateKeySource == null || privateKeySource.isSnapshotSource()) {
-            return this;
-        }
-        return CredentialsProvider.snapshot(this);
-    }
-
     /**
      * {@inheritDoc}
      */
@@ -290,7 +284,9 @@ public long getPrivateKeysLastModified() {
          *
          * @return {@code true} if and only if the source is self contained.
          * @since 1.7
+         * @deprecated no more used since FileOnMaster- and Users- PrivateKeySource are deprecated too
          */
+        @Deprecated
         public boolean isSnapshotSource() {
             return false;
         }
@@ -371,7 +367,9 @@ public String getDisplayName() {
 
     /**
      * Let the user reference a file on the disk.
+     * @deprecated This approach has security vulnerability and should be migrated to {@link DirectEntryPrivateKeySource}
      */
+    @Deprecated
     public static class FileOnMasterPrivateKeySource extends PrivateKeySource {
 
         /**
@@ -394,8 +392,6 @@ public static class FileOnMasterPrivateKeySource extends PrivateKeySource {
          */
         private transient volatile long nextCheckLastModified;
 
-
-        @DataBoundConstructor
         public FileOnMasterPrivateKeySource(String privateKeyFile) {
             this.privateKeyFile = privateKeyFile;
         }
@@ -436,7 +432,12 @@ private Object readResolve() {
                 // this is a borked upgrade, not actually the file name but is actually the key contents
                 return new DirectEntryPrivateKeySource(privateKeyFile);
             }
-            return this;
+
+            Jenkins.getActiveInstance().checkPermission(Jenkins.RUN_SCRIPTS);
+
+            LOGGER.log(Level.INFO, "SECURITY-440: Migrating FileOnMasterPrivateKeySource to DirectEntryPrivateKeySource");
+            // read the content of the file and then migrate to Direct
+            return new DirectEntryPrivateKeySource(getPrivateKeys());
         }
 
         @Override
@@ -453,26 +454,13 @@ public long getPrivateKeysLastModified() {
             }
             return lastModified;
         }
-
-        /**
-         * {@inheritDoc}
-         */
-        @Extension
-        public static class DescriptorImpl extends PrivateKeySourceDescriptor {
-
-            /**
-             * {@inheritDoc}
-             */
-            @Override
-            public String getDisplayName() {
-                return Messages.BasicSSHUserPrivateKey_FileOnMasterPrivateKeySourceDisplayName();
-            }
-        }
     }
 
     /**
      * Let the user
+     * @deprecated This approach has security vulnerability and should be migrated to {@link DirectEntryPrivateKeySource}
      */
+    @Deprecated
     public static class UsersPrivateKeySource extends PrivateKeySource {
 
         /**
@@ -490,10 +478,6 @@ public static class UsersPrivateKeySource extends PrivateKeySource {
          */
         private transient volatile long nextCheckLastModified;
 
-        @DataBoundConstructor
-        public UsersPrivateKeySource() {
-        }
-
         private List<File> files() {
             List<File> files = new ArrayList<File>();
             File sshHome = new File(new File(System.getProperty("user.home")), ".ssh");
@@ -535,51 +519,27 @@ public long getPrivateKeysLastModified() {
             return lastModified;
         }
 
-        /**
-         * {@inheritDoc}
-         */
-        @Extension
-        public static class DescriptorImpl extends PrivateKeySourceDescriptor {
+        private Object readResolve() {
+            Jenkins.getActiveInstance().checkPermission(Jenkins.RUN_SCRIPTS);
 
-            /**
-             * {@inheritDoc}
-             */
-            @Override
-            public String getDisplayName() {
-                return Messages.BasicSSHUserPrivateKey_UsersPrivateKeySourceDisplayName();
-            }
+            LOGGER.log(Level.INFO, "SECURITY-440: Migrating UsersPrivateKeySource to DirectEntryPrivateKeySource");
+            // read the content of the file and then migrate to Direct
+            return new DirectEntryPrivateKeySource(getPrivateKeys());
         }
     }
 
-    /**
-     * @since 1.7
-     */
-    @Extension
-    public static class CredentialsSnapshotTakerImpl extends CredentialsSnapshotTaker<SSHUserPrivateKey> {
-
-        /**
-         * {@inheritDoc}
-         */
-        @Override
-        public Class<SSHUserPrivateKey> type() {
-            return SSHUserPrivateKey.class;
-        }
-
-        /**
-         * {@inheritDoc}
-         */
-        @Override
-        public SSHUserPrivateKey snapshot(SSHUserPrivateKey credentials) {
-            if (credentials instanceof BasicSSHUserPrivateKey) {
-                final PrivateKeySource keySource = ((BasicSSHUserPrivateKey) credentials).getPrivateKeySource();
-                if (keySource.isSnapshotSource()) {
-                    return credentials;
-                }
-            }
-            final Secret passphrase = credentials.getPassphrase();
-            return new BasicSSHUserPrivateKey(credentials.getScope(), credentials.getId(), credentials.getUsername(),
-                    new DirectEntryPrivateKeySource(credentials.getPrivateKeys()),
-                    passphrase == null ? null : passphrase.getEncryptedValue(), credentials.getDescription());
+    static {
+        try {
+            // the critical field allow the permission check to make the XML read to fail completely in case of violation
+            // TODO: Remove reflection once baseline is updated past 2.85.
+            Method m = XStream2.class.getMethod("addCriticalField", Class.class, String.class);
+            m.invoke(Items.XSTREAM2, BasicSSHUserPrivateKey.class, "privateKeySource");
+        } catch (IllegalAccessException e) {
+            throw new ExceptionInInitializerError(e);
+        } catch (InvocationTargetException e) {
+            throw new ExceptionInInitializerError(e);
+        } catch (NoSuchMethodException e) {
+            throw new ExceptionInInitializerError(e);
         }
     }
 }
diff --git a/src/main/resources/com/cloudbees/jenkins/plugins/sshcredentials/impl/Messages.properties b/src/main/resources/com/cloudbees/jenkins/plugins/sshcredentials/impl/Messages.properties
@@ -22,6 +22,4 @@
 # THE SOFTWARE.
 #
 BasicSSHUserPrivateKey.DirectEntryPrivateKeySourceDisplayName=Enter directly
-BasicSSHUserPrivateKey.FileOnMasterPrivateKeySourceDisplayName=From a file on Jenkins master
-BasicSSHUserPrivateKey.UsersPrivateKeySourceDisplayName=From the Jenkins master ~/.ssh
-BasicSSHUserPrivateKey.DisplayName=SSH Username with private key
+BasicSSHUserPrivateKey.DisplayName=SSH Username with private key
diff --git a/src/main/resources/com/cloudbees/jenkins/plugins/sshcredentials/impl/Messages_de.properties b/src/main/resources/com/cloudbees/jenkins/plugins/sshcredentials/impl/Messages_de.properties
@@ -22,6 +22,4 @@
 # THE SOFTWARE.
 #
 BasicSSHUserPrivateKey.DirectEntryPrivateKeySourceDisplayName=Direkt eingeben
-BasicSSHUserPrivateKey.FileOnMasterPrivateKeySourceDisplayName=Aus einer Datei auf dem Jenkins Master
-BasicSSHUserPrivateKey.UsersPrivateKeySourceDisplayName=Aus ~/.ssh des Jenkins Masters
-BasicSSHUserPrivateKey.DisplayName=SSH Benutzername und privater Schl\u00fcssel
+BasicSSHUserPrivateKey.DisplayName=SSH Benutzername und privater Schl\u00fcssel
diff --git a/src/main/resources/com/cloudbees/jenkins/plugins/sshcredentials/impl/Messages_ja.properties b/src/main/resources/com/cloudbees/jenkins/plugins/sshcredentials/impl/Messages_ja.properties
@@ -22,6 +22,4 @@
 # THE SOFTWARE.
 #
 BasicSSHUserPrivateKey.DirectEntryPrivateKeySourceDisplayName=\u76f4\u63a5\u5165\u529b
-BasicSSHUserPrivateKey.FileOnMasterPrivateKeySourceDisplayName=Jenkins\u30de\u30b9\u30bf\u30fc\u4e0a\u306e\u30d5\u30a1\u30a4\u30eb\u304b\u3089
-BasicSSHUserPrivateKey.UsersPrivateKeySourceDisplayName=Jenkins\u30de\u30b9\u30bf\u30fc\u4e0a\u306e~/.ssh\u304b\u3089
-BasicSSHUserPrivateKey.DisplayName=SSH \u30e6\u30fc\u30b6\u30fc\u540d\u3068\u79d8\u5bc6\u9375
+BasicSSHUserPrivateKey.DisplayName=SSH \u30e6\u30fc\u30b6\u30fc\u540d\u3068\u79d8\u5bc6\u9375
diff --git a/src/test/java/com/cloudbees/jenkins/plugins/sshcredentials/impl/BasicSSHUserPrivateKeyTest.java b/src/test/java/com/cloudbees/jenkins/plugins/sshcredentials/impl/BasicSSHUserPrivateKeyTest.java
@@ -51,24 +51,6 @@ public class BasicSSHUserPrivateKeyTest {
 
     @Rule public JenkinsRule r = new JenkinsRule();
 
-    @Test public void masterKeysOnSlave() throws Exception {
-        FilePath keyfile = r.jenkins.getRootPath().child("key");
-        keyfile.write("stuff", null);
-        SSHUserPrivateKey key = new BasicSSHUserPrivateKey(CredentialsScope.SYSTEM, "mycreds", "git", new BasicSSHUserPrivateKey.FileOnMasterPrivateKeySource(keyfile.getRemote()), null, null);
-        assertEquals("[stuff]", key.getPrivateKeys().toString());
-        // TODO would be more interesting to use a Docker fixture to demonstrate that the file load is happening only from the master side
-        assertEquals("[stuff]", r.createOnlineSlave().getChannel().call(new LoadPrivateKeys(key)));
-    }
-    private static class LoadPrivateKeys extends MasterToSlaveCallable<String,Exception> {
-        private final SSHUserPrivateKey key;
-        LoadPrivateKeys(SSHUserPrivateKey key) {
-            this.key = key;
-        }
-        @Override public String call() throws Exception {
-            return key.getPrivateKeys().toString();
-        }
-    }
-
     @LocalData
     @Test
     public void readOldCredentials() throws Exception {
diff --git a/src/test/java/com/cloudbees/jenkins/plugins/sshcredentials/impl/BasicSSHUserPrivateKeyTest_SEC440.java b/src/test/java/com/cloudbees/jenkins/plugins/sshcredentials/impl/BasicSSHUserPrivateKeyTest_SEC440.java
@@ -0,0 +1,63 @@
+package com.cloudbees.jenkins.plugins.sshcredentials.impl;
+
+import com.cloudbees.hudson.plugins.folder.Folder;
+import hudson.FilePath;
+import hudson.cli.CLICommandInvoker;
+import hudson.cli.UpdateJobCommand;
+import hudson.model.Job;
+import jenkins.model.Jenkins;
+import org.junit.Rule;
+import org.junit.Test;
+import org.jvnet.hudson.test.Issue;
+import org.jvnet.hudson.test.JenkinsRule;
+import org.jvnet.hudson.test.recipes.LocalData;
+
+import static hudson.cli.CLICommandInvoker.Matcher.failedWith;
+import static hudson.cli.CLICommandInvoker.Matcher.succeeded;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.not;
+
+//TODO merge it into BasicSSHUserPrivateKeyTest after security patch
+public class BasicSSHUserPrivateKeyTest_SEC440 {
+    
+    @Rule
+    public JenkinsRule r = new JenkinsRule();
+    
+    {r.timeout = 0;}
+    
+    @Test
+    @Issue("SECURITY-440")
+    @LocalData("updateJob")
+    public void userWithoutRunScripts_cannotMigrateDangerousPrivateKeySource() throws Exception {
+        Folder folder = r.jenkins.createProject(Folder.class, "folder1");
+        
+        FilePath updateFolder = r.jenkins.getRootPath().child("update_folder.xml");
+        
+        { // as user with just configure, you cannot migrate
+            CLICommandInvoker.Result result = new CLICommandInvoker(r, new UpdateJobCommand())
+                    .authorizedTo(Jenkins.READ, Job.READ, Job.CONFIGURE)
+                    .withStdin(updateFolder.read())
+                    .invokeWithArgs("folder1");
+            
+            assertThat(result.stderr(), containsString("user is missing the Overall/RunScripts permission"));
+            assertThat(result, failedWith(-1));
+            
+            // config file not touched
+            String configFileContent = folder.getConfigFile().asString();
+            assertThat(configFileContent, not(containsString("FileOnMasterPrivateKeySource")));
+            assertThat(configFileContent, not(containsString("BasicSSHUserPrivateKey")));
+        }
+        { // but as admin with RUN_SCRIPTS, you can
+            CLICommandInvoker.Result result = new CLICommandInvoker(r, new UpdateJobCommand())
+                    .authorizedTo(Jenkins.ADMINISTER)
+                    .withStdin(updateFolder.read())
+                    .invokeWithArgs("folder1");
+            
+            assertThat(result, succeeded());
+            String configFileContent = folder.getConfigFile().asString();
+            assertThat(configFileContent, containsString("BasicSSHUserPrivateKey"));
+            assertThat(configFileContent, not(containsString("FileOnMasterPrivateKeySource")));
+        }
+    }
+}
diff --git a/src/test/resources/com/cloudbees/jenkins/plugins/sshcredentials/impl/BasicSSHUserPrivateKeyTest_SEC440/updateJob/update_folder.xml b/src/test/resources/com/cloudbees/jenkins/plugins/sshcredentials/impl/BasicSSHUserPrivateKeyTest_SEC440/updateJob/update_folder.xml
@@ -0,0 +1,41 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<com.cloudbees.hudson.plugins.folder.Folder plugin="cloudbees-folder@6.4">
+  <actions/>
+  <description></description>
+  <properties>
+    <com.cloudbees.hudson.plugins.folder.properties.FolderCredentialsProvider_-FolderCredentialsProperty>
+      <domainCredentialsMap class="hudson.util.CopyOnWriteMap$Hash">
+        <entry>
+          <com.cloudbees.plugins.credentials.domains.Domain plugin="credentials@2.1.16">
+            <specifications/>
+          </com.cloudbees.plugins.credentials.domains.Domain>
+          <java.util.concurrent.CopyOnWriteArrayList>
+            <com.cloudbees.jenkins.plugins.sshcredentials.impl.BasicSSHUserPrivateKey plugin="ssh-credentials@1.13">
+              <id>From SSH</id>
+              <username>from_ssh</username>
+              <privateKeySource class="com.cloudbees.jenkins.plugins.sshcredentials.impl.BasicSSHUserPrivateKey$UsersPrivateKeySource"/>
+            </com.cloudbees.jenkins.plugins.sshcredentials.impl.BasicSSHUserPrivateKey>
+          </java.util.concurrent.CopyOnWriteArrayList>
+        </entry>
+      </domainCredentialsMap>
+    </com.cloudbees.hudson.plugins.folder.properties.FolderCredentialsProvider_-FolderCredentialsProperty>
+  </properties>
+  <folderViews class="com.cloudbees.hudson.plugins.folder.views.DefaultFolderViewHolder">
+    <views>
+      <hudson.model.AllView>
+        <owner class="com.cloudbees.hudson.plugins.folder.Folder" reference="../../../.."/>
+        <name>All</name>
+        <filterExecutors>false</filterExecutors>
+        <filterQueue>false</filterQueue>
+        <properties class="hudson.model.View$PropertyList"/>
+      </hudson.model.AllView>
+    </views>
+    <tabBar class="hudson.views.DefaultViewsTabBar"/>
+  </folderViews>
+  <healthMetrics>
+    <com.cloudbees.hudson.plugins.folder.health.WorstChildHealthMetric>
+      <nonRecursive>false</nonRecursive>
+    </com.cloudbees.hudson.plugins.folder.health.WorstChildHealthMetric>
+  </healthMetrics>
+  <icon class="com.cloudbees.hudson.plugins.folder.icons.StockFolderIcon"/>
+</com.cloudbees.hudson.plugins.folder.Folder>

Original file line number	Diff line number	Diff line change
`@@ -22,6 +22,4 @@`
`22`	`22`	`# THE SOFTWARE.`
`23`	`23`	`#`
`24`	`24`	`BasicSSHUserPrivateKey.DirectEntryPrivateKeySourceDisplayName=Enter directly`
`25`		`-BasicSSHUserPrivateKey.FileOnMasterPrivateKeySourceDisplayName=From a file on Jenkins master`
`26`		`-BasicSSHUserPrivateKey.UsersPrivateKeySourceDisplayName=From the Jenkins master ~/.ssh`
`27`		`-BasicSSHUserPrivateKey.DisplayName=SSH Username with private key`
	`25`	`+BasicSSHUserPrivateKey.DisplayName=SSH Username with private key`