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.

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

Don't free the EVP_PKEY on error in set0_tmp_dh_pkey() functions #17209

Closed
wants to merge 1 commit into from

Conversation

mattcaswell
Copy link
Member

We should not be freeing the caller's key in the event of error.

Fixes #17196

We should not be freeing the caller's key in the event of error.

Fixes openssl#17196
@mattcaswell mattcaswell added branch: master Merge to master branch approval: review pending This pull request needs review by a committer branch: 3.0 Merge to openssl-3.0 branch labels Dec 6, 2021
@t8m t8m added approval: done This pull request has the required number of approvals triaged: bug The issue/pr is/fixes a bug and removed approval: review pending This pull request needs review by a committer labels Dec 6, 2021
@vdukhovni
Copy link

I should perhaps ask whether the security level check should avoid a time of check vs. time of use issue?

Currently the DH key is checked against the security level that's effective at the time at which the key is configured, but the security level can change after that, perhaps for a specific SSL connection, ...

I don't know whether an already configured key is rechecked at time of use? The right thing to do may be to ignore out of spec keys (as if not configured), or (more intrusive) fail the attempted handshake? But a check at time of configuration may not quite provide the expected semantics?

@paulidale
Copy link
Contributor

@vdukhovni, that sounds like a different issue to this.

@vdukhovni
Copy link

@vdukhovni, that sounds like a different issue to this.

Yes, I stand by the approval on the pull request. But it just occurred to me while I was thinking about the semantics, and thought I'd mention it before I forget. If you think it may warrant further discussion, we could open a new ticket, but if it seems unimportant, we could drop it...

@paulidale
Copy link
Contributor

A new ticket seems reasonable. It's not something I'm aware has been discussed previously and it's not clear what semantics make the most sense.

@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.

@mattcaswell
Copy link
Member Author

Pushed. Thanks.

@mattcaswell mattcaswell closed this Dec 7, 2021
openssl-machine pushed a commit that referenced this pull request Dec 7, 2021
We should not be freeing the caller's key in the event of error.

Fixes #17196

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Viktor Dukhovni <viktor@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #17209)
openssl-machine pushed a commit that referenced this pull request Dec 7, 2021
We should not be freeing the caller's key in the event of error.

Fixes #17196

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

(cherry picked from commit e819b57)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: done This pull request has the required number of approvals branch: master Merge to master branch branch: 3.0 Merge to openssl-3.0 branch triaged: bug The issue/pr is/fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SSL_CTX_set0_tmp_dh_pkey and SSL_set0_tmp_dh_pkey erroneously free provided key in error path
5 participants