-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Added ARIA Algo switch case in cmac cipher start #8763
base: development
Are you sure you want to change the base?
Conversation
Signed-off-by: jk-arm <jothikumar.mani@arm.com>
library/cmac.c
Outdated
@@ -178,6 +178,7 @@ int mbedtls_cipher_cmac_starts(mbedtls_cipher_context_t *ctx, | |||
|
|||
switch (type) { | |||
case MBEDTLS_CIPHER_AES_128_ECB: | |||
case MBEDTLS_CIPHER_ARIA_128_ECB: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without looking any further at this, this indentation appears totally wrong
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
corrected the same now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, we should add #if defined(MBEDTLS_ARIA_C)
(and similar for AES) around the switch cases to save on code size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@daverodgman added the same, just ignore my multiple empty commits, there were some issues on my side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add defines for AES and DES while you are there - thanks!
Signed-off-by: jk-arm <jothikumar.mani@arm.com>
I guess the aim is that this PR is sufficient to enable us to perform ARIA-CMAC via the cipher interface? If so, please add tests in What is the impact for PSA - does this also enable for PSA? If so, we should add tests for that as well. |
We also need to update the docs in |
What's the context for this PR please? Do we have an external request to add support for this, or is this connected to the PSA arch tests? |
Hi Dave, i am the psa-arch-test proj lead, we are in the development of crypto 1.1.0 compliance testcases, during the ARIA scenario development we noticed that support in cmac.c file is missing, and i had discussed with @gilles-peskine-arm to raise a pull request. |
One thing to check which may or may not result in a CI failure is whether all relevant buffer calculation macros take ARIA into account. Failing to do so could cause a buffer overflow in ARIA-only configurations or in (ARIA+DES)-only configurations (sounds exotic, but may be needed in some Korean banking settings?). |
Signed-off-by: jk-arm <jothikumar.mani@arm.com>
@jk-arm Please make sure that all the steps in our guide on how to implement a new cryptographic mechanism are fulfilled for ARIA+CMAC. They already are for ARIA and for CMAC separately, but there may be other places than |
Hi @gilles-peskine-arm, |
It will also need a changelog entry |
Description
Please write a few sentences describing the overall goals of the pull request's commits.
PR checklist
Please tick as appropriate and edit the reasons (e.g.: "backport: not needed because this is a new feature")
Notes for the submitter
Please refer to the contributing guidelines, especially the
checklist for PR contributors.