Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use readFully() to read exactly the number of bytes we expect from the Stream #28515

Merged
merged 16 commits into from
May 4, 2018
Merged

Use readFully() to read exactly the number of bytes we expect from the Stream #28515

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
2ea6d2a
Changes the encryption/decryption method
jkakavas Feb 2, 2018
8241318
Zerofill sensitive data
jkakavas Feb 2, 2018
63a4fcd
Changed how data is read from the stream
jkakavas Feb 8, 2018
4cc56cf
Merge remote-tracking branch 'origin/master' into fix-for-bcfips
jkakavas Feb 8, 2018
13ae59b
Replaced * imports
jkakavas Feb 8, 2018
248c73d
Merge remote-tracking branch 'origin/master' into fix-for-bcfips
jkakavas Mar 9, 2018
a31ab4f
Merge remote-tracking branch 'origin/master' into fix-for-bcfips
jkakavas Mar 13, 2018
e3d79a5
Addresses feedback
jkakavas Mar 13, 2018
f3274bb
Merge remote-tracking branch 'origin/master' into fix-for-bcfips
jkakavas Mar 14, 2018
0526e03
Merge remote-tracking branch 'origin/master' into fix-for-bcfips
jkakavas Apr 16, 2018
f93e9bf
Ensure readFully reads all bytes
jkakavas Apr 16, 2018
460ee62
Remove irrelevant test
jkakavas Apr 16, 2018
e853a6a
Adds test for trailing garbage
jkakavas Apr 16, 2018
3c56cfd
Addresses feedback
jkakavas Apr 17, 2018
1539793
Merge remote-tracking branch 'origin/master' into fix-for-bcfips
jkakavas May 3, 2018
9b90e9b
Retain root cause when throwing SecurityException
jkakavas May 3, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 13 additions & 13 deletions server/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java
View file
Open in desktop
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 @@ -317,38 +318,39 @@ public void decrypt(char[] password) throws GeneralSecurityException, IOExceptio
DataInputStream input = new DataInputStream(bytesStream)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should add a test that would have failed using the old method. This would probably require a small refactoring to do this; I am thinking instead of new ByteArrayInputStream we call a method that could be overridden in a test. There we would have a special stream where a call to read would not fully populate the byte array

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to override any methods. We can form streams of bytes in whatever order we want in tests.
However, if I understand the problem correctly, it has to do with read boundaries. Eg, an input stream may choose to return less bytes to a read call than the array can contain, since it may just copy the rest of an existing buffer (eg a decrypted block in CipherInputStream). By calling readFully, multiple read calls are made to ensure the array is filled completely or EOF is reached. I think this will be very difficult to mimic, given the behavior of read here will be implementation dependent. The best we could do is add tests to trigger each of the possible EOFs from readFully calls by truncating the keystore at certain points (although this would probably be difficult to do within the encrypted bytes).

I do think we should have a test to ensure there cannot be garbage after the encrypted data.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Revisiting this after coming back.

I do think we should have a test to ensure there cannot be garbage after the encrypted data

Care to elaborate @rjernst ? You mean attempting to readFully() into a larger array and verifying that we get the EOFException rather than keep reading garbage ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean adding a test with garbage at the end, so that we know the last readFully logic works. The intermediate readFully's are harder, as I described above.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do think we should have a test to ensure there cannot be garbage after the encrypted data.

If I understand correctly, we should fail if there is garbage right? Currently this is not the case at least based on the test @jkakavas added.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

readFully reads exactly the number of bytes we instruct it to, or throws an EOFException. I interpreted @rjernst comment above to mean that we need to have a test verifying that even if there more garbage, readFully won't read them, but read the bytes it should(this is what the test I added does, and it should not fail )
Another way to interpret the comment I guess, would be to write less bytes to would be to write fewer bytes to the DataOutputStream than we indicate with the writeInt above and make sure that on decryption, readFully throws the EOFException instead of reading in garbage in order to fill our byte array - I can add that too.
Writing about this, the second interpretation makes much more sense I guess..

Copy link
Member

@rjernst rjernst Mar 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was not my intention. I meant it the way @jaymode suggested: we should fail when trailing garbage is present, and have a test for that.

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

return bytes.toByteArray();
}

Expand Down
158 changes: 153 additions & 5 deletions server/src/test/java/org/elasticsearch/common/settings/KeyStoreWrapperTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {

Expand Down Expand Up @@ -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());
Expand Down