Fix ssl_free() and thus BIO_free() to respect BIO_NOCLOSE by DDvO · Pull Request #16688 · openssl/openssl

DDvO · 2021-09-27T12:34:30Z

I recently integrated the CMP client (alternatively to EST) in a BRSKI implementation in a way that an existing TLS connection is reused for the HTTP(S) transfer of CMP messages.
To this end, I embedded the SSL connection represented by the variable SSL *ssl as follows:

     BIO *sbio = BIO_new(BIO_f_ssl());
     if (sbio == NULL || BIO_set_ssl(sbio, ssl,  BIO_NOCLOSE) <= 0
         ...

After the CMP exchange is done, I call BIO_free(sbio)
Yet after doing so, the ssl connection is not more usable for a further BRSKI message exchange,
and it turns out this is because ssl_free() and thus BIO_free() does not respect BIO_NOCLOSE.
(BTW, looking at the overall code base, BIO_NOCLOSE seems to have little influence also on other BIOs).

The present PR fixes this bug.

t8m

The CI failure - leaks - is relevant.

DDvO · 2021-09-27T13:18:23Z

The CI failure - leaks - is relevant.

Sigh, yes. So this old bug hid further issues.
Any hints how (best) to fix these leaks?

include/openssl/bio.h.in

DDvO · 2021-09-29T10:52:10Z

I meanwhile analyzed the situation and found and fixed further issues,
such that all tests pass again, with no memory leaks reported any more.

It turns out that the BIO_{NO,}CLOSE input was not even recorded by BIO_set_ssl(), which now gets fixed here as well.
(I also noticed that BIO_ctrl() and ssl_ctrl(), when called with BIO_C_SET_SSL, ignore any given flags, but meanwhile I found that this is a different issue).

DDvO · 2021-09-29T11:01:39Z

I think the severity: fips change label, which got (re-)added automatically here, is meanwhile a false positive:
I had to change the implementation of BIO_set_ssl(), and just because it happens to be implemented as a macro in bio.h.in, this change appears as a change of the public API.

t8m · 2021-09-29T11:15:36Z

This still changes public API - the macro is part of it. It is slightly better but I am not sure this should be merged. Would there be any way to fix without changing public headers?

DDvO · 2021-09-29T11:27:23Z

I've meanwhile moved the call of BIO_set_shutdown() into the body of
long BIO_ctrl(BIO *b, int cmd, long larg, void *parg)
where it then needs an extra guard.

levitte · 2021-09-29T11:32:08Z

I've meanwhile moved the call of BIO_set_shutdown() into the body of long BIO_ctrl(BIO *b, int cmd, long larg, void *parg) where it then needs an extra guard.

I think this is the wrong thing to do. Further comments as review comments.

crypto/bio/bio_lib.c

DDvO · 2021-09-30T09:20:37Z

Can I add the branch 3.0 tag here?

t8m

LGTM. IMO this is a clear bug fix so definitely applicable to 3.0.

t8m · 2021-09-30T10:05:08Z

Of course the s_socket.c change is applicable only to master but that commit can be just skipped when cherry picking.

kaduk

While it does look on the face of it like this is a clear bugfix and thus candidate for backport, I worry that it might fall into the category of bugs that are so longstanding that everything that cares has worked around the bug already, so that fixing the bug is more disruptive than leaving it in.

ssl/bio_ssl.c

t8m · 2021-10-01T07:10:14Z

While it does look on the face of it like this is a clear bugfix and thus candidate for backport, I worry that it might fall into the category of bugs that are so longstanding that everything that cares has worked around the bug already, so that fixing the bug is more disruptive than leaving it in.

I do not really see a scenario where this fix could break anyone. Would you please elaborate?

t8m · 2021-10-01T07:11:31Z

I do not see how anyone would accidentally set BIO_NOCLOSE and expect that the SSL_shutdown() is done anyway. You would have to explicitly set the BIO_NOCLOSE but why would you do that if that does not have any real effect with the current code?

openssl-machine · 2021-10-01T11:00:11Z

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

kaduk · 2021-10-02T03:52:10Z

While it does look on the face of it like this is a clear bugfix and thus candidate for backport, I worry that it might fall into the category of bugs that are so longstanding that everything that cares has worked around the bug already, so that fixing the bug is more disruptive than leaving it in.

I do not really see a scenario where this fix could break anyone. Would you please elaborate?

I don't have a specific scenario in mind, this just qualitatively feels like some other cases where we've fixed a clear bug and then had it cause problems in practice due to software that had worked around it already.

Purely hypothetical, but consider an application using a common framework to manage multiple BIO types that just always set BIO_NOCLOSE unconditionally on all BIOs. That gets the precondition that the flag is set, and the framework then adds a workaround to match the observed behavior for SSL BIOs.

Is that a plausible scenario? I don't know. But it's enough to make me ask the question of whether the bugfix could be disruptive.

t8m · 2021-10-04T08:53:01Z

I do not really see how this could be a plausible scenario.

Reviewed-by: Tomas Mraz <[email protected]> (Merged from #16688)

Reviewed-by: Tomas Mraz <[email protected]> (Merged from #16688) (cherry picked from commit dce910a)

Reviewed-by: Tomas Mraz <[email protected]> (Merged from #16688) (cherry picked from commit 34901b0)

t8m · 2021-10-04T09:00:08Z

Merged to master and 3.0 (only the two applicable commits). Thank you.

DDvO · 2021-10-05T11:06:56Z

Purely hypothetical, but consider an application using a common framework to manage multiple BIO types that just always set BIO_NOCLOSE unconditionally on all BIOs. That gets the precondition that the flag is set, and the framework then adds a workaround to match the observed behavior for SSL BIOs.

I also thought that it might be problematic to fix this longstanding bug because existing software may rely in the buggy behavior. Yet at least none of our tests broke.
A typical workaround for the bug may have been not to close the SSL connection, such that after this bugfix a mem leak might occur.
But well, typically the BIO does not get closed until exit, and than all open connections get closed anyway.

DDvO · 2021-10-05T11:07:39Z

Merged to master and 3.0 (only the two applicable commits). Thank you.

Thanks @t8m for handling the discussion on between and merging this to both branches.

Reviewed-by: Tomas Mraz <[email protected]> (Merged from openssl#16688)

DDvO · 2021-11-25T08:52:53Z

I just realized that most of #16688, namely

the BIO_NOCLOSE issue and its fix
the improvements to BIO_f_ssl.pod

pertains to earlier OpenSSL versions as well.
Ok to cherry-pick to 1.1.1?

t8m · 2021-11-25T09:24:13Z

Please create a separate PR against 1.1.1 I suppose this one won't cherry pick cleanly.

DDvO · 2021-11-25T10:01:22Z

Good point; done in #17135.

DDvO added approval: otc review pending triaged: bug The issue/pr is/fixes a bug labels Sep 27, 2021

t8m requested changes Sep 27, 2021

View reviewed changes

t8m added branch: master Applies to master branch and removed approval: otc review pending labels Sep 27, 2021

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

t8m reviewed Sep 29, 2021

View reviewed changes

include/openssl/bio.h.in Outdated Show resolved Hide resolved

DDvO force-pushed the fix_ssl_free_BIO_NOCLOSE branch from bd68f2e to f8eb0f1 Compare September 29, 2021 10:51

DDvO removed the severity: fips change The pull request changes FIPS provider sources label Sep 29, 2021

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

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

levitte reviewed Sep 29, 2021

View reviewed changes

crypto/bio/bio_lib.c Outdated Show resolved Hide resolved

DDvO requested a review from t8m September 29, 2021 11:53

DDvO added 3 commits September 30, 2021 10:52

Fix ssl_free() and thus BIO_free() to respect BIO_NOCLOSE

d26e2da

apps/lib/s_socket.c: Fix mem leak on host name in init_client()

869498e

BIO_f_ssl.pod: Make clear where an SSL BIOs are expected as an argument

3d8fb5c

DDvO force-pushed the fix_ssl_free_BIO_NOCLOSE branch from ea97f2b to 3d8fb5c Compare September 30, 2021 09:14

DDvO added the triaged: documentation The issue/pr deals with documentation (errors) label Sep 30, 2021

t8m approved these changes Sep 30, 2021

View reviewed changes

t8m added the branch: 3.0 Applies to openssl-3.0 branch label Sep 30, 2021

t8m added the approval: done This pull request has the required number of approvals label Sep 30, 2021

kaduk reviewed Sep 30, 2021

View reviewed changes

ssl/bio_ssl.c Show resolved Hide resolved

t8m added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Oct 4, 2021

openssl-machine pushed a commit that referenced this pull request Oct 4, 2021

Fix ssl_free() and thus BIO_free() to respect BIO_NOCLOSE

dce910a

Reviewed-by: Tomas Mraz <[email protected]> (Merged from #16688)

openssl-machine pushed a commit that referenced this pull request Oct 4, 2021

apps/lib/s_socket.c: Fix mem leak on host name in init_client()

2342d9b

Reviewed-by: Tomas Mraz <[email protected]> (Merged from #16688)

openssl-machine pushed a commit that referenced this pull request Oct 4, 2021

BIO_f_ssl.pod: Make clear where an SSL BIOs are expected as an argument

34901b0

Reviewed-by: Tomas Mraz <[email protected]> (Merged from #16688)

openssl-machine pushed a commit that referenced this pull request Oct 4, 2021

Fix ssl_free() and thus BIO_free() to respect BIO_NOCLOSE

0569756

Reviewed-by: Tomas Mraz <[email protected]> (Merged from #16688) (cherry picked from commit dce910a)

openssl-machine pushed a commit that referenced this pull request Oct 4, 2021

BIO_f_ssl.pod: Make clear where an SSL BIOs are expected as an argument

aaf7185

Reviewed-by: Tomas Mraz <[email protected]> (Merged from #16688) (cherry picked from commit 34901b0)

t8m closed this Oct 4, 2021

t8m mentioned this pull request Oct 4, 2021

Mem leak in s_client #16737

Closed

DDvO added a commit to siemens/openssl that referenced this pull request Nov 25, 2021

Fix ssl_free() and thus BIO_free() to respect BIO_NOCLOSE

0351d96

Reviewed-by: Tomas Mraz <[email protected]> (Merged from openssl#16688)

DDvO added a commit to siemens/openssl that referenced this pull request Nov 25, 2021

BIO_f_ssl.pod: Make clear where an SSL BIOs are expected as an argument

01456f3

Reviewed-by: Tomas Mraz <[email protected]> (Merged from openssl#16688)

DDvO reopened this Nov 25, 2021

DDvO mentioned this pull request Nov 25, 2021

Fix ssl_free() and thus BIO_free() to respect BIO_NOCLOSE #16688 - backport to 1.1.1 #17135

Closed

DDvO closed this Nov 25, 2021

Uh oh!

Conversation

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

Uh oh!

Uh oh!

t8m left a comment

Choose a reason for hiding this comment

Uh oh!

DDvO commented Sep 27, 2021

Uh oh!

Uh oh!

DDvO commented Sep 29, 2021

Uh oh!

DDvO commented Sep 29, 2021

Uh oh!

t8m commented Sep 29, 2021

Uh oh!

DDvO commented Sep 29, 2021

Uh oh!

levitte commented Sep 29, 2021

Uh oh!

Uh oh!

DDvO commented Sep 30, 2021

Uh oh!

t8m left a comment

Choose a reason for hiding this comment

Uh oh!

t8m commented Sep 30, 2021

Uh oh!

kaduk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

t8m commented Oct 1, 2021

Uh oh!

t8m commented Oct 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openssl-machine commented Oct 1, 2021

Uh oh!

kaduk commented Oct 2, 2021

Uh oh!

t8m commented Oct 4, 2021

Uh oh!

t8m commented Oct 4, 2021

Uh oh!

DDvO commented Oct 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DDvO commented Oct 5, 2021

Uh oh!

DDvO commented Nov 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

t8m commented Nov 25, 2021

Uh oh!

DDvO commented Nov 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

DDvO commented Sep 27, 2021 •

edited

Loading

t8m commented Oct 1, 2021 •

edited

Loading

DDvO commented Oct 5, 2021 •

edited

Loading

DDvO commented Nov 25, 2021 •

edited

Loading

DDvO commented Nov 25, 2021 •

edited

Loading