Use readFully() to read exactly the number of bytes we expect from the Stream
#28515
Conversation
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).
jkakavas
added
review
:Core/Infra/Settings
There was a problem hiding this comment.
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).
This sounds like it could be a bug in the BC implementation? Can you create a small reproducer and report it upstream. I am not a fan of adding workarounds like this unless we have a upstream issue so that we can plan to remove the workaround in the future.
If we're going to remove the use of CipherInputStream, we should add it to forbidden apis otherwise it will crop up somewhere else.
|
+1 to what Jay said. |
|
Thanks for the comments. I started a discussion upstream and provided them with test cases for reproduction.
Apart from this approach being a workaround to the original version, do you see anything that is inherently wrong with using
It does seem to affect other AES modes,I was able to reproduce it with AES/CBC/PKCS7Padding too for instance. |
Thank you.
There is nothing wrong with this aspect explicitly. In many instances the Cipher*Stream make things simpler, which is why I prefer their usage. Also, we should add comments in the code about why we do things a certain way with links to things like the upstream discussion so that the next person to come across this has a clue why things were done a certain way. |
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
|
Based on the feedback from the upstream mailing list, it turns out this has to do with how the data is read from the stream, and thus affects only the |
| @@ -19,6 +19,7 @@ | |||
|
|
|||
| package org.elasticsearch.common.settings; | |||
|
|
|||
|
|
|||
| @@ -317,38 +319,33 @@ public void decrypt(char[] password) throws GeneralSecurityException, IOExceptio | |||
| DataInputStream input = new DataInputStream(bytesStream)) { | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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..
There was a problem hiding this comment.
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.
jkakavas
changed the title
readFully() to read exactly the number of bytes we expect from the Stream
Adds test that ensures readFully() does not read garbage after the encrypted data.
Ensures we fail if for some reason readFully can't consume the entire stream (for instance if stream contains trailing garbage)
| throw new SecurityException("Keystore has been corrupted or tampered with"); | ||
| } | ||
| input.readFully(encryptedBytes); | ||
| } catch (EOFException e) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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"); | ||
| } | ||
| } catch (EOFException e) { | ||
| throw new SecurityException("Keystore has been corrupted or tampered with"); |
There was a problem hiding this comment.
Same comment about dropping the root cause as above.
| @@ -104,6 +107,57 @@ public void testUpgradeNoop() throws Exception { | |||
| assertEquals(seed.toString(), keystore.getString(KeyStoreWrapper.SEED_SETTING.getKey()).toString()); | |||
| } | |||
|
|
|||
| public void testFailWhenStreamNotConsumed() throws Exception { | |||
There was a problem hiding this comment.
This seems to test just one case. Can it be duplicated/refactored so it tests each of the readFully calls?
There was a problem hiding this comment.
This one was attempting to test the case where for some reason the stream is not fully consumed. The only case that can be effectively tested in the same manner is the readFully(encryptedBytes) - again leaving bytes in the stream by indicating a smaller size.
If we write a smaller int as an indication of the size of the salt or the iv byte arrays, we'll get unpredictable behavior in the subsequent readInt() calls ( for iv, encryptedBytes ) that might cause NegativeArraySizeException and these are actually the tests I was postponing to the followup
As discussed, we can address the slightly more complicated test case of cross buffer boundaries in a follow-up PR
I did however add a couple of more tests, checking that we fail when these two readFully calls either don't or can't consume the stream.
Adds aditional explicit check for full stream consumption Adds tests
|
@rjernst I addressed your comments, I think this is ready (pending green CI), I would appreciate a 👍 before merging |
* master: Docs: remove transport_client from CCS role example (elastic#30263) [Rollup] Validate timezone in range queries (elastic#30338) Use readFully() to read bytes from CipherInputStream (elastic#28515) Fix docs Recently merged elastic#29229 had a doc bug that broke the doc build. This commit fixes. Test: remove cluster permission from CCS user (elastic#30262) Add Get Settings API support to java high-level rest client (elastic#29229) Watcher: Remove unneeded index deletion in tests
* master: (35 commits) DOCS: Correct mapping tags in put-template api DOCS: Fix broken link in the put index template api Add put index template api to high level rest client (#30400) Relax testAckedIndexing to allow document updating [Docs] Add snippets for POS stop tags default value Move respect accept header on no handler to 6.3.1 Respect accept header on no handler (#30383) [Test] Add analysis-nori plugin to the vagrant tests [Docs] Fix bad link [Docs] Fix end of section in the korean plugin docs Expose the Lucene Korean analyzer module in a plugin (#30397) Docs: remove transport_client from CCS role example (#30263) [Rollup] Validate timezone in range queries (#30338) Use readFully() to read bytes from CipherInputStream (#28515) Fix docs Recently merged #29229 had a doc bug that broke the doc build. This commit fixes. Test: remove cluster permission from CCS user (#30262) Add Get Settings API support to java high-level rest client (#29229) Watcher: Remove unneeded index deletion in tests Set the new lucene version for 6.4.0 [ML][TEST] Clean up jobs in ModelPlotIT ...
* elastic-master: Watcher: Mark watcher as started only after loading watches (#30403) Pass the task to broadcast actions (#29672) Disable REST default settings testing until #29229 is back-ported Correct wording in log message (#30336) Do not fail snapshot when deleting a missing snapshotted file (#30332) AwaitsFix testCreateShrinkIndexToN DOCS: Correct mapping tags in put-template api DOCS: Fix broken link in the put index template api Add put index template api to high level rest client (#30400) Relax testAckedIndexing to allow document updating [Docs] Add snippets for POS stop tags default value Move respect accept header on no handler to 6.3.1 Respect accept header on no handler (#30383) [Test] Add analysis-nori plugin to the vagrant tests [Docs] Fix bad link [Docs] Fix end of section in the korean plugin docs Expose the Lucene Korean analyzer module in a plugin (#30397) Docs: remove transport_client from CCS role example (#30263) [Rollup] Validate timezone in range queries (#30338) Use readFully() to read bytes from CipherInputStream (#28515) Fix docs Recently merged #29229 had a doc bug that broke the doc build. This commit fixes. Test: remove cluster permission from CCS user (#30262) Add Get Settings API support to java high-level rest client (#29229) Watcher: Remove unneeded index deletion in tests
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
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
Instead of using
read()and checking that the bytes read are what we expect,use
readFully()which will read exactly the number of byteswhile keep reading until the end of the stream or throw an
EOFExceptionif not all bytes can be read.This approach keeps the simplicity of using CipherInputStream while
working as expected with all Security Providers (SunJCE, BouncyCastle, BouncyCastleFips )
Calls explicitlydoFinal()on the byte array of plaintext andciphertext, instead of making use ofCipher{Input,Output}Stream.While functionally equivelant, CipherInputStream merhod does notwork well with BouncyCastle (FIPS) Security Provider's implementationof AES GCM as the byte array that is read from theCipherInputStreambackedDataInputStreamis always some bytes short (17 vs 20 forthe case ofkeystore.seedvalue).