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

Make CMAC properly fail if cipher is not CBC mode one #19401

Closed
wants to merge 5 commits into from
Closed

Make CMAC properly fail if cipher is not CBC mode one #19401

wants to merge 5 commits into from

Conversation

t8m
Copy link
Member

@t8m t8m commented Oct 12, 2022

Also avoid misleading users in the demo by use of the alias name.

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

@t8m
demos/mac/cmac-aes256: Clarify the cipher algorithm used
c673164 
The currently used cipher is aes256 which is an alias to AES-256-CBC,
so the demo is correct.
However it might be misleading so make it clear the CBC mode
cipher is used.
@t8m
cmac_set_ctx_params(): Fail if cipher mode is not CBC
7ccf045 
Also add negative test cases for CMAC and GMAC using
a cipher with wrong mode.
@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: bug The issue/pr is/fixes a bug triaged: documentation The issue/pr deals with documentation (errors) branch: 3.0 Merge to openssl-3.0 branch labels Oct 12, 2022
@t8m
Copy link
Member Author

t8m commented Oct 12, 2022 •
edited

I think it is a bug that we allow non-CBC-mode ciphers with CMAC as that is completely insecure use of the crypto primitive. So I propose this should be also added to 3.0.

beldmit
beldmit approved these changes Oct 12, 2022
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 if tests passed

@beldmit beldmit removed the approval: otc review pending This pull request needs review by an OTC member label Oct 12, 2022
@github-actions github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Oct 12, 2022
@t8m
Copy link
Member Author

t8m commented Oct 12, 2022

Heh, we misuse CMAC in evp_extra_test.c. Fixup commit added.

@t8m
Copy link
Member Author

t8m commented Oct 12, 2022

@beldmit still OK?

@beldmit
Copy link
Member

beldmit commented Oct 12, 2022

Yes, still OK if tests passed

@mattcaswell
Copy link
Member

we allow non-CBC-mode ciphers with CMAC

Is this in the RFC? So CFB/OFB modes, for example, are not ok? I'm wondering how breaking this change might end up being if we backport to 3.0...

@mattcaswell
Copy link
Member

CI failure looks relevant

@t8m
Copy link
Member Author

t8m commented Oct 12, 2022

Is this in the RFC? So CFB/OFB modes, for example, are not ok? I'm wondering how breaking this change might end up being if we backport to 3.0...

This is by definition of the CMAC. https://en.wikipedia.org/wiki/One-key_MAC
And yes, it is also in the AES-CMAC definition in RFC 4493.

@t8m
Copy link
Member Author

t8m commented Oct 12, 2022 •
edited

Is this in the RFC? So CFB/OFB modes, for example, are not ok? I'm wondering how breaking this change might end up being if we backport to 3.0...

In theory one could use the CMAC MAC with other mode cipher but the result would not be interoperable and with completely bogus security properties - potentially even completely insecure one.

@mattcaswell
Copy link
Member

In theory one could use the CMAC MAC with other mode cipher but the result would not be interoperable and with completely bogus security properties - potentially even completely insecure one

That may be the case - but my bet is that this will break people. Not a decision to make without some OTC input IMO.

@mattcaswell
Copy link
Member

mattcaswell commented Oct 12, 2022 •
edited

OTC Question: Should we fix this, and if so in which branches?

@mattcaswell mattcaswell added the hold: need otc decision The OTC needs to make a decision label Oct 12, 2022
@t8m
Copy link
Member Author

t8m commented Oct 12, 2022

That may be the case - but my bet is that this will break people. Not a decision to make without some OTC input IMO.

It will break only those who accidentally have a serious security issue in their code.

And yeah, I agree this should be discussed within OTC.

paulidale
paulidale approved these changes Oct 12, 2022
View reviewed changes
providers/implementations/macs/cmac_prov.c
if (!ossl_prov_cipher_load_from_params(&macctx->cipher, params, ctx))
return 0;

