Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Apr 7, 2020
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
163fd88
Add validation to ensure the length of the final frame in the final
WesleyRosenblum Mar 26, 2020
b58b5c1
Validate that frame length is positive for framed data
WesleyRosenblum Mar 27, 2020
5346309
Reverting removal of variable frame length code
WesleyRosenblum Apr 2, 2020
b8d754e
Reverting removal of variable frame length code
WesleyRosenblum Apr 2, 2020
6e38615
Fix spacing after if
WesleyRosenblum Apr 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/main/java/com/amazonaws/encryptionsdk/internal/FrameDecryptionHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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_) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@WesleyRosenblum WesleyRosenblum Mar 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

throw new BadCiphertextException("Final frame length exceeds frame length.");
}
} else {
protectedContentLen = frameSize_;
}
Expand Down
19 changes: 18 additions & 1 deletion src/test/java/com/amazonaws/encryptionsdk/internal/FrameDecryptionHandlerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,14 @@

import static org.junit.Assert.assertTrue;

import java.nio.ByteBuffer;
import java.security.SecureRandom;

import javax.crypto.SecretKey;
import javax.crypto.spec.SecretKeySpec;

import com.amazonaws.encryptionsdk.TestUtils;
import com.amazonaws.encryptionsdk.exception.BadCiphertextException;
import org.junit.Before;
import org.junit.Test;

Expand Down Expand Up @@ -72,4 +75,18 @@ public void decryptMaxContentLength() {
frameDecryptionHandler_.processBytes(in, 0, in.length, out, 0);
frameDecryptionHandler_.processBytes(in, 0, Integer.MAX_VALUE, out, 0);
}
}

@Test(expected = BadCiphertextException.class)
public void finalFrameLengthTooLarge() {

final ByteBuffer byteBuffer = ByteBuffer.allocate(25);
byteBuffer.put(TestUtils.unsignedBytesToSignedBytes(
new int[] {255, 255, 255, 255, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1}));
byteBuffer.putInt(AwsCrypto.getDefaultFrameSize() + 1);

final byte[] in = byteBuffer.array();
final byte[] out = new byte[in.length];

frameDecryptionHandler_.processBytes(in, 0, in.length, out, 0);
}
}