Skip to content

Commit

Permalink
Use readFully() to read bytes from CipherInputStream (#30640)
Browse files Browse the repository at this point in the history 
Changes how data is read from CipherInputStream in
 KeyStoreWrapper. 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.

See also: #28515
  • Loading branch information
@jkakavas
jkakavas authored May 16, 2018
1 parent 4207078 commit 22749e3
Show file tree
Hide file tree
Showing 2 changed files with 164 additions and 18 deletions.
26 changes: 13 additions & 13 deletions server/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -318,38 +319,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);
}
}

Expand All @@ -361,7 +363,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());
Expand All @@ -371,7 +372,6 @@ private byte[] encrypt(char[] password, byte[] salt, byte[] iv) throws GeneralSe
output.write(entry.bytes);
}
}

return bytes.toByteArray();
}

Expand Down
156 changes: 151 additions & 5 deletions server/src/test/java/org/elasticsearch/common/settings/KeyStoreWrapperTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,30 +19,33 @@

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.hamcrest.Matchers;
Expand Down Expand Up @@ -121,6 +124,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());
Expand Down

0 comments on commit 22749e3

Please sign in to comment.