Keyblob should parse field lengths as unsigned shorts #71

slyubomirsky · 2018-08-28T20:58:50Z

Issue #, if available:
No number

Description of changes:
Done at the request of Rustan Leino: Currently, if the KeyBlob deserializer reads a negative length for the key provider ID, key provider info, or the key itself, this does not directly result in an exception. Instead, when the deserializer later tries to read the provider ID, provider info, or key, Arrays.copyOfRange() throws an IllegalArgumentException. From a client's perspective, this seems like a misleading exception, since the client passed all the correct arguments but the message's contents are what led to the exception. This change instead throws a ParseException if a negative length is read, corresponding to the line "the bytes does not properly represent the desired object" in the Javadoc for ParseException.

Unit tests are included for the situation described. The unit tests throw an IllegalArgumentException in the present version of the code; with the changes, they terminate normally but without deserializing the field that had a negative length. (This happens because deserialize() catches the ParseException. That may not be desired behavior in that case, since a client might retry deserialize(), not knowing that the message contents are what led to the failure to deserialize and thus never succeeding. Perhaps deserialize() should distinguish between bad message bodies versus not having enough bytes with different exceptions?)

As noted in the below conversation, according to https://docs.aws.amazon.com/encryption-sdk/latest/developer-guide/message-format.html#data-key-provider-id, the field lengths in KeyBlob should be unsigned shorts but were being parsed as signed shorts. This was leading to exceptions if a "negative" length was parsed, when the data format specification states that 2-byte unsigned values should be permitted.

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

mattsb42-aws · 2018-08-28T21:03:29Z

src/main/java/com/amazonaws/encryptionsdk/model/KeyBlob.java

