Skip to content

Commit 79b9483

Browse files
slyubomirskyslyubomirsky
committed
Ensure KeyBlob treats field lengths as unsigned shorts, modify tests accordingly
1 parent f3bdc18 commit 79b9483

File tree

2 files changed

+59
-110
lines changed

2 files changed

+59
-110
lines changed

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

Lines changed: 24 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
import com.amazonaws.encryptionsdk.EncryptedDataKey;
2121
import com.amazonaws.encryptionsdk.exception.AwsCryptoException;
2222
import com.amazonaws.encryptionsdk.exception.ParseException;
23+
import com.amazonaws.encryptionsdk.internal.Constants;
2324
import com.amazonaws.encryptionsdk.internal.PrimitivesParser;
2425

2526
/**
@@ -41,11 +42,11 @@
4142
* </ol>
4243
*/
4344
public final class KeyBlob implements EncryptedDataKey {
44-
private short keyProviderIdLen_ = -1;
45+
private int keyProviderIdLen_ = -1;
4546
private byte[] keyProviderId_;
46-
private short keyProviderInfoLen_ = -1;
47+
private int keyProviderInfoLen_ = -1;
4748
private byte[] keyProviderInfo_;
48-
private short encryptedKeyLen_ = -1;
49+
private int encryptedKeyLen_ = -1;
4950
private byte[] encryptedKey_;
5051

5152
private boolean isComplete_ = false;
@@ -78,34 +79,6 @@ public KeyBlob(final EncryptedDataKey edk) {
7879
setKeyProviderInfo(edk.getProviderInformation());
7980
}
8081

81-
/**
82-
* Parses a field corresponding to the length of a byte array. It looks
83-
* for two bytes representing a short primitive type in the provided bytes
84-
* starting at the specified off.
85-
*
86-
* <p>
87-
* If successful, it returns the length read. On failure, it throws a parse exception.
88-
*
89-
* @param b
90-
* the byte array to parse.
91-
* @param off
92-
* the offset in the byte array to use when parsing.
93-
* @param name
94-
* the name of the field being parsed.
95-
* @return
96-
* the value parsed.
97-
* @throws ParseException
98-
* if there are not sufficient bytes to parse the length
99-
* or the length is negative
100-
*/
101-
private short parseFieldLength(final byte[] b, final int off, final String name) throws ParseException {
102-
short len = PrimitivesParser.parseShort(b, off);
103-
if (len < 0) {
104-
throw new ParseException("Parsed negative length for " + name);
105-
}
106-
return len;
107-
}
108-
10982
/**
11083
* Parse the key provider identifier length in the provided bytes. It looks
11184
* for 2 bytes representing a short primitive type in the provided bytes
@@ -124,10 +97,10 @@ private short parseFieldLength(final byte[] b, final int off, final String name)
12497
* primitive.
12598
* @throws ParseException
12699
* if there are not sufficient bytes to parse the identifier
127-
* length or the length is negative.
100+
* length.
128101
*/
129102
private int parseKeyProviderIdLen(final byte[] b, final int off) throws ParseException {
130-
keyProviderIdLen_ = parseFieldLength(b, off, "key provider id");
103+
keyProviderIdLen_ = PrimitivesParser.parseUnsignedShort(b, off);
131104
return Short.SIZE / Byte.SIZE;
132105
}
133106

@@ -178,10 +151,10 @@ private int parseKeyProviderId(final byte[] b, final int off) throws ParseExcept
178151
* primitive type.
179152
* @throws ParseException
180153
* if there are not sufficient bytes to parse the provider info
181-
* length or the length is negative.
154+
* length.
182155
*/
183156
private int parseKeyProviderInfoLen(final byte[] b, final int off) throws ParseException {
184-
keyProviderInfoLen_ = parseFieldLength(b, off, "key provider info");
157+
keyProviderInfoLen_ = PrimitivesParser.parseUnsignedShort(b, off);
185158
return Short.SIZE / Byte.SIZE;
186159
}
187160

@@ -234,7 +207,7 @@ private int parseKeyProviderInfo(final byte[] b, final int off) throws ParseExce
234207
* if there are not sufficient bytes to parse the key length.
235208
*/
236209
private int parseKeyLen(final byte[] b, final int off) throws ParseException {
237-
encryptedKeyLen_ = parseFieldLength(b, off, "key");
210+
encryptedKeyLen_ = PrimitivesParser.parseUnsignedShort(b, off);
238211
return Short.SIZE / Byte.SIZE;
239212
}
240213

@@ -331,13 +304,13 @@ public byte[] toByteArray() {
331304
final int outLen = 3 * (Short.SIZE / Byte.SIZE) + keyProviderIdLen_ + keyProviderInfoLen_ + encryptedKeyLen_;
332305
final ByteBuffer out = ByteBuffer.allocate(outLen);
333306

334-
out.putShort(keyProviderIdLen_);
307+
out.putShort((short) keyProviderIdLen_);
335308
out.put(keyProviderId_, 0, keyProviderIdLen_);
336309

337-
out.putShort(keyProviderInfoLen_);
310+
out.putShort((short) keyProviderInfoLen_);
338311
out.put(keyProviderInfo_, 0, keyProviderInfoLen_);
339312

340-
out.putShort(encryptedKeyLen_);
313+
out.putShort((short) encryptedKeyLen_);
341314
out.put(encryptedKey_, 0, encryptedKeyLen_);
342315

343316
return out.array();
@@ -361,7 +334,7 @@ public boolean isComplete() {
361334
* @return
362335
* the length of the key provider identifier.
363336
*/
364-
public short getKeyProviderIdLen() {
337+
public int getKeyProviderIdLen() {
365338
return keyProviderIdLen_;
366339
}
367340

@@ -382,7 +355,7 @@ public String getProviderId() {
382355
* @return
383356
* the length of the key provider info.
384357
*/
385-
public short getKeyProviderInfoLen() {
358+
public int getKeyProviderInfoLen() {
386359
return keyProviderInfoLen_;
387360
}
388361

@@ -403,7 +376,7 @@ public byte[] getProviderInformation() {
403376
* @return
404377
* the length of the encrypted data key.
405378
*/
406-
public short getEncryptedDataKeyLen() {
379+
public int getEncryptedDataKeyLen() {
407380
return encryptedKeyLen_;
408381
}
409382

@@ -426,12 +399,12 @@ public byte[] getEncryptedDataKey() {
426399
*/
427400
public void setKeyProviderId(final String keyProviderId) {
428401
final byte[] keyProviderIdBytes = keyProviderId.getBytes(StandardCharsets.UTF_8);
429-
if (keyProviderIdBytes.length > Short.MAX_VALUE) {
402+
if (keyProviderIdBytes.length > Constants.UNSIGNED_SHORT_MAX_VAL) {
430403
throw new AwsCryptoException(
431-
"Key provider identifier length exceeds the max value of a short primitive.");
404+
"Key provider identifier length exceeds the max value of an unsigned short primitive.");
432405
}
433406
keyProviderId_ = keyProviderIdBytes;
434-
keyProviderIdLen_ = (short) keyProviderId_.length;
407+
keyProviderIdLen_ = keyProviderId_.length;
435408
}
436409

437410
/**
@@ -442,12 +415,12 @@ public void setKeyProviderId(final String keyProviderId) {
442415
* identifier.
443416
*/
444417
public void setKeyProviderInfo(final byte[] keyProviderInfo) {
445-
if (keyProviderInfo.length > Short.MAX_VALUE) {
418+
if (keyProviderInfo.length > Constants.UNSIGNED_SHORT_MAX_VAL) {
446419
throw new AwsCryptoException(
447-
"Key provider identifier information length exceeds the max value of a short primitive.");
420+
"Key provider identifier information length exceeds the max value of an unsigned short primitive.");
448421
}
449422
keyProviderInfo_ = keyProviderInfo.clone();
450-
keyProviderInfoLen_ = (short) keyProviderInfo.length;
423+
keyProviderInfoLen_ = keyProviderInfo.length;
451424
}
452425

453426
/**
@@ -457,10 +430,10 @@ public void setKeyProviderInfo(final byte[] keyProviderInfo) {
457430
* the bytes containing the encrypted data key.
458431
*/
459432
public void setEncryptedDataKey(final byte[] encryptedDataKey) {
460-
if (encryptedDataKey.length > Short.MAX_VALUE) {
461-
throw new AwsCryptoException("Key length exceeds the max value of a short primitive.");
433+
if (encryptedDataKey.length > Constants.UNSIGNED_SHORT_MAX_VAL) {
434+
throw new AwsCryptoException("Key length exceeds the max value of an unsigned short primitive.");
462435
}
463436
encryptedKey_ = encryptedDataKey.clone();
464-
encryptedKeyLen_ = (short) encryptedKey_.length;
437+
encryptedKeyLen_ = encryptedKey_.length;
465438
}
466439
}

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

Lines changed: 35 additions & 59 deletions
Original file line numberDiff line numberDiff line change
@@ -15,10 +15,9 @@
1515

1616
import static org.junit.Assert.assertArrayEquals;
1717
import static org.junit.Assert.assertEquals;
18-
import static org.junit.Assert.assertFalse;
1918

20-
import java.nio.ByteBuffer;
2119
import java.nio.charset.StandardCharsets;
20+
import java.util.Arrays;
2221
import java.util.HashMap;
2322
import java.util.Map;
2423

@@ -28,6 +27,7 @@
2827
import com.amazonaws.encryptionsdk.CryptoAlgorithm;
2928
import com.amazonaws.encryptionsdk.DataKey;
3029
import com.amazonaws.encryptionsdk.exception.AwsCryptoException;
30+
import com.amazonaws.encryptionsdk.internal.Constants;
3131
import com.amazonaws.encryptionsdk.internal.RandomBytesGenerator;
3232
import com.amazonaws.encryptionsdk.internal.StaticMasterKey;
3333

@@ -78,7 +78,7 @@ public void overlyLargeKeyProviderIdLen() {
7878

7979
final DataKey<StaticMasterKey> mockDataKey = masterKeyProvider_.generateDataKey(ALGORITHM, encryptionContext);
8080

81-
final int providerId_Len = Short.MAX_VALUE + 1;
81+
final int providerId_Len = Constants.UNSIGNED_SHORT_MAX_VAL + 1;
8282
final byte[] providerId_Bytes = RandomBytesGenerator.generate(providerId_Len);
8383
final String providerId_ = new String(providerId_Bytes, StandardCharsets.UTF_8);
8484

@@ -95,15 +95,15 @@ public void overlyLargeKeyProviderInfoLen() {
9595

9696
final DataKey<StaticMasterKey> mockDataKey = masterKeyProvider_.generateDataKey(ALGORITHM, encryptionContext);
9797

98-
final int providerInfo_Len = Short.MAX_VALUE + 1;
98+
final int providerInfo_Len = Constants.UNSIGNED_SHORT_MAX_VAL + 1;
9999
final byte[] providerInfo_ = RandomBytesGenerator.generate(providerInfo_Len);
100100

101101
new KeyBlob(providerId_, providerInfo_, mockDataKey.getEncryptedDataKey());
102102
}
103103

104104
@Test(expected = AwsCryptoException.class)
105105
public void overlyLargeKey() {
106-
final int keyLen = Short.MAX_VALUE + 1;
106+
final int keyLen = Constants.UNSIGNED_SHORT_MAX_VAL + 1;
107107
final byte[] encryptedKeyBytes = RandomBytesGenerator.generate(keyLen);
108108

109109
new KeyBlob(providerId_, providerInfo_.getBytes(StandardCharsets.UTF_8), encryptedKeyBytes);
@@ -173,75 +173,51 @@ public void checkKeyLen() {
173173
assertEquals(mockDataKey_.getEncryptedDataKey().length, reconstructedKeyBlob.getEncryptedDataKeyLen());
174174
}
175175

176-
private byte[] negativeKeyProviderIdLenTestVector() {
177-
// key provider id len of -1, key provider info len of 2, and key len of 3
178-
return new byte[]{
179-
(byte)0xff, (byte)0xff, (byte)0x01, (byte)0x00, (byte)0x02, (byte)0x02, (byte)0x03,
180-
(byte)0x00, (byte)0x03, (byte)0x04, (byte)0x05, (byte)0x06
181-
};
182-
}
183-
184-
private byte[] negativeKeyProviderInfoLenTestVector() {
185-
// key provider id len of 1, key provider info len of -2, key len of 3
186-
return new byte[] {
187-
(byte)0x00, (byte)0x01, (byte)0x01, (byte)0xff, (byte)0xfe, (byte)0x02, (byte)0x03,
188-
(byte)0x00, (byte)0x03, (byte)0x04, (byte)0x05, (byte)0x06
189-
};
190-
}
176+
private KeyBlob generateRandomKeyBlob(int idLen, int infoLen, int keyLen) {
177+
final byte[] idBytes = RandomBytesGenerator.generate(idLen);
178+
// negative bytes translate into U+FFFD, so no thanks
179+
for (int i = 0; i < idBytes.length; i++) {
180+
if (idBytes[i] < 0) {
181+
idBytes[i] = (byte) (idBytes[i] - Byte.MIN_VALUE);
182+
}
183+
}
184+
final byte[] infoBytes = RandomBytesGenerator.generate(infoLen);
185+
final byte[] keyBytes = RandomBytesGenerator.generate(keyLen);
191186

192-
private byte[] negativeKeyLenTestVector() {
193-
// key provider id len of 1, key provider info len of 2, key len of -3
194-
return new byte[] {
195-
(byte)0x00, (byte)0x01, (byte)0x01, (byte)0x00, (byte)0x00, (byte)0x02, (byte)0x03,
196-
(byte)0xff, (byte)0xfd, (byte)0x04, (byte)0x05, (byte)0x06
197-
};
187+
return new KeyBlob(new String(idBytes, StandardCharsets.UTF_8), infoBytes, keyBytes);
198188
}
199189

200-
private void assertIncomplete(final byte[] vector) {
201-
assertFalse(deserialize(vector).isComplete());
190+
private void assertKeyBlobsEqual(KeyBlob b1, KeyBlob b2) {
191+
assertArrayEquals(b1.getProviderId().getBytes(StandardCharsets.UTF_8),
192+
b2.getProviderId().getBytes(StandardCharsets.UTF_8));
193+
assertArrayEquals(b1.getProviderInformation(), b2.getProviderInformation());
194+
assertArrayEquals(b1.getEncryptedDataKey(), b2.getEncryptedDataKey());
202195
}
203196

204197
@Test
205-
public void checkNegativeKeyProviderIdLen() {
206-
final byte[] keyBlobBytes = createKeyBlobBytes();
207-
208-
// manually set the keyProviderIdLen to negative
209-
final byte[] negativeKeyProviderIdLen = ByteBuffer.allocate(Short.BYTES)
210-
.putShort((short) -1).array();
211-
System.arraycopy(negativeKeyProviderIdLen, 0, keyBlobBytes, 0, Short.BYTES);
198+
public void checkKeyProviderIdLenUnsigned() {
199+
// provider id length is too large for a signed short but fits in unsigned
200+
final KeyBlob blob = generateRandomKeyBlob(Short.MAX_VALUE + 1, Short.MAX_VALUE, Short.MAX_VALUE);
201+
final byte[] arr = blob.toByteArray();
212202

213-
// a negative field length throws a parse exception, so deserialization is incomplete
214-
assertIncomplete(keyBlobBytes);
215-
assertIncomplete(negativeKeyProviderIdLenTestVector());
203+
assertKeyBlobsEqual(deserialize(arr), blob);
216204
}
217205

218206
@Test
219-
public void checkNegativeKeyProviderInfoLen() {
220-
final byte[] keyBlobBytes = createKeyBlobBytes();
221-
222-
// manually set the keyProviderInfoLen to negative
223-
final byte[] negativeKeyProviderInfoLen = ByteBuffer.allocate(Short.BYTES)
224-
.putShort((short) -1).array();
225-
int offset = Short.BYTES + providerId_.length();
226-
System.arraycopy(negativeKeyProviderInfoLen, 0, keyBlobBytes, offset, Short.BYTES);
207+
public void checkKeyProviderInfoLenUnsigned() {
208+
// provider info length is too large for a signed short but fits in unsigned
209+
final KeyBlob blob = generateRandomKeyBlob(Short.MAX_VALUE, Short.MAX_VALUE + 2, Short.MAX_VALUE);
210+
final byte[] arr = blob.toByteArray();
227211

228-
// a negative field length throws a parse exception, so deserialization is incomplete
229-
assertIncomplete(keyBlobBytes);
230-
assertIncomplete(negativeKeyProviderInfoLenTestVector());
212+
assertKeyBlobsEqual(deserialize(arr), blob);
231213
}
232214

233215
@Test
234216
public void checkNegativeKeyLen() {
235-
final byte[] keyBlobBytes = createKeyBlobBytes();
236-
237-
// we will manually set the keyLen to negative
238-
final byte[] negativeKeyLen = ByteBuffer.allocate(Short.BYTES)
239-
.putShort((short) -1).array();
240-
int offset = Short.BYTES + providerId_.length() + Short.BYTES + providerInfo_.length();
241-
System.arraycopy(negativeKeyLen, 0, keyBlobBytes, offset, Short.BYTES);
217+
// key length is too large for a signed short but fits in unsigned
218+
final KeyBlob blob = generateRandomKeyBlob(Short.MAX_VALUE, Short.MAX_VALUE, Short.MAX_VALUE + 3);
219+
final byte[] arr = blob.toByteArray();
242220

243-
// negative key len throws parse exception so deserialization is incomplete
244-
assertIncomplete(keyBlobBytes);
245-
assertIncomplete(negativeKeyLenTestVector());
221+
assertKeyBlobsEqual(deserialize(arr), blob);
246222
}
247223
}

0 commit comments

Comments
 (0)