Bug: function `aws_cryptosdk_priv_hdr_parse_aad` can parse invalid headers successfuly

[giltho]

Hi !

Description

There is a bug in the way aws_cryptosdk_priv_hdr_parse_aad handle the aad_len fields of the serialised header :

If the given aad_len is big enough (bigger than the length of the cursor), the phase of parsing the encryption context will simply be ignored.
This is due to the fact that the structure returned by byte_cursor_advance is not checked, and the fact that aws_cryptosdk_enc_ctx_deserialize will succeed if the aad_length is 0 (it probably should not ?).

In more details, if the aad_len is greater than the length of the cursor, aws_byte_cursor_advance will return a cursor with a NULL pointer and size 0. Therefore, aws_cryptosdk_enc_ctx_deserialize will be passed a cursor that has len = 0, and will do nothing, returning AWS_OP_SUCCESS.

Consequences

I am not entirely sure of the security issues that could happen. One way of using this is to have no encryption context, and a very big aad_len, in which case the aad_len will have close to no impact, except that the header is technically broken.

Another way to "attack" this would be to use a technically valid header, but stream it only partially, such that not enough data are transmitted to technically read the entire encryption context. The cursor will be too small to parse everything, and hdr_parse will kind of assume that there is no encryption context and parse the encryption context as if it was an the edk, and then finish parsing. Possibly, even start parsing the body as the rest of the header ? I have no idea if that could be dangerous or not.

Reproducing

I haven't had the time to construct a full example of the second case, but for the first case, it can be reproduced very easily by taking the test_header_2 case and replace the AAD length with 0xff, 0x00 or something else big. The test will pass correctly although it should not :

static const uint8_t test_header_2[] = { // same as test_header_1 with no AAD section
    //version, type, alg ID
    0x01,  0x80,  0x02,  0x14,
    //message ID
    0x11,  0x22,  0x33,  0x44,  0x55,  0x66,  0x77,  0x88,  0x11,  0x22,  0x33,  0x44,  0x55,  0x66,  0x77,  0x88,
    // ** MODIFIED AAD length** (too many bytes)
    0xFF, 0x00,
    //edk count
    0x00, 0x03,
    //edk #0 (all empty fields)
    0x00,  0x00,  0x00,  0x00,  0x00,  0x00,
    //edk #1
    //provider ID len + data
    0x00,  0x04,  0x10,  0x11,  0x12,  0x00,
    //prov info len + data
    0x00,  0x04,  0x01,  0x02,  0x03,  0x04,
    //encrypted data key
    0x00,  0x08,  0x11,  0x02,  0x03,  0x04,  0x05,  0x06,  0x07,  0x88,
    //edk #2 (all empty fields)
    0x00,  0x00,  0x00,  0x00,  0x00,  0x00,

    //content type
    0x02,

    //reserved
    0x00,  0x00,  0x00,  0x00,
    //iv len
    0x0c,
    //frame length
    0x00,  0x00,  0x10,  0x00,
    //iv  FIXME: this IV and authentication tag is not valid, change when we implement authentication
    0x00,  0x01,  0x02,  0x03,  0x04,  0x05,  0x06,  0x07,  0x08,  0x09,  0x0a,  0x0b,
    //header auth
    0xde,  0xad,  0x00,  0x00,  0x00,  0x00,  0x00,  0x00,  0x00,  0x00,  0x00,  0x00,  0x00,  0x00,  0xbe, 0xef,
    // extra byte - used to verify that we can parse with extra trailing junk
    0xFF
};

Simple fix

A quite simple fix is to add a check after this line :

struct aws_byte_cursor aad = aws_byte_cursor_advance_nospec(cur, aad_len);
if (!aad->ptr) return aws_cryptosdk_priv_hdr_parse_err_short_buf(hdr);
// Because cur->ptr cannot be NULL and aad_len > 0, 
// the only case where aad->ptr is NULL is when aad_len > cur->len.

[seebees]

Thank you for bringing your concerns to our attention! We will investigate these immediately and follow up with you within 5 business days to provide a status.

If you have potential security concern regarding AWS cloud services or open source projects, please email aws-security@amazon.com. Messages to that alias will receive a non-automated response within 24 hours, more details. If you wish to protect your email, you may use PGP; our key is here. If you would like AWS Security to encrypt its communications to you, please provide us with your public PGP key also.

[antonf-amzn]

@giltho,

Thank you for contacting us. We reviewed your report and your change, and would like to accept your pull request. Thank you for reporting the issue and for submitting this PR.

Could you please confirm that your contribution is made under the terms of the Apache 2.0 license?

Thank you

[giltho]

Hi !
I confirm that my contribution was made under the terms of the Apache 2.0 license