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.

Sign up for GitHub

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

ESP32 TFM fix for RSA key size 512 and 2048 #6286

Closed
wants to merge 1 commit into from
Closed

ESP32 TFM fix for RSA key size 512 and 2048 #6286

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 42 additions & 15 deletions wolfcrypt/src/tfm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2818,8 +2818,17 @@ int fp_exptmod(fp_int * G, fp_int * X, fp_int * P, fp_int * Y)

#if defined(WOLFSSL_ESP32WROOM32_CRYPT_RSA_PRI) && \
!defined(NO_WOLFSSL_ESP32WROOM32_CRYPT_RSA_PRI)
int x = fp_count_bits (X);
#endif
#if defined(WOLFSSL_RSA_KEY_SIZE)
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is WOLFSSL_RSA_KEY_SIZE documented? Did you consider RSA_MAX_SIZE? Why is the logic not a >= 512? If this is ESP32 specific please use a macro that includes it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is the reason for these issues understood? is the underlying bug/issue not understood yet?

No, not yet. I do not know exactly why the hardware-accelerated calc fails for certain RSA lengths. I wanted to at least get a fix in place before diving into the HW. It may take some time to fully resolve this. I chose to instead get other, higher visibility changes in place and backburner this for now.

Where is WOLFSSL_RSA_KEY_SIZE documented?

I neglected to add documentation on WOLFSSL_RSA_KEY_SIZE. I can add it if you are in favor of keeping this, or some other equivalent functionality.

The intent is for this value to be used for explicit user RSA key sizes. We could put this in the default ESP32 user_settings.h.

I didn't use RSA_MAX_SIZE as I wanted to define an explicit size, not a maximum size. Also, it appears other settings may imply a specific RSA_MAX_SIZE value.

Why is the logic not a >= 512

There's a specific problem for lengths of exactly 512 and a different problem for 2048 length.

If this is ESP32 specific please use a macro that includes it.

The TFM library only uses this gate when the WOLFSSL_ESP32WROOM32_CRYPT_RSA_PRI is enabled. Otherwise, I use it in the demo app that I used when testing #6205.

Would you prefer an ESP32-specific name? It would seem best to have a macro for RSA lengths for the end user. The use of this macro in TFM does however, leave a bit to be desired. Once the HW math is fixed, the only use of this would be be the end user when calling something like wc_MakeRsaKey.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since this code is only for ESP32 I would remove WOLFSSL_RSA_KEY_SIZE logic and have this check always enabled and document the workaround for HW issue. The WOLFSSL_RSA_KEY_SIZE logic here is too limiting. RSA should support a range of key sizes.

#if WOLFSSL_RSA_KEY_SIZE != 512
/* there's a known problem with length = 512
** see https://github.com/wolfSSL/wolfssl/issues/6205
*/
int x = fp_count_bits (X);
#endif
#else
#warning "WOLFSSL_RSA_KEY_SIZE not defined"
#endif /* WOLFSSL_RSA_KEY_SIZE */
#endif /* WOLFSSL_ESP32WROOM32_CRYPT_RSA_PRI */

/* handle modulus of zero and prevent overflows */
if (fp_iszero(P) || (P->used > (FP_SIZE/2))) {
Expand All @@ -2840,10 +2849,19 @@ int fp_exptmod(fp_int * G, fp_int * X, fp_int * P, fp_int * Y)

#if defined(WOLFSSL_ESP32WROOM32_CRYPT_RSA_PRI) && \
!defined(NO_WOLFSSL_ESP32WROOM32_CRYPT_RSA_PRI)
if(x > EPS_RSA_EXPT_XBTIS) {
return esp_mp_exptmod(G, X, x, P, Y);
}
#endif
#if defined(WOLFSSL_RSA_KEY_SIZE)
#if WOLFSSL_RSA_KEY_SIZE != 512
/* there's a known problem with length = 512
** see https://github.com/wolfSSL/wolfssl/issues/6205
*/
if(x > EPS_RSA_EXPT_XBTIS) {
return esp_mp_exptmod(G, X, x, P, Y);
}
#endif
#else
#warning "WOLFSSL_RSA_KEY_SIZE not defined"
#endif /* WOLFSSL_RSA_KEY_SIZE */
#endif /* WOLFSSL_ESP32WROOM32_CRYPT_RSA_PRI */

if (X->sign == FP_NEG) {
#ifndef POSITIVE_EXP_ONLY /* reduce stack if assume no negatives */
Expand Down Expand Up @@ -4310,15 +4328,24 @@ int wolfcrypt_mp_mulmod (mp_int * a, mp_int * b, mp_int * c, mp_int * d)
int mp_mulmod (mp_int * a, mp_int * b, mp_int * c, mp_int * d)
#endif
{
#if defined(WOLFSSL_ESP32WROOM32_CRYPT_RSA_PRI) && \
!defined(NO_WOLFSSL_ESP32WROOM32_CRYPT_RSA_PRI)
int A = fp_count_bits (a);
int B = fp_count_bits (b);

if( A >= ESP_RSA_MULM_BITS && B >= ESP_RSA_MULM_BITS)
return esp_mp_mulmod(a, b, c, d);
else
#endif
#if defined(WOLFSSL_ESP32WROOM32_CRYPT_RSA_PRI) && \
!defined(NO_WOLFSSL_ESP32WROOM32_CRYPT_RSA_PRI)
#if defined(WOLFSSL_RSA_KEY_SIZE)
#if WOLFSSL_RSA_KEY_SIZE != 2048
/* there's a known problem with length = 2048
** see https://github.com/wolfSSL/wolfssl/issues/6205
*/
int A = fp_count_bits (a);
int B = fp_count_bits (b);

if( A >= ESP_RSA_MULM_BITS && B >= ESP_RSA_MULM_BITS) {
return esp_mp_mulmod(a, b, c, d);
}
#endif
#else
#warning "WOLFSSL_RSA_KEY_SIZE not defined"
#endif /* WOLFSSL_RSA_KEY_SIZE */
#endif /* WOLFSSL_ESP32WROOM32_CRYPT_RSA_PRI */
return fp_mulmod(a, b, c, d);
}

Expand Down