[openssl-commits] [openssl] OpenSSL_1_0_2-stable update

[Matt Caswell]

The branch OpenSSL_1_0_2-stable has been updated
       via  e3d1a4e56572c71db5f297a50b8aa97bd7b39d3a (commit)
       via  ec642d5aaaea85ac84597dc8ee56afb2114b6eda (commit)
      from  5c5fef4d7aba0ef20cc88d7e34b22cec0d2881bb (commit)


- Log -----------------------------------------------------------------
commit e3d1a4e56572c71db5f297a50b8aa97bd7b39d3a
Author: Matt Caswell <matt at openssl.org>
Date:   Mon Jul 17 16:55:32 2017 +0100

    Remove some dead code
    
    The intention of the removed code was to check if the previous operation
    carried. However this does not work. The "mask" value always ends up being
    a constant and is all ones - thus it has no effect. This check is no longer
    required because of the previous commit.
    
    Reviewed-by: Rich Salz <rsalz at openssl.org>
    (Merged from https://github.com/openssl/openssl/pull/3832)
    
    (cherry picked from commit d5475e319575a45b20f560bdfae56cbfb165cb01)

commit ec642d5aaaea85ac84597dc8ee56afb2114b6eda
Author: Matt Caswell <matt at openssl.org>
Date:   Wed Jun 28 15:18:30 2017 +0100

    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 at openssl.org>
    (Merged from https://github.com/openssl/openssl/pull/3832)
    
    (cherry picked from commit 335d0a4646981c9d96b62811bcfd69a96a1a67d9)

-----------------------------------------------------------------------

Summary of changes:
 crypto/evp/e_aes_cbc_hmac_sha1.c   | 13 +++++++++----
 crypto/evp/e_aes_cbc_hmac_sha256.c | 13 +++++++++----
 2 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/crypto/evp/e_aes_cbc_hmac_sha1.c b/crypto/evp/e_aes_cbc_hmac_sha1.c
index d114710..b25fc6d 100644
--- a/crypto/evp/e_aes_cbc_hmac_sha1.c
+++ b/crypto/evp/e_aes_cbc_hmac_sha1.c
@@ -579,12 +579,17 @@ static int aesni_cbc_hmac_sha1_cipher(EVP_CIPHER_CTX *ctx, unsigned char *out,
             maxpad |= (255 - maxpad) >> (sizeof(maxpad) * 8 - 8);
             maxpad &= 255;
 
-            ret &= constant_time_ge(maxpad, pad);
+            mask = constant_time_ge(maxpad, pad);
+            ret &= mask;
+            /*
+             * If pad is invalid then we will fail the above test but we must
+             * continue anyway because we are in constant time code. However,
+             * we'll use the maxpad value instead of the supplied pad to make
+             * sure we perform well defined pointer arithmetic.
+             */
+            pad = constant_time_select(mask, pad, maxpad);
 
             inp_len = len - (SHA_DIGEST_LENGTH + pad + 1);
-            mask = (0 - ((inp_len - len) >> (sizeof(inp_len) * 8 - 1)));
-            inp_len &= mask;
-            ret &= (int)mask;
 
             key->aux.tls_aad[plen - 2] = inp_len >> 8;
             key->aux.tls_aad[plen - 1] = inp_len;
diff --git a/crypto/evp/e_aes_cbc_hmac_sha256.c b/crypto/evp/e_aes_cbc_hmac_sha256.c
index 917ae07..aaa724a 100644
--- a/crypto/evp/e_aes_cbc_hmac_sha256.c
+++ b/crypto/evp/e_aes_cbc_hmac_sha256.c
@@ -590,12 +590,17 @@ static int aesni_cbc_hmac_sha256_cipher(EVP_CIPHER_CTX *ctx,
             maxpad |= (255 - maxpad) >> (sizeof(maxpad) * 8 - 8);
             maxpad &= 255;
 
-            ret &= constant_time_ge(maxpad, pad);
+            mask = constant_time_ge(maxpad, pad);
+            ret &= mask;
+            /*
+             * If pad is invalid then we will fail the above test but we must
+             * continue anyway because we are in constant time code. However,
+             * we'll use the maxpad value instead of the supplied pad to make
+             * sure we perform well defined pointer arithmetic.
+             */
+            pad = constant_time_select(mask, pad, maxpad);
 
             inp_len = len - (SHA256_DIGEST_LENGTH + pad + 1);
-            mask = (0 - ((inp_len - len) >> (sizeof(inp_len) * 8 - 1)));
-            inp_len &= mask;
-            ret &= (int)mask;
 
             key->aux.tls_aad[plen - 2] = inp_len >> 8;
             key->aux.tls_aad[plen - 1] = inp_len;