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

rsa: Add SP800-56Br2 6.4.1.2.1 (3.c) check #22403

Closed
wants to merge 1 commit into from
Closed

rsa: Add SP800-56Br2 6.4.1.2.1 (3.c) check #22403

wants to merge 1 commit into from

Conversation

neverpanic
Copy link
Contributor

The code did not yet check that the length of the RSA key is positive and even.

@beldmit beldmit added branch: master Merge to master branch approval: review pending This pull request needs review by a committer approval: otc review pending This pull request needs review by an OTC member triaged: bug The issue/pr is/fixes a bug labels Oct 16, 2023
@github-actions github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Oct 16, 2023
@beldmit
Copy link
Member

beldmit commented Oct 16, 2023

Should we also backport it to 3.0/3.1? Do we need tests for it?

beldmit
beldmit approved these changes Oct 16, 2023
View reviewed changes
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 removed the approval: otc review pending This pull request needs review by an OTC member label Oct 16, 2023
@neverpanic
Copy link
Contributor Author

Should we also backport it to 3.0/3.1? Do we need tests for it?

I added a test.

@neverpanic
rsa: Add SP800-56Br2 6.4.1.2.1 (3.c) check
0d873f9 
The code did not yet check that the length of the RSA key is positive
and even.

Signed-off-by: Clemens Lang <cllang@redhat.com>
@t8m t8m added the tests: present The PR has suitable tests present label Oct 16, 2023
tom-cosgrove-arm
tom-cosgrove-arm approved these changes Oct 18, 2023
View reviewed changes
Copy link
Contributor

@tom-cosgrove-arm tom-cosgrove-arm left a comment

Choose a reason for hiding this comment

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

LGTM

@tom-cosgrove-arm tom-cosgrove-arm 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 Oct 18, 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 Oct 19, 2023
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

@mattcaswell mattcaswell added approval: review pending This pull request needs review by a committer and removed approval: ready to merge The 24 hour grace period has passed, ready to merge labels Oct 20, 2023
@mattcaswell
Copy link
Member

Needs a reconfirmation from @beldmit or a second approval from someone else.

@beldmit
Copy link
Member

beldmit commented Oct 20, 2023

Reconfirm

@mattcaswell mattcaswell 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 Oct 20, 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 Oct 21, 2023
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

@slontis
Copy link
Member

slontis commented Oct 23, 2023

Looks like this was added in SP800-56B r2 (It was not part of SP800-56Br1)..
My preference would be to omit the nbits <= 0 test since it is redundant, and will never hit this branch due to the test..

@paulidale
Copy link
Contributor

paulidale commented Oct 23, 2023 •
edited

This very much looks like a breaking change of behaviour.
Are we certain that there are no keys out there with an odd modulus length?

I suspect the fix is to condition this on FIPS or add a FIPS setting to enable/disable it.

Yes, this is new in r2.

@t8m
Copy link
Member

t8m commented Oct 23, 2023

I suspect the fix is to condition this on FIPS or add a FIPS setting to enable/disable it.

It was discussed above that this function is never run outside of FIPS module. Apart from the unit test.

@paulidale
Copy link
Contributor

Which would mean a settable flag to alter the behaviour.

@t8m
Copy link
Member

t8m commented Oct 23, 2023

This very much looks like a breaking change of behaviour.
Are we certain that there are no keys out there with an odd modulus length?

IMO there are no such keys generated by any FIPS approved methods ever since they were added in the FIPS 186-x standards. So the only potential breakage is to use a key generated by other means with the FIPS provider.

@mattcaswell
Copy link
Member

Pushed. Thanks.

openssl-machine pushed a commit that referenced this pull request Oct 25, 2023
@neverpanic @mattcaswell
rsa: Add SP800-56Br2 6.4.1.2.1 (3.c) check
8b26854 
The code did not yet check that the length of the RSA key is positive
and even.

Signed-off-by: Clemens Lang <cllang@redhat.com>

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Tom Cosgrove <tom.cosgrove@arm.com>
(Merged from #22403)
@neverpanic neverpanic deleted the cllang-sp800-56br2-64121-3c branch October 25, 2023 10:17
wanghao75 pushed a commit to openeuler-mirror/openssl that referenced this pull request Nov 4, 2023
@neverpanic
rsa: Add SP800-56Br2 6.4.1.2.1 (3.c) check
4b5ada2 
The code did not yet check that the length of the RSA key is positive
and even.

Signed-off-by: Clemens Lang <cllang@redhat.com>

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Tom Cosgrove <tom.cosgrove@arm.com>
(Merged from openssl/openssl#22403)

Signed-off-by: fly2x <fly2x@hitls.org>
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

@beldmit beldmit beldmit approved these changes

@tom-cosgrove-arm tom-cosgrove-arm tom-cosgrove-arm 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 severity: fips change The pull request changes FIPS provider sources tests: present The PR has suitable tests present 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
8 participants
@neverpanic @beldmit @openssl-machine @mattcaswell @slontis @paulidale @t8m @tom-cosgrove-arm