From e6aa166246d1734f4798a9e31f78842f4c85c28b Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Wed, 18 Jan 2017 13:21:54 -0500 Subject: [PATCH] Merge pull request #105 from jenkinsci-cert/SECURITY-304-t3 [SECURITY-304] Encrypt new secrets with CBC and random IV instead of ECB --- core/src/main/java/hudson/model/Item.java | 2 +- .../java/hudson/util/HistoricalSecrets.java | 84 ++++++++++ core/src/main/java/hudson/util/Secret.java | 147 +++++++++++------- .../main/java/hudson/util/SecretRewriter.java | 4 +- .../security/CryptoConfidentialKey.java | 88 ++++++++++- .../hudson/util/SecretRewriterTest.groovy | 33 ++-- .../test/groovy/hudson/util/SecretTest.groovy | 27 +++- .../java/hudson/util/SecretCompatTest.java | 121 ++++++++++++++ .../security/RekeySecretAdminMonitorTest.java | 10 +- test/src/test/java/lib/form/PasswordTest.java | 27 +++- .../canReadPreSec304Secrets/config.xml | 34 ++++ .../jobs/OldSecret/config.xml | 27 ++++ .../secrets/hudson.util.Secret | Bin 0 -> 272 bytes .../secrets/master.key | 1 + 14 files changed, 517 insertions(+), 88 deletions(-) create mode 100644 core/src/main/java/hudson/util/HistoricalSecrets.java create mode 100644 test/src/test/java/hudson/util/SecretCompatTest.java create mode 100644 test/src/test/resources/hudson/util/SecretCompatTest/canReadPreSec304Secrets/config.xml create mode 100644 test/src/test/resources/hudson/util/SecretCompatTest/canReadPreSec304Secrets/jobs/OldSecret/config.xml create mode 100644 test/src/test/resources/hudson/util/SecretCompatTest/canReadPreSec304Secrets/secrets/hudson.util.Secret create mode 100644 test/src/test/resources/hudson/util/SecretCompatTest/canReadPreSec304Secrets/secrets/master.key diff --git a/core/src/main/java/hudson/model/Item.java b/core/src/main/java/hudson/model/Item.java index 834cf97b4858..7c2aeba5e29a 100644 --- a/core/src/main/java/hudson/model/Item.java +++ b/core/src/main/java/hudson/model/Item.java @@ -229,7 +229,7 @@ public interface Item extends PersistenceRoot, SearchableModelObject, AccessCont Permission DISCOVER = new Permission(PERMISSIONS, "Discover", Messages._AbstractProject_DiscoverPermission_Description(), READ, PermissionScope.ITEM); /** * Ability to view configuration details. - * If the user lacks {@link CONFIGURE} then any {@link Secret}s must be masked out, even in encrypted form. + * If the user lacks {@link #CONFIGURE} then any {@link Secret}s must be masked out, even in encrypted form. * @see Secret#ENCRYPTED_VALUE_PATTERN */ Permission EXTENDED_READ = new Permission(PERMISSIONS,"ExtendedRead", Messages._AbstractProject_ExtendedReadPermission_Description(), CONFIGURE, Boolean.getBoolean("hudson.security.ExtendedReadPermission"), new PermissionScope[]{PermissionScope.ITEM}); diff --git a/core/src/main/java/hudson/util/HistoricalSecrets.java b/core/src/main/java/hudson/util/HistoricalSecrets.java new file mode 100644 index 000000000000..37a6fa39e170 --- /dev/null +++ b/core/src/main/java/hudson/util/HistoricalSecrets.java @@ -0,0 +1,84 @@ +/* + * The MIT License + * + * Copyright (c) 2004-2010, Sun Microsystems, Inc., Kohsuke Kawaguchi + * Copyright (c) 2016, CloudBees Inc. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ +package hudson.util; + +import com.trilead.ssh2.crypto.Base64; +import hudson.Util; +import jenkins.model.Jenkins; +import jenkins.security.CryptoConfidentialKey; +import org.kohsuke.accmod.Restricted; +import org.kohsuke.accmod.restrictions.NoExternalUse; + +import javax.crypto.Cipher; +import javax.crypto.SecretKey; +import java.io.IOException; +import java.security.GeneralSecurityException; + +import static java.nio.charset.StandardCharsets.UTF_8; + +/** + * Historical algorithms for decrypting {@link Secret}s. + */ +@Restricted(NoExternalUse.class) +public class HistoricalSecrets { + + /*package*/ static Secret decrypt(String data, CryptoConfidentialKey key) throws IOException, GeneralSecurityException { + byte[] in = Base64.decode(data.toCharArray()); + Secret s = tryDecrypt(key.decrypt(), in); + if (s!=null) return s; + + // try our historical key for backward compatibility + Cipher cipher = Secret.getCipher("AES"); + cipher.init(Cipher.DECRYPT_MODE, getLegacyKey()); + return tryDecrypt(cipher, in); + } + + /*package*/ static Secret tryDecrypt(Cipher cipher, byte[] in) { + try { + String plainText = new String(cipher.doFinal(in), UTF_8); + if(plainText.endsWith(MAGIC)) + return new Secret(plainText.substring(0,plainText.length()-MAGIC.length())); + return null; + } catch (GeneralSecurityException e) { + return null; // if the key doesn't match with the bytes, it can result in BadPaddingException + } + } + + /** + * Turns {@link Jenkins#getSecretKey()} into an AES key. + * + * @deprecated + * This is no longer the key we use to encrypt new information, but we still need this + * to be able to decrypt what's already persisted. + */ + @Deprecated + /*package*/ static SecretKey getLegacyKey() throws GeneralSecurityException { + String secret = Secret.SECRET; + if(secret==null) return Jenkins.getInstance().getSecretKeyAsAES128(); + return Util.toAes128Key(secret); + } + + private static final String MAGIC = "::::MAGIC::::"; +} diff --git a/core/src/main/java/hudson/util/Secret.java b/core/src/main/java/hudson/util/Secret.java index 9a4dcb750a05..0ea02d7d4718 100644 --- a/core/src/main/java/hudson/util/Secret.java +++ b/core/src/main/java/hudson/util/Secret.java @@ -2,6 +2,7 @@ * The MIT License * * Copyright (c) 2004-2010, Sun Microsystems, Inc., Kohsuke Kawaguchi + * Copyright (c) 2016, CloudBees Inc. * * Permission is hereby granted, free of charge, to any person obtaining a copy * of this software and associated documentation files (the "Software"), to deal @@ -29,12 +30,12 @@ import com.thoughtworks.xstream.io.HierarchicalStreamReader; import com.thoughtworks.xstream.io.HierarchicalStreamWriter; import com.trilead.ssh2.crypto.Base64; +import java.util.Arrays; import jenkins.model.Jenkins; import hudson.Util; import jenkins.security.CryptoConfidentialKey; import org.kohsuke.stapler.Stapler; -import javax.crypto.SecretKey; import javax.crypto.Cipher; import java.io.Serializable; import java.io.UnsupportedEncodingException; @@ -44,6 +45,8 @@ import org.kohsuke.accmod.Restricted; import org.kohsuke.accmod.restrictions.NoExternalUse; +import static java.nio.charset.StandardCharsets.UTF_8; + /** * Glorified {@link String} that uses encryption in the persisted form, to avoid accidental exposure of a secret. * @@ -58,13 +61,20 @@ * @author Kohsuke Kawaguchi */ public final class Secret implements Serializable { + private static final byte PAYLOAD_V1 = 1; /** * Unencrypted secret text. */ private final String value; + private byte[] iv; + + /*package*/ Secret(String value) { + this.value = value; + } - private Secret(String value) { + /*package*/ Secret(String value, byte[] iv) { this.value = value; + this.iv = iv; } /** @@ -100,20 +110,6 @@ public int hashCode() { return value.hashCode(); } - /** - * Turns {@link Jenkins#getSecretKey()} into an AES key. - * - * @deprecated - * This is no longer the key we use to encrypt new information, but we still need this - * to be able to decrypt what's already persisted. - */ - @Deprecated - /*package*/ static SecretKey getLegacyKey() throws GeneralSecurityException { - String secret = SECRET; - if(secret==null) return Jenkins.getInstance().getSecretKeyAsAES128(); - return Util.toAes128Key(secret); - } - /** * Encrypts {@link #value} and returns it in an encoded printable form. * @@ -121,56 +117,95 @@ public int hashCode() { */ public String getEncryptedValue() { try { - Cipher cipher = KEY.encrypt(); - // add the magic suffix which works like a check sum. - return new String(Base64.encode(cipher.doFinal((value+MAGIC).getBytes("UTF-8")))); + synchronized (this) { + if (iv == null) { //if we were created from plain text or other reason without iv + iv = KEY.newIv(); + } + } + Cipher cipher = KEY.encrypt(iv); + byte[] encrypted = cipher.doFinal(this.value.getBytes(UTF_8)); + byte[] payload = new byte[1 + 8 + iv.length + encrypted.length]; + int pos = 0; + // For PAYLOAD_V1 we use this byte shifting model, V2 probably will need DataOutput + payload[pos++] = PAYLOAD_V1; + payload[pos++] = (byte)(iv.length >> 24); + payload[pos++] = (byte)(iv.length >> 16); + payload[pos++] = (byte)(iv.length >> 8); + payload[pos++] = (byte)(iv.length); + payload[pos++] = (byte)(encrypted.length >> 24); + payload[pos++] = (byte)(encrypted.length >> 16); + payload[pos++] = (byte)(encrypted.length >> 8); + payload[pos++] = (byte)(encrypted.length); + System.arraycopy(iv, 0, payload, pos, iv.length); + pos+=iv.length; + System.arraycopy(encrypted, 0, payload, pos, encrypted.length); + return "{"+new String(Base64.encode(payload))+"}"; } catch (GeneralSecurityException e) { throw new Error(e); // impossible - } catch (UnsupportedEncodingException e) { - throw new Error(e); // impossible } } /** - * Pattern matching a possible output of {@link #getEncryptedValue}. - * Basically, any Base64-encoded value. - * You must then call {@link #decrypt} to eliminate false positives. + * Pattern matching a possible output of {@link #getEncryptedValue} + * Basically, any Base64-encoded value optionally wrapped by {@code {}}. + * You must then call {@link #decrypt(String)} to eliminate false positives. + * @see #ENCRYPTED_VALUE_PATTERN */ @Restricted(NoExternalUse.class) - public static final Pattern ENCRYPTED_VALUE_PATTERN = Pattern.compile("[A-Za-z0-9+/]+={0,2}"); + public static final Pattern ENCRYPTED_VALUE_PATTERN = Pattern.compile("\\{?[A-Za-z0-9+/]+={0,2}}?"); /** * Reverse operation of {@link #getEncryptedValue()}. Returns null * if the given cipher text was invalid. */ public static Secret decrypt(String data) { - if(data==null) return null; - try { - byte[] in = Base64.decode(data.toCharArray()); - Secret s = tryDecrypt(KEY.decrypt(), in); - if (s!=null) return s; + if (data == null) return null; - // try our historical key for backward compatibility - Cipher cipher = getCipher("AES"); - cipher.init(Cipher.DECRYPT_MODE, getLegacyKey()); - return tryDecrypt(cipher, in); - } catch (GeneralSecurityException e) { - return null; - } catch (UnsupportedEncodingException e) { - throw new Error(e); // impossible - } catch (IOException e) { - return null; - } - } - - /*package*/ static Secret tryDecrypt(Cipher cipher, byte[] in) throws UnsupportedEncodingException { - try { - String plainText = new String(cipher.doFinal(in), "UTF-8"); - if(plainText.endsWith(MAGIC)) - return new Secret(plainText.substring(0,plainText.length()-MAGIC.length())); - return null; - } catch (GeneralSecurityException e) { - return null; // if the key doesn't match with the bytes, it can result in BadPaddingException + if (data.startsWith("{") && data.endsWith("}")) { //likely CBC encrypted/containing metadata but could be plain text + byte[] payload; + try { + payload = Base64.decode(data.substring(1, data.length()-1).toCharArray()); + } catch (IOException e) { + return null; + } + switch (payload[0]) { + case PAYLOAD_V1: + // For PAYLOAD_V1 we use this byte shifting model, V2 probably will need DataOutput + int ivLength = ((payload[1] & 0xff) << 24) + | ((payload[2] & 0xff) << 16) + | ((payload[3] & 0xff) << 8) + | (payload[4] & 0xff); + int dataLength = ((payload[5] & 0xff) << 24) + | ((payload[6] & 0xff) << 16) + | ((payload[7] & 0xff) << 8) + | (payload[8] & 0xff); + if (payload.length != 1 + 8 + ivLength + dataLength) { + // not valid v1 + return null; + } + byte[] iv = Arrays.copyOfRange(payload, 9, 9 + ivLength); + byte[] code = Arrays.copyOfRange(payload, 9+ivLength, payload.length); + String text; + try { + text = new String(KEY.decrypt(iv).doFinal(code), UTF_8); + } catch (GeneralSecurityException e) { + // it's v1 which cannot be historical, but not decrypting + return null; + } + return new Secret(text, iv); + default: + return null; + } + } else { + try { + return HistoricalSecrets.decrypt(data, KEY); + } catch (GeneralSecurityException e) { + return null; + } catch (UnsupportedEncodingException e) { + throw new Error(e); // impossible + } catch (IOException e) { + return null; + } } } @@ -228,8 +263,6 @@ public Object unmarshal(HierarchicalStreamReader reader, final UnmarshallingCont } } - private static final String MAGIC = "::::MAGIC::::"; - /** * Workaround for JENKINS-6459 / http://java.net/jira/browse/GLASSFISH-11862 * @see #getCipher(String) @@ -246,6 +279,14 @@ public Object unmarshal(HierarchicalStreamReader reader, final UnmarshallingCont */ private static final CryptoConfidentialKey KEY = new CryptoConfidentialKey(Secret.class.getName()); + /** + * Reset the internal secret key for testing. + */ + @Restricted(NoExternalUse.class) + /*package*/ static void resetKeyForTest() { + KEY.resetForTest(); + } + private static final long serialVersionUID = 1L; static { diff --git a/core/src/main/java/hudson/util/SecretRewriter.java b/core/src/main/java/hudson/util/SecretRewriter.java index 6350adf0f3a9..8b8a574e6ad6 100644 --- a/core/src/main/java/hudson/util/SecretRewriter.java +++ b/core/src/main/java/hudson/util/SecretRewriter.java @@ -40,7 +40,7 @@ public class SecretRewriter { public SecretRewriter() throws GeneralSecurityException { cipher = Secret.getCipher("AES"); - key = Secret.getLegacyKey(); + key = HistoricalSecrets.getLegacyKey(); } /** @deprecated SECURITY-376: {@code backupDirectory} is ignored */ @@ -62,7 +62,7 @@ private String tryRewrite(String s) throws IOException, InvalidKeyException { return s; // not a valid base64 } cipher.init(Cipher.DECRYPT_MODE, key); - Secret sec = Secret.tryDecrypt(cipher, in); + Secret sec = HistoricalSecrets.tryDecrypt(cipher, in); if(sec!=null) // matched return sec.getEncryptedValue(); // replace by the new encrypted value else // not encrypted with the legacy key. leave it unmodified diff --git a/core/src/main/java/jenkins/security/CryptoConfidentialKey.java b/core/src/main/java/jenkins/security/CryptoConfidentialKey.java index dd1dad9e37b9..f4027060131a 100644 --- a/core/src/main/java/jenkins/security/CryptoConfidentialKey.java +++ b/core/src/main/java/jenkins/security/CryptoConfidentialKey.java @@ -1,9 +1,15 @@ package jenkins.security; +import hudson.Main; import hudson.util.Secret; +import jenkins.model.Jenkins; +import org.kohsuke.accmod.Restricted; +import org.kohsuke.accmod.restrictions.DoNotUse; +import org.kohsuke.accmod.restrictions.NoExternalUse; import javax.crypto.Cipher; import javax.crypto.SecretKey; +import javax.crypto.spec.IvParameterSpec; import javax.crypto.spec.SecretKeySpec; import java.io.IOException; import java.security.GeneralSecurityException; @@ -15,6 +21,9 @@ * @since 1.498 */ public class CryptoConfidentialKey extends ConfidentialKey { + @Restricted(NoExternalUse.class) //TODO remove when in mainline + public static final int DEFAULT_IV_LENGTH = 16; + private volatile SecretKey secret; public CryptoConfidentialKey(String id) { super(id); @@ -35,7 +44,7 @@ private SecretKey getKey() { store(payload); } // Due to the stupid US export restriction JDK only ships 128bit version. - secret = new SecretKeySpec(payload,0,128/8, ALGORITHM); + secret = new SecretKeySpec(payload,0,128/8, KEY_ALGORITHM); } } } @@ -47,10 +56,12 @@ private SecretKey getKey() { /** * Returns a {@link Cipher} object for encrypting with this key. + * @deprecated use {@link #encrypt(byte[])} */ + @Deprecated public Cipher encrypt() { try { - Cipher cipher = Secret.getCipher(ALGORITHM); + Cipher cipher = Secret.getCipher(KEY_ALGORITHM); cipher.init(Cipher.ENCRYPT_MODE, getKey()); return cipher; } catch (GeneralSecurityException e) { @@ -58,12 +69,68 @@ public Cipher encrypt() { } } + /** + * Returns a {@link Cipher} object for encrypting with this key using the provided initialization vector. + * @param iv the initialization vector + * @return the cipher + */ + @Restricted(NoExternalUse.class) //TODO remove when in mainline + public Cipher encrypt(byte[] iv) { + try { + Cipher cipher = Secret.getCipher(ALGORITHM); + cipher.init(Cipher.ENCRYPT_MODE, getKey(), new IvParameterSpec(iv)); + return cipher; + } catch (GeneralSecurityException e) { + throw new AssertionError(e); + } + } + + /** + * Returns a {@link Cipher} object for decrypting with this key using the provided initialization vector. + * @param iv the initialization vector + * @return the cipher + */ + @Restricted(NoExternalUse.class) //TODO remove when in mainline + public Cipher decrypt(byte[] iv) { + try { + Cipher cipher = Secret.getCipher(ALGORITHM); + cipher.init(Cipher.DECRYPT_MODE, getKey(), new IvParameterSpec(iv)); + return cipher; + } catch (GeneralSecurityException e) { + throw new AssertionError(e); + } + } + + /** + * Generates a new Initialization Vector. + * @param length the length of the salt + * @return some random bytes + * @see #encrypt(byte[]) + */ + @Restricted(NoExternalUse.class) //TODO remove when in mainline + public byte[] newIv(int length) { + return ConfidentialStore.get().randomBytes(length); + } + + /** + * Generates a new Initialization Vector of default length. + * @return some random bytes + * @see #newIv(int) + * @see #encrypt(byte[]) + */ + @Restricted(NoExternalUse.class) //TODO remove when in mainline + public byte[] newIv() { + return newIv(DEFAULT_IV_LENGTH); + } + /** * Returns a {@link Cipher} object for decrypting with this key. + * @deprecated use {@link #decrypt(byte[])} */ + @Deprecated public Cipher decrypt() { try { - Cipher cipher = Secret.getCipher(ALGORITHM); + Cipher cipher = Secret.getCipher(KEY_ALGORITHM); cipher.init(Cipher.DECRYPT_MODE, getKey()); return cipher; } catch (GeneralSecurityException e) { @@ -72,5 +139,18 @@ public Cipher decrypt() { } - private static final String ALGORITHM = "AES"; + private static final String KEY_ALGORITHM = "AES"; + private static final String ALGORITHM = "AES/CBC/PKCS5Padding"; + + /** + * Reset the internal secret key for testing. + */ + @Restricted(NoExternalUse.class) + public void resetForTest() { + if (Main.isUnitTest) { + this.secret = null; + } else { + throw new IllegalStateException("Only for testing"); + } + } } diff --git a/core/src/test/groovy/hudson/util/SecretRewriterTest.groovy b/core/src/test/groovy/hudson/util/SecretRewriterTest.groovy index f198bdd8a395..b748703ac172 100644 --- a/core/src/test/groovy/hudson/util/SecretRewriterTest.groovy +++ b/core/src/test/groovy/hudson/util/SecretRewriterTest.groovy @@ -23,42 +23,49 @@ class SecretRewriterTest { @Rule public TemporaryFolder tmp = new TemporaryFolder() + def FOO_PATTERN = /\{[A-Za-z0-9+\/]+={0,2}}<\/foo>/ + def MSG_PATTERN = /\{[A-Za-z0-9+\/]+={0,2}}<\/msg>/ + def FOO_PATTERN2 = /(\{[A-Za-z0-9+\/]+={0,2}}<\/foo>){2}/ + def ABC_FOO_PATTERN = /\s\{[A-Za-z0-9+\/]+={0,2}}<\/foo>\s<\/abc>/ + @Test void singleFileRewrite() { def o = encryptOld('foobar') // old def n = encryptNew('foobar') // new roundtrip "${o}", - "${n}" + {assert it ==~ FOO_PATTERN} + roundtrip "${o}${o}", - "${n}${n}" + {assert it ==~ FOO_PATTERN2} roundtrip "${n}", - "${n}" + {assert it == "${n}"} roundtrip " thisIsLegalBase64AndLongEnoughThatItCouldLookLikeSecret ", - " thisIsLegalBase64AndLongEnoughThatItCouldLookLikeSecret " + {assert it == "thisIsLegalBase64AndLongEnoughThatItCouldLookLikeSecret"} // to be rewritten, it needs to be between a tag - roundtrip "$o", "$o" - roundtrip "$o", "$o" + roundtrip "$o", {assert it == "$o"} + roundtrip "$o", {assert it == "$o"} // - roundtrip "\n$o\n", "\n$n\n" + roundtrip "\n$o\n", {assert it ==~ ABC_FOO_PATTERN} } - void roundtrip(String before, String after) { + void roundtrip(String before, Closure check) { def sr = new SecretRewriter(null); def f = File.createTempFile("test", "xml", tmp.root) f.text = before sr.rewrite(f,null) - assert after.replaceAll(System.getProperty("line.separator"), "\n").trim()==f.text.replaceAll(System.getProperty("line.separator"), "\n").trim() + check(f.text.replaceAll(System.getProperty("line.separator"), "\n").trim()) + //assert after.replaceAll(System.getProperty("line.separator"), "\n").trim()==f.text.replaceAll(System.getProperty("line.separator"), "\n").trim() } String encryptOld(str) { def cipher = Secret.getCipher("AES"); - cipher.init(Cipher.ENCRYPT_MODE, Secret.legacyKey); - return new String(Base64.encode(cipher.doFinal((str + Secret.MAGIC).getBytes("UTF-8")))) + cipher.init(Cipher.ENCRYPT_MODE, HistoricalSecrets.legacyKey); + return new String(Base64.encode(cipher.doFinal((str + HistoricalSecrets.MAGIC).getBytes("UTF-8")))) } String encryptNew(str) { @@ -99,11 +106,11 @@ class SecretRewriterTest { assert 6==sw.rewriteRecursive(t, st) dirs.each { p-> - assert new File(t,"$p/foo.xml").text.trim()==answer + assert new File(t,"$p/foo.xml").text.trim() ==~ MSG_PATTERN } // t2 is only reachable by following a symlink. this should be covered, too - assert new File(t2,"foo.xml").text.trim()==answer.trim(); + assert new File(t2,"foo.xml").text.trim() ==~ MSG_PATTERN } } diff --git a/core/src/test/groovy/hudson/util/SecretTest.groovy b/core/src/test/groovy/hudson/util/SecretTest.groovy index f7a0ba3ad00a..8f39ae7ec026 100644 --- a/core/src/test/groovy/hudson/util/SecretTest.groovy +++ b/core/src/test/groovy/hudson/util/SecretTest.groovy @@ -31,7 +31,8 @@ import org.junit.Rule import org.junit.Test import java.util.Random; -import javax.crypto.Cipher; +import javax.crypto.Cipher +import java.util.regex.Pattern; /** * @author Kohsuke Kawaguchi @@ -43,6 +44,8 @@ public class SecretTest { @Rule public MockSecretRule mockSecretRule = new MockSecretRule() + static final Pattern ENCRYPTED_VALUE_PATTERN = Pattern.compile("\\{?[A-Za-z0-9+/]+={0,2}}?"); + @Test void testEncrypt() { def secret = Secret.fromString("abc"); @@ -54,6 +57,11 @@ public class SecretTest { // can we round trip? assert secret==Secret.fromString(secret.encryptedValue); + + //Two consecutive encryption requests of the same object should result in the same encrypted value - SECURITY-304 + assert secret.encryptedValue == secret.encryptedValue + //Two consecutive encryption requests of different objects with the same value should not result in the same encrypted value - SECURITY-304 + assert secret.encryptedValue != Secret.fromString(secret.plainText).encryptedValue } @Test @@ -62,9 +70,16 @@ public class SecretTest { String plaintext = RandomStringUtils.random(new Random().nextInt(i)); String ciphertext = Secret.fromString(plaintext).getEncryptedValue(); //println "${plaintext} → ${ciphertext}" - assert Secret.ENCRYPTED_VALUE_PATTERN.matcher(ciphertext).matches(); + assert ENCRYPTED_VALUE_PATTERN.matcher(ciphertext).matches(); } - assert !Secret.ENCRYPTED_VALUE_PATTERN.matcher("hello world").matches(); + //Not "plain" text + assert !ENCRYPTED_VALUE_PATTERN.matcher("hello world").matches(); + //Not "plain" text + assert !ENCRYPTED_VALUE_PATTERN.matcher("helloworld!").matches(); + //legacy key + assert ENCRYPTED_VALUE_PATTERN.matcher("abcdefghijklmnopqr0123456789").matches(); + //legacy key + assert ENCRYPTED_VALUE_PATTERN.matcher("abcdefghijklmnopqr012345678==").matches(); } @Test @@ -77,7 +92,7 @@ public class SecretTest { def s = Secret.fromString("Mr.Jenkins"); def xml = Jenkins.XSTREAM.toXML(s); assert !xml.contains(s.plainText) - assert xml.contains(s.encryptedValue) + assert xml ==~ /\{[A-Za-z0-9+\/]+={0,2}}<\/hudson\.util\.Secret>/ def o = Jenkins.XSTREAM.fromXML(xml); assert o==s : xml; @@ -104,11 +119,11 @@ public class SecretTest { */ @Test void migrationFromLegacyKeyToConfidentialStore() { - def legacy = Secret.legacyKey + def legacy = HistoricalSecrets.legacyKey ["Hello world","","\u0000unprintable"].each { str -> def cipher = Secret.getCipher("AES"); cipher.init(Cipher.ENCRYPT_MODE, legacy); - def old = new String(Base64.encode(cipher.doFinal((str + Secret.MAGIC).getBytes("UTF-8")))) + def old = new String(Base64.encode(cipher.doFinal((str + HistoricalSecrets.MAGIC).getBytes("UTF-8")))) def s = Secret.fromString(old) assert s.plainText==str : "secret by the old key should decrypt" assert s.encryptedValue!=old : "but when encrypting, ConfidentialKey should be in use" diff --git a/test/src/test/java/hudson/util/SecretCompatTest.java b/test/src/test/java/hudson/util/SecretCompatTest.java new file mode 100644 index 000000000000..8c440c4ff9c2 --- /dev/null +++ b/test/src/test/java/hudson/util/SecretCompatTest.java @@ -0,0 +1,121 @@ +/* + * The MIT License + * + * Copyright (c) 2004-2010, Sun Microsystems, Inc., Kohsuke Kawaguchi + * Copyright (c) 2016, CloudBees Inc. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ +package hudson.util; + +import hudson.model.FreeStyleProject; +import hudson.model.ParameterDefinition; +import hudson.model.ParametersDefinitionProperty; +import hudson.model.PasswordParameterDefinition; +import org.hamcrest.core.Is; +import org.junit.After; +import org.junit.Before; +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 java.io.IOException; +import java.util.regex.Pattern; + +import static org.hamcrest.core.Is.isA; +import static org.hamcrest.core.IsNot.not; +import static org.hamcrest.core.StringContains.containsString; +import static org.junit.Assert.*; + +/** + * Tests {@link Secret}. + */ +public class SecretCompatTest { + + @Rule + public JenkinsRule j = new JenkinsRule() { + @Override + public void before() throws Throwable { + Secret.resetKeyForTest(); //As early as possible + super.before(); + } + }; + + @After + public void after() { + Secret.resetKeyForTest(); + } + + + @Test + @Issue("SECURITY-304") + public void encryptedValueStaysTheSameAfterRoundtrip() throws Exception { + FreeStyleProject project = j.createFreeStyleProject(); + project.addProperty(new ParametersDefinitionProperty(new PasswordParameterDefinition("p", "s3cr37", "Keep this a secret"))); + project = j.configRoundtrip(project); + String round1 = project.getConfigFile().asString(); + project = j.configRoundtrip(project); + String round2 = project.getConfigFile().asString(); + assertEquals(round1, round2); + + + //But reconfiguring will make it a new value + project = j.jenkins.getItemByFullName(project.getFullName(), FreeStyleProject.class); + project.removeProperty(ParametersDefinitionProperty.class); + project.addProperty(new ParametersDefinitionProperty(new PasswordParameterDefinition("p", "s3cr37", "Keep this a secret"))); + project = j.configRoundtrip(project); + String round3 = project.getConfigFile().asString(); + assertNotEquals(round2, round3); + //Saving again will produce the same + project = j.configRoundtrip(project); + String round4 = project.getConfigFile().asString(); + assertEquals(round3, round4); + } + + @Test + @Issue("SECURITY-304") + @LocalData + public void canReadPreSec304Secrets() throws Exception { + FreeStyleProject project = j.jenkins.getItemByFullName("OldSecret", FreeStyleProject.class); + String oldxml = project.getConfigFile().asString(); + //It should be unchanged on disk + assertThat(oldxml, containsString("z/Dd3qrHdQ6/C5lR7uEafM/jD3nQDrGprw3XsfZ/0vo=")); + ParametersDefinitionProperty property = project.getProperty(ParametersDefinitionProperty.class); + ParameterDefinition definition = property.getParameterDefinitions().get(0); + assertTrue(definition instanceof PasswordParameterDefinition); + Secret secret = ((PasswordParameterDefinition) definition).getDefaultValueAsSecret(); + assertEquals("theSecret", secret.getPlainText()); + + //OK it was read correctly from disk, now the first roundtrip should update the encrypted value + + project = j.configRoundtrip(project); + String newXml = project.getConfigFile().asString(); + assertNotEquals(oldxml, newXml); //This could have changed because Jenkins has moved on, so not really a good check + assertThat(newXml, not(containsString("z/Dd3qrHdQ6/C5lR7uEafM/jD3nQDrGprw3XsfZ/0vo="))); + Pattern p = Pattern.compile("\\{[A-Za-z0-9+/]+={0,2}}"); + assertTrue(p.matcher(newXml).find()); + + //But the next roundtrip should result in the same data + project = j.configRoundtrip(project); + String round2 = project.getConfigFile().asString(); + assertEquals(newXml, round2); + } +} diff --git a/test/src/test/java/jenkins/security/RekeySecretAdminMonitorTest.java b/test/src/test/java/jenkins/security/RekeySecretAdminMonitorTest.java index ea7a08ba4587..413b4a6eed3d 100644 --- a/test/src/test/java/jenkins/security/RekeySecretAdminMonitorTest.java +++ b/test/src/test/java/jenkins/security/RekeySecretAdminMonitorTest.java @@ -11,6 +11,7 @@ import hudson.util.Secret; import hudson.util.SecretHelper; import org.apache.commons.io.FileUtils; +import org.hamcrest.CoreMatchers; import org.jvnet.hudson.test.HudsonTestCase; import org.jvnet.hudson.test.recipes.Recipe.Runner; import org.xml.sax.SAXException; @@ -20,6 +21,9 @@ import java.io.File; import java.io.IOException; import java.lang.annotation.Annotation; +import java.util.regex.Pattern; + +import static org.junit.Assert.assertThat; /** * @author Kohsuke Kawaguchi @@ -28,6 +32,8 @@ public class RekeySecretAdminMonitorTest extends HudsonTestCase { @Inject RekeySecretAdminMonitor monitor; + final String plain_regex_match = ".*\\{[A-Za-z0-9+/]+={0,2}}.*"; + @Override protected void setUp() throws Exception { SecretHelper.set(TEST_KEY); @@ -76,8 +82,8 @@ private void putSomeOldData(File dir) throws Exception { private void verifyRewrite(File dir) throws Exception { File xml = new File(dir, "foo.xml"); - assertEquals("" + encryptNew(TEST_KEY) + "".trim(), - FileUtils.readFileToString(xml).trim()); + Pattern pattern = Pattern.compile(""+plain_regex_match+""); + assertTrue(pattern.matcher(FileUtils.readFileToString(xml).trim()).matches()); } // TODO sometimes fails: "Invalid request submission: {json=[Ljava.lang.String;@2c46358e, .crumb=[Ljava.lang.String;@35661457}" diff --git a/test/src/test/java/lib/form/PasswordTest.java b/test/src/test/java/lib/form/PasswordTest.java index 595b0e7e8515..0a9f5eb14956 100644 --- a/test/src/test/java/lib/form/PasswordTest.java +++ b/test/src/test/java/lib/form/PasswordTest.java @@ -43,10 +43,13 @@ import java.util.Arrays; import java.util.Collections; import java.util.Locale; +import java.util.regex.Pattern; + import jenkins.model.Jenkins; import org.acegisecurity.Authentication; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.not; +import static org.hamcrest.core.Is.is; import static org.junit.Assert.assertThat; import org.jvnet.hudson.test.HudsonTestCase; import org.jvnet.hudson.test.Issue; @@ -78,10 +81,19 @@ public String getDisplayName() { } } - @Issue("SECURITY-266") + // TODO in trunk switch to @Issue({"SECURITY-266","SECURITY-304"}) + @Issue("SECURITY-266, SECURITY-304") public void testExposedCiphertext() throws Exception { boolean saveEnabled = Item.EXTENDED_READ.getEnabled(); try { + + //final String plain_regex_match = ".*\\{[A-Za-z0-9+/]+={0,2}}.*"; + final String xml_regex_match = "\\{[A-Za-z0-9+/]+={0,2}}"; + final Pattern xml_regex_pattern = Pattern.compile(xml_regex_match); + final String staticTest = "\n\nvalue=\"{AQAAABAAAAAgXhXgopokysZkduhl+v1gm0UhUBBbjKDVpKz7bGk3mIO53cNTRdlu7LC4jZYEc+vF}\"\n"; + //Just a quick verification on what could be on the page and that the regexp is correctly set up + assertThat(xml_regex_pattern.matcher(staticTest).find(), is(true)); + jenkins.setSecurityRealm(createDummySecurityRealm()); // TODO 1.645+ use MockAuthorizationStrategy GlobalMatrixAuthorizationStrategy pmas = new GlobalMatrixAuthorizationStrategy(); @@ -93,7 +105,7 @@ public void testExposedCiphertext() throws Exception { pmas.add(Item.CREATE, "dev"); // so we can show CopyJobCommand would barf; more realistic would be to grant it only in a subfolder jenkins.setAuthorizationStrategy(pmas); Secret s = Secret.fromString("s3cr3t"); - String sEnc = s.getEncryptedValue(); + //String sEnc = s.getEncryptedValue(); FreeStyleProject p = createFreeStyleProject("p"); p.setDisplayName("Unicode here ←"); p.setDescription("This+looks+like+Base64+but+is+not+a+secret"); @@ -102,14 +114,15 @@ public void testExposedCiphertext() throws Exception { // Control case: an administrator can read and write configuration freely. wc.login("admin"); HtmlPage configure = wc.getPage(p, "configure"); - assertThat(configure.getWebResponse().getContentAsString(), containsString(sEnc)); + assertThat(xml_regex_pattern.matcher(configure.getWebResponse().getContentAsString()).find(), is(true)); submit(configure.getFormByName("config")); VulnerableProperty vp = p.getProperty(VulnerableProperty.class); assertNotNull(vp); assertEquals(s, vp.secret); Page configXml = wc.goTo(p.getUrl() + "config.xml", "application/xml"); String xmlAdmin = configXml.getWebResponse().getContentAsString(); - assertThat(xmlAdmin, containsString("" + sEnc + "")); + + assertThat(Pattern.compile("" + xml_regex_match + "").matcher(xmlAdmin).find(), is(true)); assertThat(xmlAdmin, containsString("" + p.getDisplayName() + "")); assertThat(xmlAdmin, containsString("" + p.getDescription() + "")); // CLICommandInvoker does not work here, as it sets up its own SecurityRealm + AuthorizationStrategy. @@ -131,11 +144,11 @@ public void testExposedCiphertext() throws Exception { // Test case: another user with EXTENDED_READ but not CONFIGURE should not get access even to encrypted secrets. wc.login("dev"); configure = wc.getPage(p, "configure"); - assertThat(configure.getWebResponse().getContentAsString(), not(containsString(sEnc))); + assertThat(xml_regex_pattern.matcher(configure.getWebResponse().getContentAsString()).find(), is(false)); configXml = wc.goTo(p.getUrl() + "config.xml", "application/xml"); String xmlDev = configXml.getWebResponse().getContentAsString(); - assertThat(xmlDev, not(containsString(sEnc))); - assertEquals(xmlAdmin.replace(sEnc, "********"), xmlDev); + assertThat(xml_regex_pattern.matcher(xmlDev).find(), is(false)); + assertEquals(xmlAdmin.replaceAll(xml_regex_match, "********"), xmlDev); getJobCommand = new GetJobCommand(); Authentication devAuth = User.get("dev").impersonate(); getJobCommand.setTransportAuth(devAuth); diff --git a/test/src/test/resources/hudson/util/SecretCompatTest/canReadPreSec304Secrets/config.xml b/test/src/test/resources/hudson/util/SecretCompatTest/canReadPreSec304Secrets/config.xml new file mode 100644 index 000000000000..f47b1c32c9a2 --- /dev/null +++ b/test/src/test/resources/hudson/util/SecretCompatTest/canReadPreSec304Secrets/config.xml @@ -0,0 +1,34 @@ + + + + 1.625.4-SNAPSHOT (private-12/16/2016 18:04 GMT-rsandell) + 2 + NORMAL + true + + + false + + ${ITEM_ROOTDIR}/workspace + ${ITEM_ROOTDIR}/builds + + + + + 5 + 0 + + + + All + false + false + + + + All + 0 + + + + \ No newline at end of file diff --git a/test/src/test/resources/hudson/util/SecretCompatTest/canReadPreSec304Secrets/jobs/OldSecret/config.xml b/test/src/test/resources/hudson/util/SecretCompatTest/canReadPreSec304Secrets/jobs/OldSecret/config.xml new file mode 100644 index 000000000000..e094ad77fd42 --- /dev/null +++ b/test/src/test/resources/hudson/util/SecretCompatTest/canReadPreSec304Secrets/jobs/OldSecret/config.xml @@ -0,0 +1,27 @@ + + + + + false + + + + + alice + theSecret + z/Dd3qrHdQ6/C5lR7uEafM/jD3nQDrGprw3XsfZ/0vo= + + + + + + true + false + false + false + + false + + + + \ No newline at end of file diff --git a/test/src/test/resources/hudson/util/SecretCompatTest/canReadPreSec304Secrets/secrets/hudson.util.Secret b/test/src/test/resources/hudson/util/SecretCompatTest/canReadPreSec304Secrets/secrets/hudson.util.Secret new file mode 100644 index 0000000000000000000000000000000000000000..cdec3920fbc06ca5f07e25e884ea246c80f898d4 GIT binary patch literal 272 zcmV+r0q_2*6%{s0ht|6$Z8z))t|l)&1ny;IB`WdC+cNvem;y-ic$|YUZcP z7)L*-wH8Ml#ULpjze85UAdBs5{CyvOY1|MBD9#gu-M-3yCnC>9c}H&cV&KMWsX`xd zvgnC7{{@5o4f7ZkQ=z8rXE;Ztvx2o_XNBer_PlM$sL~1(njgRLp(=QJ+8C*6+wlE` zaK`~?T4b6>p-#u>XiKswSqwa literal 0 HcmV?d00001 diff --git a/test/src/test/resources/hudson/util/SecretCompatTest/canReadPreSec304Secrets/secrets/master.key b/test/src/test/resources/hudson/util/SecretCompatTest/canReadPreSec304Secrets/secrets/master.key new file mode 100644 index 000000000000..2572203a97bd --- /dev/null +++ b/test/src/test/resources/hudson/util/SecretCompatTest/canReadPreSec304Secrets/secrets/master.key @@ -0,0 +1 @@ +4311ab5e95e3da7b9b1360b52cac6f0f666db7b48f9f701296cc07c8f00612b451f1874e584d49560810619e8a6ff6b19f8f58ae1305c515fc62a7b60ea3a69e6058cad16b2c8df317952b749fdaaecab013431da55bb4ea4b8eee754fa043261b51a99a2b537fd57f867cdcb1e209f3bba735a8672dbfc3f10b0e2209a81683 \ No newline at end of file