diff --git a/pom.xml b/pom.xml index 996f4cd..9dcc395 100644 --- a/pom.xml +++ b/pom.xml @@ -66,8 +66,8 @@ - 1.609 - 6 + 1.625 + 7 @@ -95,10 +95,16 @@ org.jenkins-ci.plugins credentials - 2.1.0 + 2.1.17 + + org.jenkins-ci.plugins + cloudbees-folder + 6.4 + test + 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 index 7a278f1..cc7bb8d 100644 --- 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,13 +26,13 @@ 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; @@ -40,6 +40,8 @@ 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; @@ -47,6 +49,8 @@ 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 files() { List files = new ArrayList(); 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 { - - /** - * {@inheritDoc} - */ - @Override - public Class 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 index c7d9186..25f0fec 100644 --- 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 \ No newline at end of file +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 index 23a52ff..46881d7 100644 --- 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 \ No newline at end of file +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 index 33123c1..fc9721c 100644 --- 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 \ No newline at end of file +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 index 8a0007b..aac473a 100644 --- 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 { - 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 new file mode 100644 index 0000000..e12e723 --- /dev/null +++ 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 new file mode 100644 index 0000000..b30e0a1 --- /dev/null +++ b/src/test/resources/com/cloudbees/jenkins/plugins/sshcredentials/impl/BasicSSHUserPrivateKeyTest_SEC440/updateJob/update_folder.xml @@ -0,0 +1,41 @@ + + + + + + + + + + + + + + From SSH + from_ssh + + + + + + + + + + + + All + false + false + + + + + + + + false + + + + \ No newline at end of file