Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix memory leaks in ssl_old_test.c #22060

Closed
wants to merge 1 commit into from
Closed

Fix memory leaks in ssl_old_test.c #22060

wants to merge 1 commit into from

Conversation

bernd-edlinger
Copy link
Member

This fixes a few memory leaks reported in #22049.

If SSL_CTX_set0_tmp_dh_pkey rejects the temp dh key due to security restrictions (even when @SECLEVEL=0 is used!) then the caller has to delete the PKEY object.
That is different to how the deprecated
SSL_CTX_set_tmp_dh_pkey was designed to work.

Checklist
  • documentation is added or updated
  • tests are added or updated

@bernd-edlinger
Fix memory leaks in ssl_old_test.c
08cc5bc 
This fixes a few memory leaks reported in openssl#22049.

If SSL_CTX_set0_tmp_dh_pkey rejects the temp dh key
due to security restrictions (even when @SECLEVEL=0 is used!)
then the caller has to delete the PKEY object.
That is different to how the deprecated
SSL_CTX_set_tmp_dh_pkey was designed to work.
@bernd-edlinger bernd-edlinger added branch: master Merge to master branch branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 labels Sep 11, 2023
@t8m t8m added approval: review pending This pull request needs review by a committer triaged: bug The issue/pr is/fixes a bug tests: exempted The PR is exempt from requirements for testing labels Sep 11, 2023
nhorman
nhorman reviewed Sep 11, 2023
View reviewed changes
test/ssl_old_test.c
if (!SSL_CTX_set0_tmp_dh_pkey(s_ctx, dhpkey))
EVP_PKEY_free(dhpkey);
if (!SSL_CTX_set0_tmp_dh_pkey(s_ctx2, dhpkey))
EVP_PKEY_free(dhpkey);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be a nit, but It seems to me like the issue is that we're not properly passing ownership of the key to the ssl context, opting instead to increase the refcount under the assumption that both SSL_CTX_set0_tmp_dh_pkey calls will succeed.

would it be better to create an SSL_CTX_set1_tmp_dh_pkey function that properly increases the refcount of the passed in key iff the key is actually assigned? Then would could just cause the failure here to goto err and free the EVP_PKEY once at the exit to the function, rather than having to worry about the need to free this after each failed call where its referenced?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given this is a fairly legacy API (not deprecated but still), I would not bother.

@paulidale paulidale added the approval: done This pull request has the required number of approvals label Sep 12, 2023
@openssl-machine openssl-machine 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 Sep 13, 2023
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

@t8m t8m removed the approval: review pending This pull request needs review by a committer label Sep 13, 2023
@bernd-edlinger
Copy link
Member Author

Merged to all branches. Thanks!

openssl-machine pushed a commit that referenced this pull request Sep 13, 2023
@bernd-edlinger
Fix memory leaks in ssl_old_test.c
ba3c334 
This fixes a few memory leaks reported in #22049.

If SSL_CTX_set0_tmp_dh_pkey rejects the temp dh key
due to security restrictions (even when @SECLEVEL=0 is used!)
then the caller has to delete the PKEY object.
That is different to how the deprecated
SSL_CTX_set_tmp_dh_pkey was designed to work.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #22060)

(cherry picked from commit 21f0b80)
openssl-machine pushed a commit that referenced this pull request Sep 13, 2023
@bernd-edlinger
Fix memory leaks in ssl_old_test.c
21f0b80 
This fixes a few memory leaks reported in #22049.

If SSL_CTX_set0_tmp_dh_pkey rejects the temp dh key
due to security restrictions (even when @SECLEVEL=0 is used!)
then the caller has to delete the PKEY object.
That is different to how the deprecated
SSL_CTX_set_tmp_dh_pkey was designed to work.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #22060)
openssl-machine pushed a commit that referenced this pull request Sep 13, 2023
@bernd-edlinger
Fix memory leaks in ssl_old_test.c
1362474 
This fixes a few memory leaks reported in #22049.

If SSL_CTX_set0_tmp_dh_pkey rejects the temp dh key
due to security restrictions (even when @SECLEVEL=0 is used!)
then the caller has to delete the PKEY object.
That is different to how the deprecated
SSL_CTX_set_tmp_dh_pkey was designed to work.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #22060)

(cherry picked from commit 21f0b80)
wanghao75 pushed a commit to openeuler-mirror/openssl that referenced this pull request Sep 23, 2023
@bernd-edlinger
Fix memory leaks in ssl_old_test.c
4b59957 
This fixes a few memory leaks reported in #22049.

If SSL_CTX_set0_tmp_dh_pkey rejects the temp dh key
due to security restrictions (even when @SECLEVEL=0 is used!)
then the caller has to delete the PKEY object.
That is different to how the deprecated
SSL_CTX_set_tmp_dh_pkey was designed to work.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from openssl/openssl#22060)

Signed-off-by: fly2x <fly2x@hitls.org>
wanghao75 pushed a commit to openeuler-mirror/openssl that referenced this pull request Sep 23, 2023
@bernd-edlinger
Fix memory leaks in ssl_old_test.c
561e9ee 
This fixes a few memory leaks reported in #22049.

If SSL_CTX_set0_tmp_dh_pkey rejects the temp dh key
due to security restrictions (even when @SECLEVEL=0 is used!)
then the caller has to delete the PKEY object.
That is different to how the deprecated
SSL_CTX_set_tmp_dh_pkey was designed to work.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from openssl/openssl#22060)

(cherry picked from commit 21f0b80)
Signed-off-by: fly2x <fly2x@hitls.org>
wanghao75 pushed a commit to openeuler-mirror/openssl that referenced this pull request Sep 23, 2023
@bernd-edlinger
Fix memory leaks in ssl_old_test.c
472b1f4 
This fixes a few memory leaks reported in #22049.

If SSL_CTX_set0_tmp_dh_pkey rejects the temp dh key
due to security restrictions (even when @SECLEVEL=0 is used!)
then the caller has to delete the PKEY object.
That is different to how the deprecated
SSL_CTX_set_tmp_dh_pkey was designed to work.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from openssl/openssl#22060)

(cherry picked from commit 21f0b80)
Signed-off-by: fly2x <fly2x@hitls.org>
xl32 pushed a commit to xl32/openssl that referenced this pull request Sep 29, 2023
@bernd-edlinger @xl32
Fix memory leaks in ssl_old_test.c
770c9e6 
This fixes a few memory leaks reported in openssl#22049.

If SSL_CTX_set0_tmp_dh_pkey rejects the temp dh key
due to security restrictions (even when @SECLEVEL=0 is used!)
then the caller has to delete the PKEY object.
That is different to how the deprecated
SSL_CTX_set_tmp_dh_pkey was designed to work.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from openssl#22060)

(cherry picked from commit 21f0b80)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nhorman nhorman nhorman left review comments

@t8m t8m t8m approved these changes

@paulidale paulidale paulidale approved these changes

Assignees
No one assigned
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 tests: exempted The PR is exempt from requirements for testing triaged: bug The issue/pr is/fixes a bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@bernd-edlinger @openssl-machine @nhorman @t8m @paulidale