Use readFully() to read bytes from CipherInputStream (#28515)

Changes how data is read from 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 JCE and BCFIPS Security Providers

Author: jkakavas

server/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java

@@ -31,6 +31,7 @@
 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;
@@ -317,38 +318,39 @@ 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) {
+            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", e);
         }
 
         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)) {
-
             entries.set(new HashMap<>());
             int numEntries = input.readInt();
             while (numEntries-- > 0) {
                 String setting = input.readUTF();
                 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));
             }
+            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", e);
         }
     }
 
@@ -360,7 +362,6 @@ private byte[] encrypt(char[] password, byte[] salt, byte[] iv) throws GeneralSe
         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<String, Entry> mapEntry : entries.get().entrySet()) {
                 output.writeUTF(mapEntry.getKey());
@@ -370,7 +371,6 @@ private byte[] encrypt(char[] password, byte[] salt, byte[] iv) throws GeneralSe
                 output.write(entry.bytes);
             }
         }
-
         return bytes.toByteArray();
     }
 

server/src/test/java/org/elasticsearch/common/settings/KeyStoreWrapperTests.java

@@ -19,36 +19,41 @@
 
 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.EOFException;
 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.elasticsearch.common.Randomness;
 import org.elasticsearch.core.internal.io.IOUtils;
-import org.elasticsearch.bootstrap.BootstrapSettings;
 import org.elasticsearch.env.Environment;
 import org.elasticsearch.test.ESTestCase;
 import org.junit.After;
 import org.junit.Before;
 
+import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.instanceOf;
 
 public class KeyStoreWrapperTests extends ESTestCase {
 
@@ -104,6 +109,149 @@ public void testUpgradeNoop() throws Exception {
         assertEquals(seed.toString(), keystore.getString(KeyStoreWrapper.SEED_SETTING.getKey()).toString());
     }
 
+    public void testFailWhenCannotConsumeSecretStream() 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);
+            // 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);
+        }
+
+        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 {
+        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();
+            // 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"));
+        assertThat(e.getCause(), instanceOf(EOFException.class));
+    }
+
+    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);
+        }
+
+        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);
+        }
+
+        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"));
+    }
+
+    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());