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 DH priv_key when DH_generate_key() fails #96

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
1 participant
@vdukhovni
Copy link

vdukhovni commented Aug 3, 2018

The call to BN_clear_free() on dh->priv_key leaves a dangling pointer
that will be freed again when the caller of dh_gen_key() calls
DH_free().

Fortunately, AFAIK failrues in DH_generate_key() are very unlikely,
so this should not be observed in practice.

Viktor Dukhovni
Don't free DH priv_key when DH_generate_key() fails
The call to BN_clear_free on dh->priv_key leaves a dangling pointer
that will be freed again when the caller of dh_gen_key() calls
DH_free().

Fortunately, AFAIK failrues in DH_generate_key() are very unlikely,
so this should not be observed in practice.

djmdjm added a commit to djmdjm/openbsd-openssh-src that referenced this pull request Aug 4, 2018

invalidate dh->priv_key after freeing it in error path; avoids
unlikely double-free later. Reported by Viktor Dukhovni via
openssh/openssh-portable#96
feedback jsing@ tb@

bob-beck pushed a commit to openbsd/src that referenced this pull request Aug 4, 2018

invalidate dh->priv_key after freeing it in error path; avoids
unlikely double-free later. Reported by Viktor Dukhovni via
openssh/openssh-portable#96
feedback jsing@ tb@

djmdjm added a commit that referenced this pull request Aug 6, 2018

upstream: invalidate dh->priv_key after freeing it in error path;
avoids unlikely double-free later. Reported by Viktor Dukhovni via
#96 feedback jsing@ tb@

OpenBSD-Commit-ID: e317eb17c3e05500ae851f279ef6486f0457c805

cotequeiroz added a commit to cotequeiroz/openssh-portable that referenced this pull request Aug 29, 2018

upstream: invalidate dh->priv_key after freeing it in error path;
avoids unlikely double-free later. Reported by Viktor Dukhovni via
openssh#96 feedback jsing@ tb@

OpenBSD-Commit-ID: e317eb17c3e05500ae851f279ef6486f0457c805
@vdukhovni

This comment has been minimized.

Copy link

vdukhovni commented Aug 30, 2018

I see the dangling pointer has been addressed, but really the better fix IMHO is to let the ephemeral key get freed when the containing DH structure is freed. The present solution of setting the key to NULL will not work with OpenSSL 1.1 where the DH structure becomes opaque.

Please consider not freeing the key instead, it is freshly generated and not sensitive....

@vdukhovni vdukhovni closed this Aug 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment