fix: The final frame can not be larger than the Frame Length #166

WesleyRosenblum · 2020-03-27T00:01:11Z

Add validation to ensure the length of the final frame in the final frame header does not exceed the frame size specified in the message header.

See aws/aws-encryption-sdk-javascript#281

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.

frame header does not exceed the frame size specified in the message header.

seebees · 2020-03-27T00:04:32Z

src/main/java/com/amazonaws/encryptionsdk/internal/FrameDecryptionHandler.java

@@ -133,6 +133,11 @@ public ProcessingSummary processBytes(final byte[] in, final int off, final int
                int protectedContentLen = -1;
                if (currentFrameHeaders_.isFinalFrame()) {
                    protectedContentLen = currentFrameHeaders_.getFrameContentLength();
+
+                    // The final frame should not be able to exceed the frameLength
+                    if(frameSize_ > 0 && protectedContentLen > frameSize_) {


frame size can be 0 and the final frame content length can be strictly equal to the frame size.

I'm only throwing the exception if the final frame size is strictly greater than the frame size. If the frame size is 0, then I can't compare the last frame size so I don't throw the exception in that case.

According to

aws-encryption-sdk-java/src/main/java/com/amazonaws/encryptionsdk/internal/FrameDecryptionHandler.java

Line 122 in 163fd88

if (frameSize_ == 0) {

frameSize_ can be 0, but from https://docs.aws.amazon.com/encryption-sdk/latest/developer-guide/message-format.html#header-frame-length it is not the intention of the frameLength to be 0 in the header.

@SalusaSecondus do you know if there a reason we allow the frame size in the header to be 0 even for framed data?

This code should never execute if the frame length is 0.

encryption:

aws-encryption-sdk-java/src/main/java/com/amazonaws/encryptionsdk/internal/EncryptionHandler.java

Lines 120 to 146 in 163fd88

ContentType contentType;

if (frameSize > 0) {

contentType = ContentType.FRAME;

} else if (frameSize == 0) {

contentType = ContentType.SINGLEBLOCK;

} else {

throw Utils.cannotBeNegative("Frame size");

}

final CiphertextHeaders unsignedHeaders = createCiphertextHeaders(contentType, frameSize);

try {

encryptionKey_ = cryptoAlgo_.getEncryptionKeyFromDataKey(result.getCleartextDataKey(), unsignedHeaders);

} catch (final InvalidKeyException ex) {

throw new AwsCryptoException(ex);

}

ciphertextHeaders_ = signCiphertextHeaders(unsignedHeaders);

ciphertextHeaderBytes_ = ciphertextHeaders_.toByteArray();

byte[] messageId_ = ciphertextHeaders_.getMessageId();

switch (contentType) {

case FRAME:

contentCryptoHandler_ = new FrameEncryptionHandler(encryptionKey_, nonceLen_, cryptoAlgo_, messageId_,

frameSize);

break;

case SINGLEBLOCK:

contentCryptoHandler_ = new BlockEncryptionHandler(encryptionKey_, nonceLen_, cryptoAlgo_, messageId_);

break;

decryption:

aws-encryption-sdk-java/src/main/java/com/amazonaws/encryptionsdk/internal/DecryptionHandler.java

Lines 497 to 505 in 163fd88

switch (contentType) {

case FRAME:

contentCryptoHandler_ = new FrameDecryptionHandler(decryptionKey_, (byte) nonceLen, cryptoAlgo_,

messageId, frameLen);

break;

case SINGLEBLOCK:

contentCryptoHandler_ = new BlockDecryptionHandler(decryptionKey_, (byte) nonceLen, cryptoAlgo_,

messageId);

break;

The comment on the quoted lines makes me think that this was something that was added in a long time ago, possibly for a very early version of the message format because the frame size is not included in the frame header.

aws-encryption-sdk-java/src/main/java/com/amazonaws/encryptionsdk/internal/FrameDecryptionHandler.java

Lines 123 to 124 in 163fd88

// if frame size in ciphertext headers is 0, the frame size

// will need to be parsed in individual frame headers.

Is it possible for the ciphertext to be corrupted/manipulated in some way that leaves the frame size as 0, or would that be caught upstream and cause decryption to fail?

I've added validation to ensure the frame size is not zero if the data is framed

From discussing with @SalusaSecondus, frame size = 0 in the header was intended to indicate a variable length encoding. This feature was not fully implemented and is not documented, so it should be removed, but there are more remnants of it scattered in other parts of the code, so I've opened #167 to track that. For this PR I will only validate the final frame length if the header frame length is > 0

mattsb42-aws

We also need a check on the encrypt path. Arguably, that's the more important path to cover.

mattsb42-aws · 2020-03-27T19:44:59Z

src/main/java/com/amazonaws/encryptionsdk/internal/FrameDecryptionHandler.java

@@ -133,6 +133,11 @@ public ProcessingSummary processBytes(final byte[] in, final int off, final int
                int protectedContentLen = -1;
                if (currentFrameHeaders_.isFinalFrame()) {
                    protectedContentLen = currentFrameHeaders_.getFrameContentLength();
+
+                    // The final frame should not be able to exceed the frameLength
+                    if(frameSize_ > 0 && protectedContentLen > frameSize_) {


This code should never execute if the frame length is 0.

encryption:

aws-encryption-sdk-java/src/main/java/com/amazonaws/encryptionsdk/internal/EncryptionHandler.java

Lines 120 to 146 in 163fd88

ContentType contentType;

if (frameSize > 0) {

contentType = ContentType.FRAME;

} else if (frameSize == 0) {

contentType = ContentType.SINGLEBLOCK;

} else {

throw Utils.cannotBeNegative("Frame size");

}

final CiphertextHeaders unsignedHeaders = createCiphertextHeaders(contentType, frameSize);

try {

encryptionKey_ = cryptoAlgo_.getEncryptionKeyFromDataKey(result.getCleartextDataKey(), unsignedHeaders);

} catch (final InvalidKeyException ex) {

throw new AwsCryptoException(ex);

}

ciphertextHeaders_ = signCiphertextHeaders(unsignedHeaders);

ciphertextHeaderBytes_ = ciphertextHeaders_.toByteArray();

byte[] messageId_ = ciphertextHeaders_.getMessageId();

switch (contentType) {

case FRAME:

contentCryptoHandler_ = new FrameEncryptionHandler(encryptionKey_, nonceLen_, cryptoAlgo_, messageId_,

frameSize);

break;

case SINGLEBLOCK:

contentCryptoHandler_ = new BlockEncryptionHandler(encryptionKey_, nonceLen_, cryptoAlgo_, messageId_);

break;

decryption:

aws-encryption-sdk-java/src/main/java/com/amazonaws/encryptionsdk/internal/DecryptionHandler.java

Lines 497 to 505 in 163fd88

switch (contentType) {

case FRAME:

contentCryptoHandler_ = new FrameDecryptionHandler(decryptionKey_, (byte) nonceLen, cryptoAlgo_,

messageId, frameLen);

break;

case SINGLEBLOCK:

contentCryptoHandler_ = new BlockDecryptionHandler(decryptionKey_, (byte) nonceLen, cryptoAlgo_,

messageId);

break;

The comment on the quoted lines makes me think that this was something that was added in a long time ago, possibly for a very early version of the message format because the frame size is not included in the frame header.

aws-encryption-sdk-java/src/main/java/com/amazonaws/encryptionsdk/internal/FrameDecryptionHandler.java

Lines 123 to 124 in 163fd88

// if frame size in ciphertext headers is 0, the frame size

// will need to be parsed in individual frame headers.

WesleyRosenblum · 2020-03-27T20:07:16Z

We also need a check on the encrypt path. Arguably, that's the more important path to cover.

I thought the decrypt path was needed because we are deserializing data that may have been manipulated or corrupted somehow. The encrypt path is in our control so as long we trust ourselves to write the correct size frame for the final frame I don't see why the check is necessary?

SalusaSecondus · 2020-04-02T16:51:10Z

src/main/java/com/amazonaws/encryptionsdk/internal/FrameDecryptionHandler.java

-                if (frameSize_ == 0) {
-                    // if frame size in ciphertext headers is 0, the frame size
-                    // will need to be parsed in individual frame headers.
-                    currentFrameHeaders_.includeFrameSize(true);
-                }


Why are we removing this logic?

After discussion, I'll revert this part of the change and have created a separate issue to track removing this code (which was a partially implemented variable frame length feature): #167

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

SalusaSecondus

LGTM

Will address frame lenghth = 0 in #167

* Add a basic example for encrypting and decrypting with a KMS CMK (#136) * *Issue #, if available:* #108 *Description of changes:* Add a basic example for encrypting and decrypting with a KMS CMK. 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. * Add test and Maven plugin to include examples directory as test source * Update docs in prep for 1.6.1 (#133) * Update docs in prep for 1.6.1 * Actually bump version for release * Fix for new versions of gpg * Refactor JceMasterKey to extract logic to be shared by raw keyrings. (#139) * Refactor JceMasterKey to extract logic to be shared by raw keyrings. *Issue #, if available:* #102 *Description of changes:* In anticipation of the RawAesKeyring and RawRsaKeyring needing logic currently embedded in the JceMasterKey, this change extracts that logic into the JceKeyCipher class so it may be shared. By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. - [ ] Were any files moved? Moving files changes their URL, which breaks all hyperlinks to the files. * fix: The final frame can not be larger than the Frame Length (#166) * Add validation to ensure the length of the final frame in the final frame header does not exceed the frame size specified in the message header. * Validate that frame length is positive for framed data * Reverting removal of variable frame length code * Reverting removal of variable frame length code * Fix spacing after if Co-authored-by: SalusaSecondus <[email protected]> Co-authored-by: Greg Rubin <[email protected]>

Add validation to ensure the length of the final frame in the final

163fd88

frame header does not exceed the frame size specified in the message header.

WesleyRosenblum requested review from SalusaSecondus and seebees March 27, 2020 00:01

seebees previously requested changes Mar 27, 2020

View reviewed changes

mattsb42-aws previously requested changes Mar 27, 2020

View reviewed changes

Validate that frame length is positive for framed data

b58b5c1

SalusaSecondus suggested changes Apr 2, 2020

View reviewed changes

WesleyRosenblum added 3 commits April 2, 2020 10:40

Reverting removal of variable frame length code

5346309

Reverting removal of variable frame length code

b8d754e

Fix spacing after if

6e38615

SalusaSecondus approved these changes Apr 2, 2020

View reviewed changes

WesleyRosenblum merged commit dcbc562 into master Apr 7, 2020

WesleyRosenblum deleted the framelength branch April 7, 2020 18:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: The final frame can not be larger than the Frame Length #166

fix: The final frame can not be larger than the Frame Length #166

WesleyRosenblum commented Mar 27, 2020

seebees Mar 27, 2020

WesleyRosenblum Mar 27, 2020

seebees Mar 27, 2020

WesleyRosenblum Mar 27, 2020 •

edited

Loading

mattsb42-aws Mar 27, 2020

WesleyRosenblum Mar 27, 2020

WesleyRosenblum Mar 27, 2020

WesleyRosenblum Apr 2, 2020

mattsb42-aws left a comment

mattsb42-aws Mar 27, 2020

WesleyRosenblum commented Mar 27, 2020

SalusaSecondus Apr 2, 2020

WesleyRosenblum Apr 2, 2020

SalusaSecondus left a comment

	ContentType contentType;
	if (frameSize > 0) {
	contentType = ContentType.FRAME;
	} else if (frameSize == 0) {
	contentType = ContentType.SINGLEBLOCK;
	} else {
	throw Utils.cannotBeNegative("Frame size");
	}

	final CiphertextHeaders unsignedHeaders = createCiphertextHeaders(contentType, frameSize);
	try {
	encryptionKey_ = cryptoAlgo_.getEncryptionKeyFromDataKey(result.getCleartextDataKey(), unsignedHeaders);
	} catch (final InvalidKeyException ex) {
	throw new AwsCryptoException(ex);
	}
	ciphertextHeaders_ = signCiphertextHeaders(unsignedHeaders);
	ciphertextHeaderBytes_ = ciphertextHeaders_.toByteArray();
	byte[] messageId_ = ciphertextHeaders_.getMessageId();

	switch (contentType) {
	case FRAME:
	contentCryptoHandler_ = new FrameEncryptionHandler(encryptionKey_, nonceLen_, cryptoAlgo_, messageId_,
	frameSize);
	break;
	case SINGLEBLOCK:
	contentCryptoHandler_ = new BlockEncryptionHandler(encryptionKey_, nonceLen_, cryptoAlgo_, messageId_);
	break;

	switch (contentType) {
	case FRAME:
	contentCryptoHandler_ = new FrameDecryptionHandler(decryptionKey_, (byte) nonceLen, cryptoAlgo_,
	messageId, frameLen);
	break;
	case SINGLEBLOCK:
	contentCryptoHandler_ = new BlockDecryptionHandler(decryptionKey_, (byte) nonceLen, cryptoAlgo_,
	messageId);
	break;

	// if frame size in ciphertext headers is 0, the frame size
	// will need to be parsed in individual frame headers.

fix: The final frame can not be larger than the Frame Length #166

fix: The final frame can not be larger than the Frame Length #166

Conversation

WesleyRosenblum commented Mar 27, 2020

Check any applicable:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WesleyRosenblum Mar 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattsb42-aws left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WesleyRosenblum commented Mar 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SalusaSecondus left a comment

Choose a reason for hiding this comment

WesleyRosenblum Mar 27, 2020 •

edited

Loading