OSEC

Neohapsis is currently accepting applications for employment. For more information, please visit our website www.neohapsis.com or email hr@neohapsis.com
Re: reviewing OpenSSL's lib/libssl/src/crypto/asn1

From: Dirk Engling (erdgeisterdgeist.org)
Date: Sun Apr 20 2014 - 21:43:52 CDT


On 21.04.14 01:13, Bob Beck wrote:

> this list is for diffs. post them. keep them reviewable - of
> meaningful size and doing a certain thing.
> (as opposed to this diff changes 15 things.. )

Find attached my patches for some memory leaks, use after frees and some
minor house keeping as follows.

======== 8< ======== 8< ======== 8< ======== 8< ======== 8< ======== 8<

memleak_realloc.patch
* fix memory leak in a2i_ASN1_ENUMERATED, a2i_ASN1_STRING
  and a2i_ASN1_INTEGER, in case of goto err
* use realloc instead of s ? realloc(s,l) : malloc(l)
* Files: f_enum.c f_int.c f_string.c

double_free.patch
* remove unnecessary NULL checks before free()
* fix double free in d2i_ASN1_bytes by setting ret->data = NULL
  after free, before potential goto err;
* Files: a_bytes.c

calloc_length.patch
* remove unnecessary NULL checks before free()
* use calloc instead of malloc and memset
* remove unnecessary temp variable d
* move loop counter j in for() header
* make calculation of actual length in BN_to_ASN1_ENUMERATED
  more transparent
* Files: a_enum.c a_int.c

bad_macros.patch
* remove M_ASN1_New_Malloc, M_ASN1_New, M_ASN1_New_Error marcos,
  they hide a malloc and are only used once
* Files: asn1_mac.h x_pkey.c

nullcheck.patch
* remove unnecessary NULL checks before free()
* Files: a_bitstr.c a_gentm.c a_object.c a_utctm.c ameth_lib.c
  asn1_gen.c asn_mime.c asn_pack.c bio_asn1.c bio_ndef.c t_x509.c
  tasn_dec.c tasn_utl.c x_info.c x_name.c

calloc_realloc.patch
* remove unnecessary NULL checks before free()
* use calloc instead of malloc and memset
* use realloc instead of s ? realloc(s,l) : malloc(l)
* Files: ameth_lib.c asn1_lib.c tasn_new.c

sizeof.patch
* use sizeof(buf) instead of numeric value for passing
  to OPENSSL_cleanse
* Files: n_pkey.c

double_null.patch
* remove unnecessary double *pval = NULL in ASN1_primitive_free
* Files: tasn_fre.c

reuse.patch
* remove unnecessary NULL checks before free()
* prevent potential use after free of ret->name in x509_cb
* Files: x_x509.c

======== 8< ======== 8< ======== 8< ======== 8< ======== 8< ======== 8<

Some notes on my first glimpse at the code:

* ASN1_PCTX_new could use a calloc and avoid filling all members
* ASN1_BIT_STRING_set_bit and a2i_ASN1_INTEGER use CRYPTO_realloc_clean
  which OPENSSL_cleanse old location, in similar realloc settings,
  normal realloc was (mis)used
* still OPENSSL_free inside
* different styles for checking !p vs p == NULL vs if(!(p=malloc()))
* ASN1_STRING_set looks like a bloated and overly complex strdup wrapper
* ASN1_INTEGER_set and ASN1_ENUMERATED_set use some rather peculiar
  buffer on the stack which is then copied backwards to allocation

> #define LENGTHIKNOWNOTHINGABOUT 288 + 1
>
> If you're doing that, you're just hiding the horror and making it
> harder for someone like
> me to fix it.

I rather thought about copying constants from the RFC and naming offsets
and lengths into structures as such, also using sizeof(buf) instead of
the hard coded value as in sizeof.patch.

Regards,

  erdgeist