if (EVP_CIPHER_get_mode(ossl_prov_cipher_cipher(&macctx->cipher))
Copy link
Contributor

Choose a reason for hiding this comment

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

Nicely backwards compatible if the returned error is ignored 😃

@paulidale paulidale 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 12, 2022
@openssl-machine openssl-machine removed the approval: done This pull request has the required number of approvals label Oct 13, 2022
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

@openssl-machine openssl-machine added the approval: ready to merge The 24 hour grace period has passed, ready to merge label Oct 13, 2022
@t8m t8m added the branch: 3.1 Merge to openssl-3.1 label Oct 24, 2022
@t8m
Copy link
Member Author

t8m commented Oct 25, 2022

OTC: This is ok for 3.1 and above, not for 3.0.

@t8m t8m removed hold: need otc decision The OTC needs to make a decision branch: 3.0 Merge to openssl-3.0 branch labels Oct 25, 2022
@t8m t8m 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 Nov 10, 2022
@t8m
Copy link
Member Author

t8m commented Nov 10, 2022

Actually this needs re-approval from @beldmit

@beldmit
Copy link
Member

beldmit commented Nov 10, 2022

Still OK

@t8m t8m 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 Nov 10, 2022
@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 Nov 11, 2022
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

@t8m t8m added the tests: present The PR has suitable tests present label Nov 11, 2022
@t8m
Copy link
Member Author

t8m commented Nov 11, 2022

Merged to master and 3.1 branches. Thank you for the reviews.

@t8m t8m closed this Nov 11, 2022
openssl-machine pushed a commit that referenced this pull request Nov 11, 2022
@t8m
demos/mac/cmac-aes256: Clarify the cipher algorithm used
66c4f14 
The currently used cipher is aes256 which is an alias to AES-256-CBC,
so the demo is correct.
However it might be misleading so make it clear the CBC mode
cipher is used.

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #19401)

(cherry picked from commit 9270f67)
openssl-machine pushed a commit that referenced this pull request Nov 11, 2022
@t8m
demos/mac/cmac-aes256: Clarify the cipher algorithm used
9270f67 
The currently used cipher is aes256 which is an alias to AES-256-CBC,
so the demo is correct.
However it might be misleading so make it clear the CBC mode
cipher is used.

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #19401)
openssl-machine pushed a commit that referenced this pull request Nov 11, 2022
@t8m
cmac_set_ctx_params(): Fail if cipher mode is not CBC
d90a4c7 
Also add negative test cases for CMAC and GMAC using
a cipher with wrong mode.

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #19401)

(cherry picked from commit 94976a1)
openssl-machine pushed a commit that referenced this pull request Nov 11, 2022
@t8m
cmac_set_ctx_params(): Fail if cipher mode is not CBC
94976a1 
Also add negative test cases for CMAC and GMAC using
a cipher with wrong mode.

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #19401)
openssl-machine pushed a commit that referenced this pull request Nov 11, 2022
@t8m
test_CMAC_keygen(): Avoid using ECB cipher with CMAC
66ecce2 
Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #19401)

(cherry picked from commit a0783b8)
openssl-machine pushed a commit that referenced this pull request Nov 11, 2022
@t8m
test_CMAC_keygen(): Avoid using ECB cipher with CMAC
a0783b8 
Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #19401)
beldmit pushed a commit to beldmit/openssl that referenced this pull request Dec 26, 2022
@t8m @beldmit
demos/mac/cmac-aes256: Clarify the cipher algorithm used
28876b7 
The currently used cipher is aes256 which is an alias to AES-256-CBC,
so the demo is correct.
However it might be misleading so make it clear the CBC mode
cipher is used.

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from openssl#19401)
beldmit pushed a commit to beldmit/openssl that referenced this pull request Dec 26, 2022
@t8m @beldmit
cmac_set_ctx_params(): Fail if cipher mode is not CBC
d3853b0 
Also add negative test cases for CMAC and GMAC using
a cipher with wrong mode.

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from openssl#19401)
beldmit pushed a commit to beldmit/openssl that referenced this pull request Dec 26, 2022
@t8m @beldmit
test_CMAC_keygen(): Avoid using ECB cipher with CMAC
03a01be 
Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from openssl#19401)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@beldmit beldmit beldmit approved these changes

@paulidale paulidale paulidale 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 branch: 3.1 Merge to openssl-3.1 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 triaged: documentation The issue/pr deals with documentation (errors)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@t8m @beldmit @mattcaswell @openssl-machine @paulidale