Fix leak in ERR_get_state() when OPENSSL_init_crypto() isn't called yet
authorRichard Levitte <levitte@openssl.org>
Tue, 12 Dec 2017 01:05:38 +0000 (02:05 +0100)
committerRichard Levitte <levitte@openssl.org>
Tue, 12 Dec 2017 17:03:46 +0000 (18:03 +0100)
commit2717f2b7eb845594271a94969767af2c4521e004
treeddd19a234c2497488b7d56b17323a208abd6715b
parent0aa0e13a6a367dc27bfa59bd2ab1e90645c3158b
Fix leak in ERR_get_state() when OPENSSL_init_crypto() isn't called yet

If OPENSSL_init_crypto() hasn't been called yet when ERR_get_state()
is called, it need to be called early, so the base initialization is
done.  On some platforms (those who support DSO functionality and
don't define OPENSSL_USE_NODELETE), that includes a call of
ERR_set_mark(), which calls this function again.
Furthermore, we know that ossl_init_thread_start(), which is called
later in ERR_get_state(), calls OPENSSL_init_crypto(0, NULL), except
that's too late.
Here's what happens without an early call of OPENSSL_init_crypto():

    => ERR_get_state():
         => CRYPTO_THREAD_get_local():
         <= NULL;
         # no state is found, so it gets allocated.
         => ossl_init_thread_start():
              => OPENSSL_init_crypto():
                   # Here, base_inited is set to 1
                   # before ERR_set_mark() call
                   => ERR_set_mark():
                        => ERR_get_state():
                             => CRYPTO_THREAD_get_local():
                             <= NULL;
                             # no state is found, so it gets allocated!!!!!
                             => ossl_init_thread_start():
                                  => OPENSSL_init_crypto():
                                       # base_inited is 1,
                                       # so no more init to be done
                                  <= 1
                             <=
                             => CRYPTO_thread_set_local():
                             <=
                        <=
                   <=
              <= 1
         <=
         => CRYPTO_thread_set_local()      # previous value removed!
    <=

Result: double allocation, and we have a leak.

By calling the base OPENSSL_init_crypto() early, we get this instead:

    => ERR_get_state():
         => OPENSSL_init_crypto():
              # Here, base_inited is set to 1
              # before ERR_set_mark() call
              => ERR_set_mark():
                   => ERR_get_state():
                        => OPENSSL_init_crypto():
                             # base_inited is 1,
                             # so no more init to be done
                        <= 1
                        => CRYPTO_THREAD_get_local():
                        <= NULL;
                        # no state is found, so it gets allocated
                        # let's assume we got 0xDEADBEEF
                        => ossl_init_thread_start():
                             => OPENSSL_init_crypto():
                                  # base_inited is 1,
                                  # so no more init to be done
                             <= 1
                        <= 1
                        => CRYPTO_thread_set_local():
                        <=
                   <=
              <=
         <= 1
         => CRYPTO_THREAD_get_local():
         <= 0xDEADBEEF
    <= 0xDEADBEEF

Result: no leak.

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

(cherry picked from commit aef84bb4efbddfd95d042f3f5f1d362ed7d4faeb)
crypto/err/err.c