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.

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

Feature/multiple aes siv ads #7911

Merged
merged 8 commits into from
Sep 12, 2024

Conversation

ptsiewie
Copy link
Contributor

@ptsiewie ptsiewie commented Aug 28, 2024

Description

The AES SIV algorithm implementation in WolfSSL would accept only exactly one vector of associated data, even though the definition of the algorithm of RFC5297 allows any number no larger than 126.

Fixes zd#18509

Testing

The tests in test.c have been extended to include the two examples given in the RFC5297 document. The second of these two examples uses two ADs.

Checklist

  • added tests
  • updated/added doxygen
  • updated appropriate READMEs
  • Updated manual and documentation

@wolfSSL-Bot
Copy link

Can one of the admins verify this patch?

@dgarske
Copy link
Contributor

dgarske commented Aug 28, 2024

Okay to test. Contributor agreement in progress.

Copy link
Contributor

@dgarske dgarske left a comment

Choose a reason for hiding this comment

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

Over to @wolfSSL-Bot and @SparkiDev for further review. Thank you for adding a test case.

keySz);
/* Loop over authenticated associated data AD1..ADn */
byte tmpi = 0;
for(word32 ai = 0; ai < numAssoc; ++ai) {
Copy link
Contributor

Choose a reason for hiding this comment

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

For portability please declare word32 ai on line above. Code consistency please use for ( add space.

@@ -13826,7 +13843,8 @@ int wc_AesSivEncrypt(const byte* key, word32 keySz, const byte* assoc,
word32 assocSz, const byte* nonce, word32 nonceSz,
const byte* in, word32 inSz, byte* siv, byte* out)
{
return AesSivCipher(key, keySz, assoc, assocSz, nonce, nonceSz, in, inSz,
const AesSivAssoc ad0 = { .assoc = assoc, .assocSz=assocSz };
Copy link
Contributor

Choose a reason for hiding this comment

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

This syntax is not very portable. Please use:

AesSivAssoc ad0;
ad0.assoc = assoc;
ad0.assocSz = assocSz;

@ptsiewie ptsiewie requested a review from dgarske August 29, 2024 06:47
@dgarske
Copy link
Contributor

dgarske commented Aug 29, 2024

Okay to test. Contributor agreement in progress. @ptsiewie sorry for the delay in getting your agreement approved. Expect it next week.

dgarske
dgarske previously approved these changes Aug 29, 2024
Copy link
Contributor

@dgarske dgarske left a comment

Choose a reason for hiding this comment

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

Feature looks good to me. @SparkiDev please review. Also should we wrap any of this new SIV support in a new build macro?

SparkiDev
SparkiDev previously approved these changes Aug 29, 2024
@SparkiDev
Copy link
Contributor

The old API goes through the new API so we will always include code.

@SparkiDev
Copy link
Contributor

Hi @ptsiewie

Errors in ctidy check

TLDR: change '1u' to '1U'

Error:
shellcheck configure.../var/lib/jenkins/workspace/PRB-multi-test-script/wolfssl/wolfcrypt/src/aes.c:13850:43: warning: integer literal has suffix 'u', which is not uppercase [readability-uppercase-literal-suffix]
return AesSivCipher(key, keySz, &ad0, 1u, nonce, nonceSz, in, inSz,
^~
U
/var/lib/jenkins/workspace/PRB-multi-test-script/wolfssl/wolfcrypt/src/aes.c:13864:43: warning: integer literal has suffix 'u', which is not uppercase [readability-uppercase-literal-suffix]
return AesSivCipher(key, keySz, &ad0, 1u, nonce, nonceSz, in, inSz,
^~
U

@SparkiDev SparkiDev removed their assignment Aug 29, 2024
…ense, especially since in the RFC 5297 document they're actually counting the ADs from 1.
@ptsiewie ptsiewie dismissed stale reviews from SparkiDev and dgarske via a398f00 August 30, 2024 07:23
@dgarske
Copy link
Contributor

dgarske commented Aug 30, 2024

Okay to test. Retest this please

@dgarske
Copy link
Contributor

dgarske commented Aug 30, 2024

Reminder, contributor agreement in progress, but not yet approved. Person needed for that is on vacation this week.

@dgarske
Copy link
Contributor

dgarske commented Sep 10, 2024

Contributor agreement approved and on file. Thank you @ptsiewie!

@SparkiDev SparkiDev assigned dgarske and unassigned SparkiDev Sep 10, 2024
@dgarske dgarske assigned douzzer and unassigned dgarske Sep 10, 2024
@ptsiewie
Copy link
Contributor Author

Do I need to do something before this branch will be merged?

@dgarske dgarske merged commit 9e2a7b3 into wolfSSL:master Sep 12, 2024
129 checks passed
@dgarske
Copy link
Contributor

dgarske commented Sep 12, 2024

Hi @ptsiewie ,

Thank you for your work on this. I have merged it. Sorry about the delay.

Thanks,
David Garske, wolfSSL

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants