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 no xof #24387

Closed
wants to merge 2 commits into from
Closed

rsa no xof #24387

wants to merge 2 commits into from

Conversation

xnox
Copy link
Contributor

@xnox xnox commented May 13, 2024 •
edited

  • rsa-pss: add tests checking for SHAKE usage in RSA-PSS
    FIPS 186-5, RFC 8692, RFC 8702 all agree and specify that Shake shall
    be used directly as MGF (not as a hash in MGF1). Add tests that try to
    specify shake hash as MGF1 to ensure that fails.

    Separately the above standards specify how to use SHAKE as a message
    digest with either fixed or minimum output lengths. However, currently
    shake is not part of allowed hashes.

    Note that rsa_setup_md()/rsa_setup_mgf1_md() call
    ossl_digest_rsa_sign_get_md_nid() ->
    ossl_digest_get_approved_nid_with_sha1() ->
    ossl_digest_get_approved_nid() which only contain sha1/sha2/sha3
    digests without XOF.

    The digest test case will need to be replace if/when shake with
    minimum output lengths is added to ossl_digest_get_approved_nid().

  • rsa-oaep: block SHAKE usage in FIPS mode
    NIST SP 800-56 rev2 only allows using approved hash algorithms in
    OAEP. Unlike FIPS 186-5 it doesn't have text allowing to use XOF SHAKE
    functions. Maybe future revisions of SP 800-56 will adopt similar text
    to FIPS 186-5 and allow XOF as MD and MGF (not MGF1).

    RFC documents do not specify if SHAKE is allowed or blocked for usage
    (i.e. there is no equivalent of RFC 8692 or RFC 8702 for OAEP). Status
    quo allows their usage.

    Add test cases for SHAKE in RSA-OAEP as allowed in default provider,
    and blocked in fips.

xnox added 2 commits May 13, 2024 18:23
@xnox
rsa-pss: add tests checking for SHAKE usage in RSA-PSS
4e47c4c 
FIPS 186-5, RFC 8692, RFC 8702 all agree and specify that Shake shall
be used directly as MGF (not as a hash in MGF1). Add tests that try to
specify shake hash as MGF1 to ensure that fails.

Separately the above standards specify how to use SHAKE as a message
digest with either fixed or minimum output lengths. However, currently
shake is not part of allowed hashes.

Note that rsa_setup_md()/rsa_setup_mgf1_md() call
ossl_digest_rsa_sign_get_md_nid() ->
ossl_digest_get_approved_nid_with_sha1() ->
ossl_digest_get_approved_nid() which only contain sha1/sha2/sha3
digests without XOF.

The digest test case will need to be replace if/when shake with
minimum output lengths is added to ossl_digest_get_approved_nid().
@xnox
rsa-oaep: block SHAKE usage in FIPS mode
93c69c8 
NIST SP 800-56 rev2 only allows using approved hash algorithms in
OAEP. Unlike FIPS 186-5 it doesn't have text allowing to use XOF SHAKE
functions. Maybe future revisions of SP 800-56 will adopt similar text
to FIPS 186-5 and allow XOF as MD and MGF (not MGF1).

RFC documents do not specify if SHAKE is allowed or blocked for usage
(i.e. there is no equivalent of RFC 8692 or RFC 8702 for OAEP). Status
quo allows their usage.

Add test cases for SHAKE in RSA-OAEP as allowed in default provider,
and blocked in fips.
@github-actions github-actions bot added the severity: fips change The pull request changes FIPS provider sources label May 13, 2024
@t8m t8m 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: feature The issue/pr requests/adds a feature tests: present The PR has suitable tests present labels May 13, 2024
t8m
t8m approved these changes May 13, 2024
View reviewed changes
Copy link
Member

@t8m t8m left a comment

Choose a reason for hiding this comment

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

This is an example of functionality where I do not expect any real-world use-case breakage. So IMO OK as is.

@t8m t8m removed the approval: otc review pending This pull request needs review by an OTC member label May 13, 2024
@xnox
Copy link
Contributor Author

xnox commented May 13, 2024

This is an example of functionality where I do not expect any real-world use-case breakage. So IMO OK as is.

Yes this is way too new and way too niche to be out there in practice. And many modules that did submit with shake support, did caught this and blocked it already. (stuff pre 186-5 being published). And I don't see anybody implementing 186-5 stuff yet. I.e. I don't see any libraries using shake as MGF directly - maybe they are all waiting for OpenSSL to implement it =)

@paulidale
Copy link
Contributor

This will require an OTC decision since it is a breaking change.
We've had similar niche cases in the past which were not allowed by the OTC (thinking truncated digests and DRBGs).

@paulidale paulidale added the hold: need otc decision The OTC needs to make a decision label May 14, 2024
@paulidale
Copy link
Contributor

OTC: should we condone this break in the API or not?

In the past we've been conservative with respect to potential real world usage. This PR is asking for the opposite.

@slontis
Copy link
Member

slontis commented May 14, 2024

Should this have a CHANGES.md entry?

@t8m
Copy link
Member

t8m commented May 14, 2024

OTC: We are OK with this not requiring a way to enable in FIPS MODULE.

The reason is that it is unlikely to be used by applications and for the eventual rare use-case as this is non-approved the fallback is to use the default provider.

@t8m t8m removed the hold: need otc decision The OTC needs to make a decision label May 14, 2024
@xnox
Copy link
Contributor Author

xnox commented May 20, 2024

@t8m should label "approval:review pending" be replaced with "approval:done" given the above comment?

@paulidale
Copy link
Contributor

No, it still needs a second review.

OTC decisions and reviews are independent generally.

beldmit
beldmit approved these changes May 21, 2024
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 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 May 21, 2024
@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 May 22, 2024
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

@t8m
Copy link
Member

t8m commented May 22, 2024

Merged to the master branch. Thank you for your contribution.

@t8m t8m closed this May 22, 2024
openssl-machine pushed a commit that referenced this pull request May 22, 2024
@xnox @t8m
rsa-pss: add tests checking for SHAKE usage in RSA-PSS
973ddaa 
FIPS 186-5, RFC 8692, RFC 8702 all agree and specify that Shake shall
be used directly as MGF (not as a hash in MGF1). Add tests that try to
specify shake hash as MGF1 to ensure that fails.

Separately the above standards specify how to use SHAKE as a message
digest with either fixed or minimum output lengths. However, currently
shake is not part of allowed hashes.

Note that rsa_setup_md()/rsa_setup_mgf1_md() call
ossl_digest_rsa_sign_get_md_nid() ->
ossl_digest_get_approved_nid_with_sha1() ->
ossl_digest_get_approved_nid() which only contain sha1/sha2/sha3
digests without XOF.

The digest test case will need to be replace if/when shake with
minimum output lengths is added to ossl_digest_get_approved_nid().

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #24387)
openssl-machine pushed a commit that referenced this pull request May 22, 2024
@xnox @t8m
rsa-oaep: block SHAKE usage in FIPS mode
1bfc8d1 
NIST SP 800-56 rev2 only allows using approved hash algorithms in
OAEP. Unlike FIPS 186-5 it doesn't have text allowing to use XOF SHAKE
functions. Maybe future revisions of SP 800-56 will adopt similar text
to FIPS 186-5 and allow XOF as MD and MGF (not MGF1).

RFC documents do not specify if SHAKE is allowed or blocked for usage
(i.e. there is no equivalent of RFC 8692 or RFC 8702 for OAEP). Status
quo allows their usage.

Add test cases for SHAKE in RSA-OAEP as allowed in default provider,
and blocked in fips.

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #24387)
@xnox
Copy link
Contributor Author

xnox commented May 22, 2024

is this now breaking all mainline builds? seems like everything is red =( will check.

xnox added a commit to xnox/openssl that referenced this pull request May 22, 2024
@xnox
test: remove the just added, but now unrealistic, shake128 OAEP tests
510f164 
These were added as a POC in openssl#24387. However, such combinations are no
longer unusable since openssl#24105 got merged.

This should unbreak all build failures on mainline.

Partially reverts: 1bfc8d1 (rsa-oaep: block SHAKE usage in FIPS
mode, 2024-05-13)

Signed-off-by: Dimitri John Ledkov <dimitri.ledkov@surgut.co.uk>
@xnox xnox mentioned this pull request May 22, 2024
Closed
openssl-machine pushed a commit that referenced this pull request May 22, 2024
@xnox @t8m
test: remove the just added, but now unrealistic, shake128 OAEP tests
a0da3cb 
These were added as a POC in #24387. However, such combinations are no
longer unusable since #24105 got merged.

This should unbreak all build failures on mainline.

Partially reverts: 1bfc8d1 (rsa-oaep: block SHAKE usage in FIPS
mode, 2024-05-13)

Signed-off-by: Dimitri John Ledkov <dimitri.ledkov@surgut.co.uk>

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #24463)
jvdsn pushed a commit to jvdsn/openssl that referenced this pull request Jun 3, 2024
@xnox @jvdsn
rsa-pss: add tests checking for SHAKE usage in RSA-PSS
70a0415 
FIPS 186-5, RFC 8692, RFC 8702 all agree and specify that Shake shall
be used directly as MGF (not as a hash in MGF1). Add tests that try to
specify shake hash as MGF1 to ensure that fails.

Separately the above standards specify how to use SHAKE as a message
digest with either fixed or minimum output lengths. However, currently
shake is not part of allowed hashes.

Note that rsa_setup_md()/rsa_setup_mgf1_md() call
ossl_digest_rsa_sign_get_md_nid() ->
ossl_digest_get_approved_nid_with_sha1() ->
ossl_digest_get_approved_nid() which only contain sha1/sha2/sha3
digests without XOF.

The digest test case will need to be replace if/when shake with
minimum output lengths is added to ossl_digest_get_approved_nid().

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#24387)
jvdsn pushed a commit to jvdsn/openssl that referenced this pull request Jun 3, 2024
@xnox @jvdsn
rsa-oaep: block SHAKE usage in FIPS mode
c8a5d41 
NIST SP 800-56 rev2 only allows using approved hash algorithms in
OAEP. Unlike FIPS 186-5 it doesn't have text allowing to use XOF SHAKE
functions. Maybe future revisions of SP 800-56 will adopt similar text
to FIPS 186-5 and allow XOF as MD and MGF (not MGF1).

RFC documents do not specify if SHAKE is allowed or blocked for usage
(i.e. there is no equivalent of RFC 8692 or RFC 8702 for OAEP). Status
quo allows their usage.

Add test cases for SHAKE in RSA-OAEP as allowed in default provider,
and blocked in fips.

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#24387)
jvdsn pushed a commit to jvdsn/openssl that referenced this pull request Jun 3, 2024
@xnox @jvdsn
test: remove the just added, but now unrealistic, shake128 OAEP tests
2187f74 
These were added as a POC in openssl#24387. However, such combinations are no
longer unusable since openssl#24105 got merged.

This should unbreak all build failures on mainline.

Partially reverts: 1bfc8d1 (rsa-oaep: block SHAKE usage in FIPS
mode, 2024-05-13)

Signed-off-by: Dimitri John Ledkov <dimitri.ledkov@surgut.co.uk>

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#24463)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@t8m t8m t8m approved these changes

@beldmit beldmit beldmit 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: feature The issue/pr requests/adds a feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@xnox @paulidale @slontis @t8m @openssl-machine @beldmit