-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Add support for using a different MGF1 hash with RSA-OAEP #6779
base: master
Are you sure you want to change the base?
Conversation
Thanks for the pull request. I'd rather step up LTC than cherry-pick a single commit, but doing that is a bit complicated so I'll do it if @jforissier doesn't beat me to it after the upcoming OP-TEE release. For the sake of this review, we can ignore "core: ltc: Add possibility to use different hash algorithms in RSAES-AEP" and assume it will be available upstream. It would be nice with a test in xtest to see that this continues to work. |
Please squash the "core: pass TEE_ATTR_RSA_OAEP_MGF_HASH to RSA-OAEP implementations" fixup commits. |
Sure, sounds good to me.
I can take a look. Any preferences where this should go? |
It should be in the 4xxx range of tests, either as a new case or by extending an already present case. The latter might be the easiest. |
I've started updating LTC with #6786. Once that's merged I'll create another PR to update LTC to the latest. |
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.
For commit "core: ltc: Add possibility to use different hash algorithms in RSAES-OAEP":
I think the reference to the libtomcrypt commit should be a Link:
tag than a raw commit SHA1 that is rather expected to be a ref in the current Git revision, e.g.:
Link: https://github.com/libtom/libtomcrypt/commit/63091c9e5c776ec1c5a7a5d2973395a24e82b3ba`
Signed-off-by: Steffen Jaeckel <s@jaeckel.eu>
Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
Otherwise,
Acked-by: Etienne Carriere <etienne.carriere@foss.st.com>
for this commit.
Minor comments for commit "core: pass TEE_ATTR_RSA_OAEP_MGF_HASH to RSA-OAEP implementations".
core/lib/libtomcrypt/rsa.c
Outdated
if (algo != TEE_ALG_RSAES_PKCS1_V1_5) { | ||
res = tee_algo_to_ltc_hashindex(mgf_algo, <c_mgfindex); | ||
if (res != TEE_SUCCESS) { | ||
EMSG("tee_algo_to_ltc_hashindex() returned %d for mgf algo %x", (int)res, mgf_algo); |
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.
s/algo %x",
/algo %#"PRIx32,
/
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.
Done.
lib/libmbedtls/core/rsa.c
Outdated
if (md_algo == MBEDTLS_MD_NONE) { | ||
/* Using a different MGF1 algorithm is not supported. */ | ||
if (md_algo == MBEDTLS_MD_NONE || | ||
md_algo != tee_algo_to_mbedtls_hash_algo(mgf_algo)) { |
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.
Maybe a debug message here could help:
if (md_algo == MBEDTLS_MD_NONE) {
res = TEE_ERROR_NOT_SUPPORTED;
goto out;
}
+ if (md_algo != tee_algo_to_mbedtls_hash_algo(mgf_algo)) {
+ DMSG("Using a different MGF1 algorithm is not supported");
+ res = TEE_ERROR_NOT_SUPPORTED;
+ goto 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.
Yes, that makes sense. Done.
56edae5
to
e23655e
Compare
I updated the commit message for now, but I believe Jens plans to update LTC separately, and at that point I can just drop this commit from the PR. |
This pull request has been marked as a stale pull request because it has been open (more than) 30 days with no activity. Remove the stale label or add a comment, otherwise this pull request will automatically be closed in 5 days. Note, that you can always re-open a closed issue at any time. |
Done in #6820 (merged)
Please do, thanks! |
core/lib/libtomcrypt/rsa.c
Outdated
res = tee_algo_to_ltc_hashindex(mgf_algo, <c_mgfindex); | ||
if (res != TEE_SUCCESS) { | ||
EMSG("tee_algo_to_ltc_hashindex() returned %d for mgf algo %#"PRIx32, | ||
(int)res, mgf_algo); |
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.
res
should also be printed as a PRIx32.
I see now that this is copied from above, but two wrongs don't make one right.
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.
Ack, also changed the message above since I'm here.
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 apply:
Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
OP-TEE currently doesn't support using a different hash for MGF1 with RSA-OAEP. However, this is required for AOSP compatibility (e.g. in EncryptionOperationsTest.RsaOaepWithMGFDigestSuccess [1]). Pass the MGF1 attribute to crypto implementations. Note that only libtomcrypt supports this feature at the moment, so other implementations will either fail or fall back to libtomcrypt when passed a different MGF1 hash. Link: https://android.googlesource.com/platform/hardware/interfaces/+/refs/heads/main/security/keymint/aidl/vts/functional/KeyMintTest.cpp#5552 [1] Signed-off-by: Sami Tolvanen <samitolvanen@google.com> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
OP-TEE currently doesn't support using a different hash algorithm for MGF1 with RSA-OAEP. However, this is required for AOSP compatibility (e.g. in the EncryptionOperationsTest.RsaOaepWithMGFDigestSuccess KeyMint VTS test).
This PR cherry-picks the upstream libtomcrypt commit 63091c9e5c77 ("Add possibility to use different hash algorithms in RSAES-OAEP") and passes the MGF1 hash from the
TEE_ATTR_RSA_OAEP_MGF_HASH
attribute to crypto implementations.