[tor-commits] [tor/maint-0.2.7] Fix a dangling pointer issue in our RSA keygen code

[nickm at torproject.org]

commit c4c4380a5e43e97ff9533aa42c73183dd1054c41
Author: Nick Mathewson <nickm at torproject.org>
Date:   Fri May 20 13:58:52 2016 -0400

    Fix a dangling pointer issue in our RSA keygen code
    
    If OpenSSL fails to generate an RSA key, do not retain a dangling
    pointer to the previous (uninitialized) key value. The impact here
    should be limited to a difficult-to-trigger crash, if OpenSSL is
    running an engine that makes key generation failures possible, or if
    OpenSSL runs out of memory. Fixes bug 19152; bugfix on
    0.2.1.10-alpha. Found by Yuan Jochen Kang, Suman Jana, and Baishakhi
    Ray.
    
    This is potentially scary stuff, so let me walk through my analysis.
    I think this is a bug, and a backport candidate, but not remotely
    triggerable in any useful way.
    
    Observation 1a:
    
    Looking over the OpenSSL code here, the only way we can really fail in
    the non-engine case is if malloc() fails.  But if malloc() is failing,
    then tor_malloc() calls should be tor_asserting -- the only way that an
    attacker could do an exploit here would be to figure out some way to
    make malloc() fail when openssl does it, but work whenever Tor does it.
    
    (Also ordinary malloc() doesn't fail on platforms like Linux that
    overcommit.)
    
    Observation 1b:
    
    Although engines are _allowed_ to fail in extra ways, I can't find much
    evidence online  that they actually _do_ fail in practice. More evidence
    would be nice, though.
    
    Observation 2:
    
    We don't call crypto_pk_generate*() all that often, and we don't do it
    in response to external inputs. The only way to get it to happen
    remotely would be by causing a hidden service to build new introduction
    points.
    
    Observation 3a:
    
    So, let's assume that both of the above observations are wrong, and the
    attacker can make us generate a crypto_pk_env_t with a dangling pointer
    in its 'key' field, and not immediately crash.
    
    This dangling pointer will point to what used to be an RSA structure,
    with the fields all set to NULL.  Actually using this RSA structure,
    before the memory is reused for anything else, will cause a crash.
    
    In nearly every function where we call crypto_pk_generate*(), we quickly
    use the RSA key pointer -- either to sign something, or to encode the
    key, or to free the key.  The only exception is when we generate an
    intro key in rend_consider_services_intro_points().  In that case, we
    don't actually use the key until the intro circuit is opened -- at which
    point we encode it, and use it to sign an introduction request.
    
    So in order to exploit this bug to do anything besides crash Tor, the
    attacker needs to make sure that by the time the introduction circuit
    completes, either:
      * the e, d, and n BNs look valid, and at least one of the other BNs is
        still NULL.
    OR
      * all 8 of the BNs must look valid.
    
    To look like a valid BN, *they* all need to have their 'top' index plus
    their 'd' pointer indicate an addressable region in memory.
    
    So actually getting useful data of of this, rather than a crash, is
    going to be pretty damn hard.  You'd have to force an introduction point
    to be created (or wait for one to be created), and force that particular
    crypto_pk_generate*() to fail, and then arrange for the memory that the
    RSA points to to in turn point to 3...8 valid BNs, all by the time the
    introduction circuit completes.
    
    Naturally, the signature won't check as valid [*], so the intro point
    will reject the ESTABLISH_INTRO cell.  So you need to _be_ the
    introduction point, or you don't actually see this information.
    
    [*] Okay, so if you could somehow make the 'rsa' pointer point to a
    different valid RSA key, then you'd get a valid signature of an
    ESTABLISH_INTRO cell using a key that was supposed to be used for
    something else ... but nothing else looks like that, so you can't use
    that signature elsewhere.
    
    Observation 3b:
    
    Your best bet as an attacker would be to make the dangling RSA pointer
    actually contain a fake method, with a fake RSA_private_encrypt
    function that actually pointed to code you wanted to execute.  You'd
    still need to transit 3 or 4 pointers deep though in order to make that
    work.
    
    Conclusion:
    
    By 1, you probably can't trigger this without Tor crashing from OOM.
    
    By 2, you probably can't trigger this reliably.
    
    By 3, even if I'm wrong about 1 and 2, you have to jump through a pretty
    big array of hoops in order to get any kind of data leak or code
    execution.
    
    So I'm calling it a bug, but not a security hole. Still worth
    patching.
---
 changes/rsa_init_bug | 7 +++++++
 src/common/crypto.c  | 4 +++-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/changes/rsa_init_bug b/changes/rsa_init_bug
new file mode 100644
index 0000000..6b5fb4f
--- /dev/null
+++ b/changes/rsa_init_bug
@@ -0,0 +1,7 @@
+  o Major bugfixes (key management):
+    - If OpenSSL fails to generate an RSA key, do not retain a dangling pointer
+      to the previous (uninitialized) key value. The impact here should be
+      limited to a difficult-to-trigger crash, if OpenSSL is running an
+      engine that makes key generation failures possible, or if OpenSSL runs
+      out of memory. Fixes bug 19152; bugfix on 0.2.1.10-alpha. Found by
+      Yuan Jochen Kang, Suman Jana, and Baishakhi Ray.
diff --git a/src/common/crypto.c b/src/common/crypto.c
index 925beb3..63b274e 100644
--- a/src/common/crypto.c
+++ b/src/common/crypto.c
@@ -466,8 +466,10 @@ crypto_pk_generate_key_with_bits(crypto_pk_t *env, int bits)
 {
   tor_assert(env);
 
-  if (env->key)
+  if (env->key) {
     RSA_free(env->key);
+    env->key = NULL;
+  }
 
   {
     BIGNUM *e = BN_new();