@@ -98,7 +98,11 @@ public KeyBlob(final EncryptedDataKey edk) {
     *             length.
     */
    private int parseKeyProviderIdLen(final byte[] b, final int off) throws ParseException {
-        keyProviderIdLen_ = PrimitivesParser.parseShort(b, off);
+        short len = PrimitivesParser.parseShort(b, off);


These methods are all doing the same thing, just with different error messages. This should be centralized into a single helper method that all three use.

mattsb42-aws · 2018-08-28T21:05:07Z

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

@@ -170,4 +172,51 @@ public void checkKeyLen() {

        assertEquals(mockDataKey_.getEncryptedDataKey().length, reconstructedKeyBlob.getEncryptedDataKeyLen());
    }
+


In addition to the direct method calls, we should have a static test vector for each of these that contains bytes that will trigger the correct error when parsed.

How should these vectors be provided? As a byte array literal? Are there similar tests that contain static test vectors, for comparison?

We're working on building out more standardized approaches for these, but for the time being, yes, just some byte array literals will do nicely.

I added the vectors as functions returning byte array literals, but wasn't sure if a field would be preferable. I would be glad to present them in whatever manner would be most useful.

mattsb42-aws · 2018-08-29T17:32:06Z

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

+
+	// a negative field length throws a parse exception, so deserialization is incomplete
+	assertIncomplete(keyBlobBytes);
+	assertIncomplete(negativeKeyProviderIdLenTestVector());


Sorry, I should have clarified. What I'm looking for is a separate test that calls the full deserialize path on a byte array crafted to contain the bad values.

That being said, the vectors you provided triggered paths in my brain that I had skipped over, that change what this issue is.

Per the message format spec[1], every one of these values is an unsigned integer. What would be better to be testing is to test the methods in PrimitivesParser to verify that none of them can result in negative values.

[1] https://docs.aws.amazon.com/encryption-sdk/latest/developer-guide/message-format.html

I see; in that case, there are methods for parsing unsigned values in PrimitivesParser that should be used here

…accordingly

slyubomirsky · 2018-08-29T20:11:46Z

Static test vectors pose more of an issue with the changes as they stand now, as they would have to be quite large to trigger the case here. Is there any particularly convenient way to provide one in this case? I would appreciate any recommendation

mattsb42-aws

LGTM but I'd like to get one of our Java folks to look at it too.

johnwalker · 2018-08-30T19:53:03Z

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

+    @Test
+    public void checkKeyProviderIdLenUnsigned() {
+        // provider id length is too large for a signed short but fits in unsigned
+        final KeyBlob blob = generateRandomKeyBlob(Short.MAX_VALUE + 1, Short.MAX_VALUE, Short.MAX_VALUE);


Why use Short.MAX_VALUE instead of the UNSIGNED_SHORT_MAX_VAL?

It was an arbitrary choice (just needed it to be too large to be a positive signed short but a valid unsigned short), I can change it to make the intent of the test clearer

SalusaSecondus · 2018-08-30T19:55:10Z

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

+
+    private KeyBlob generateRandomKeyBlob(int idLen, int infoLen, int keyLen) {
+        final byte[] idBytes = RandomBytesGenerator.generate(idLen);
+        // negative bytes translate into U+FFFD, so no thanks


If this array is intended to be parsed as UTF-8, then you shouldn't just generate random bytes (even with some amount of checks). Why not just fill it with 'A'?

SalusaSecondus · 2018-08-30T19:56:31Z

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

+    }
+
+    @Test
+    public void checkKeyLenUnsigned() {


Can you add a test for when it is too long to fit even in an unsigned short?

Good idea; I will

It occurs to me now that such tests already exist, namely overlyLargeKeyProviderIdLen(), overlyLargeKeyProviderInfoLen(), and overlyLargeKey(). They check that instantiating such a KeyBlob results in an exception. (I had changed them earlier to make sure they were treating Constants.UNSIGNED_SHORT_MAX_VAL as the max length.)

…h max unsigned short value

slyubomirsky · 2018-08-31T00:08:41Z

Note: Technically a char could be used to store these unsigned 16-bit values, since the Java language standard guarantees that chars must be unsigned. This could be useful to have a compiler-provided guarantee that these lengths are non-negative, though admittedly a little strange.

Steven Lyubomirsky added 2 commits August 28, 2018 13:19

Have KeyBlob throw ParseException if a parsed field length is negative

8da3e2c

Add unit test for case of message containing a negative length

a9deee3

mattsb42-aws requested changes Aug 28, 2018

View reviewed changes

slyubomirsky added 2 commits August 28, 2018 14:22

Factor out negative length check into a helper function

0f7f1e5

Add test vectors for KeyBlob negative field tests

f3bdc18

mattsb42-aws reviewed Aug 29, 2018

View reviewed changes

Ensure KeyBlob treats field lengths as unsigned shorts, modify tests …

79b9483

…accordingly

slyubomirsky changed the title ~~Keyblob parse exception~~ Keyblob should parse field lengths as unsigned shorts Aug 29, 2018

slyubomirsky added 3 commits August 29, 2018 13:24

Whitespace fix

c94dc6b

Fix test indents

80625d7

Fix keyblob test name, remove redundant include

8552c32

mattsb42-aws approved these changes Aug 30, 2018

View reviewed changes

johnwalker reviewed Aug 30, 2018

View reviewed changes

SalusaSecondus suggested changes Aug 30, 2018

View reviewed changes

KeyBlobTest: don't use random bytes to make strings; test lengths wit…

7b4b616

…h max unsigned short value

slyubomirsky mentioned this pull request Sep 1, 2018

JML Specifications for KeyBlob #72

Merged

SalusaSecondus approved these changes Sep 5, 2018

View reviewed changes

SalusaSecondus merged commit 7344d35 into aws:master Sep 5, 2018

		@@ -170,4 +172,51 @@ public void checkKeyLen() {

		assertEquals(mockDataKey_.getEncryptedDataKey().length, reconstructedKeyBlob.getEncryptedDataKeyLen());
		}

Keyblob should parse field lengths as unsigned shorts #71

Keyblob should parse field lengths as unsigned shorts #71

Uh oh!

Conversation

slyubomirsky commented Aug 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slyubomirsky Aug 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slyubomirsky Aug 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slyubomirsky Aug 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slyubomirsky commented Aug 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattsb42-aws left a comment

Choose a reason for hiding this comment

Uh oh!

johnwalker Aug 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slyubomirsky commented Aug 31, 2018

Uh oh!

Uh oh!

slyubomirsky commented Aug 28, 2018 •

edited

Loading

slyubomirsky Aug 28, 2018 •

edited

Loading

slyubomirsky Aug 29, 2018 •

edited

Loading

slyubomirsky Aug 29, 2018 •

edited

Loading

slyubomirsky commented Aug 29, 2018 •

edited

Loading

johnwalker Aug 30, 2018 •

edited

Loading