From 8b566aea470820016899deb6b7666192e4d08f3d Mon Sep 17 00:00:00 2001 From: Tim Vernum Date: Wed, 22 Apr 2020 16:38:51 +1000 Subject: [PATCH] Fix use of password protected PKCS#8 keys for SSL (#55567) PEMUtils would incorrectly fill the encryption password with zeros (the '\0' character) after decrypting a PKCS#8 key. Since PEMUtils did not take ownership of this password it should not zero it out because it does not know whether the caller will use that password array again. This is actually what PEMKeyConfig does - it uses the key encryption password as the password for the ephemeral keystore that it creates in order to build a KeyManager. Backport of: #55457 --- .../elasticsearch/common/ssl/PemUtils.java | 3 +- .../common/ssl/PemKeyConfigTests.java | 20 +++++- .../src/test/resources/certs/README.txt | 4 ++ .../resources/certs/cert1/cert1-pkcs8.key | 28 +++++++++ .../resources/certs/cert2/cert2-pkcs8.key | 29 +++++++++ .../xpack/core/ssl/PemUtils.java | 1 - .../xpack/core/ssl/PEMKeyConfigTests.java | 63 +++++++++++++++++++ 7 files changed, 143 insertions(+), 5 deletions(-) create mode 100644 libs/ssl-config/src/test/resources/certs/cert1/cert1-pkcs8.key create mode 100644 libs/ssl-config/src/test/resources/certs/cert2/cert2-pkcs8.key create mode 100644 x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ssl/PEMKeyConfigTests.java diff --git a/libs/ssl-config/src/main/java/org/elasticsearch/common/ssl/PemUtils.java b/libs/ssl-config/src/main/java/org/elasticsearch/common/ssl/PemUtils.java index cf46efae22901..17046b89c376c 100644 --- a/libs/ssl-config/src/main/java/org/elasticsearch/common/ssl/PemUtils.java +++ b/libs/ssl-config/src/main/java/org/elasticsearch/common/ssl/PemUtils.java @@ -350,7 +350,6 @@ private static PrivateKey parsePKCS8Encrypted(BufferedReader bReader, char[] key EncryptedPrivateKeyInfo encryptedPrivateKeyInfo = new EncryptedPrivateKeyInfo(keyBytes); SecretKeyFactory secretKeyFactory = SecretKeyFactory.getInstance(encryptedPrivateKeyInfo.getAlgName()); SecretKey secretKey = secretKeyFactory.generateSecret(new PBEKeySpec(keyPassword)); - Arrays.fill(keyPassword, '\u0000'); Cipher cipher = Cipher.getInstance(encryptedPrivateKeyInfo.getAlgName()); cipher.init(Cipher.DECRYPT_MODE, secretKey, encryptedPrivateKeyInfo.getAlgParameters()); PKCS8EncodedKeySpec keySpec = encryptedPrivateKeyInfo.getKeySpec(cipher); @@ -639,7 +638,7 @@ private static String getEcCurveNameFromOid(String oidString) throws GeneralSecu case "1.3.132.0.39": return "sect571r1"; } - throw new GeneralSecurityException("Error parsing EC named curve identifier. Named curve with OID: " + oidString + throw new GeneralSecurityException("Error parsing EC named curve identifier. Named curve with OID: " + oidString + " is not supported"); } diff --git a/libs/ssl-config/src/test/java/org/elasticsearch/common/ssl/PemKeyConfigTests.java b/libs/ssl-config/src/test/java/org/elasticsearch/common/ssl/PemKeyConfigTests.java index 8a5bb469e3c2c..4d44c6723c9cb 100644 --- a/libs/ssl-config/src/test/java/org/elasticsearch/common/ssl/PemKeyConfigTests.java +++ b/libs/ssl-config/src/test/java/org/elasticsearch/common/ssl/PemKeyConfigTests.java @@ -45,7 +45,7 @@ public class PemKeyConfigTests extends ESTestCase { private static final int IP_NAME = 7; private static final int DNS_NAME = 2; - public void testBuildKeyConfigFromPemFilesWithoutPassword() throws Exception { + public void testBuildKeyConfigFromPkcs1PemFilesWithoutPassword() throws Exception { final Path cert = getDataPath("/certs/cert1/cert1.crt"); final Path key = getDataPath("/certs/cert1/cert1.key"); final PemKeyConfig keyConfig = new PemKeyConfig(cert, key, new char[0]); @@ -53,7 +53,7 @@ public void testBuildKeyConfigFromPemFilesWithoutPassword() throws Exception { assertCertificateAndKey(keyConfig, "CN=cert1"); } - public void testBuildKeyConfigFromPemFilesWithPassword() throws Exception { + public void testBuildKeyConfigFromPkcs1PemFilesWithPassword() throws Exception { final Path cert = getDataPath("/certs/cert2/cert2.crt"); final Path key = getDataPath("/certs/cert2/cert2.key"); final PemKeyConfig keyConfig = new PemKeyConfig(cert, key, "c2-pass".toCharArray()); @@ -61,6 +61,22 @@ public void testBuildKeyConfigFromPemFilesWithPassword() throws Exception { assertCertificateAndKey(keyConfig, "CN=cert2"); } + public void testBuildKeyConfigFromPkcs8PemFilesWithoutPassword() throws Exception { + final Path cert = getDataPath("/certs/cert1/cert1.crt"); + final Path key = getDataPath("/certs/cert1/cert1-pkcs8.key"); + final PemKeyConfig keyConfig = new PemKeyConfig(cert, key, new char[0]); + assertThat(keyConfig.getDependentFiles(), Matchers.containsInAnyOrder(cert, key)); + assertCertificateAndKey(keyConfig, "CN=cert1"); + } + + public void testBuildKeyConfigFromPkcs8PemFilesWithPassword() throws Exception { + final Path cert = getDataPath("/certs/cert2/cert2.crt"); + final Path key = getDataPath("/certs/cert2/cert2-pkcs8.key"); + final PemKeyConfig keyConfig = new PemKeyConfig(cert, key, "c2-pass".toCharArray()); + assertThat(keyConfig.getDependentFiles(), Matchers.containsInAnyOrder(cert, key)); + assertCertificateAndKey(keyConfig, "CN=cert2"); + } + public void testKeyManagerFailsWithIncorrectPassword() throws Exception { final Path cert = getDataPath("/certs/cert2/cert2.crt"); final Path key = getDataPath("/certs/cert2/cert2.key"); diff --git a/libs/ssl-config/src/test/resources/certs/README.txt b/libs/ssl-config/src/test/resources/certs/README.txt index ac4b5603c069e..5f40a858a96e4 100644 --- a/libs/ssl-config/src/test/resources/certs/README.txt +++ b/libs/ssl-config/src/test/resources/certs/README.txt @@ -79,3 +79,7 @@ elasticsearch-certutil ca --pem --out ${PWD}/ca1-b.zip --days 9999 --ca-dn "CN=T unzip ca1-b.zip mv ca ca1-b +# 12. Convert certifcate keys to pkcs8 + +openssl pkcs8 -topk8 -inform PEM -in cert1/cert1.key -outform PEM -out cert1/cert1-pkcs8.key -nocrypt +openssl pkcs8 -topk8 -inform PEM -in cert2/cert2.key -outform PEM -out cert2/cert2-pkcs8.key -passin pass:"c2-pass" -passout pass:"c2-pass" diff --git a/libs/ssl-config/src/test/resources/certs/cert1/cert1-pkcs8.key b/libs/ssl-config/src/test/resources/certs/cert1/cert1-pkcs8.key new file mode 100644 index 0000000000000..1e8f219eecee7 --- /dev/null +++ b/libs/ssl-config/src/test/resources/certs/cert1/cert1-pkcs8.key @@ -0,0 +1,28 @@ +-----BEGIN PRIVATE KEY----- +MIIEvAIBADANBgkqhkiG9w0BAQEFAASCBKYwggSiAgEAAoIBAQCWz6ITDTlkTLue +B30Jx0+7sWHdlM5ObZjWhMQ1eyJD0gYU/gkH2C88IN/PtSv04tzFS6PA4KPDLIya +AhczPlGElSansiui//CpieCI4tt5c2BgVo3XdJaylYoW3CRILUrlSBOMUmJCQEok +verxMrz8DeppNxRfj99pQkoxUkmFMZj/C7XNVYrTttdF1li5FUtWJxw234OUfum3 +PQIzz6YTmoPtLrJ2fB8I4CH8R5hwGcryhBSAqq8pgy61aTPCgEBZ1c4Dvl65X8dG +2QEVPjwMZnnbGjvlZefOgkmAWJ1VjihA3GVgO2mx4tB4D2x5K/OAxh2foZkDVhqJ +fBkOblLnAgMBAAECggEAEKKUkR9rTjn8lADldPesPtrhHaz1WMdUDY2ViwSrEeoP +y6791gStqSdDKMkmMRv5GDYwuOzOg4/dbnt+jaN5IHPHUMYhdBhhNoJD5zWG2g20 ++stxV+u/V7GRCtZ7lg6Q7VuW9Gp99irbQtREHxjmqbLrQXHW6HeZQCYUwv39qBhU +mjjILGc0OsyD1SMXhuf/f245/oLuYRUlFOXsRPnBxt2XXQQ5ZbabrSEk5AtI5j03 +V5p8UQ75XZdvzQ8La3cyq3n5PufQIoLH+n3gXtADP6Gx+SxjehNRRfAvQqglMt0m +uMKgbiZYn9F7eoQomqYx9PhekkqsXsd+BwpELIfTqQKBgQD/lNVArgbUNzgudIKY +Jv4QJC+YZEG0roCWHdPFvgjAYcMYx8Lxbg6C3TcDxkT3amMhMd3X5wvKQoGyZpWY +S2LIPtgvkbea6ZHVh0wUQFJl3E5N4P3n5otjFOh8+wMhQg9IV5xFbCuLtPJhy9Qb +INyEvS8Nzoan3n9kUWC3bq7ECwKBgQCXDt6DYxwfCz1xIB+i5Qtspt/qNfPLbW3o +J6okYgP+SN1TCzremtxC+YW0Udau/eso7T2GBvcp3eBTPy320APcXvXxtJnDMsBL +A7m6M8dlBVFOi262AIZ4o+BerPJVZ6jjmOLgN9CXneMbRoDUPOhlIDf+jBy5foJU +Y8vNCeF6FQKBgAd71S66qcqG/2ck1DoeUiwo0xf0P5RJ08wRfYT5xonTkwHjv4qQ +PW6JibXblWNlQxfSvPs4cbjvb5rItDKsam0QogXqj2TC2BlXh9vD8mW3KLfREb47 +mvNAxnn6Y6ISrB3jKtlBjJjfqIVCkahlsu9UFs+hr4G02ygV1e4pGIb3AoGAGMmF +1cVzndx4TpHY3x/6ie+wGnyT7rOcL1Yi4yl6QkWum6viExkSP6M2P2qWccyUw/h5 ++f42nJYd80sQvclQeN7UOL9L4+32A9kuptFMTNVcjCjxF8hqSG2Lqb1zXnROEFrM +D8LY5agw1g7xoOIFuGJbDdfr9rw9op9ll9WhPCkCgYBIJPHnm5OGX2s35cMoVc3n +TQriv5D04E4SR8O4NQRGkGiFR+jLx07l6JOYaKrHk/SGjEQLvS+fDl8nPhmlInst +ykJ+rZN7OSck/cxkrBN0Ez+k4NI8xTlgJbhP2hwRC6Lk8o5MuVWjVAqsbzDp/9gx +9m4Zr3M/8uZMtkRp2j2e6Q== +-----END PRIVATE KEY----- diff --git a/libs/ssl-config/src/test/resources/certs/cert2/cert2-pkcs8.key b/libs/ssl-config/src/test/resources/certs/cert2/cert2-pkcs8.key new file mode 100644 index 0000000000000..5463978f1c885 --- /dev/null +++ b/libs/ssl-config/src/test/resources/certs/cert2/cert2-pkcs8.key @@ -0,0 +1,29 @@ +-----BEGIN ENCRYPTED PRIVATE KEY----- +MIIE6TAbBgkqhkiG9w0BBQMwDgQI3LMAczRe1pICAggABIIEyIfF+k/Z7mEPN+gu +rrV4slxmFjnIzyjoOsWbFBN9j5W8kZGewzWvjnYAzVMVeMUXXW27EMOC818aF8Ry +20BUVkmGNAUFVEslayeF0/S2sYR9xBPqYGLF0DI2VDTBIn8zrTdvQ0DYuuLf4g3W +nY69paCGSVjP24xrnEck+X6PjwIQ89MsJlVLn2chPEetvgu2jB+OdRneSA+lqBSW +2sDuKeXAmYdm3nCVz9do6T22OC9PUkzICGUvMyhYO4CD9dI3FFHa4ncKsr4+htfp +p3eppDczSneDKTPmGJg41UrEVdfENvqYLbYrX2ZZRnWUeuLsGbZUsWyfMT5TT2yz +sZXSS9+WRF8IDDhzKjsoSpsuNwMj8KMmc4oIrBNPysI+Bv0sQzPBlMJkHJ+Swb+W +I2wWC7NS7cwGFEEzwXKYAYI/34e/fAa+oOHd/aNdpGDhhnRyG+Eut3GbojlODiXN +ntLhDIZ+lQclJEkmxP2QfhIvjqkNBkPv+8Xt2Ami3ueF2iNj9PxQHUdx3aht1VMg +uVNLc0qLl98fDYx7+U2q5Y4L1mViZrCwGW+lcaY6a8hscPuasqW9aolFTE95YLVU +yFeUOZlUh1g5FYepISESR5jm5k9wf/WV2cp/KyAzCftdx2iRQtgvyQtSITFtthDa +kR7jI/T/HE2aPqiEcd0Sx+66aSH2JspseJ4VsLGkpg2lU0FPtPaAsl8lIhlfdoDA +43kOPKe5q2YT4QaUNZJROAuyJAlRbE8+LNlfAlZCm0UTQhgiXPzkEuVQFM4DNplq +FkBZnW4R/hD2rP81oy/SISxIANTyAB0FfCRxrvSRP6xe0XMYTIqsUVt9gpszI6AY +B+KTKlJR68cI8Gs1Jq6otn1ExAlCX45mwfaFO6Be89U0YNCZXdJ0X66SFNPmsMrg +eNbRLGziUhyoVUuPEhLXrxtuaGUO0LdHDSCQQGVNKCwNKbcWOE1clYWcJ1Lj0a7T +BTb4y1Nqt7LOHm5En5RtSaourgEKvi6PS2EcemBSUG4xSSP1pEoV/nQnvl2PFsG7 +182571HDc7FRZplzE4fhRKetS5WUuKax2FIqljQXS0CJZwV65boJwfJ2FosxdHj4 +eZK/wYHxi8R8LQB478bLqp3xZt28/EhsMjtLy/nO9SZfSy7Ajgq2mk2w4HZke23K +qv6JY8/ofu5nBp0QSJIPQj35arkxXnEYjzgqJD1IIDNckt3jRSpeX3OMfmVWDXus +a/wEwNaBACo5lgUYAwzXiPFyLEa6RkbD04z55GqsIjwZhIKrC1glyb0gmtxyY/N8 +QfPaSc4aXgUQK2mrgsR22eKf735MhM0pHOykErJzrIV92lRfSuQvfIoqTeFV0fWY +Yxns7HEIBln7/9udgZpf+EWnzM3kpPjRX1iYNtAsMAKEMrK/lnt3jZTkKY2Drem5 +NGjtEWNmzKOLH/S0TLcpHldoDJGdno7CytNGasrkFTaDLEhF68QI85zbMmyotKiZ +FKooG78oyxWCYqqPJ9vVBGwsMNfDMJP56T9UNhL72FwNqzSCaEcAUlDm2zCiQWKd +NpOfYzRx8uLp7UyzXbl959GjHWfTA74Yidug13eSHRgKwLXLuro05awG8yNXeDeL +wDswgDyKmlV7JDJjQw== +-----END ENCRYPTED PRIVATE KEY----- diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ssl/PemUtils.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ssl/PemUtils.java index 841f27d3cc3f9..b6cb24e63801c 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ssl/PemUtils.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ssl/PemUtils.java @@ -329,7 +329,6 @@ private static PrivateKey parsePKCS8Encrypted(BufferedReader bReader, char[] key EncryptedPrivateKeyInfo encryptedPrivateKeyInfo = new EncryptedPrivateKeyInfo(keyBytes); SecretKeyFactory secretKeyFactory = SecretKeyFactory.getInstance(encryptedPrivateKeyInfo.getAlgName()); SecretKey secretKey = secretKeyFactory.generateSecret(new PBEKeySpec(keyPassword)); - Arrays.fill(keyPassword, '\u0000'); Cipher cipher = Cipher.getInstance(encryptedPrivateKeyInfo.getAlgName()); cipher.init(Cipher.DECRYPT_MODE, secretKey, encryptedPrivateKeyInfo.getAlgParameters()); PKCS8EncodedKeySpec keySpec = encryptedPrivateKeyInfo.getKeySpec(cipher); diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ssl/PEMKeyConfigTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ssl/PEMKeyConfigTests.java new file mode 100644 index 0000000000000..4577241c0267c --- /dev/null +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ssl/PEMKeyConfigTests.java @@ -0,0 +1,63 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +package org.elasticsearch.xpack.core.ssl; + +import org.elasticsearch.common.settings.SecureString; +import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.env.Environment; +import org.elasticsearch.env.TestEnvironment; +import org.elasticsearch.test.ESTestCase; +import org.hamcrest.Matchers; + +import javax.net.ssl.X509ExtendedKeyManager; + +import static org.hamcrest.Matchers.notNullValue; + +public class PEMKeyConfigTests extends ESTestCase { + + public static final SecureString NO_PASSWORD = new SecureString("".toCharArray()); + public static final SecureString TESTNODE_PASSWORD = new SecureString("testnode".toCharArray()); + + public void testEncryptedPkcs8RsaKey() throws Exception { + verifyKeyConfig("testnode.crt", "key_pkcs8_encrypted.pem", TESTNODE_PASSWORD); + } + + public void testUnencryptedPkcs8RsaKey() throws Exception { + verifyKeyConfig("testnode.crt", "rsa_key_pkcs8_plain.pem", NO_PASSWORD); + } + + public void testUnencryptedPkcs8DsaKey() throws Exception { + verifyKeyConfig("testnode.crt", "dsa_key_pkcs8_plain.pem", NO_PASSWORD); + } + + public void testUnencryptedPkcs8EcKey() throws Exception { + verifyKeyConfig("testnode.crt", "ec_key_pkcs8_plain.pem", NO_PASSWORD); + } + + public void testEncryptedPkcs1RsaKey() throws Exception { + verifyKeyConfig("testnode.crt", "testnode-aes" + randomFrom(128, 192, 256) + ".pem", TESTNODE_PASSWORD); + } + + public void testUnencryptedPkcs1RsaKey() throws Exception { + verifyKeyConfig("testnode.crt", "testnode-unprotected.pem", NO_PASSWORD); + } + + private void verifyKeyConfig(String certName, String keyName, SecureString keyPassword) throws Exception { + final Environment env = TestEnvironment.newEnvironment(buildEnvSettings(Settings.EMPTY)); + PEMKeyConfig config = new PEMKeyConfig(getPath(keyName), keyPassword, getPath(certName)); + assertThat(config.certificates(env), Matchers.iterableWithSize(1)); + X509ExtendedKeyManager keyManager = config.createKeyManager(env); + assertThat(keyManager, notNullValue()); + assertThat(keyManager.getPrivateKey("key"), notNullValue()); + } + + private String getPath(String fileName) { + return getDataPath("/org/elasticsearch/xpack/security/transport/ssl/certs/simple/" + fileName) + .toAbsolutePath().toString(); + } + +}