-
Notifications
You must be signed in to change notification settings - Fork 811
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
PKCS#11 RSA Padding Offload #7750
Conversation
Can one of the admins verify this patch? |
2aa7ee2
to
4587da7
Compare
Okay to test. Contributor agreement on file. |
4587da7
to
b1398d2
Compare
Seeing errors like:
|
f5cf473
to
f193f66
Compare
@dgarske Forced push - this is passing now |
Thanks @space88man , this is great work. I can see some issues with the asynchronous RSA cases (stack variable used). I like the new RsaPadding struct and the new function RsaFunctionPad API, but it's a pretty big change. |
23ba8ac
to
7f709ef
Compare
7f709ef
to
10d1434
Compare
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.
Maximum 80 characters per line:
overlong lines added:
/tests/api.c:76660 if (info->pk.type == WC_PK_TYPE_RSA || info->pk.type == WC_PK_TYPE_RSA_PSS) {
/tests/api.c:76692 info->pk.rsa.type, &key, info->pk.rsa.rng, info->pk.rsa.padding);
/wolfcrypt/src/cryptocb.c:422 word32* outLen, int type, RsaKey* key, WC_RNG* rng, RsaPadding *padding)
/wolfcrypt/src/rsa.c:3135 ret = wc_CryptoCb_RsaPadding(in, inLen, out, outLen, type, key, rng, padding);
/wolfcrypt/src/rsa.c:3143 ret = wc_RsaPad_ex(in, inLen, out, in2Len, padding->pad_value, rng, padding->pad_type,
/wolfcrypt/src/rsa.c:3144 padding->hash, padding->mgf, padding->label, padding->labelSz, padding->saltLen,
/wolfcrypt/src/rsa.c:3153 ret = wc_CryptoCb_RsaPadding(in2, in2Len, out, outLen, type, key, rng, padding);
/wolfcrypt/src/rsa.c:3179 if ((type == RSA_PUBLIC_ENCRYPT || type == RSA_PRIVATE_ENCRYPT) && (padding->pad_type != WC_RSA_NO_PAD)) {
/wolfcrypt/src/rsa.c:3181 ret = wc_RsaPad_ex(in, inLen, out, in2Len, padding->pad_value, rng, padding->pad_type,
/wolfcrypt/src/rsa.c:3182 padding->hash, padding->mgf, padding->label, padding->labelSz, padding->saltLen,
/wolfcrypt/src/rsa.c:3253 return wc_RsaFunction_ex(in, inLen, out, outLen, type, key, rng, 1, &padding);
/wolfcrypt/src/rsa.c:3257 word32* outLen, int type, RsaKey* key, WC_RNG* rng, RsaPadding *padding)
/wolfcrypt/src/rsa.c:3260 return wc_RsaFunction_ex(in, inLen, out, outLen, type, key, rng, 1, padding);
/wolfcrypt/src/rsa.c:3393 ret = wc_RsaPad_ex(in, inLen, out, (word32)sz, pad_value, rng, pad_type,
/wolfcrypt/src/rsa.c:3416 ret = wc_RsaFunctionPad(in, (word32)inLen, out, &key->dataLen, rsa_type, key,
/wolfcrypt/src/wc_pkcs11.c:2163 rv = session->func->C_GetMechanismInfo(session->slotId, mechanism, &mechInfo);
/wolfssl/wolfcrypt/cryptocb.h:484 WOLFSSL_LOCAL int wc_CryptoCb_RsaPadding(const byte* in, word32 inLen, byte* out,
/wolfssl/wolfcrypt/rsa.h:310 word32* outLen, int type, RsaKey* key, WC_RNG* rng, RsaPadding* padding);```
10d1434
to
d64617a
Compare
Incoming push - there are some existing lines len > 80 chars but I won't touch those to minimize the diff. |
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.
[check-source-text] [2 of 7] [wolfssl]
overlong lines added:
/wolfcrypt/src/rsa.c:3188 padding->label, padding->labelSz, padding->saltLen,
/wolfssl/wolfcrypt/cryptocb.h:484 WOLFSSL_LOCAL int wc_CryptoCb_RsaPadding(const byte* in, word32 inLen, byte* out,
/wolfssl/wolfcrypt/rsa.h:310 word32* outLen, int type, RsaKey* key, WC_RNG* rng, RsaPadding* padding);
[clang-tidy-defaults] [7 of 7] [wolfssl]
configure... real 0m11.163s user 0m5.349s sys 0m2.509s
build.../tmp/workspace/PRB-multi-test-script/wolfssl/wolfcrypt/src/rsa.c:3184:43: warning: The left operand of '!=' is a garbage value [clang-analyzer-core.UndefinedBinaryOperatorResult]
(padding && padding->pad_type != WC_RSA_NO_PAD)) {
^
/tmp/workspace/PRB-multi-test-script/wolfssl/wolfcrypt/src/rsa.c:4297:12: note: Calling 'wc_RsaPSS_Sign_ex'
return wc_RsaPSS_Sign_ex(in, inLen, out, outLen, hash, mgf,
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
...
make: *** [Makefile:5159: all] Error 2
real 0m24.244s user 0m23.129s sys 0m0.696s
clang-tidy-defaults fail_analytic_build
failed config: '--srcdir' '.' '--disable-jobserver' '--enable-option-checking=fatal' 'CPPFLAGS=-DKEEP_OUR_CERT -DKEEP_PEER_CERT -DWOLFSSL_ALT_NAMES -DNO_WOLFSSL_CIPHER_SUITE_TEST -DWOLFSSL_OLD_PRIME_CHECK -pedantic -DSP_ALLOC -DWOLFSSL_CLANG_TIDY' 'CC=/tmp/workspace/PRB-multi-test-script/testing/git-hooks/clang-tidy-builder.sh' 'CLANG=/usr/lib/llvm-14/bin/clang-14' 'CFLAGS=-Wunreachable-code-aggressive -Wthread-safety -Wloop-analysis -Wenum-compare-conditional -fcolor-diagnostics -fcomplete-member-pointers -Wheader-hygiene -Wstring-conversion -Wtautological-overlap-compare -g -fdebug-types-section -Wunreachable-code-break -Wunreachable-code-return -Wimplicit-fallthrough'
BUILD_ENV: 'CLANG_TIDY=/usr/lib/llvm-14/bin/clang-tidy' 'CLANG=/usr/lib/llvm-14/bin/clang-14'
final tally: 6 of 7 checks succeeded, 0 skipped, 0 forced, and 1 failed, for wolfssl.
exiting with status 2
@dgarske Probably some face-palm thing but it is really not clear to me what clang-static-analyzer is complaining about here. I found a similar issue in mbedtls: Mbed-TLS/mbedtls#1002 which was declared as a false positive. The comparison: |
I’ll see if I can locate the cause. |
@space88man , great work on this PR. I think there is some really useful work here, but it is a bit more intrusive then we can accept as-is. I'd like to take bits of this and implement in a slightly different way. I plan to open a new PR and provide a link to it. Thanks, David Garske, wolfSSL |
Sure - totally understandable; I look forward to testing out the alternative PR. Before I close this - would you like me to PR only the pkcs11.h changes, that has no impact to the rest of the code base? |
Hi @space88man , yes go ahead and trim this down to just the pkcs11.h changes for now. I hope yo get to this in the next couple of days. Thanks, David Garske, wolfSSL |
d64617a
to
00bef7f
Compare
The original draft/WIP can be referenced here: https://github.com/space88man/wolfssl/tree/wip2-padding-refactor. I've reduced this PR to the |
00bef7f
to
d2d71c2
Compare
Retest this please - saw a GitHub origin timeout |
Retest this please - FIPS timeout |
Retest this please. Windows test machine was down... |
Description
This PR# allows tokens to perform
CKM_RSA_PKCS
(sign/encrypt),CKM_RSA_PKCS_PSS
(sign),CKM_RSA_PKCS_OAEP
(encrypt). These mechanims are the post-hash versions: i.e. wolfCrypt has already created the message digest.It is for use cases where tokens do not expose
CKM_RSA_X_509
, eg., LUNA in FIPS-modeThe changes to callback and pkcs#11 can be gated by
-DWOLF_CRYPTO_CB_NOPAD -DNO_PKCS11_RSA_PAD
.Breaks ABI? Might do so as it changes the size of some structs.
Testing
There is a test set here: https://github.com/user-attachments/files/16215326/testing-v2.zip
It includes:
Without PR - token should only see CKM_RSA_X_509.
With PR - token should see CKM_RSA_PKCS / CKM_RSA_PKCS_PSS / CKM_RSA_PKCS_OAEP
Checklist
Updates
the other two crypto callbacks are
wc_AriaCryptoCb
andwc_CAAM_router
:make check
now passesmake check
also passes when PKCS-offload is disabled (CFLAGS=-DWOLF_CRYPTO_CB_NOPAD …
)scan-build
is leaving one warningcode formatting (line length <= 80 chars)