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

DH: Make DH_bits(), DH_size(), and DH_security_bits() check that there are key parameters #13955

Closed
wants to merge 1 commit into from

Conversation

sahanaprasad07
Copy link

@sahanaprasad07 sahanaprasad07 commented Jan 25, 2021

Fixes #13569
Signed-off-by: Sahana Prasad sahana@redhat.com

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

@sahanaprasad07 sahanaprasad07 changed the title DH: Make DH_bits() and DH_size() check that there are key parameters WIP : DH: Make DH_bits() and DH_size() check that there are key parameters Jan 25, 2021
Copy link
Member

@beldmit beldmit left a comment

Choose a reason for hiding this comment

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

LGTM

@beldmit beldmit added approval: otc review pending This pull request needs review by an OTC member branch: master Merge to master branch labels Jan 25, 2021
@sahanaprasad07 sahanaprasad07 changed the title WIP : DH: Make DH_bits() and DH_size() check that there are key parameters DH: Make DH_bits() and DH_size() check that there are key parameters Jan 25, 2021
@t8m
Copy link
Member

t8m commented Jan 25, 2021

The #13569 is not fully fixed by this. You need to also add the check to the DH_security_bits() call. It would be also nice if you could somehow somewhere incorporate the testcase from #13569.

@sahanaprasad07 sahanaprasad07 changed the title DH: Make DH_bits() and DH_size() check that there are key parameters DH: Make DH_bits(), DH_size(), and DH_security_bits() check that there are key parameters Jan 25, 2021
…e are key parameters

Fixes openssl#13569
Signed-off-by: Sahana Prasad <sahana@redhat.com>
@kroeckx
Copy link
Member

kroeckx commented Jan 25, 2021 via email

@beldmit
Copy link
Member

beldmit commented Jan 25, 2021

The fix is similar to #13611. I don't see any reason for these functions to behave differently

@sahanaprasad07 sahanaprasad07 changed the title DH: Make DH_bits(), DH_size(), and DH_security_bits() check that there are key parameters WIP: DH: Make DH_bits(), DH_size(), and DH_security_bits() check that there are key parameters Jan 26, 2021
@sahanaprasad07
Copy link
Author

The #13569 is not fully fixed by this. You need to also add the check to the DH_security_bits() call. It would be also nice if you could somehow somewhere incorporate the testcase from #13569.

I will try to add the test.

@t8m
Copy link
Member

t8m commented Jan 26, 2021

@kroeckx A crash is also a failure, isn't it? Anyway, you're right that #13569 indicates some deeper issue in the decoder or related code.

@petrovr
Copy link

petrovr commented Feb 8, 2021 via email

@levitte
Copy link
Member

levitte commented Feb 17, 2021

I'm looking at this PR now, and while I can understand the simplicity, it also indicates a deeper problem. Basically, those parameters should never be NULL, so the problem may lie elsewhere. Incidently, #14191 fixes some problems we have observed, and we have issue #14192 that takes up another aspect where the decoder process lacks precision. I wouldn't be surprised if #14191 is enough a change that trying #13569 with that change gave a different outcome.

@t8m
Copy link
Member

t8m commented Feb 17, 2021

I agree that the true fix for the #13569 lies elsewhere. However that does not mean this PR cannot stand by itself. Currently if you call DH_size() on dh returned directly from DH_new(), it will crash. And IMO that's something that can be corrected on master.

@levitte
Copy link
Member

levitte commented Feb 17, 2021

Currently if you call DH_size() on dh returned directly from DH_new(), it will crash. And IMO that's something that can be corrected on master.

Ah, that's a good point. So yeahok, and that light, I see no reason not to approve this.

@levitte levitte added approval: done This pull request has the required number of approvals and removed approval: otc review pending This pull request needs review by an OTC member labels Feb 17, 2021
@t8m t8m changed the title WIP: DH: Make DH_bits(), DH_size(), and DH_security_bits() check that there are key parameters DH: Make DH_bits(), DH_size(), and DH_security_bits() check that there are key parameters Feb 17, 2021
@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 Feb 18, 2021
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

openssl-machine pushed a commit that referenced this pull request Feb 18, 2021
…e are key parameters

Fixes #13569
Signed-off-by: Sahana Prasad <sahana@redhat.com>

Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
(Merged from #13955)
@beldmit
Copy link
Member

beldmit commented Feb 18, 2021

Merged. Many thanks!

@beldmit beldmit closed this Feb 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Segfault loading DH PEM key with FIPS + base provider 3.0.0alpha9
8 participants