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

Warn to use a constant-time comparison for MAC and AEAD tag #9461

Open
wants to merge 5 commits into
base: development
Choose a base branch
from
Open

Warn to use a constant-time comparison for MAC and AEAD tag #9461

wants to merge 5 commits into from

Conversation

gilles-peskine-arm
Copy link
Contributor

Documentation improvements related to MAC and AEAD:

  • Mention mbedtls_ct_memcmp() in the documentation of functions that return an authentication tag. Closes mbedtls should provide a constant time MD / HMAC check function #3040.
  • Don't call Poly1305 a MAC. It's only a one-time authenticator.
  • Multipart CCM is implemented in mbedtls_ccm_xxx (but not mbedtls_cipher_xxx). We forgot to remove the notes that say that it isn't implemented yet.

PR checklist

  • changelog not required because: doc only
  • development PR here
  • framework PR not required
  • 3.6 PR TODO
  • 2.28 PR TODO
  • tests not required because: doc only

Notes for the submitter

Please refer to the contributing guidelines, especially the
checklist for PR contributors.

Help make review efficient:

  • Multiple simple commits
    • please structure your PR into a series of small commits, each of which does one thing
  • Avoid force-push
    • please do not force-push to update your PR - just add new commit(s)
  • See our Guidelines for Contributors for more details about the review process.

@gilles-peskine-arm
Warn to use constant-time memcmp when checking a MAC or tag
cf4e4d1 
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
@gilles-peskine-arm
Poly1305 is not a MAC, it's a one-time authenticator
5242774 
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
@gilles-peskine-arm
Multipart CCM has been implemented since 3.1.0
3509535 
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
@gilles-peskine-arm gilles-peskine-arm added enhancement needs-review Every commit must be reviewed by at least two team members, component-crypto Crypto primitives and low-level interfaces needs-reviewer This PR needs someone to pick it up for review priority-medium Medium priority - this can be reviewed as time permits size-xs Estimated task size: extra small (a few hours at most) labels Aug 8, 2024
noncombatant
noncombatant reviewed Oct 30, 2024
View reviewed changes
Copy link

@noncombatant noncombatant left a comment

Choose a reason for hiding this comment

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

I can't Approve, but looks good to me with a suggestion.

tf-psa-crypto/drivers/builtin/include/mbedtls/chachapoly.h Outdated
*
* \param ctx The ChaCha20-Poly1305 context to use. This must be initialized.
* \param mac The buffer to where the 128-bit (16 bytes) MAC is written.
* \param mac The buffer to where the 128-bit (16 bytes) authentication
Copy link

Choose a reason for hiding this comment

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

Tiny nit: I might say "(16-byte)"

tf-psa-crypto/drivers/builtin/include/mbedtls/chachapoly.h Outdated
@@ -270,8 +275,8 @@ int mbedtls_chachapoly_finish(mbedtls_chachapoly_context *ctx,
* This pointer can be \c NULL if `ilen == 0`.
* \param output The buffer to where the encrypted or decrypted data
* is written. This pointer can be \c NULL if `ilen == 0`.
* \param tag The buffer to where the computed 128-bit (16 bytes) MAC
* is written. This must not be \c NULL.
* \param tag The buffer to where the computed 128-bit (16 bytes)
Copy link

Choose a reason for hiding this comment

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

same

@gilles-peskine-arm
Wording nit
02199a6 
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
noncombatant
noncombatant reviewed Nov 1, 2024
View reviewed changes
tf-psa-crypto/drivers/builtin/include/mbedtls/chachapoly.h Outdated
@@ -230,13 +230,18 @@ int mbedtls_chachapoly_update(mbedtls_chachapoly_context *ctx,

/**
* \brief This function finished the ChaCha20-Poly1305 operation and
Copy link

Choose a reason for hiding this comment

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

Typo: should be "finishes" (and then "generates the authentication tag" below).

@gilles-peskine-arm
Fix typos
72799e3 
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
noncombatant
noncombatant reviewed Dec 3, 2024
View reviewed changes
Copy link

@noncombatant noncombatant left a comment

Choose a reason for hiding this comment

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

Looks good to me.

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

@noncombatant noncombatant noncombatant left review comments

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
component-crypto Crypto primitives and low-level interfaces enhancement needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review priority-medium Medium priority - this can be reviewed as time permits size-xs Estimated task size: extra small (a few hours at most)
Projects
Non-roadmap pull requests
Status: In Development
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

mbedtls should provide a constant time MD / HMAC check function
2 participants
@gilles-peskine-arm @noncombatant