ParsedCiphertext throws an error if it is not complete #119

shayvana · 2019-07-24T18:08:40Z

Added a check as well as a test to make sure isComplete_ is true before returning from the ParsedCiphertext constructor.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Check any applicable:

Were any files moved? Moving files changes their URL, which breaks all hyperlinks to the files.

…nstructor and throwing a BadCiphertextException if it is not.

…rtext constructor

shayvana · 2019-07-24T18:10:22Z

@lavaleri @josecorella

src/main/java/com/amazonaws/encryptionsdk/ParsedCiphertext.java

src/test/java/com/amazonaws/encryptionsdk/model/CiphertextHeadersTest.java

…ing ParsedCiphertext with an incomplete ciphertext

src/test/java/com/amazonaws/encryptionsdk/model/CiphertextHeadersTest.java

lavaleri · 2019-08-05T17:54:06Z

src/test/java/com/amazonaws/encryptionsdk/ParsedCiphertextTest.java

+import static org.mockito.Mockito.spy;
+
+public class ParsedCiphertextTest extends CiphertextHeaders {
+    final byte[] ciphertext_ = {0};


This is "ciphertext" that we expect to fail being parsed, so let's move this into the incompleteCiphertext test.

Agreed. Also, please improve the name of this variable and ensure that you handle incomplete ciphertexts which are longer than a single zero byte (such as only a single byte short).

src/test/java/com/amazonaws/encryptionsdk/ParsedCiphertextTest.java

src/main/java/com/amazonaws/encryptionsdk/ParsedCiphertext.java

SalusaSecondus · 2019-08-06T18:40:23Z

src/main/java/com/amazonaws/encryptionsdk/ParsedCiphertext.java

@@ -1,11 +1,11 @@
 /*
 * Copyright 2016 Amazon.com, Inc. or its affiliates. All Rights Reserved.


Please update copyright.

src/main/java/com/amazonaws/encryptionsdk/ParsedCiphertext.java

SalusaSecondus · 2019-08-06T18:46:56Z

src/test/java/com/amazonaws/encryptionsdk/ParsedCiphertextTest.java

+import static org.mockito.Mockito.spy;
+
+public class ParsedCiphertextTest extends CiphertextHeaders {
+    final byte[] ciphertext_ = {0};


Agreed. Also, please improve the name of this variable and ensure that you handle incomplete ciphertexts which are longer than a single zero byte (such as only a single byte short).

…constructor

lavaleri

Here are the test cases which I think are important for the ParsedCiphertextTest file:

Test that you can construct a good ParsedCiphertext, and assert the following:

assertNotNull on the ciphertext field
assertNotNull on the offset

Test that after construction of a good ParsedCiphertext, the offset is a value that makes sense

assertEquals that the offset of the ParsedCiphertext is equal to the length of the ciphertext inputted on construction

Assert that constructing a ParsedCiphertext throws if you give it a single byte
Assert that constructing a ParsedCIphertext throws if you give it an almost valid ciphertext

Note that earlier I suggested adding a test to check that serialization/deserialization of the ParsedCiphertext works, but looking at this file again this isn't a useful test because we haven't changed behavior on deserialize or toArrayBytes.

@SalusaSecondus I've been off the Java train for awhile, am I missing any important test case here, or misrepresenting any Java testing conventions?

src/main/java/com/amazonaws/encryptionsdk/ParsedCiphertext.java

src/test/java/com/amazonaws/encryptionsdk/ParsedCiphertextTest.java

src/test/java/com/amazonaws/encryptionsdk/model/CiphertextHeadersTest.java

src/test/java/com/amazonaws/encryptionsdk/ParsedCiphertextTest.java

… a single byte and an almost valid ciphertext

lavaleri · 2019-08-16T18:52:02Z

Amendment to the above comment. For test (1) assert > 0 on the offset, not assertNotNull. For test (2) I was incorrect in thinking that the offset is the length of the ciphertext. It is the length of the ciphertext headers of the ciphertext you pass in. You do not need to include (2) in your next PR. For test (3), since again it is only parsing the headers of the ciphertext, you want to try to pass to it a ciphertext that almost has correct headers.

…cryption-sdk-java into ParsedCiphertext-bug-fix

lavaleri · 2019-08-16T19:47:14Z

src/test/java/com/amazonaws/encryptionsdk/ParsedCiphertextTest.java

+        final ParsedCiphertext pCt = new ParsedCiphertext(cipherText);
+
+        assertNotNull(pCt.getCiphertext());
+        assert pCt.getOffset() > 0;


Sorry I wasn't clear here, we still want to use junit. This should be assertTrue(pCT.getOffset()>0)

SalusaSecondus · 2019-08-16T20:22:59Z

src/test/java/com/amazonaws/encryptionsdk/ParsedCiphertextTest.java

+                masterKeyProvider,
+                plaintextBytes,
+                encryptionContext).getResult();
+        byte[] incompleteCipherText = Arrays.copyOfRange(cipherText, 1, cipherText.length);


This isn't testing what you think because it is dropping the first byte of the header while maintaining the result of the ciphertext. This will result in a complete but invalid) header. That isn't what you are trying to test.

Try this:

ParsedCiphertext parsed = new ParsedCiphertext(cipherText); byte[] incompleteCiphertext = Arrays.copyOf(parsed.getCiphertext(), parsed.getOffset() - 1);

lavaleri · 2019-08-16T20:30:48Z

src/test/java/com/amazonaws/encryptionsdk/ParsedCiphertextTest.java

+
+    @Test(expected = BadCiphertextException.class)
+    public void incompleteSingleByteCiphertext() {
+        final byte[] cipherText = {0};


With the fix we made to checking the version, we need to make sure this is a valid version (i.e. VersionInfo.CURRENT_CIPHERTEXT_VERSION).

SalusaSecondus

Looks good to me.

shayvana added 2 commits July 23, 2019 11:39

Adding check for if ciphertext is complete in the ParsedCiphertext co…

b3df03b

…nstructor and throwing a BadCiphertextException if it is not.

Wrote test for check for if ciphertext is complete in the ParsedCiphe…

62b36df

…rtext constructor

shayvana added 2 commits July 24, 2019 15:03

Fixed ParsedCiphertext bug

5ebec3e

Code does not break build

b9b64ed

lizroth requested a review from lavaleri July 30, 2019 20:33

lavaleri suggested changes Jul 30, 2019

View reviewed changes

shayvana added 3 commits July 31, 2019 11:49

Removed test in CiphertextHeadersTest

e326d12

Created test file for ParsedCiphertext and wrote a test for initializ…

85da442

…ing ParsedCiphertext with an incomplete ciphertext

Wrote happy test for ParsedCiphertext constructor

3aad539

lavaleri reviewed Aug 6, 2019

View reviewed changes

lavaleri requested a review from seebees August 6, 2019 18:36

SalusaSecondus suggested changes Aug 6, 2019

View reviewed changes

lavaleri removed the request for review from seebees August 6, 2019 18:52

shayvana added 4 commits August 12, 2019 12:32

Removed CiphertextHeadersTest file from changes

da0938b

Removed declaration of RuntimeException in a throws statement on the …

a0c93ec

…constructor

Wrote tests for incomplete ciphertexts of zero and single byte length

db1fc9d

Wrote test for encrypting and decrypting with ParsedCiphertext

39883c2

lavaleri suggested changes Aug 15, 2019

View reviewed changes

shayvana added 5 commits August 16, 2019 11:02

Added test for good ParsedCipherText

9590174

Added test cases for constructing an incomplete ParsedCipherText with…

ece073c

… a single byte and an almost valid ciphertext

Updated copyright

27a4459

Added back test file I accidentally deleted

fb23a71

Removed assertEquals check

5e5c2db

shayvana and others added 4 commits August 16, 2019 11:58

Asserted that offset is greater than 0

ff29cee

Merge branch 'master' into ParsedCiphertext-bug-fix

566975a

Removed unneeded import statement and CiphertextHeadersTest from changes

7b7b703

Merge branch 'ParsedCiphertext-bug-fix' of github.com:sprovine/aws-en…

d89ff00

…cryption-sdk-java into ParsedCiphertext-bug-fix

lavaleri suggested changes Aug 16, 2019

View reviewed changes

Updated offset check in good ciphertext test

22c3e7d

SalusaSecondus suggested changes Aug 16, 2019

View reviewed changes

lavaleri reviewed Aug 16, 2019

View reviewed changes

shayvana added 2 commits August 16, 2019 13:45

Updated test for almost valid ciphertext input for ParsedCipherText

3dd630c

Set incomplete single byte ciphertext to a valid version

07f60a6

lavaleri approved these changes Aug 16, 2019

View reviewed changes

SalusaSecondus approved these changes Aug 16, 2019

View reviewed changes

Merge branch 'master' into ParsedCiphertext-bug-fix

ad9f4af

SalusaSecondus merged commit f2daec3 into aws:master Oct 8, 2019

SalusaSecondus mentioned this pull request Oct 21, 2019

Truncated encrypted data throw NullPointerException #103

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ParsedCiphertext throws an error if it is not complete #119

ParsedCiphertext throws an error if it is not complete #119

shayvana commented Jul 24, 2019

shayvana commented Jul 24, 2019

lavaleri Aug 5, 2019

SalusaSecondus Aug 6, 2019

SalusaSecondus Aug 6, 2019

SalusaSecondus Aug 6, 2019

lavaleri left a comment

lavaleri commented Aug 16, 2019

lavaleri Aug 16, 2019

SalusaSecondus Aug 16, 2019

lavaleri Aug 16, 2019

SalusaSecondus left a comment

		@@ -1,11 +1,11 @@
		/*
		* Copyright 2016 Amazon.com, Inc. or its affiliates. All Rights Reserved.

ParsedCiphertext throws an error if it is not complete #119

ParsedCiphertext throws an error if it is not complete #119

Conversation

shayvana commented Jul 24, 2019

Check any applicable:

shayvana commented Jul 24, 2019

lavaleri Aug 5, 2019

Choose a reason for hiding this comment

SalusaSecondus Aug 6, 2019

Choose a reason for hiding this comment

SalusaSecondus Aug 6, 2019

Choose a reason for hiding this comment

SalusaSecondus Aug 6, 2019

Choose a reason for hiding this comment

lavaleri left a comment

Choose a reason for hiding this comment

lavaleri commented Aug 16, 2019

lavaleri Aug 16, 2019

Choose a reason for hiding this comment

SalusaSecondus Aug 16, 2019

Choose a reason for hiding this comment

lavaleri Aug 16, 2019

Choose a reason for hiding this comment

SalusaSecondus left a comment

Choose a reason for hiding this comment