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

Provide CMAC high level API replacement #81

Merged
merged 2 commits into from
Mar 14, 2024
Merged

Provide CMAC high level API replacement #81

merged 2 commits into from
Mar 14, 2024

Conversation

wumiaont
Copy link
Contributor

@wumiaont wumiaont commented Mar 12, 2024

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

@mlok-nokia
Copy link

@rlhui @judyjoseph Please assign to the right people to review it.

@mlok-nokia
Copy link

@JunhongMao @skeesara-nokia Please review this PR

@wumiaont
Copy link
Contributor Author

@xumia Please help to review.

Copy link
Collaborator

@Pterosaur Pterosaur left a 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);
Copy link

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

Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Collaborator

@xumia xumia Mar 16, 2024

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?

Copy link

@r12f r12f Mar 16, 2024

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:

image

Copy link
Contributor Author

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

@mlok-nokia
Copy link

This PR is only applicable to master branch.

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

Successfully merging this pull request may close these issues.

8 participants