combined patch for the more or less obvious issues: #2127

bernd-edlinger · 2016-12-21T11:20:04Z

#2087 #2094 #2103 #2104 #2105 #2106 #2107 #2108 #2110 #2111 #2112 #2115

levitte · 2016-12-21T13:48:14Z

crypto/asn1/tasn_new.c

@@ -186,7 +186,7 @@ static int asn1_item_ex_combine_new(ASN1_VALUE **pval, const ASN1_ITEM *it,
        for (i = 0, tt = it->templates; i < it->tcount; tt++, i++) {
            pseqval = asn1_get_field_ptr(pval, tt);
            if (!ASN1_template_new(pseqval, tt))
-                goto memerr;
+                goto auxerr;


I understand why you made that choice. Unfortunately, that will give off the wrong error code, so what I'd do if I was you is to change this back and add a ASN1_item_ex_free(pval, it); in the memerr: code.

Oh, and it's possible you need to check if pval != NULL in that case...

bernd-edlinger · 2016-12-22T08:41:43Z

updated patch should keep return code as before,
and only clean up *pval if it is actually initialized.
Note that ASN1_OP_NEW_PRE does not always initialize
*pval if the return code is zero. see sig_cb in dsa/dsa_asn1.c

bernd-edlinger · 2016-12-22T08:43:46Z

PS: CCLA was signed and sent to legal@opensslfoundation.org today.

bernd-edlinger · 2016-12-23T07:17:27Z

I noticed there is a further memory leak in ex_data.c if lh_insert fails.
The issue is similar to what can happen in the SSL_SESSION cache.

bernd-edlinger · 2016-12-23T09:12:49Z

oops, I doubt that the error in the AppVeyor build is due to my patch, so please ignore:

'ml64' is not recognized as an internal or external command,
operable program or batch file.
nmake /f ms%MAK%
'nmake' is not recognized as an internal or external command,
operable program or batch file.
Command exited with code 1

bernd-edlinger · 2016-12-29T11:46:19Z

Thinking at it again,
I noticed that not calling the free_func may result in memory leaks,
so there needs to be a fallback strategy, if allocating "storage" in
int_free_ex_data fails.

In my testing I did not have any free_func != NULL,
probably because I was not using any engine.

But I see that dynamic_data_ctx_free_func is one example of
what might happen if the free_func is not called.

Furthermore, but I have not seen any examples of these
function pointers, the return codes of new_func and dup_func
seem to be ignored as well.

bernd-edlinger · 2016-12-29T12:58:05Z

Oops, and the data flow in the new_func is reversed.
I really doubt that ever worked.

levitte · 2017-02-01T00:03:36Z

crypto/ex_data.c

    for (i = 0; i < mx; i++) {
-        if (storage[i] && storage[i]->free_func) {
+        if (storage)


if (storage != NULL)

levitte · 2017-02-01T00:04:04Z

crypto/ex_data.c

+            f = sk_CRYPTO_EX_DATA_FUNCS_value(item->meth, i);
+            CRYPTO_r_unlock(CRYPTO_LOCK_EX_DATA);
+        }
+        if (f && f->free_func) {


if (f != NULL && f->free_func != NULL) {

levitte · 2017-02-01T00:05:13Z

crypto/ex_data.c

            ptr = CRYPTO_get_ex_data(ad, i);
-            storage[i]->free_func(obj, ptr, ad, i,
-                                  storage[i]->argl, storage[i]->argp);
+            f->free_func(obj, ptr, ad, i, f->argl, f->argp);
        }
    }
    if (storage)
        OPENSSL_free(storage);


Remove the condition, as NULL is valid input for OPENSSL_free()

levitte · 2017-02-01T00:06:12Z

crypto/ex_data.c

        }
    }
    if (storage)
        OPENSSL_free(storage);
+ err:
    if (ad->sk) {
        sk_void_free(ad->sk);


Remove condition, as NULL is valid input for sk_void_free()

levitte · 2017-02-01T00:07:03Z

crypto/hmac/hm_pmeth.c

    if (!HMAC_CTX_copy(&dctx->ctx, &sctx->ctx))
-        return 0;
+        goto err;
    if (sctx->ktmp.data) {


While you're at it, you could change this to if (sctx->ktmp.data != NULL) {

levitte · 2017-02-01T00:07:30Z

crypto/rand/md_rand.c

-        MD_Update(&m, local_md, MD_DIGEST_LENGTH);
+        if (!MD_Init(&m) ||
+            !MD_Update(&m, local_md, MD_DIGEST_LENGTH))
+           goto err;


Indent by one more space

levitte · 2017-02-01T00:08:38Z

crypto/rand/md_rand.c

-            MD_Update(&m, &(state[st_idx]), MD_DIGEST_LENGTH / 2);
-        MD_Final(&m, local_md);
+            if (!MD_Update(&m, &(state[st_idx]), MD_DIGEST_LENGTH / 2))
+                goto err;


Since you're making changes here anyway, it would be nice if you put curlies around the else part

levitte · 2017-02-01T00:09:27Z

ssl/s3_pkt.c

@@ -699,6 +699,7 @@ int ssl3_write_bytes(SSL *s, int type, const void *buf_, int len)
        len >= 4 * (int)(max_send_fragment = s->max_send_fragment) &&
        s->compress == NULL && s->msg_callback == NULL &&
        SSL_USE_EXPLICIT_IV(s) &&
+        s->enc_write_ctx &&


s->enc_write_ctx != NULL &&

levitte · 2017-02-01T00:15:57Z

I'm a little confused by this PR. Is it only for 1.0.2?

Fixed a memory leak in ASN1_digest and ASN1_item_digest. asn1_template_noexp_d2i call ASN1_item_ex_free(&skfield,...) on error. Reworked error handling in asn1_item_ex_combine_new: - call ASN1_item_ex_free and return the correct error code if ASN1_template_new failed. - dont call ASN1_item_ex_free if ASN1_OP_NEW_PRE failed. Reworked error handing in x509_name_ex_d2i and x509_name_encode. Fixed error handling in int_ctx_new and EVP_PKEY_CTX_dup. Fixed a memory leak in def_get_class if lh_EX_CLASS_ITEM_insert fails due to OOM: - to figure out if the insertion succeeded, use lh_EX_CLASS_ITEM_retrieve again. - on error, p will be NULL, and gen needs to be cleaned up again. int_free_ex_data needs to have a fallback solution if unable to allocate "storage": - if free_func is non-zero this must be called to clean up all memory. Fixed error handling in pkey_hmac_copy. Fixed error handling in ssleay_rand_add and ssleay_rand_bytes. Fixed error handling in X509_STORE_new. Fixed a memory leak in ssl3_get_key_exchange. Check for null pointer in ssl3_write_bytes. Check for null pointer in ssl3_get_cert_verify. Fixed a memory leak in ssl_cert_dup. Fixes openssl#2087 openssl#2094 openssl#2103 openssl#2104 openssl#2105 openssl#2106 openssl#2107 openssl#2108 openssl#2110 openssl#2111 openssl#2112 openssl#2115

bernd-edlinger · 2017-02-01T07:44:00Z

Yes, this PR is for 1.0.2 only,
I have another pull request for 1.1.0.

levitte · 2017-02-01T08:49:58Z

Okie. This looks good to me, I just want to see the build results (Travis seems to have ground to a halt for the moment)

richsalz · 2017-02-04T18:06:59Z

ping @levitte to reconfirm.

bernd-edlinger · 2017-02-04T18:58:51Z

@richsalz I think you accidentally closed this PR, I cannot re-open this PR, is that a problem,
can you still do the merge, or should I open a new PR for 1.0.2?

Fixed a memory leak in ASN1_digest and ASN1_item_digest. asn1_template_noexp_d2i call ASN1_item_ex_free(&skfield,...) on error. Reworked error handling in asn1_item_ex_combine_new: - call ASN1_item_ex_free and return the correct error code if ASN1_template_new failed. - dont call ASN1_item_ex_free if ASN1_OP_NEW_PRE failed. Reworked error handing in x509_name_ex_d2i and x509_name_encode. Fixed error handling in int_ctx_new and EVP_PKEY_CTX_dup. Fixed a memory leak in def_get_class if lh_EX_CLASS_ITEM_insert fails due to OOM: - to figure out if the insertion succeeded, use lh_EX_CLASS_ITEM_retrieve again. - on error, p will be NULL, and gen needs to be cleaned up again. int_free_ex_data needs to have a fallback solution if unable to allocate "storage": - if free_func is non-zero this must be called to clean up all memory. Fixed error handling in pkey_hmac_copy. Fixed error handling in ssleay_rand_add and ssleay_rand_bytes. Fixed error handling in X509_STORE_new. Fixed a memory leak in ssl3_get_key_exchange. Check for null pointer in ssl3_write_bytes. Check for null pointer in ssl3_get_cert_verify. Fixed a memory leak in ssl_cert_dup. Fixes #2087 #2094 #2103 #2104 #2105 #2106 #2107 #2108 #2110 #2111 #2112 #2115 Reviewed-by: Richard Levitte <levitte@openssl.org> Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #2127)

richsalz · 2017-02-06T21:17:53Z

748cb9a on 1.0.2 Is there a version for 1.1.0/master still pending? Or should I just close all those issues?
Thanks!

bernd-edlinger · 2017-02-07T06:44:55Z

All issues are fixed on all branches. Please go ahead and close all issues. Thanks!

richsalz · 2017-02-07T15:12:06Z

@bernd-edlinger thank you very much for all your work on improving OpenSSL!

levitte added the hold: cla required The contributor needs to submit a license agreement label Dec 21, 2016

levitte requested changes Dec 21, 2016

View reviewed changes

bernd-edlinger mentioned this pull request Dec 26, 2016

memory leaks in x_name.c #2109

Closed

kroeckx added CLA OK and removed hold: cla required The contributor needs to submit a license agreement labels Dec 31, 2016

richsalz removed the CLA OK label Jan 28, 2017

bernd-edlinger force-pushed the fix_OOM_error_handling branch from c4e3bc0 to 856dc71 Compare January 31, 2017 08:56

levitte requested changes Feb 1, 2017

View reviewed changes

bernd-edlinger force-pushed the fix_OOM_error_handling branch from 856dc71 to ca8ed57 Compare February 1, 2017 07:43

levitte added the branch: 1.0.2 Merge to OpenSSL_1_0_2-stable branch label Feb 1, 2017

This was referenced Feb 2, 2017

[rt.openssl.org #3909] Crash due to non-checking of return value / errno of malloc #2369

Closed

[rt.openssl.org #4160] Candidate bug, malloc failure related issues in s3_enc.c, hm_pmeth.c #2465

Closed

richsalz added the reviewed label Feb 4, 2017

richsalz self-assigned this Feb 4, 2017

richsalz closed this Feb 4, 2017

richsalz reopened this Feb 4, 2017

levitte approved these changes Feb 6, 2017

View reviewed changes

levitte added the approval: done This pull request has the required number of approvals label Feb 6, 2017

richsalz closed this Feb 6, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

combined patch for the more or less obvious issues: #2127

combined patch for the more or less obvious issues: #2127

bernd-edlinger commented Dec 21, 2016

levitte Dec 21, 2016

levitte Dec 21, 2016

bernd-edlinger Dec 22, 2016

bernd-edlinger commented Dec 22, 2016

bernd-edlinger commented Dec 22, 2016

bernd-edlinger commented Dec 23, 2016

bernd-edlinger commented Dec 23, 2016

bernd-edlinger commented Dec 29, 2016

bernd-edlinger commented Dec 29, 2016

levitte Feb 1, 2017

levitte Feb 1, 2017

levitte Feb 1, 2017

levitte Feb 1, 2017

levitte Feb 1, 2017

levitte Feb 1, 2017

levitte Feb 1, 2017

levitte Feb 1, 2017

levitte commented Feb 1, 2017

bernd-edlinger commented Feb 1, 2017

levitte commented Feb 1, 2017

richsalz commented Feb 4, 2017

bernd-edlinger commented Feb 4, 2017

richsalz commented Feb 6, 2017

bernd-edlinger commented Feb 7, 2017

richsalz commented Feb 7, 2017

combined patch for the more or less obvious issues: #2127

combined patch for the more or less obvious issues: #2127

Conversation

bernd-edlinger commented Dec 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bernd-edlinger commented Dec 22, 2016

bernd-edlinger commented Dec 22, 2016

bernd-edlinger commented Dec 23, 2016

bernd-edlinger commented Dec 23, 2016

bernd-edlinger commented Dec 29, 2016

bernd-edlinger commented Dec 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levitte commented Feb 1, 2017

bernd-edlinger commented Feb 1, 2017

levitte commented Feb 1, 2017

richsalz commented Feb 4, 2017

bernd-edlinger commented Feb 4, 2017

richsalz commented Feb 6, 2017

bernd-edlinger commented Feb 7, 2017

richsalz commented Feb 7, 2017