RSA call to TFM overwrites math operands

[gojimmypi]

Version

latest master

Description

While working on an example Espressif app to address #6205, I noticed that the RSA call to mp_mul would modify the operand in the expression tmp = tmpb + q * tmp, specifically in the conditional check:

if (ret == 0 && mp_mul(tmp, &key->q, tmp) != MP_OKAY)

This is not immediately obvious until one views the definition and notes that all of the parameters are pointers:

int mp_mul (mp_int * a, mp_int * b, mp_int * c)

In my specific case, in attempting to see where things go sideways with the HW acceleration result not being the same as the software calcs, I have the A2, B2, and C2 values calculated in parallel and compared at runtime. A sample warning can be seen in the output log.

As seen here in the rsa.c, the use of the identical pointer tmp for both the operand a and results c ends up modifying the pointer to a to actually end up pointing to c after the call:

More explicitly, I don't think this should ever occur:

Although technically it would seem to usually work, one would think that a call to a multiplication operation should not actually ever change the operands a or b in addition to the output result, c eh?

There's (what seems to be) an attempt to save the tmp operand in tmpa, but as it is just a pointer, both tmpa and tmp of course point to the same memory location.

I have a possible update that actually does save the operands, using a new tmpc declaration. Just before returning, I copy the value of tmpc to the tmp return parameter.

This solution may not be the best, but it is a successful, operating example. Before pursuing and further polish, I'm interested in feedback if this seems reasonable.

Thank you,

• edit: there's another instance in ecc.c to calculate Y = Y * X where the pointer adjusts the underlying address of an operand A to match the address of the result C:

[gojimmypi]

I'm removing the other issue assignees at this point, pending further code review.

Although at the time, it certainly seemed like there was a problem with the RSA calls adjusting the operand pointers, it appears a more simple problem to the HW vs SW mismatch is at fault: sign. The esp32_mp.c honors the WOLFSSL_SP_INT_NEGATIVE setting. See sp_int.c. There's not a single instance of WOLFSSL_SP_INT_NEGATIVE in wolfcrypt/src/tfm.c

[douzzer]

@gojimmypi OK keep at it -- and please keep an eye out for problems rooted in passing the same pointer as operand and result to SP functions. last week I ran across a situation where gcc at -O2 or above was causing memory/register corruption and a SEGV on m68k (32 bit, syndrome was target-specific). it appeared the root cause was buggy behavior by the optimizer incidental to inlining, and I opted to back off the m68k nightly test run to -O1.

@SparkiDev will also be keenly interested in what you uncover.

re the m68k SEGV, I was also able to resolve the problem with use of an auxiliary tmp sp_int. the problematic routine was sp_todecimal() -- the sp_radix_size() patch is just for thoroughness (same code pattern).

diff --git a/wolfcrypt/src/sp_int.c b/wolfcrypt/src/sp_int.c
index 503a8988a..b742b689d 100644
--- a/wolfcrypt/src/sp_int.c
+++ b/wolfcrypt/src/sp_int.c
@@ -17814,8 +17814,10 @@ int sp_todecimal(const sp_int* a, char* str)
     else {
         /* Temporary that is divided by 10. */
         DECL_SP_INT(t, a->used + 1);
+        DECL_SP_INT(u, a->used + 1);
 
         ALLOC_SP_INT_SIZE(t, a->used + 1, err, NULL);
+        ALLOC_SP_INT_SIZE(u, a->used + 1, err, NULL);
         if (err == MP_OKAY) {
             err = sp_copy(a, t);
         }
@@ -17832,8 +17834,11 @@ int sp_todecimal(const sp_int* a, char* str)
             /* Write out little endian. */
             i = 0;
             do {
+                err = sp_copy(t, u);
+                if (err != MP_OKAY)
+                    break;
                 /* Divide by 10 and get remainder of division. */
-                (void)sp_div_d(t, 10, t, &d);
+                (void)sp_div_d(u, 10, t, &d);
                 /* Write out remainder as a character. */
                 str[i++] = (char)('0' + d);
             }
@@ -17964,18 +17969,20 @@ int sp_radix_size(const sp_int* a, int radix, int* size)
         }
         else {
             DECL_SP_INT(t, a->used);
+            DECL_SP_INT(u, a->used);
 
             /* Temporary to be divided by 10. */
-            ALLOC_SP_INT(t, a->used, err, NULL);
+            ALLOC_SP_INT_SIZE(t, a->used, err, NULL);
             if (err == MP_OKAY) {
-                t->size = a->used;
                 err = sp_copy(a, t);
             }
+            ALLOC_SP_INT_SIZE(u, a->used, err, NULL);
 
             if (err == MP_OKAY) {
                 /* Count number of times number can be divided by 10. */
                 for (i = 0; !sp_iszero(t); i++) {
-                    (void)sp_div_d(t, 10, t, &d);
+                    (void)sp_copy(t, u);
+                    (void)sp_div_d(u, 10, t, &d);
                 }
             #ifdef WOLFSSL_SP_INT_NEGATIVE
                 /* Add to count of characters for negative sign. */

[gojimmypi]

I've been unable to find a place where this is problematic. This was a concern when there was a SHA interleaving problem with occasionally unexpected results.

See also #7262 and #7505