Stop raising ERR_R_MALLOC_FAILURE in most places by levitte · Pull Request #19301 · openssl/openssl

levitte · 2022-09-29T12:17:38Z

Since OPENSSL_malloc() and friends report ERR_R_MALLOC_FAILURE, and at least handle the file name and line number they are called from, there's no need to report ERR_R_MALLOC_FAILURE where they are called directly, or when SSLfatal() and RLAYERfatal() is used, the reason ERR_R_MALLOC_FAILURE is changed to ERR_R_CRYPTO_LIB.

There were a number of places where ERR_R_MALLOC_FAILURE was reported even though it was a function from a different sub-system that was called. Those places are changed to report ERR_R_{lib}_LIB, where {lib} is the name of that sub-system.
Some of them are tricky to get right, as we have a lot of functions that belong in the ASN1 sub-system, and all the sk_ calls or from the CRYPTO sub-system.

Some extra adaptation was necessary where there were custom OPENSSL_malloc() wrappers, and some bugs are fixed alongside these changes.
Most notably, CRYPTO_secure_malloc() has been changed to also raise ERR_R_MALLOC_FAILURE when appropriate.

This is an alternative to #19072

In other words, make it raise ERR_R_MALLOC_FAILURE appropriately.

Since OPENSSL_malloc() and friends report ERR_R_MALLOC_FAILURE, and at least handle the file name and line number they are called from, there's no need to report ERR_R_MALLOC_FAILURE where they are called directly, or when SSLfatal() and RLAYERfatal() is used, the reason `ERR_R_MALLOC_FAILURE` is changed to `ERR_R_CRYPTO_LIB`. There were a number of places where `ERR_R_MALLOC_FAILURE` was reported even though it was a function from a different sub-system that was called. Those places are changed to report ERR_R_{lib}_LIB, where {lib} is the name of that sub-system. Some of them are tricky to get right, as we have a lot of functions that belong in the ASN1 sub-system, and all the `sk_` calls or from the CRYPTO sub-system. Some extra adaptation was necessary where there were custom OPENSSL_malloc() wrappers, and some bugs are fixed alongside these changes.

levitte · 2022-09-29T12:19:08Z

This is in draft for a bit, I need to re-read what I've done after a pause.
Feel free to look, comment, or even review.

DDvO · 2022-09-29T12:59:29Z

This is an alternative to #19072

This is clearly more advanced than #19072.
Yet note that that PR also has commit 6a6e27b which replaces all remnant ECerr() and EVPerr() calls in crypto/ by ERR_raise().
This is not (yet) covered here - you might simply cherry-pick that commit.

levitte · 2022-09-29T13:05:16Z

Yet note that that PR also has commit 6a6e27b which replaces all remnant ECerr() and EVPerr() calls in crypto/ by ERR_raise().

That could actually be made into a completely separate fix commit. It's quite independent from all the rest

DDvO · 2022-09-29T13:21:45Z

That could actually be made into a completely separate fix commit. It's quite independent from all the rest

I just did this in #19302, and closed #19072 in favor of the two new PRs.

Engines lacked the possibility to refer to themselves in this form: WHATEVERerr(WHATEVER_F_SOMETHING, WHATEVER_R_WHATEVER_LIB); This little change makes that possible, and gets used in e_capi.

t8m

I've found just two mistakes in this gargantuan PR. How did you do it, even with some scripting, I cannot imagine. Great work!

crypto/x509/x509_v3.c

ssl/ssl_sess.c

levitte · 2022-09-29T16:25:08Z

How did you do it, even with some scripting, I cannot imagine.

Grep, and eyeballing

levitte · 2022-09-29T17:42:38Z

I'm re-reading my changes, and clearly got blurry-eyed in some sections. Corrections coming up!

beldmit · 2022-09-29T18:02:59Z

@levitte it looks impossible to review all the affected files.
I'd suggest splitting them at least on commit per-directory basis if you want to fixup it.

levitte · 2022-09-29T18:44:03Z

@levitte it looks impossible to review all the affected files. I'd suggest splitting them at least on commit per-directory basis if you want to fixup it.

