-
Notifications
You must be signed in to change notification settings - Fork 830
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
ESP32 TFM fix for RSA key size 512 and 2048 #6286
Conversation
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.
Is the reason for these issues understood or documented in HW or is the underlying bug/issue not understood yet? I read the #6205 and it seems like the real bug is not identified yet.
@@ -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) |
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.
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.
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.
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
.
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.
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.
@@ -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) |
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.
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.
I partly agree, but I don't want to disable HW acceleration for all RSA key sizes just because some are problematic. I do agree that my solution is a bit wonky, thus the "interim fix" description. I'm going to convert this to draft and see if I can find the root problem of the HW math so that all sizes work as they should. This also means that in the meantime, any users will still encounter problems on the current release of wolfSSL, as described in #6205 |
It was definitely a worthwhile exercise to find the root cause. See #6380 So far, I've only been testing the faster keysize = 512, so I have not determined if the TFM problems with |
This lingering draft PR was fixed some time ago. See also #6205 |
Description
As described in #6205 RSA signature fails for certain key sizes on the ESP32.
This is an interim fix as described here that reverts to software calculations in the known cases that the hardware acceleration is problematic.
Specifically:
For key size 2048
esp_mp_mulmod
is not used, insteadfp_mulmod
is called.For key size 512
esp_mp_exptmod
is not used, instead the SW version is used infp_exptmod
.There's also a new compile-time warning these functions that need a key size but one is not defined:
"WOLFSSL_RSA_KEY_SIZE not defined"
See #6234 for a roadmap of all Espressif Improvements.
Fixes zd# n/a
Testing
How did you test?
See details in #6205
Checklist