From 2ea6d2addddd7b795081316d53f5b1dec5cde1ef Mon Sep 17 00:00:00 2001 From: Ioannis Kakavas Date: Fri, 2 Feb 2018 15:17:23 +0200 Subject: [PATCH 01/10] Changes the encryption/decryption method Calls explicitly `doFinal()` on the byte array of plaintext and ciphertext, instead of making use of Cipher{Input,Output}Stream. While functionally equivelant, CipherInputStream merhod does not work well with BouncyCastle (FIPS) Security Provider's implementation of AES GCM as the byte array that is read from the `CipherInputStream` backed `DataInputStream` is always some bytes short (17 vs 20 for the case of `keystore.seed` value). --- .../common/settings/KeyStoreWrapper.java | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java b/server/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java index 9b994089be0c2..c4965c11d6fcf 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java +++ b/server/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java @@ -20,8 +20,6 @@ package org.elasticsearch.common.settings; import javax.crypto.Cipher; -import javax.crypto.CipherInputStream; -import javax.crypto.CipherOutputStream; import javax.crypto.SecretKey; import javax.crypto.SecretKeyFactory; import javax.crypto.spec.GCMParameterSpec; @@ -333,10 +331,9 @@ public void decrypt(char[] password) throws GeneralSecurityException, IOExceptio } Cipher cipher = createCipher(Cipher.DECRYPT_MODE, password, salt, iv); - try (ByteArrayInputStream bytesStream = new ByteArrayInputStream(encryptedBytes); - CipherInputStream cipherStream = new CipherInputStream(bytesStream, cipher); - DataInputStream input = new DataInputStream(cipherStream)) { - + byte decryptedBytes[] = cipher.doFinal(encryptedBytes); + try (ByteArrayInputStream bytesStream = new ByteArrayInputStream(decryptedBytes); + DataInputStream input = new DataInputStream(bytesStream)) { entries.set(new HashMap<>()); int numEntries = input.readInt(); while (numEntries-- > 0) { @@ -356,11 +353,9 @@ public void decrypt(char[] password) throws GeneralSecurityException, IOExceptio private byte[] encrypt(char[] password, byte[] salt, byte[] iv) throws GeneralSecurityException, IOException { assert isLoaded(); - ByteArrayOutputStream bytes = new ByteArrayOutputStream(); Cipher cipher = createCipher(Cipher.ENCRYPT_MODE, password, salt, iv); - try (CipherOutputStream cipherStream = new CipherOutputStream(bytes, cipher); - DataOutputStream output = new DataOutputStream(cipherStream)) { - + ByteArrayOutputStream bytes = new ByteArrayOutputStream(); + try (DataOutputStream output = new DataOutputStream(bytes)) { output.writeInt(entries.get().size()); for (Map.Entry mapEntry : entries.get().entrySet()) { output.writeUTF(mapEntry.getKey()); @@ -370,8 +365,7 @@ private byte[] encrypt(char[] password, byte[] salt, byte[] iv) throws GeneralSe output.write(entry.bytes); } } - - return bytes.toByteArray(); + return cipher.doFinal(bytes.toByteArray()); } private void decryptLegacyEntries() throws GeneralSecurityException, IOException { From 8241318dade664282472973964b81378bb4d9254 Mon Sep 17 00:00:00 2001 From: Ioannis Kakavas Date: Fri, 2 Feb 2018 20:08:24 +0200 Subject: [PATCH 02/10] Zerofill sensitive data --- .../elasticsearch/common/settings/KeyStoreWrapper.java | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java b/server/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java index c4965c11d6fcf..c3c248d85884b 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java +++ b/server/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java @@ -346,6 +346,8 @@ public void decrypt(char[] password) throws GeneralSecurityException, IOExceptio } entries.get().put(setting, new Entry(entryType, entryBytes)); } + } finally { + Arrays.fill(decryptedBytes, (byte) 0); } } @@ -364,8 +366,12 @@ private byte[] encrypt(char[] password, byte[] salt, byte[] iv) throws GeneralSe output.writeInt(entry.bytes.length); output.write(entry.bytes); } + return cipher.doFinal(bytes.toByteArray()); + } finally { + final int bufferSize = bytes.size(); + bytes.reset(); + bytes.write(new byte[bufferSize]); } - return cipher.doFinal(bytes.toByteArray()); } private void decryptLegacyEntries() throws GeneralSecurityException, IOException { From 63a4fcd47c9b32a6382bc1b7e4f4913a83868f99 Mon Sep 17 00:00:00 2001 From: Ioannis Kakavas Date: Thu, 8 Feb 2018 11:20:45 +0200 Subject: [PATCH 03/10] Changed how data is read from the stream After receiving feedback from the discussion in the upstream [1], reverted back to using CipherInputStream. Instead of using `read()` and checking that the bytes read are what we expect, use `readFully()` which will read exactly the number of bytes while keep reading until the end of the stream or throw an `EOFException` if not all bytes can be read. This approach keeps the simplicity of using CipherInputStream while working as expected with both Security Providers [1] https://www.bouncycastle.org/devmailarchive/msg15559.html --- .../common/settings/KeyStoreWrapper.java | 50 +++++++------------ 1 file changed, 17 insertions(+), 33 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java b/server/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java index c3c248d85884b..e8dc61fa4df48 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java +++ b/server/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java @@ -19,18 +19,11 @@ package org.elasticsearch.common.settings; -import javax.crypto.Cipher; -import javax.crypto.SecretKey; -import javax.crypto.SecretKeyFactory; +import javax.crypto.*; import javax.crypto.spec.GCMParameterSpec; import javax.crypto.spec.PBEKeySpec; import javax.crypto.spec.SecretKeySpec; -import java.io.ByteArrayInputStream; -import java.io.ByteArrayOutputStream; -import java.io.DataInputStream; -import java.io.DataOutputStream; -import java.io.IOException; -import java.io.InputStream; +import java.io.*; import java.nio.ByteBuffer; import java.nio.CharBuffer; import java.nio.charset.StandardCharsets; @@ -315,25 +308,21 @@ public void decrypt(char[] password) throws GeneralSecurityException, IOExceptio DataInputStream input = new DataInputStream(bytesStream)) { int saltLen = input.readInt(); salt = new byte[saltLen]; - if (input.read(salt) != saltLen) { - throw new SecurityException("Keystore has been corrupted or tampered with"); - } + input.readFully(salt); int ivLen = input.readInt(); iv = new byte[ivLen]; - if (input.read(iv) != ivLen) { - throw new SecurityException("Keystore has been corrupted or tampered with"); - } + input.readFully(iv); int encryptedLen = input.readInt(); encryptedBytes = new byte[encryptedLen]; - if (input.read(encryptedBytes) != encryptedLen) { - throw new SecurityException("Keystore has been corrupted or tampered with"); - } + input.readFully(encryptedBytes); + } catch (EOFException e) { + throw new SecurityException("Keystore has been corrupted or tampered with"); } Cipher cipher = createCipher(Cipher.DECRYPT_MODE, password, salt, iv); - byte decryptedBytes[] = cipher.doFinal(encryptedBytes); - try (ByteArrayInputStream bytesStream = new ByteArrayInputStream(decryptedBytes); - DataInputStream input = new DataInputStream(bytesStream)) { + try (ByteArrayInputStream bytesStream = new ByteArrayInputStream(encryptedBytes); + CipherInputStream cipherStream = new CipherInputStream(bytesStream, cipher); + DataInputStream input = new DataInputStream(cipherStream)) { entries.set(new HashMap<>()); int numEntries = input.readInt(); while (numEntries-- > 0) { @@ -341,13 +330,11 @@ public void decrypt(char[] password) throws GeneralSecurityException, IOExceptio EntryType entryType = EntryType.valueOf(input.readUTF()); int entrySize = input.readInt(); byte[] entryBytes = new byte[entrySize]; - if (input.read(entryBytes) != entrySize) { - throw new SecurityException("Keystore has been corrupted or tampered with"); - } + input.readFully(entryBytes); entries.get().put(setting, new Entry(entryType, entryBytes)); } - } finally { - Arrays.fill(decryptedBytes, (byte) 0); + } catch (EOFException e) { + throw new SecurityException("Keystore has been corrupted or tampered with"); } } @@ -355,9 +342,10 @@ public void decrypt(char[] password) throws GeneralSecurityException, IOExceptio private byte[] encrypt(char[] password, byte[] salt, byte[] iv) throws GeneralSecurityException, IOException { assert isLoaded(); - Cipher cipher = createCipher(Cipher.ENCRYPT_MODE, password, salt, iv); ByteArrayOutputStream bytes = new ByteArrayOutputStream(); - try (DataOutputStream output = new DataOutputStream(bytes)) { + Cipher cipher = createCipher(Cipher.ENCRYPT_MODE, password, salt, iv); + try (CipherOutputStream cipherStream = new CipherOutputStream(bytes, cipher); + DataOutputStream output = new DataOutputStream(cipherStream)) { output.writeInt(entries.get().size()); for (Map.Entry mapEntry : entries.get().entrySet()) { output.writeUTF(mapEntry.getKey()); @@ -366,12 +354,8 @@ private byte[] encrypt(char[] password, byte[] salt, byte[] iv) throws GeneralSe output.writeInt(entry.bytes.length); output.write(entry.bytes); } - return cipher.doFinal(bytes.toByteArray()); - } finally { - final int bufferSize = bytes.size(); - bytes.reset(); - bytes.write(new byte[bufferSize]); } + return bytes.toByteArray(); } private void decryptLegacyEntries() throws GeneralSecurityException, IOException { From 13ae59b1cde65869f892b070cd126d91f16e8ac2 Mon Sep 17 00:00:00 2001 From: Ioannis Kakavas Date: Thu, 8 Feb 2018 11:46:48 +0200 Subject: [PATCH 04/10] Replaced * imports --- .../common/settings/KeyStoreWrapper.java | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java b/server/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java index e8dc61fa4df48..2aaf2e67083d4 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java +++ b/server/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java @@ -19,11 +19,22 @@ package org.elasticsearch.common.settings; -import javax.crypto.*; + +import javax.crypto.Cipher; +import javax.crypto.CipherInputStream; +import javax.crypto.CipherOutputStream; +import javax.crypto.SecretKey; +import javax.crypto.SecretKeyFactory; import javax.crypto.spec.GCMParameterSpec; import javax.crypto.spec.PBEKeySpec; import javax.crypto.spec.SecretKeySpec; -import java.io.*; +import java.io.ByteArrayInputStream; +import java.io.ByteArrayOutputStream; +import java.io.DataInputStream; +import java.io.DataOutputStream; +import java.io.EOFException; +import java.io.IOException; +import java.io.InputStream; import java.nio.ByteBuffer; import java.nio.CharBuffer; import java.nio.charset.StandardCharsets; From e3d79a55bf9a773dd797aafe2f24ba10f00e9b84 Mon Sep 17 00:00:00 2001 From: Ioannis Kakavas Date: Tue, 13 Mar 2018 11:47:08 +0200 Subject: [PATCH 05/10] Addresses feedback Adds test that ensures readFully() does not read garbage after the encrypted data. --- .../common/settings/KeyStoreWrapper.java | 1 - .../common/settings/KeyStoreWrapperTests.java | 66 +++++++++++++++++-- 2 files changed, 61 insertions(+), 6 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java b/server/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java index 2aaf2e67083d4..637cd63a806fb 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java +++ b/server/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java @@ -19,7 +19,6 @@ package org.elasticsearch.common.settings; - import javax.crypto.Cipher; import javax.crypto.CipherInputStream; import javax.crypto.CipherOutputStream; diff --git a/server/src/test/java/org/elasticsearch/common/settings/KeyStoreWrapperTests.java b/server/src/test/java/org/elasticsearch/common/settings/KeyStoreWrapperTests.java index 9414931f996e1..3bbe63ea7bbea 100644 --- a/server/src/test/java/org/elasticsearch/common/settings/KeyStoreWrapperTests.java +++ b/server/src/test/java/org/elasticsearch/common/settings/KeyStoreWrapperTests.java @@ -19,30 +19,32 @@ package org.elasticsearch.common.settings; +import javax.crypto.Cipher; +import javax.crypto.CipherOutputStream; import javax.crypto.SecretKey; import javax.crypto.SecretKeyFactory; +import javax.crypto.spec.GCMParameterSpec; import javax.crypto.spec.PBEKeySpec; +import javax.crypto.spec.SecretKeySpec; import java.io.ByteArrayOutputStream; +import java.io.DataOutputStream; import java.io.IOException; import java.io.InputStream; -import java.nio.CharBuffer; -import java.nio.charset.CharsetEncoder; import java.nio.charset.StandardCharsets; import java.nio.file.FileSystem; import java.nio.file.Path; import java.security.KeyStore; +import java.security.SecureRandom; import java.util.ArrayList; import java.util.Base64; import java.util.List; -import java.util.Map; -import java.util.stream.Collectors; import org.apache.lucene.codecs.CodecUtil; import org.apache.lucene.store.IOContext; import org.apache.lucene.store.IndexOutput; import org.apache.lucene.store.SimpleFSDirectory; import org.apache.lucene.util.IOUtils; -import org.elasticsearch.bootstrap.BootstrapSettings; +import org.elasticsearch.common.Randomness; import org.elasticsearch.env.Environment; import org.elasticsearch.test.ESTestCase; import org.junit.After; @@ -104,6 +106,60 @@ public void testUpgradeNoop() throws Exception { assertEquals(seed.toString(), keystore.getString(KeyStoreWrapper.SEED_SETTING.getKey()).toString()); } + public void testReadFullyDoesNotReadGarbage() throws Exception { + Path configDir = env.configFile(); + SimpleFSDirectory directory = new SimpleFSDirectory(configDir); + try (IndexOutput indexOutput = directory.createOutput("elasticsearch.keystore", IOContext.DEFAULT)) { + CodecUtil.writeHeader(indexOutput, "elasticsearch.keystore", 3); + indexOutput.writeByte((byte) 0); // No password + SecureRandom random = Randomness.createSecure(); + byte[] salt = new byte[64]; + random.nextBytes(salt); + byte[] iv = new byte[12]; + random.nextBytes(iv); + byte[] garbage = new byte[64]; + random.nextBytes(garbage); + + PBEKeySpec keySpec = new PBEKeySpec(new char[0], salt, 10000, 128); + SecretKeyFactory keyFactory = SecretKeyFactory.getInstance("PBKDF2WithHmacSHA512"); + SecretKey secretKey = keyFactory.generateSecret(keySpec); + SecretKeySpec secret = new SecretKeySpec(secretKey.getEncoded(), "AES"); + GCMParameterSpec spec = new GCMParameterSpec(128, iv); + Cipher cipher = Cipher.getInstance("AES/GCM/NoPadding"); + cipher.init(Cipher.ENCRYPT_MODE, secret, spec); + cipher.updateAAD(salt); + + ByteArrayOutputStream bytes = new ByteArrayOutputStream(); + CipherOutputStream cipherStream = new CipherOutputStream(bytes, cipher); + DataOutputStream output = new DataOutputStream(cipherStream); + + byte[] secret_value = "super_secret_value".getBytes(StandardCharsets.UTF_8); + output.writeInt(1); // One entry + output.writeUTF("string_setting"); + output.writeUTF("STRING"); + output.writeInt(secret_value.length); + output.write(secret_value); + output.write(garbage); + + cipherStream.close(); + final byte[] encryptedBytes = bytes.toByteArray(); + + indexOutput.writeInt(4 + salt.length + 4 + iv.length + 4 + encryptedBytes.length); + indexOutput.writeInt(salt.length); + indexOutput.writeBytes(salt, salt.length); + indexOutput.writeInt(iv.length); + indexOutput.writeBytes(iv, iv.length); + indexOutput.writeInt(encryptedBytes.length); + indexOutput.writeBytes(encryptedBytes, encryptedBytes.length); + CodecUtil.writeFooter(indexOutput); + } + + KeyStoreWrapper keystore = KeyStoreWrapper.load(configDir); + keystore.decrypt(new char[0]); + SecureString testValue = keystore.getString("string_setting"); + assertThat(testValue.toString(), equalTo("super_secret_value")); + } + public void testUpgradeAddsSeed() throws Exception { KeyStoreWrapper keystore = KeyStoreWrapper.create(); keystore.remove(KeyStoreWrapper.SEED_SETTING.getKey()); From f93e9bf8f7f55e4807488c7d3a3100d0dfbb4e62 Mon Sep 17 00:00:00 2001 From: Ioannis Kakavas Date: Mon, 16 Apr 2018 10:09:58 +0300 Subject: [PATCH 06/10] Ensure readFully reads all bytes --- .../org/elasticsearch/common/settings/KeyStoreWrapper.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/server/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java b/server/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java index 637cd63a806fb..e9160a9b0abec 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java +++ b/server/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java @@ -343,6 +343,9 @@ public void decrypt(char[] password) throws GeneralSecurityException, IOExceptio input.readFully(entryBytes); entries.get().put(setting, new Entry(entryType, entryBytes)); } + if (input.read() != -1) { + throw new SecurityException("Keystore has been corrupted or tampered with"); + } } catch (EOFException e) { throw new SecurityException("Keystore has been corrupted or tampered with"); } From 460ee6287ebefd1abf1eb8d5417a663e1931ea0d Mon Sep 17 00:00:00 2001 From: Ioannis Kakavas Date: Mon, 16 Apr 2018 10:10:34 +0300 Subject: [PATCH 07/10] Remove irrelevant test --- .../common/settings/KeyStoreWrapperTests.java | 56 +------------------ 1 file changed, 1 insertion(+), 55 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/common/settings/KeyStoreWrapperTests.java b/server/src/test/java/org/elasticsearch/common/settings/KeyStoreWrapperTests.java index f279a1b552096..3ad4482ae5f0f 100644 --- a/server/src/test/java/org/elasticsearch/common/settings/KeyStoreWrapperTests.java +++ b/server/src/test/java/org/elasticsearch/common/settings/KeyStoreWrapperTests.java @@ -105,61 +105,7 @@ public void testUpgradeNoop() throws Exception { keystore.decrypt(new char[0]); assertEquals(seed.toString(), keystore.getString(KeyStoreWrapper.SEED_SETTING.getKey()).toString()); } - - public void testReadFullyDoesNotReadGarbage() throws Exception { - Path configDir = env.configFile(); - SimpleFSDirectory directory = new SimpleFSDirectory(configDir); - try (IndexOutput indexOutput = directory.createOutput("elasticsearch.keystore", IOContext.DEFAULT)) { - CodecUtil.writeHeader(indexOutput, "elasticsearch.keystore", 3); - indexOutput.writeByte((byte) 0); // No password - SecureRandom random = Randomness.createSecure(); - byte[] salt = new byte[64]; - random.nextBytes(salt); - byte[] iv = new byte[12]; - random.nextBytes(iv); - byte[] garbage = new byte[64]; - random.nextBytes(garbage); - - PBEKeySpec keySpec = new PBEKeySpec(new char[0], salt, 10000, 128); - SecretKeyFactory keyFactory = SecretKeyFactory.getInstance("PBKDF2WithHmacSHA512"); - SecretKey secretKey = keyFactory.generateSecret(keySpec); - SecretKeySpec secret = new SecretKeySpec(secretKey.getEncoded(), "AES"); - GCMParameterSpec spec = new GCMParameterSpec(128, iv); - Cipher cipher = Cipher.getInstance("AES/GCM/NoPadding"); - cipher.init(Cipher.ENCRYPT_MODE, secret, spec); - cipher.updateAAD(salt); - - ByteArrayOutputStream bytes = new ByteArrayOutputStream(); - CipherOutputStream cipherStream = new CipherOutputStream(bytes, cipher); - DataOutputStream output = new DataOutputStream(cipherStream); - - byte[] secret_value = "super_secret_value".getBytes(StandardCharsets.UTF_8); - output.writeInt(1); // One entry - output.writeUTF("string_setting"); - output.writeUTF("STRING"); - output.writeInt(secret_value.length); - output.write(secret_value); - output.write(garbage); - - cipherStream.close(); - final byte[] encryptedBytes = bytes.toByteArray(); - - indexOutput.writeInt(4 + salt.length + 4 + iv.length + 4 + encryptedBytes.length); - indexOutput.writeInt(salt.length); - indexOutput.writeBytes(salt, salt.length); - indexOutput.writeInt(iv.length); - indexOutput.writeBytes(iv, iv.length); - indexOutput.writeInt(encryptedBytes.length); - indexOutput.writeBytes(encryptedBytes, encryptedBytes.length); - CodecUtil.writeFooter(indexOutput); - } - - KeyStoreWrapper keystore = KeyStoreWrapper.load(configDir); - keystore.decrypt(new char[0]); - SecureString testValue = keystore.getString("string_setting"); - assertThat(testValue.toString(), equalTo("super_secret_value")); - } - + public void testUpgradeAddsSeed() throws Exception { KeyStoreWrapper keystore = KeyStoreWrapper.create(); keystore.remove(KeyStoreWrapper.SEED_SETTING.getKey()); From e853a6aca06c6442d2fb9867763d7054aecdb27a Mon Sep 17 00:00:00 2001 From: Ioannis Kakavas Date: Mon, 16 Apr 2018 10:29:51 +0300 Subject: [PATCH 08/10] Adds test for trailing garbage Ensures we fail if for some reason readFully can't consume the entire stream (for instance if stream contains trailing garbage) --- .../common/settings/KeyStoreWrapperTests.java | 54 ++++++++++++++++++- 1 file changed, 53 insertions(+), 1 deletion(-) diff --git a/server/src/test/java/org/elasticsearch/common/settings/KeyStoreWrapperTests.java b/server/src/test/java/org/elasticsearch/common/settings/KeyStoreWrapperTests.java index 3ad4482ae5f0f..bb692597a0a15 100644 --- a/server/src/test/java/org/elasticsearch/common/settings/KeyStoreWrapperTests.java +++ b/server/src/test/java/org/elasticsearch/common/settings/KeyStoreWrapperTests.java @@ -50,6 +50,7 @@ import org.junit.After; import org.junit.Before; +import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; public class KeyStoreWrapperTests extends ESTestCase { @@ -105,7 +106,58 @@ public void testUpgradeNoop() throws Exception { keystore.decrypt(new char[0]); assertEquals(seed.toString(), keystore.getString(KeyStoreWrapper.SEED_SETTING.getKey()).toString()); } - + + public void testFailWhenStreamNotConsumed() throws Exception { + Path configDir = env.configFile(); + SimpleFSDirectory directory = new SimpleFSDirectory(configDir); + try (IndexOutput indexOutput = directory.createOutput("elasticsearch.keystore", IOContext.DEFAULT)) { + CodecUtil.writeHeader(indexOutput, "elasticsearch.keystore", 3); + indexOutput.writeByte((byte) 0); // No password + SecureRandom random = Randomness.createSecure(); + byte[] salt = new byte[64]; + random.nextBytes(salt); + byte[] iv = new byte[12]; + random.nextBytes(iv); + + PBEKeySpec keySpec = new PBEKeySpec(new char[0], salt, 10000, 128); + SecretKeyFactory keyFactory = SecretKeyFactory.getInstance("PBKDF2WithHmacSHA512"); + SecretKey secretKey = keyFactory.generateSecret(keySpec); + SecretKeySpec secret = new SecretKeySpec(secretKey.getEncoded(), "AES"); + GCMParameterSpec spec = new GCMParameterSpec(128, iv); + Cipher cipher = Cipher.getInstance("AES/GCM/NoPadding"); + cipher.init(Cipher.ENCRYPT_MODE, secret, spec); + cipher.updateAAD(salt); + + ByteArrayOutputStream bytes = new ByteArrayOutputStream(); + CipherOutputStream cipherStream = new CipherOutputStream(bytes, cipher); + DataOutputStream output = new DataOutputStream(cipherStream); + + byte[] secret_value = "super_secret_value".getBytes(StandardCharsets.UTF_8); + output.writeInt(1); // One entry + output.writeUTF("string_setting"); + output.writeUTF("STRING"); + // So that readFully during decryption will not consume the entire stream + output.writeInt(secret_value.length - 4); + output.write(secret_value); + + cipherStream.close(); + final byte[] encryptedBytes = bytes.toByteArray(); + + indexOutput.writeInt(4 + salt.length + 4 + iv.length + 4 + encryptedBytes.length); + indexOutput.writeInt(salt.length); + indexOutput.writeBytes(salt, salt.length); + indexOutput.writeInt(iv.length); + indexOutput.writeBytes(iv, iv.length); + indexOutput.writeInt(encryptedBytes.length); + indexOutput.writeBytes(encryptedBytes, encryptedBytes.length); + CodecUtil.writeFooter(indexOutput); + } + + KeyStoreWrapper keystore = KeyStoreWrapper.load(configDir); + SecurityException e = expectThrows(SecurityException.class, () -> keystore.decrypt(new char[0])); + assertThat(e.getMessage(), containsString("Keystore has been corrupted or tampered with")); + } + public void testUpgradeAddsSeed() throws Exception { KeyStoreWrapper keystore = KeyStoreWrapper.create(); keystore.remove(KeyStoreWrapper.SEED_SETTING.getKey()); From 3c56cfdf9c5495325c9b0203587645e389e58bdc Mon Sep 17 00:00:00 2001 From: Ioannis Kakavas Date: Tue, 17 Apr 2018 08:45:13 +0300 Subject: [PATCH 09/10] Addresses feedback Adds aditional explicit check for full stream consumption Adds tests --- .../common/settings/KeyStoreWrapper.java | 3 + .../common/settings/KeyStoreWrapperTests.java | 138 +++++++++++++++--- 2 files changed, 117 insertions(+), 24 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java b/server/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java index e9160a9b0abec..0b89abcc9ea2a 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java +++ b/server/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java @@ -325,6 +325,9 @@ public void decrypt(char[] password) throws GeneralSecurityException, IOExceptio int encryptedLen = input.readInt(); encryptedBytes = new byte[encryptedLen]; input.readFully(encryptedBytes); + if (input.read() != -1) { + throw new SecurityException("Keystore has been corrupted or tampered with"); + } } catch (EOFException e) { throw new SecurityException("Keystore has been corrupted or tampered with"); } diff --git a/server/src/test/java/org/elasticsearch/common/settings/KeyStoreWrapperTests.java b/server/src/test/java/org/elasticsearch/common/settings/KeyStoreWrapperTests.java index bb692597a0a15..30e538739beb7 100644 --- a/server/src/test/java/org/elasticsearch/common/settings/KeyStoreWrapperTests.java +++ b/server/src/test/java/org/elasticsearch/common/settings/KeyStoreWrapperTests.java @@ -107,7 +107,7 @@ public void testUpgradeNoop() throws Exception { assertEquals(seed.toString(), keystore.getString(KeyStoreWrapper.SEED_SETTING.getKey()).toString()); } - public void testFailWhenStreamNotConsumed() throws Exception { + public void testFailWhenCannotConsumeSecretStream() throws Exception { Path configDir = env.configFile(); SimpleFSDirectory directory = new SimpleFSDirectory(configDir); try (IndexOutput indexOutput = directory.createOutput("elasticsearch.keystore", IOContext.DEFAULT)) { @@ -118,38 +118,95 @@ public void testFailWhenStreamNotConsumed() throws Exception { random.nextBytes(salt); byte[] iv = new byte[12]; random.nextBytes(iv); + ByteArrayOutputStream bytes = new ByteArrayOutputStream(); + CipherOutputStream cipherStream = getCipherStream(bytes, salt, iv); + DataOutputStream output = new DataOutputStream(cipherStream); + // Indicate that the secret string is longer than it is so readFully() fails + possiblyAlterSecretString(output, -4); + cipherStream.close(); + final byte[] encryptedBytes = bytes.toByteArray(); + possiblyAlterEncryptedBytes(indexOutput, salt, iv, encryptedBytes, 0); + CodecUtil.writeFooter(indexOutput); + } - PBEKeySpec keySpec = new PBEKeySpec(new char[0], salt, 10000, 128); - SecretKeyFactory keyFactory = SecretKeyFactory.getInstance("PBKDF2WithHmacSHA512"); - SecretKey secretKey = keyFactory.generateSecret(keySpec); - SecretKeySpec secret = new SecretKeySpec(secretKey.getEncoded(), "AES"); - GCMParameterSpec spec = new GCMParameterSpec(128, iv); - Cipher cipher = Cipher.getInstance("AES/GCM/NoPadding"); - cipher.init(Cipher.ENCRYPT_MODE, secret, spec); - cipher.updateAAD(salt); + KeyStoreWrapper keystore = KeyStoreWrapper.load(configDir); + SecurityException e = expectThrows(SecurityException.class, () -> keystore.decrypt(new char[0])); + assertThat(e.getMessage(), containsString("Keystore has been corrupted or tampered with")); + } + public void testFailWhenCannotConsumeEncryptedBytesStream() throws Exception { + Path configDir = env.configFile(); + SimpleFSDirectory directory = new SimpleFSDirectory(configDir); + try (IndexOutput indexOutput = directory.createOutput("elasticsearch.keystore", IOContext.DEFAULT)) { + CodecUtil.writeHeader(indexOutput, "elasticsearch.keystore", 3); + indexOutput.writeByte((byte) 0); // No password + SecureRandom random = Randomness.createSecure(); + byte[] salt = new byte[64]; + random.nextBytes(salt); + byte[] iv = new byte[12]; + random.nextBytes(iv); ByteArrayOutputStream bytes = new ByteArrayOutputStream(); - CipherOutputStream cipherStream = new CipherOutputStream(bytes, cipher); + CipherOutputStream cipherStream = getCipherStream(bytes, salt, iv); DataOutputStream output = new DataOutputStream(cipherStream); - byte[] secret_value = "super_secret_value".getBytes(StandardCharsets.UTF_8); - output.writeInt(1); // One entry - output.writeUTF("string_setting"); - output.writeUTF("STRING"); - // So that readFully during decryption will not consume the entire stream - output.writeInt(secret_value.length - 4); - output.write(secret_value); + possiblyAlterSecretString(output, 0); + cipherStream.close(); + final byte[] encryptedBytes = bytes.toByteArray(); + // Indicate that the encryptedBytes is larger than it is so readFully() fails + possiblyAlterEncryptedBytes(indexOutput, salt, iv, encryptedBytes, -12); + CodecUtil.writeFooter(indexOutput); + } + KeyStoreWrapper keystore = KeyStoreWrapper.load(configDir); + SecurityException e = expectThrows(SecurityException.class, () -> keystore.decrypt(new char[0])); + assertThat(e.getMessage(), containsString("Keystore has been corrupted or tampered with")); + } + + public void testFailWhenSecretStreamNotConsumed() throws Exception { + Path configDir = env.configFile(); + SimpleFSDirectory directory = new SimpleFSDirectory(configDir); + try (IndexOutput indexOutput = directory.createOutput("elasticsearch.keystore", IOContext.DEFAULT)) { + CodecUtil.writeHeader(indexOutput, "elasticsearch.keystore", 3); + indexOutput.writeByte((byte) 0); // No password + SecureRandom random = Randomness.createSecure(); + byte[] salt = new byte[64]; + random.nextBytes(salt); + byte[] iv = new byte[12]; + random.nextBytes(iv); + ByteArrayOutputStream bytes = new ByteArrayOutputStream(); + CipherOutputStream cipherStream = getCipherStream(bytes, salt, iv); + DataOutputStream output = new DataOutputStream(cipherStream); + // So that readFully during decryption will not consume the entire stream + possiblyAlterSecretString(output, 4); cipherStream.close(); final byte[] encryptedBytes = bytes.toByteArray(); + possiblyAlterEncryptedBytes(indexOutput, salt, iv, encryptedBytes, 0); + CodecUtil.writeFooter(indexOutput); + } - indexOutput.writeInt(4 + salt.length + 4 + iv.length + 4 + encryptedBytes.length); - indexOutput.writeInt(salt.length); - indexOutput.writeBytes(salt, salt.length); - indexOutput.writeInt(iv.length); - indexOutput.writeBytes(iv, iv.length); - indexOutput.writeInt(encryptedBytes.length); - indexOutput.writeBytes(encryptedBytes, encryptedBytes.length); + KeyStoreWrapper keystore = KeyStoreWrapper.load(configDir); + SecurityException e = expectThrows(SecurityException.class, () -> keystore.decrypt(new char[0])); + assertThat(e.getMessage(), containsString("Keystore has been corrupted or tampered with")); + } + + public void testFailWhenEncryptedBytesStreamIsNotConsumed() throws Exception { + Path configDir = env.configFile(); + SimpleFSDirectory directory = new SimpleFSDirectory(configDir); + try (IndexOutput indexOutput = directory.createOutput("elasticsearch.keystore", IOContext.DEFAULT)) { + CodecUtil.writeHeader(indexOutput, "elasticsearch.keystore", 3); + indexOutput.writeByte((byte) 0); // No password + SecureRandom random = Randomness.createSecure(); + byte[] salt = new byte[64]; + random.nextBytes(salt); + byte[] iv = new byte[12]; + random.nextBytes(iv); + ByteArrayOutputStream bytes = new ByteArrayOutputStream(); + CipherOutputStream cipherStream = getCipherStream(bytes, salt, iv); + DataOutputStream output = new DataOutputStream(cipherStream); + possiblyAlterSecretString(output, 0); + cipherStream.close(); + final byte[] encryptedBytes = bytes.toByteArray(); + possiblyAlterEncryptedBytes(indexOutput, salt, iv, encryptedBytes, randomIntBetween(2, encryptedBytes.length)); CodecUtil.writeFooter(indexOutput); } @@ -158,6 +215,39 @@ public void testFailWhenStreamNotConsumed() throws Exception { assertThat(e.getMessage(), containsString("Keystore has been corrupted or tampered with")); } + private CipherOutputStream getCipherStream(ByteArrayOutputStream bytes, byte[] salt, byte[] iv) throws Exception { + PBEKeySpec keySpec = new PBEKeySpec(new char[0], salt, 10000, 128); + SecretKeyFactory keyFactory = SecretKeyFactory.getInstance("PBKDF2WithHmacSHA512"); + SecretKey secretKey = keyFactory.generateSecret(keySpec); + SecretKeySpec secret = new SecretKeySpec(secretKey.getEncoded(), "AES"); + GCMParameterSpec spec = new GCMParameterSpec(128, iv); + Cipher cipher = Cipher.getInstance("AES/GCM/NoPadding"); + cipher.init(Cipher.ENCRYPT_MODE, secret, spec); + cipher.updateAAD(salt); + return new CipherOutputStream(bytes, cipher); + } + + private void possiblyAlterSecretString(DataOutputStream output, int truncLength) throws Exception { + byte[] secret_value = "super_secret_value".getBytes(StandardCharsets.UTF_8); + output.writeInt(1); // One entry + output.writeUTF("string_setting"); + output.writeUTF("STRING"); + output.writeInt(secret_value.length - truncLength); + output.write(secret_value); + } + + private void possiblyAlterEncryptedBytes(IndexOutput indexOutput, byte[] salt, byte[] iv, byte[] encryptedBytes, int + truncEncryptedDataLength) + throws Exception { + indexOutput.writeInt(4 + salt.length + 4 + iv.length + 4 + encryptedBytes.length); + indexOutput.writeInt(salt.length); + indexOutput.writeBytes(salt, salt.length); + indexOutput.writeInt(iv.length); + indexOutput.writeBytes(iv, iv.length); + indexOutput.writeInt(encryptedBytes.length - truncEncryptedDataLength); + indexOutput.writeBytes(encryptedBytes, encryptedBytes.length); + } + public void testUpgradeAddsSeed() throws Exception { KeyStoreWrapper keystore = KeyStoreWrapper.create(); keystore.remove(KeyStoreWrapper.SEED_SETTING.getKey()); From 9b90e9b5003272d31a11e663e6055b025ae950b9 Mon Sep 17 00:00:00 2001 From: Ioannis Kakavas Date: Thu, 3 May 2018 12:59:13 +0300 Subject: [PATCH 10/10] Retain root cause when throwing SecurityException --- .../org/elasticsearch/common/settings/KeyStoreWrapper.java | 4 ++-- .../elasticsearch/common/settings/KeyStoreWrapperTests.java | 4 ++++ 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java b/server/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java index 0b89abcc9ea2a..04bbb9279dab5 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java +++ b/server/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java @@ -329,7 +329,7 @@ public void decrypt(char[] password) throws GeneralSecurityException, IOExceptio throw new SecurityException("Keystore has been corrupted or tampered with"); } } catch (EOFException e) { - throw new SecurityException("Keystore has been corrupted or tampered with"); + throw new SecurityException("Keystore has been corrupted or tampered with", e); } Cipher cipher = createCipher(Cipher.DECRYPT_MODE, password, salt, iv); @@ -350,7 +350,7 @@ public void decrypt(char[] password) throws GeneralSecurityException, IOExceptio throw new SecurityException("Keystore has been corrupted or tampered with"); } } catch (EOFException e) { - throw new SecurityException("Keystore has been corrupted or tampered with"); + throw new SecurityException("Keystore has been corrupted or tampered with", e); } } diff --git a/server/src/test/java/org/elasticsearch/common/settings/KeyStoreWrapperTests.java b/server/src/test/java/org/elasticsearch/common/settings/KeyStoreWrapperTests.java index 30e538739beb7..e22836087367c 100644 --- a/server/src/test/java/org/elasticsearch/common/settings/KeyStoreWrapperTests.java +++ b/server/src/test/java/org/elasticsearch/common/settings/KeyStoreWrapperTests.java @@ -28,6 +28,7 @@ import javax.crypto.spec.SecretKeySpec; import java.io.ByteArrayOutputStream; import java.io.DataOutputStream; +import java.io.EOFException; import java.io.IOException; import java.io.InputStream; import java.nio.charset.StandardCharsets; @@ -52,6 +53,7 @@ import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.instanceOf; public class KeyStoreWrapperTests extends ESTestCase { @@ -132,6 +134,7 @@ public void testFailWhenCannotConsumeSecretStream() throws Exception { KeyStoreWrapper keystore = KeyStoreWrapper.load(configDir); SecurityException e = expectThrows(SecurityException.class, () -> keystore.decrypt(new char[0])); assertThat(e.getMessage(), containsString("Keystore has been corrupted or tampered with")); + assertThat(e.getCause(), instanceOf(EOFException.class)); } public void testFailWhenCannotConsumeEncryptedBytesStream() throws Exception { @@ -160,6 +163,7 @@ public void testFailWhenCannotConsumeEncryptedBytesStream() throws Exception { KeyStoreWrapper keystore = KeyStoreWrapper.load(configDir); SecurityException e = expectThrows(SecurityException.class, () -> keystore.decrypt(new char[0])); assertThat(e.getMessage(), containsString("Keystore has been corrupted or tampered with")); + assertThat(e.getCause(), instanceOf(EOFException.class)); } public void testFailWhenSecretStreamNotConsumed() throws Exception {