Fix undefined behaviour in e_aes_cbc_hmac_sha256.c and e_aes_cbc_hmac_sha1.c
authorMatt Caswell <matt@openssl.org>
Wed, 28 Jun 2017 14:18:30 +0000 (15:18 +0100)
committerMatt Caswell <matt@openssl.org>
Wed, 19 Jul 2017 12:33:33 +0000 (13:33 +0100)
commitec642d5aaaea85ac84597dc8ee56afb2114b6eda
tree85be819058cbdae36781f8a8e9be0fa2988aa1a7
parent5c5fef4d7aba0ef20cc88d7e34b22cec0d2881bb
Fix undefined behaviour in e_aes_cbc_hmac_sha256.c and e_aes_cbc_hmac_sha1.c

In TLS mode of operation the padding value "pad" is obtained along with the
maximum possible padding value "maxpad". If pad > maxpad then the data is
invalid. However we must continue anyway because this is constant time code.

We calculate the payload length like this:

    inp_len = len - (SHA_DIGEST_LENGTH + pad + 1);

However if pad is invalid then inp_len ends up -ve (actually large +ve
because it is a size_t).

Later we do this:

    /* verify HMAC */
    out += inp_len;
    len -= inp_len;

This ends up with "out" pointing before the buffer which is undefined
behaviour. Next we calculate "p" like this:

    unsigned char *p =
        out + len - 1 - maxpad - SHA256_DIGEST_LENGTH;

Because of the "out + len" term the -ve inp_len value is cancelled out
so "p" points to valid memory (although technically the pointer arithmetic
is undefined behaviour again).

We only ever then dereference "p" and never "out" directly so there is
never an invalid read based on the bad pointer - so there is no security
issue.

This commit fixes the undefined behaviour by ensuring we use maxpad in
place of pad, if the supplied pad is invalid.

With thanks to Brian Carpenter for reporting this issue.

Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/3832)

(cherry picked from commit 335d0a4646981c9d96b62811bcfd69a96a1a67d9)
crypto/evp/e_aes_cbc_hmac_sha1.c
crypto/evp/e_aes_cbc_hmac_sha256.c