[erlang-patches] [PATCH] crypto: fix a few memleaks/undefined pointer dereferences

[unknown]

---
Hi,

now, those are the obviously broken cases - what I am wondering about is
whether it is correct that almost none of the OpenSSL calls in crypto are
being checked for memory allocation failures!? IIUC, OpenSSL is configured
to use enif_alloc(), which according to this ...

http://www.erlang.org/doc/man/erl_nif.html#enif_alloc

... will return NULL in case of an allocation failure. Now, is that there
just to confuse people because the function doesn't actually ever return in
case of an allocation failure, or is this really severely broken? Am I
missing something?

Regards, Florian

diff --git a/lib/crypto/c_src/crypto.c b/lib/crypto/c_src/crypto.c
index 42fb172..9e5c42a 100644
--- a/lib/crypto/c_src/crypto.c
+++ b/lib/crypto/c_src/crypto.c
@@ -1789,7 +1789,7 @@ static ERL_NIF_TERM rand_uniform_nif(ErlNifEnv* env, int argc, const ERL_NIF_TER

static ERL_NIF_TERM mod_exp_nif(ErlNifEnv* env, int argc, const ERL_NIF_TERM argv[])
{/* (Base,Exponent,Modulo,bin_hdr) */
- BIGNUM *bn_base=NULL, *bn_exponent=NULL, *bn_modulo, *bn_result;
+ BIGNUM *bn_base=NULL, *bn_exponent=NULL, *bn_modulo=NULL, *bn_result;
BN_CTX *bn_ctx;
unsigned char* ptr;
unsigned dlen;
@@ -1804,6 +1804,7 @@ static ERL_NIF_TERM mod_exp_nif(ErlNifEnv* env, int argc, const ERL_NIF_TERM arg

if (bn_base) BN_free(bn_base);
if (bn_exponent) BN_free(bn_exponent);
+ if (bn_modulo) BN_free(bn_modulo);
return enif_make_badarg(env);
}
bn_result = BN_new();
@@ -2600,7 +2601,7 @@ static ERL_NIF_TERM dh_compute_key_nif(ErlNifEnv* env, int argc, const ERL_NIF_T
static ERL_NIF_TERM srp_value_B_nif(ErlNifEnv* env, int argc, const ERL_NIF_TERM argv[])
{/* (Multiplier, Verifier, Generator, Exponent, Prime) */
BIGNUM *bn_verifier = NULL;
- BIGNUM *bn_exponent, *bn_generator, *bn_prime, *bn_multiplier, *bn_result;
+ BIGNUM *bn_exponent = NULL, *bn_generator = NULL, *bn_prime = NULL, *bn_multiplier = NULL, *bn_result;
BN_CTX *bn_ctx;
unsigned char* ptr;
unsigned dlen;
@@ -2613,9 +2614,9 @@ static ERL_NIF_TERM srp_value_B_nif(ErlNifEnv* env, int argc, const ERL_NIF_TERM
|| !get_bn_from_bin(env, argv[4], &bn_prime)) {
if (bn_multiplier) BN_free(bn_multiplier);
if (bn_verifier) BN_free(bn_verifier);
- if (bn_verifier) BN_free(bn_generator);
- if (bn_verifier) BN_free(bn_exponent);
- if (bn_verifier) BN_free(bn_prime);
+ if (bn_generator) BN_free(bn_generator);
+ if (bn_exponent) BN_free(bn_exponent);
+ if (bn_prime) BN_free(bn_prime);
return enif_make_badarg(env);
}

@@ -2739,7 +2740,7 @@ static ERL_NIF_TERM srp_host_secret_nif(ErlNifEnv* env, int argc, const ERL_NIF_
<premaster secret> = (A * v^u) ^ b % N
*/
BIGNUM *bn_b = NULL, *bn_verifier = NULL;
- BIGNUM *bn_prime, *bn_A, *bn_u, *bn_base, *bn_result;
+ BIGNUM *bn_prime = NULL, *bn_A = NULL, *bn_u = NULL, *bn_base, *bn_result;
BN_CTX *bn_ctx;
unsigned char* ptr;
unsigned dlen;

[unknown]

Post by unknown
---
Hi,
now, those are the obviously broken cases -

Thanks. I'll put the patch in the pipe for R17.

Post by unknown
what I am wondering about is
whether it is correct that almost none of the OpenSSL calls in crypto are
being checked for memory allocation failures!?

Not sure what you mean. Can you give an example.

Post by unknown
IIUC, OpenSSL is configured
to use enif_alloc(), which according to this ...
http://www.erlang.org/doc/man/erl_nif.html#enif_alloc
... will return NULL in case of an allocation failure. Now, is that there
just to confuse people because the function doesn't actually ever return in
case of an allocation failure, or is this really severely broken? Am I
missing something?

enif_alloc calls erts_alloc_fnf where "fnf" stands for "Failure Not
Fatal" which means that it will return NULL on failure.


/Sverker

[unknown]

Hi,

Post by unknown

Post by unknown
now, those are the obviously broken cases -

Thanks. I'll put the patch in the pipe for R17.

Thanks!

Post by unknown

Post by unknown
what I am wondering about is
whether it is correct that almost none of the OpenSSL calls in crypto are
being checked for memory allocation failures!?

Not sure what you mean. Can you give an example.

For example, almost none of the bignum calls (BN_.*) are checked for error
returns, the OpensSSL documentation explicitly states that you always have
to check returns because it uses dynamic memory allocation and thus
allocation failures might occur. Looking around in the code, I also found a
call to HMAC_Init(), for example, which I'm pretty sure also uses dynamic
allocation (and in any case, the OpenSSL documentation shows that is has an
error return, even though the possible causes of failure are not listed),
and then there are all those <algorithm>_Init calls and the like, of which
I don't have a clue whether they do use dynamic allocation, but in any
case, the OpenSSL documentation shows they also have error returns, and
those generally don't seem to be checked either.

Post by unknown

Post by unknown
IIUC, OpenSSL is configured
to use enif_alloc(), which according to this ...
http://www.erlang.org/doc/man/erl_nif.html#enif_alloc
... will return NULL in case of an allocation failure. Now, is that there
just to confuse people because the function doesn't actually ever return in
case of an allocation failure, or is this really severely broken? Am I
missing something?

enif_alloc calls erts_alloc_fnf where "fnf" stands for "Failure Not
Fatal" which means that it will return NULL on failure.

IC - I didn't quite manage to penetrate all the allocator indirections,
though I didn't try too hard ;-)

Regards, Florian

[unknown]

Post by unknown

Post by unknown

Post by unknown
what I am wondering about is
whether it is correct that almost none of the OpenSSL calls in crypto are
being checked for memory allocation failures!?

Not sure what you mean. Can you give an example.

For example, almost none of the bignum calls (BN_.*) are checked for error
returns, the OpensSSL documentation explicitly states that you always have
to check returns because it uses dynamic memory allocation and thus
allocation failures might occur. Looking around in the code, I also found a
call to HMAC_Init(), for example, which I'm pretty sure also uses dynamic
allocation (and in any case, the OpenSSL documentation shows that is has an
error return, even though the possible causes of failure are not listed),
and then there are all those <algorithm>_Init calls and the like, of which
I don't have a clue whether they do use dynamic allocation, but in any
case, the OpenSSL documentation shows they also have error returns, and
those generally don't seem to be checked either.

A simple fix to this would be to let the crypto_alloc (in
crypto_callback.c) to call abort() if the allocation failed as that is
the strategy otherwise in out-of-memory scenarios. Thus a "nice crash"
with an "Out of memory" message instead of a segmentation violation.

/Sverker

[unknown]

Hi,

Post by unknown
A simple fix to this would be to let the crypto_alloc (in
crypto_callback.c) to call abort() if the allocation failed as that
is the strategy otherwise in out-of-memory scenarios. Thus a "nice
crash" with an "Out of memory" message instead of a segmentation
violation.

below you find a patch that does just that. I hope it can still make it
into R17? Mind you, pointer arithmetic with and dereference of null
pointers give undefined behaviour, not (necessarily just) a segfault, so
this could have security implications.

Regards, Florian

diff --git a/lib/crypto/c_src/crypto_callback.c b/lib/crypto/c_src/crypto_callback.c
index 81106b4..750e9b1 100644
--- a/lib/crypto/c_src/crypto_callback.c
+++ b/lib/crypto/c_src/crypto_callback.c
@@ -53,11 +53,19 @@ static ErlNifRWLock** lock_vec = NULL; /* Static locks used by openssl */

static void* crypto_alloc(size_t size)
{
- return enif_alloc(size);
+ void *ret;
+
+ if (!(ret = enif_alloc(size)) && size)
+ abort();
+ return ret;
}
static void* crypto_realloc(void* ptr, size_t size)
{
- return enif_realloc(ptr, size);
+ void *ret;
+
+ if (!(ret = enif_realloc(ptr, size)) && size)
+ abort();
+ return ret;
}
static void crypto_free(void* ptr)
{

[unknown]

Thanks, I'll include this for 17.0-rc2.

/Sverker

Post by unknown
Hi,

Post by unknown
A simple fix to this would be to let the crypto_alloc (in
crypto_callback.c) to call abort() if the allocation failed as that
is the strategy otherwise in out-of-memory scenarios. Thus a "nice
crash" with an "Out of memory" message instead of a segmentation
violation.

below you find a patch that does just that. I hope it can still make it
into R17? Mind you, pointer arithmetic with and dereference of null
pointers give undefined behaviour, not (necessarily just) a segfault, so
this could have security implications.
Regards, Florian
diff --git a/lib/crypto/c_src/crypto_callback.c b/lib/crypto/c_src/crypto_callback.c
index 81106b4..750e9b1 100644
--- a/lib/crypto/c_src/crypto_callback.c
+++ b/lib/crypto/c_src/crypto_callback.c
@@ -53,11 +53,19 @@ static ErlNifRWLock** lock_vec = NULL; /* Static locks used by openssl */
static void* crypto_alloc(size_t size)
{
- return enif_alloc(size);
+ void *ret;
+
+ if (!(ret = enif_alloc(size)) && size)
+ abort();
+ return ret;
}
static void* crypto_realloc(void* ptr, size_t size)
{
- return enif_realloc(ptr, size);
+ void *ret;
+
+ if (!(ret = enif_realloc(ptr, size)) && size)
+ abort();
+ return ret;
}
static void crypto_free(void* ptr)
{