... I think that'll be something for tomorrow, although I'm hoping that someone has the stamina 😉

crypto/pem/pvkfmt.c

t8m · 2022-10-01T09:34:11Z

Should the draft status be removed? IMO this is good to go.

levitte · 2022-10-03T13:58:43Z

Second review, @openssl/committers ?

openssl-machine · 2022-10-05T12:00:15Z

This pull request is ready to merge

levitte · 2022-10-05T12:04:39Z

Merged

9167a47 Adapt CRYPTO_secure_malloc() like CRYPTO_malloc()
e077455 Stop raising ERR_R_MALLOC_FAILURE in most places
79c8dcf Add {lib}R{lib}_LIB, for our engines and other "external" modules

In other words, make it raise ERR_R_MALLOC_FAILURE appropriately. Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> (Merged from #19301)

Since OPENSSL_malloc() and friends report ERR_R_MALLOC_FAILURE, and at least handle the file name and line number they are called from, there's no need to report ERR_R_MALLOC_FAILURE where they are called directly, or when SSLfatal() and RLAYERfatal() is used, the reason `ERR_R_MALLOC_FAILURE` is changed to `ERR_R_CRYPTO_LIB`. There were a number of places where `ERR_R_MALLOC_FAILURE` was reported even though it was a function from a different sub-system that was called. Those places are changed to report ERR_R_{lib}_LIB, where {lib} is the name of that sub-system. Some of them are tricky to get right, as we have a lot of functions that belong in the ASN1 sub-system, and all the `sk_` calls or from the CRYPTO sub-system. Some extra adaptation was necessary where there were custom OPENSSL_malloc() wrappers, and some bugs are fixed alongside these changes. Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> (Merged from #19301)

Engines lacked the possibility to refer to themselves in this form: WHATEVERerr(WHATEVER_F_SOMETHING, WHATEVER_R_WHATEVER_LIB); This little change makes that possible, and gets used in e_capi. Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> (Merged from #19301)

In other words, make it raise ERR_R_MALLOC_FAILURE appropriately. Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> (Merged from openssl#19301)

Since OPENSSL_malloc() and friends report ERR_R_MALLOC_FAILURE, and at least handle the file name and line number they are called from, there's no need to report ERR_R_MALLOC_FAILURE where they are called directly, or when SSLfatal() and RLAYERfatal() is used, the reason `ERR_R_MALLOC_FAILURE` is changed to `ERR_R_CRYPTO_LIB`. There were a number of places where `ERR_R_MALLOC_FAILURE` was reported even though it was a function from a different sub-system that was called. Those places are changed to report ERR_R_{lib}_LIB, where {lib} is the name of that sub-system. Some of them are tricky to get right, as we have a lot of functions that belong in the ASN1 sub-system, and all the `sk_` calls or from the CRYPTO sub-system. Some extra adaptation was necessary where there were custom OPENSSL_malloc() wrappers, and some bugs are fixed alongside these changes. Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> (Merged from openssl#19301)

Engines lacked the possibility to refer to themselves in this form: WHATEVERerr(WHATEVER_F_SOMETHING, WHATEVER_R_WHATEVER_LIB); This little change makes that possible, and gets used in e_capi. Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> (Merged from openssl#19301)

bubble932 · 2024-07-29T08:04:21Z

@levitte I am curious about the differences in error handling between EVP_KDF_CTX_new() and EVP_MAC_CTX_new(). When OPENSSL_zalloc() fails, EVP_KDF_CTX_new() will raises ERR_R_EVP_LIB, whereas EVP_MAC_CTX_new() does not. Instead, EVP_MAC_CTX_new() only raises ERR_R_EVP_LIB when newctx() fails. This makes EVP_MAC_CTX_new() seem more reasonable in its error handling. Are there any other concerns I haven't noticed? Please enlighten me on this. Thanks.

levitte added 2 commits September 29, 2022 14:11

Adapt CRYPTO_secure_malloc() like CRYPTO_malloc()

546f8cc

In other words, make it raise ERR_R_MALLOC_FAILURE appropriately.

levitte added the branch: master Applies to master branch label Sep 29, 2022

levitte self-assigned this Sep 29, 2022

levitte requested review from a team and DDvO September 29, 2022 12:17

levitte mentioned this pull request Sep 29, 2022

ERR: Adapt ERR_raise() calls after making CRYPTO_malloc() and friends report ERR_R_MALLOC_FAILURE #19072

Closed

levitte added 2 commits September 29, 2022 14:42

fixup! Adapt CRYPTO_secure_malloc() like CRYPTO_malloc()

3a04393

fixup! Stop raising ERR_R_MALLOC_FAILURE in most places

c3b9d51

github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Sep 29, 2022

fixup! Stop raising ERR_R_MALLOC_FAILURE in most places

b65ba07

fixup! Stop raising ERR_R_MALLOC_FAILURE in most places

56a241f

DDvO mentioned this pull request Sep 29, 2022

ERR: replace remnant ECerr() and EVPerr() calls in crypto/ #19302

Closed

Add {lib}_R_{lib}_LIB, for our engines and other "external" modules

f2652f2

Engines lacked the possibility to refer to themselves in this form: WHATEVERerr(WHATEVER_F_SOMETHING, WHATEVER_R_WHATEVER_LIB); This little change makes that possible, and gets used in e_capi.

t8m added the triaged: bug The issue/pr is/fixes a bug label Sep 29, 2022

levitte marked this pull request as ready for review September 29, 2022 13:53

levitte marked this pull request as draft September 29, 2022 13:53

t8m requested changes Sep 29, 2022

View reviewed changes

crypto/x509/x509_v3.c Outdated Show resolved Hide resolved

ssl/ssl_sess.c Outdated Show resolved Hide resolved

levitte added 2 commits September 29, 2022 20:37

fixup! Adapt CRYPTO_secure_malloc() like CRYPTO_malloc()

41b19e7

fixup! Stop raising ERR_R_MALLOC_FAILURE in most places

2add736

t8m requested changes Sep 30, 2022

View reviewed changes

crypto/pem/pvkfmt.c Outdated Show resolved Hide resolved

crypto/pem/pvkfmt.c Outdated Show resolved Hide resolved

fixup! Stop raising ERR_R_MALLOC_FAILURE in most places

d18579b

t8m approved these changes Oct 1, 2022

View reviewed changes

levitte marked this pull request as ready for review October 1, 2022 19:38

hlandau approved these changes Oct 4, 2022

View reviewed changes

hlandau added the approval: done This pull request has the required number of approvals label Oct 4, 2022

openssl-machine removed the approval: done This pull request has the required number of approvals label Oct 5, 2022

openssl-machine added the approval: ready to merge The 24 hour grace period has passed, ready to merge label Oct 5, 2022

levitte closed this Oct 5, 2022

levitte deleted the alt-19072 branch October 5, 2022 12:05

openssl deleted a comment Oct 8, 2022

paulidale mentioned this pull request Oct 10, 2022

Adding signature algorithms via provider interface #19312

Closed

FdaSilvaYY mentioned this pull request Dec 27, 2022

Attribute certificate (RFC 5755) #15857

Closed

5 tasks

Uh oh!

Conversation

levitte commented Sep 29, 2022

Uh oh!

levitte commented Sep 29, 2022

Uh oh!

DDvO commented Sep 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

levitte commented Sep 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DDvO commented Sep 29, 2022

Uh oh!

t8m left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

levitte commented Sep 29, 2022

Uh oh!

levitte commented Sep 29, 2022

Uh oh!

beldmit commented Sep 29, 2022

Uh oh!

levitte commented Sep 29, 2022

Uh oh!

Uh oh!

Uh oh!

t8m commented Oct 1, 2022

Uh oh!

levitte commented Oct 3, 2022

Uh oh!

openssl-machine commented Oct 5, 2022

Uh oh!

levitte commented Oct 5, 2022

Uh oh!

bubble932 commented Jul 29, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

DDvO commented Sep 29, 2022 •

edited

Loading

levitte commented Sep 29, 2022 •

edited

Loading