[openssl] OpenSSL_1_1_1-stable update

nic.tuv at gmail.com nic.tuv at gmail.com
Fri Apr 24 15:08:49 UTC 2020


The branch OpenSSL_1_1_1-stable has been updated
       via  bb17971acb0d891b734991091244b90019968c65 (commit)
       via  3655e35b78df095b367b3334b6515f6d9436fd95 (commit)
       via  6a01f6f4b41d045e2a3abcb10163633d769db76a (commit)
       via  cd45a57aafddb908eb3a56e118b4c01899765d18 (commit)
      from  4738d9e060eccdb01d443c3cefe8101cbf1d4bfa (commit)


- Log -----------------------------------------------------------------
commit bb17971acb0d891b734991091244b90019968c65
Author: Nicola Tuveri <nic.tuv at gmail.com>
Date:   Tue Apr 21 18:34:17 2020 +0300

    Fix typo from #10631
    
    Reviewed-by: Matt Caswell <matt at openssl.org>
    Reviewed-by: Shane Lontis <shane.lontis at oracle.com>
    (Merged from https://github.com/openssl/openssl/pull/11595)
    
    (cherry picked from commit 4692e98bdbaebb6f078e89a75c54395839e68b24)

commit 3655e35b78df095b367b3334b6515f6d9436fd95
Author: Nicola Tuveri <nic.tuv at gmail.com>
Date:   Tue Jan 21 17:08:16 2020 +0200

    [BN] harden `BN_copy()` against leaks from memory accesses
    
    `BN_copy()` (and indirectly `BN_dup()`) do not propagate the
    `BN_FLG_CONSTTIME` flag: the propagation has been turned on and off a
    few times in the past years, because in some conditions it has shown
    unintended consequences in some code paths.
    
    Without turning the propagation on once more, we can still improve
    `BN_copy()` by avoiding to leak `src->top` in case `src` is flagged with
    `BN_FLG_CONSTTIME`.
    In this case we can instead use `src->dmax` as the number of words
    allocated for `dst` and for the `memcpy` operation.
    
    Barring compiler or runtime optimizations, if the caller provides `src`
    flagged as const time and preallocated to a public size, no leak should
    happen due to the copy operation.
    
    (cherry picked from commit 2d9167ed0b588dacbdd0303fb6041ffe1d8b3a92)
    
    Reviewed-by: Matt Caswell <matt at openssl.org>
    (Merged from https://github.com/openssl/openssl/pull/11127)

commit 6a01f6f4b41d045e2a3abcb10163633d769db76a
Author: Nicola Tuveri <nic.tuv at gmail.com>
Date:   Tue Jan 21 17:00:41 2020 +0200

    [EC] harden EC_KEY against leaks from memory accesses
    
    We should never leak the bit length of the secret scalar in the key,
    so we always set the `BN_FLG_CONSTTIME` flag on the internal `BIGNUM`
    holding the secret scalar.
    
    This is important also because `BN_dup()` (and `BN_copy()`) do not
    propagate the `BN_FLG_CONSTTIME` flag from the source `BIGNUM`, and
    this brings an extra risk of inadvertently losing the flag, even when
    the called specifically set it.
    
    The propagation has been turned on and off a few times in the past
    years because in some conditions has shown unintended consequences in
    some code paths, so at the moment we can't fix this in the BN layer.
    
    In `EC_KEY_set_private_key()` we can work around the propagation by
    manually setting the flag after `BN_dup()` as we know for sure that
    inside the EC module the `BN_FLG_CONSTTIME` is always treated
    correctly and should not generate unintended consequences.
    
    Setting the `BN_FLG_CONSTTIME` flag alone is never enough, we also have
    to preallocate the `BIGNUM` internal buffer to a fixed public size big
    enough that operations performed during the processing never trigger
    a realloc which would leak the size of the scalar through memory
    accesses.
    
    Fixed Length
    ------------
    
    The order of the large prime subgroup of the curve is our choice for
    a fixed public size, as that is generally the upper bound for
    generating a private key in EC cryptosystems and should fit all valid
    secret scalars.
    
    For preallocating the `BIGNUM` storage we look at the number of "words"
    required for the internal representation of the order, and we
    preallocate 2 extra "words" in case any of the subsequent processing
    might temporarily overflow the order length.
    
    Future work
    -----------
    
    A separate commit addresses further hardening of `BN_copy()` (and
    indirectly `BN_dup()`).
    
    (cherry picked from commit 0401d766afcd022748763f5614188301c9856c6e)
    
    Reviewed-by: Matt Caswell <matt at openssl.org>
    (Merged from https://github.com/openssl/openssl/pull/11127)

commit cd45a57aafddb908eb3a56e118b4c01899765d18
Author: Nicola Tuveri <nic.tuv at gmail.com>
Date:   Tue Jan 7 01:19:13 2020 +0200

    [EC] Constify internal EC_KEY pointer usage
    
    A pair of internal functions related to EC_KEY handling could benefit
    from declaring `EC_KEY *` variables as `const`, providing clarity for
    callers and readers of the code, in addition to enlisting the compiler
    in preventing some mistakes.
    
    (cherry picked from commit cd701de96a147260c2290d85af8a0656120a8ff8)
    
    In master `id2_ECParameters` and most of the ASN1 public functions have
    been properly constified in their signature.
    
    Unfortunately this has been deemed not doable in a patch release for
    1.1.1 as, in subtle ways, this would break API compatibility.
    See the discussion at https://github.com/openssl/openssl/pull/9347 for
    more details about this.
    
    This constification commit should still be portable w.r.t. our criteria,
    as the constification happens only on internal functions.
    
    The fix here is to explicitly discard the const qualifier before the
    call to `i2d_ECParameters`, which should be safe anyway because we can
    expect `i2d_ECParameters()` to treat the first argument as if it was
    const.
    
    Reviewed-by: Matt Caswell <matt at openssl.org>
    (Merged from https://github.com/openssl/openssl/pull/11127)

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

Summary of changes:
 crypto/bn/bn_lib.c   |  8 ++++--
 crypto/ec/ec_ameth.c | 16 ++++++++---
 crypto/ec/ec_key.c   | 76 +++++++++++++++++++++++++++++++++++++++++++++++++---
 3 files changed, 92 insertions(+), 8 deletions(-)

diff --git a/crypto/bn/bn_lib.c b/crypto/bn/bn_lib.c
index 86d4956c8a..759d4c70ed 100644
--- a/crypto/bn/bn_lib.c
+++ b/crypto/bn/bn_lib.c
@@ -322,15 +322,19 @@ BIGNUM *BN_dup(const BIGNUM *a)
 
 BIGNUM *BN_copy(BIGNUM *a, const BIGNUM *b)
 {
+    int bn_words;
+
     bn_check_top(b);
 
+    bn_words = BN_get_flags(b, BN_FLG_CONSTTIME) ? b->dmax : b->top;
+
     if (a == b)
         return a;
-    if (bn_wexpand(a, b->top) == NULL)
+    if (bn_wexpand(a, bn_words) == NULL)
         return NULL;
 
     if (b->top > 0)
-        memcpy(a->d, b->d, sizeof(b->d[0]) * b->top);
+        memcpy(a->d, b->d, sizeof(b->d[0]) * bn_words);
 
     a->neg = b->neg;
     a->top = b->top;
diff --git a/crypto/ec/ec_ameth.c b/crypto/ec/ec_ameth.c
index 2210383739..b7b82e54a3 100644
--- a/crypto/ec/ec_ameth.c
+++ b/crypto/ec/ec_ameth.c
@@ -23,7 +23,7 @@ static int ecdh_cms_decrypt(CMS_RecipientInfo *ri);
 static int ecdh_cms_encrypt(CMS_RecipientInfo *ri);
 #endif
 
-static int eckey_param2type(int *pptype, void **ppval, EC_KEY *ec_key)
+static int eckey_param2type(int *pptype, void **ppval, const EC_KEY *ec_key)
 {
     const EC_GROUP *group;
     int nid;
@@ -43,7 +43,17 @@ static int eckey_param2type(int *pptype, void **ppval, EC_KEY *ec_key)
         pstr = ASN1_STRING_new();
         if (pstr == NULL)
             return 0;
-        pstr->length = i2d_ECParameters(ec_key, &pstr->data);
+
+        /*
+         * The cast in the following line is intentional as the
+         * `i2d_ECParameters` signature can't be constified (see discussion at
+         * https://github.com/openssl/openssl/pull/9347 where related and
+         * required constification backports were rejected).
+         *
+         * This cast should be safe anyway, because we can expect
+         * `i2d_ECParameters()` to treat the first argument as if it was const.
+         */
+        pstr->length = i2d_ECParameters((EC_KEY *)ec_key, &pstr->data);
         if (pstr->length <= 0) {
             ASN1_STRING_free(pstr);
             ECerr(EC_F_ECKEY_PARAM2TYPE, ERR_R_EC_LIB);
@@ -57,7 +67,7 @@ static int eckey_param2type(int *pptype, void **ppval, EC_KEY *ec_key)
 
 static int eckey_pub_encode(X509_PUBKEY *pk, const EVP_PKEY *pkey)
 {
-    EC_KEY *ec_key = pkey->pkey.ec;
+    const EC_KEY *ec_key = pkey->pkey.ec;
     void *pval = NULL;
     int ptype;
     unsigned char *penc = NULL, *p;
diff --git a/crypto/ec/ec_key.c b/crypto/ec/ec_key.c
index 08aaac5d8a..261087ce23 100644
--- a/crypto/ec/ec_key.c
+++ b/crypto/ec/ec_key.c
@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2018 The OpenSSL Project Authors. All Rights Reserved.
+ * Copyright 2002-2020 The OpenSSL Project Authors. All Rights Reserved.
  * Copyright (c) 2002, Oracle and/or its affiliates. All rights reserved
  *
  * Licensed under the OpenSSL license (the "License").  You may not use
@@ -14,6 +14,7 @@
 #include "internal/refcount.h"
 #include <openssl/err.h>
 #include <openssl/engine.h>
+#include "crypto/bn.h"
 
 EC_KEY *EC_KEY_new(void)
 {
@@ -416,17 +417,86 @@ const BIGNUM *EC_KEY_get0_private_key(const EC_KEY *key)
 
 int EC_KEY_set_private_key(EC_KEY *key, const BIGNUM *priv_key)
 {
+    int fixed_top;
+    const BIGNUM *order = NULL;
+    BIGNUM *tmp_key = NULL;
+
     if (key->group == NULL || key->group->meth == NULL)
         return 0;
+
+    /*
+     * Not only should key->group be set, but it should also be in a valid
+     * fully initialized state.
+     *
+     * Specifically, to operate in constant time, we need that the group order
+     * is set, as we use its length as the fixed public size of any scalar used
+     * as an EC private key.
+     */
+    order = EC_GROUP_get0_order(key->group);
+    if (order == NULL || BN_is_zero(order))
+        return 0; /* This should never happen */
+
     if (key->group->meth->set_private != NULL
         && key->group->meth->set_private(key, priv_key) == 0)
         return 0;
     if (key->meth->set_private != NULL
         && key->meth->set_private(key, priv_key) == 0)
         return 0;
+
+    /*
+     * We should never leak the bit length of the secret scalar in the key,
+     * so we always set the `BN_FLG_CONSTTIME` flag on the internal `BIGNUM`
+     * holding the secret scalar.
+     *
+     * This is important also because `BN_dup()` (and `BN_copy()`) do not
+     * propagate the `BN_FLG_CONSTTIME` flag from the source `BIGNUM`, and
+     * this brings an extra risk of inadvertently losing the flag, even when
+     * the caller specifically set it.
+     *
+     * The propagation has been turned on and off a few times in the past
+     * years because in some conditions has shown unintended consequences in
+     * some code paths, so at the moment we can't fix this in the BN layer.
+     *
+     * In `EC_KEY_set_private_key()` we can work around the propagation by
+     * manually setting the flag after `BN_dup()` as we know for sure that
+     * inside the EC module the `BN_FLG_CONSTTIME` is always treated
+     * correctly and should not generate unintended consequences.
+     *
+     * Setting the BN_FLG_CONSTTIME flag alone is never enough, we also have
+     * to preallocate the BIGNUM internal buffer to a fixed public size big
+     * enough that operations performed during the processing never trigger
+     * a realloc which would leak the size of the scalar through memory
+     * accesses.
+     *
+     * Fixed Length
+     * ------------
+     *
+     * The order of the large prime subgroup of the curve is our choice for
+     * a fixed public size, as that is generally the upper bound for
+     * generating a private key in EC cryptosystems and should fit all valid
+     * secret scalars.
+     *
+     * For preallocating the BIGNUM storage we look at the number of "words"
+     * required for the internal representation of the order, and we
+     * preallocate 2 extra "words" in case any of the subsequent processing
+     * might temporarily overflow the order length.
+     */
+    tmp_key = BN_dup(priv_key);
+    if (tmp_key == NULL)
+        return 0;
+
+    BN_set_flags(tmp_key, BN_FLG_CONSTTIME);
+
+    fixed_top = bn_get_top(order) + 2;
+    if (bn_wexpand(tmp_key, fixed_top) == NULL) {
+        BN_clear_free(tmp_key);
+        return 0;
+    }
+
     BN_clear_free(key->priv_key);
-    key->priv_key = BN_dup(priv_key);
-    return (key->priv_key == NULL) ? 0 : 1;
+    key->priv_key = tmp_key;
+
+    return 1;
 }
 
 const EC_POINT *EC_KEY_get0_public_key(const EC_KEY *key)


More information about the openssl-commits mailing list