Skip to content

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

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

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

Changes from 5 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
25 changes: 10 additions & 15 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 @@ -19,6 +19,7 @@

package org.elasticsearch.common.settings;


Copy link
Member

@jaymode jaymode Feb 8, 2018

Choose a reason for hiding this comment

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

nit: unnecessary change?

import javax.crypto.Cipher;
import javax.crypto.CipherInputStream;
import javax.crypto.CipherOutputStream;
Expand All @@ -31,6 +32,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 +319,33 @@ public void decrypt(char[] password) throws GeneralSecurityException, IOExceptio
DataInputStream input = new DataInputStream(bytesStream)) {
Copy link
Member

@jaymode jaymode Feb 8, 2018

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

@rjernst rjernst Feb 12, 2018

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
Contributor Author

@jkakavas jkakavas Mar 13, 2018

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

@rjernst rjernst Mar 13, 2018

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

@jaymode jaymode Mar 14, 2018

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
Contributor Author

@jkakavas jkakavas Mar 14, 2018

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) {
throw new SecurityException("Keystore has been corrupted or tampered with");
}
input.readFully(encryptedBytes);
} catch (EOFException e) {
Copy link
Member

@rjernst rjernst Apr 16, 2018

Choose a reason for hiding this comment

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

While the exception message was the same in all cases before, we still would have been able to tell where the failed read happened (ie where in the bytes the corrupt/tamped data occurs). Are we sure we want to drop the root cause exception here? I think that at least warrants a comment here as to why.

Copy link
Contributor Author

@jkakavas jkakavas Apr 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

You're right, any of the readX can throw an EOFException and I would have masked it. We should know where the failure occurred.

I passed the original exception as a cause to SecurityException to fix that

throw new SecurityException("Keystore has been corrupted or tampered with");
}

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));
}
} catch (EOFException e) {
throw new SecurityException("Keystore has been corrupted or tampered with");
Copy link
Member

@rjernst rjernst Apr 16, 2018

Choose a reason for hiding this comment

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

Same comment about dropping the root cause as above.

}
}

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

return bytes.toByteArray();
}

Expand Down