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 Segfault in EVP_PKEY_CTX_dup when the ctx has an undefined operation. #13505

Closed
wants to merge 3 commits into from
Closed

Fix Segfault in EVP_PKEY_CTX_dup when the ctx has an undefined operation. #13505

wants to merge 3 commits into from

Conversation

slontis
Copy link
Member

@slontis slontis commented Nov 25, 2020

Fixes #12438

Note: This worked in 1.1.1 so just returning an error is not valid.

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

@slontis slontis added branch: master Merge to master branch approval: review pending This pull request needs review by a committer labels Nov 25, 2020
@slontis slontis added this to the 3.0.0 beta1 milestone Nov 25, 2020
@slontis slontis requested a review from levitte November 25, 2020 05:45
@slontis
Copy link
Member Author

slontis commented Nov 25, 2020 •
edited

@levitte I am not sure if this is the correct way to dup the keymt pointer.. I have tried to do something similar to what other 'defined' operations do.

slontis
slontis commented Nov 25, 2020
View reviewed changes
crypto/evp/pmeth_lib.c
}
if (pctx->op.kex.exchprovctx != NULL) {
if (!ossl_assert(pctx->op.kex.exchange != NULL))
return NULL;
goto end;
Copy link
Member Author

Choose a reason for hiding this comment

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

This one and others were a potential memory leak..

@slontis slontis added the triaged: OTC evaluated This issue/pr was triaged by OTC label Dec 1, 2020
@paulnelsontx paulnelsontx added this to Triaged in 3.0.0 estimator Dec 1, 2020
@slontis slontis closed this Dec 2, 2020
3.0.0 estimator automation moved this from Triaged to Done Dec 2, 2020
@slontis slontis reopened this Dec 2, 2020
3.0.0 estimator automation moved this from Done to In progress Dec 2, 2020
@paulnelsontx paulnelsontx moved this from In progress to review pending in 3.0.0 estimator Dec 2, 2020
@slontis
Copy link
Member Author

slontis commented Dec 7, 2020

rebased

@slontis
Copy link
Member Author

slontis commented Dec 9, 2020

ping

kaduk
kaduk approved these changes Dec 10, 2020
View reviewed changes
Copy link
Contributor

@kaduk kaduk left a comment

Choose a reason for hiding this comment

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

I'm not fully up to speed with all of this stuff (e.g., evp_pkey_export_to_provider()) but the memory-leak fixes are right, and I convinced myself that it's not harmful and probably addresses the indicated issue. (Also, it does fix the test case it added.)
Approved, with nit fix to be made while merging.

crypto/evp/pmeth_lib.c Outdated Show resolved Hide resolved
doc/man3/EVP_PKEY_CTX_new.pod Outdated Show resolved Hide resolved
@kaduk kaduk added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Dec 11, 2020
@slontis
Copy link
Member Author

slontis commented Dec 11, 2020

Updated to address @kaduk comments.

kaduk
kaduk approved these changes Dec 11, 2020
View reviewed changes
Copy link
Contributor

@kaduk kaduk left a comment

Choose a reason for hiding this comment

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

Reconfirm +1 -- thanks for the fixups

@openssl-machine
Copy link
Collaborator

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 kaduk 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 Dec 12, 2020
openssl-machine pushed a commit that referenced this pull request Dec 14, 2020
@slontis
Fix Segfault in EVP_PKEY_CTX_dup when the ctx has an undefined operat…
ac7750b 
…ion.

Fixes #12438

Note: This worked in 1.1.1 so just returning an error is not valid.

Reviewed-by: Ben Kaduk <kaduk@mit.edu>
(Merged from #13505)
@slontis
Copy link
Member Author

slontis commented Dec 14, 2020

Thanks. Merged to master.

@slontis slontis closed this Dec 14, 2020
3.0.0 estimator automation moved this from review pending to Done Dec 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@t8m t8m t8m left review comments

@kaduk kaduk kaduk approved these changes

@levitte levitte Awaiting requested review from levitte

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 triaged: OTC evaluated This issue/pr was triaged by OTC
Projects
No open projects
3.0.0 estimator
Done
Milestone
3.0.0 beta1
Development

Successfully merging this pull request may close these issues.

EVP_PKEY_CTX_dup Segmentation fault
4 participants
@slontis @openssl-machine @kaduk @t8m