-
Notifications
You must be signed in to change notification settings - Fork 23
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
Provide CMAC high level API replacement #81
Conversation
@rlhui @judyjoseph Please assign to the right people to review it. |
@JunhongMao @skeesara-nokia Please review this PR |
@xumia Please help to review. |
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.
lgtm
goto fail; | ||
ret = 0; | ||
fail: | ||
EVP_MAC_CTX_free(ctx); |
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.
are we missing EVP_MAC_free(mac);
here?
reference: Example in https://www.openssl.org/docs/man3.1/man3/EVP_MAC_init.htm
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.
@wumiaont , can you check this comment?
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.
Yes. That does seem to be missing. I will double check and verify that fix. Thanks for finding this out.
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.
The mac parameter is an input variable, it may be a little different with the sample code. Is the caller to free it? Is it safe to free mac?
aes_siv_encrypt --> aes_s2v --> omac1_aes_vector
See https://github.com/sonic-net/sonic-wpa-supplicant/blob/413704a6ccef8321667712c518e595878ff02251/src/crypto/aes-siv.c#L127C2-L127C23
u8 v[AES_BLOCK_SIZE];
@r12f , @wumiaont , please double check if necessary.
If needed, do we need to similar action in line1288?
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.
@xumia , maybe I am reading the wrong code... this is what I am seeing. The mac is allocated using EVP_MAC_fetch in this function:
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.
From openssl WIKI example code there it's needed to free mac object created by fetch. https://www.openssl.org/docs/man3.0/man3/EVP_MAC_fetch.html. Fixed in #82
This PR is only applicable to master branch. |
Replace wpa-supplicant openssl CMAC wrapper API to use high level EVP APIs. With this change CMAC handlings for openssl will be taken over by symcrypt provider in FIPs mode.
Test: Tested against whole macsec testing suites and all passed with the change.
This is porting from hostap wpa_supplicant for CMAC Openssl hihe level API replacement.
https://w1.fi/cgit/hostap/commit/?id=0c61f6234fd27c43b46d9bdb8ecf72be2e85cc38