Skip to content

Commit

Permalink
[SECURITY-1322]
Browse files Browse the repository at this point in the history
Co-Authored-By: Wadeck Follonier <wadeck.follonier@gmail.com>
  • Loading branch information
jeffret-b and Wadeck committed May 15, 2019
1 parent 8715181 commit 40d0b5c
Show file tree
Hide file tree
Showing 8 changed files with 243 additions and 151 deletions.
6 changes: 6 additions & 0 deletions pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,12 @@
<version>2.2.0</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>cloudbees-folder</artifactId>
<version>6.7</version>
<scope>test</scope>
</dependency>
</dependencies>

<build>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,7 @@
*/
package com.cloudbees.plugins.credentials.impl;

import com.cloudbees.plugins.credentials.CredentialsProvider;
import com.cloudbees.plugins.credentials.CredentialsScope;
import com.cloudbees.plugins.credentials.CredentialsSnapshotTaker;
import com.cloudbees.plugins.credentials.SecretBytes;
import com.cloudbees.plugins.credentials.common.StandardCertificateCredentials;
import edu.umd.cs.findbugs.annotations.CheckForNull;
Expand All @@ -35,19 +33,20 @@
import hudson.Util;
import hudson.model.AbstractDescribableImpl;
import hudson.model.Descriptor;
import hudson.remoting.Channel;
import hudson.model.Items;
import hudson.util.FormValidation;
import hudson.util.HttpResponses;
import hudson.util.IOUtils;
import hudson.util.Secret;
import java.io.ByteArrayInputStream;
import java.io.ByteArrayOutputStream;
import java.io.File;
import java.io.FileInputStream;
import java.io.IOException;
import java.io.InputStream;
import java.io.ObjectStreamException;
import java.io.Serializable;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
import java.security.KeyStore;
import java.security.KeyStoreException;
import java.security.NoSuchAlgorithmException;
Expand All @@ -59,10 +58,12 @@
import java.util.logging.LogRecord;
import java.util.logging.Logger;
import javax.servlet.ServletException;

import hudson.util.XStream2;
import jenkins.model.Jenkins;
import net.jcip.annotations.GuardedBy;
import org.apache.commons.codec.binary.Base64;
import org.apache.commons.fileupload.FileItem;
import org.apache.commons.io.FileUtils;
import org.apache.commons.lang.StringUtils;
import org.kohsuke.accmod.Restricted;
import org.kohsuke.accmod.restrictions.NoExternalUse;
Expand Down Expand Up @@ -138,19 +139,6 @@ private static char[] toCharArray(@NonNull Secret password) {
return plainText == null ? null : plainText.toCharArray();
}

/**
* When serializing over a {@link Channel} ensure that we send a self-contained version.
*
* @return the object instance to write to the stream.
*/
private Object writeReplace() {
if (/* XStream */Channel.current() == null || /* already safe to serialize */ keyStoreSource
.isSnapshotSource()) {
return this;
}
return CredentialsProvider.snapshot(this);
}

/**
* Returns the {@link KeyStore} containing the certificate.
*
Expand Down Expand Up @@ -268,7 +256,9 @@ public static abstract class KeyStoreSource extends AbstractDescribableImpl<KeyS
*
* @return {@code true} if and only if the source is self contained.
* @since 1.14
* @deprecated No longer need to distinguish snapshot sources.
*/
@Deprecated
public boolean isSnapshotSource() {
return false;
}
Expand Down Expand Up @@ -364,7 +354,9 @@ protected static FormValidation validateCertificateKeystore(String type, byte[]

/**
* Let the user reference a file on the disk.
* @deprecated This approach has security vulnerabilities and should be migrated to {@link UploadedKeyStoreSource}
*/
@Deprecated
public static class FileOnMasterKeyStoreSource extends KeyStoreSource {

/**
Expand All @@ -377,13 +369,6 @@ public static class FileOnMasterKeyStoreSource extends KeyStoreSource {
*/
private final String keyStoreFile;

/**
* Our constructor.
*
* @param keyStoreFile the path of the file on the master.
*/
@SuppressWarnings("unused") // by stapler
@DataBoundConstructor
public FileOnMasterKeyStoreSource(String keyStoreFile) {
this.keyStoreFile = keyStoreFile;
}
Expand All @@ -407,15 +392,6 @@ public byte[] getKeyStoreBytes() {
}
}

/**
* Returns the private key file name.
*
* @return the private key file name.
*/
public String getKeyStoreFile() {
return keyStoreFile;
}

/**
* {@inheritDoc}
*/
Expand All @@ -434,63 +410,28 @@ public String toString() {
"}";
}

/**
* {@inheritDoc}
*/
@Extension
public static class DescriptorImpl extends KeyStoreSourceDescriptor {

/**
* {@inheritDoc}
*/
@Override
public String getDisplayName() {
return Messages.CertificateCredentialsImpl_FileOnMasterKeyStoreSourceDisplayName();
private Object readResolve() {
if (!Jenkins.getActiveInstance().hasPermission(Jenkins.RUN_SCRIPTS)) {
LOGGER.warning("SECURITY-1322: Permission failure migrating FileOnMasterKeyStoreSource to UploadedKeyStoreSource for a Certificate. An administrator may need to perform the migration.");
Jenkins.getActiveInstance().checkPermission(Jenkins.RUN_SCRIPTS);
}

/**
* Checks the keystore file path.
*
* @param value the file path.
* @param password the password.
* @return the {@link FormValidation} results.
*/
@SuppressWarnings("unused") // stapler form validation
@Restricted(NoExternalUse.class)
public FormValidation doCheckKeyStoreFile(@QueryParameter String value,
@QueryParameter String password) {
if (StringUtils.isBlank(value)) {
return FormValidation.error(Messages.CertificateCredentialsImpl_KeyStoreFileUnspecified());
}
File file = new File(value);
if (file.isFile()) {
try {
return validateCertificateKeystore("PKCS12", FileUtils.readFileToByteArray(file), password);
} catch (IOException e) {
return FormValidation
.error(Messages.CertificateCredentialsImpl_KeyStoreFileUnreadable(value), e);
}
} else {
return FormValidation.error(Messages.CertificateCredentialsImpl_KeyStoreFileDoesNotExist(value));
}
}
LOGGER.log(Level.INFO, "SECURITY-1322: Migrating FileOnMasterKeyStoreSource to UploadedKeyStoreSource. The containing item may need to be saved to complete the migration.");
SecretBytes secretBytes = SecretBytes.fromBytes(getKeyStoreBytes());
return new UploadedKeyStoreSource(secretBytes);
}

}

/**
* Let the user reference a file on the disk.
* Let the user reference an uploaded file.
*/
public static class UploadedKeyStoreSource extends KeyStoreSource implements Serializable {
/**
* Ensure consistent serialization.
*/
private static final long serialVersionUID = 1L;

/**
* Our logger.
*/
private static final Logger LOGGER = Logger.getLogger(FileOnMasterKeyStoreSource.class.getName());

/**
* The old uploaded keystore.
*/
Expand Down Expand Up @@ -750,55 +691,21 @@ public HttpResponse doUpload(@NonNull StaplerRequest req) throws ServletExceptio
}
}

/**
* The {@link CredentialsSnapshotTaker} for {@link StandardCertificateCredentials}.
*
* @since 1.14
*/
@Extension
public static class CredentialsSnapshotTakerImpl extends CredentialsSnapshotTaker<StandardCertificateCredentials> {

/**
* {@inheritDoc}
*/
@Override
public Class<StandardCertificateCredentials> type() {
return StandardCertificateCredentials.class;
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, CertificateCredentialsImpl.class, "keyStoreSource");
}

/**
* {@inheritDoc}
*/
@Override
public StandardCertificateCredentials snapshot(StandardCertificateCredentials credentials) {
if (credentials instanceof CertificateCredentialsImpl) {
final KeyStoreSource keyStoreSource = ((CertificateCredentialsImpl) credentials).getKeyStoreSource();
if (keyStoreSource.isSnapshotSource()) {
return credentials;
}
return new CertificateCredentialsImpl(credentials.getScope(), credentials.getId(),
credentials.getDescription(), credentials.getPassword().getEncryptedValue(),
new UploadedKeyStoreSource(SecretBytes.fromBytes(keyStoreSource.getKeyStoreBytes())));
}
ByteArrayOutputStream bos = new ByteArrayOutputStream();
final char[] password = credentials.getPassword().getPlainText().toCharArray();
try {
credentials.getKeyStore().store(bos, password);
bos.close();
} catch (KeyStoreException e) {
return credentials;
} catch (IOException e) {
return credentials;
} catch (NoSuchAlgorithmException e) {
return credentials;
} catch (CertificateException e) {
return credentials;
} finally {
Arrays.fill(password, (char) 0);
}
return new CertificateCredentialsImpl(credentials.getScope(), credentials.getId(),
credentials.getDescription(), credentials.getPassword().getEncryptedValue(),
new UploadedKeyStoreSource(SecretBytes.fromBytes(bos.toByteArray())));
catch (IllegalAccessException e) {
throw new ExceptionInInitializerError(e);
}
catch (InvocationTargetException e) {
throw new ExceptionInInitializerError(e);
}
catch (NoSuchMethodException e) {
throw new ExceptionInInitializerError(e);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,6 @@
UsernamePasswordCredentialsImpl.DisplayName=Username with password
CertificateCredentialsImpl.DisplayName=Certificate
CertificateCredentialsImpl.EmptyKeystore=Empty keystore
CertificateCredentialsImpl.FileOnMasterKeyStoreSourceDisplayName=From a PKCS#12 file on Jenkins master
CertificateCredentialsImpl.KeyStoreFileDoesNotExist=The file "{0}" does not exist
CertificateCredentialsImpl.KeyStoreFileUnreadable=Could not read file "{0}"
CertificateCredentialsImpl.KeyStoreFileUnspecified=You must specify the file path
CertificateCredentialsImpl.LoadKeyFailed=Could retrieve key "{0}"
CertificateCredentialsImpl.LoadKeyFailedQueryEmptyPassword=Could retrieve key "{0}". You may need to provide a password
CertificateCredentialsImpl.LoadKeystoreFailed=Could not load keystore
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,6 @@
UsernamePasswordCredentialsImpl.DisplayName=Benutzername und Passwort
CertificateCredentialsImpl.DisplayName=Zertifikat
CertificateCredentialsImpl.EmptyKeystore=leerer Zertifikatsspeicher ("keystore")
CertificateCredentialsImpl.FileOnMasterKeyStoreSourceDisplayName=Aus einer PKCS#12-Datei auf dem Jenkins Master
CertificateCredentialsImpl.KeyStoreFileDoesNotExist=Die Datei "{0}" existiert nicht
CertificateCredentialsImpl.KeyStoreFileUnreadable=Konnte Datei "{0}" nicht lesen
CertificateCredentialsImpl.KeyStoreFileUnspecified=Sie m\u00FCssen einen Dateipfad angeben
CertificateCredentialsImpl.LoadKeyFailed=Konnte den Schl\u00FCssel "{0}" nicht abrufen
CertificateCredentialsImpl.LoadKeyFailedQueryEmptyPassword=Konnte den Schl\u00FCssel "{0}" nicht abrufen. M\u00F6glicherweise m\u00FCssen Sie ein Passwort eingeben
CertificateCredentialsImpl.LoadKeystoreFailed=Konnte den Zertifikatsspeicher ("keystore") nicht laden
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,6 @@
UsernamePasswordCredentialsImpl.DisplayName=Nom d''utilisateur et mot de passe
CertificateCredentialsImpl.DisplayName=Certificat
CertificateCredentialsImpl.EmptyKeystore=Magasin de cl\u00e9 vide
CertificateCredentialsImpl.FileOnMasterKeyStoreSourceDisplayName=Depuis un fichier PKCS#12 sur la machine ma\u00eetre de Jenkins
CertificateCredentialsImpl.KeyStoreFileDoesNotExist=Le fichier "{0}" n''existe pas
CertificateCredentialsImpl.KeyStoreFileUnreadable=Impossible de lire le fichier "{0}"
CertificateCredentialsImpl.KeyStoreFileUnspecified=Vous devez sp\u00e9cifier un chemin du fichier
CertificateCredentialsImpl.LoadKeyFailed=Impossible de r\u00e9cup\u00e9rer la cl\u00e9 "{0}"
CertificateCredentialsImpl.LoadKeyFailedQueryEmptyPassword=Impossible de r\u00e9cup\u00e9rer la cl\u00e9 "{0}". Vous pouvez avoir besoin de fournir un mot de passe.
CertificateCredentialsImpl.LoadKeystoreFailed=Impossible de charger le magasin de cl\u00e9
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,6 @@
UsernamePasswordCredentialsImpl.DisplayName=\u30e6\u30fc\u30b6\u30fc\u540d\u3068\u30d1\u30b9\u30ef\u30fc\u30c9
CertificateCredentialsImpl.DisplayName=\u8a3c\u660e\u66f8
CertificateCredentialsImpl.EmptyKeystore=\u7a7a\u306e\u30ad\u30fc\u30b9\u30c8\u30a2
CertificateCredentialsImpl.FileOnMasterKeyStoreSourceDisplayName=\
Jenkins\u30de\u30b9\u30bf\u30fc\u4e0a\u306ePKCS#12\u8a3c\u660e\u66f8\u30d5\u30a1\u30a4\u30eb
CertificateCredentialsImpl.KeyStoreFileDoesNotExist=\u30d5\u30a1\u30a4\u30eb "{0}" \u306f\u5b58\u5728\u3057\u307e\u305b\u3093\u3002
CertificateCredentialsImpl.KeyStoreFileUnreadable=\u30d5\u30a1\u30a4\u30eb "{0}" \u3092\u8aad\u3081\u307e\u305b\u3093\u3067\u3057\u305f\u3002
CertificateCredentialsImpl.KeyStoreFileUnspecified=\u30d5\u30a1\u30a4\u30eb\u306e\u30d1\u30b9\u3092\u6307\u5b9a\u3057\u3066\u304f\u3060\u3055\u3044\u3002
CertificateCredentialsImpl.LoadKeyFailed=\u30ad\u30fc"{0}"\u3092\u53d6\u5f97\u3067\u304d\u307e\u3057\u305f\u3002
CertificateCredentialsImpl.LoadKeyFailedQueryEmptyPassword=\u30ad\u30fc"{0}"\u3092\u53d6\u5f97\u3067\u304d\u307e\u3057\u305f\u304c\u3001\u30d1\u30b9\u30ef\u30fc\u30c9\u304c\u5fc5\u8981\u3067\u3059\u3002
CertificateCredentialsImpl.LoadKeystoreFailed=\u30ad\u30fc\u30b9\u30c8\u30a2\u3092\u30ed\u30fc\u30c9\u3067\u304d\u307e\u305b\u3093\u3002
Expand Down
Loading

0 comments on commit 40d0b5c

Please sign in to comment.