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 16:24:24 +0000 (17:24 +0100)
commitaef84bb4efbddfd95d042f3f5f1d362ed7d4faeb
tree979d89579f45a94bb15e6403252a8edccf887eb8
parentea7df7ea449ef85a163fde917906e6e3da9388e5
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)
crypto/err/err.c