Strange control flow in rsa_prepare_blinding()

[pkolbus]

Note: This is just a template, so feel free to use/remove the unnecessary things

Description

• Type: Bug
• Priority: Major

---

Bug

OS
FreeRTOS

mbed TLS build:
Version: 2.16.8 or git commit id
OS version: 9
Configuration: please attach config.h file where possible
Compiler and options (if you used a pre-built binary, please indicate how you obtained it): gcc-arm-none-eabi-8-2018-q4-major
Additional environment information:

Peer device TLS stack and version
n/a
Version:

Expected behavior
No dead code

Actual behavior
Starting with commit 49e94e3, the do/while loop in rsa_prepare_blinding() was changed to a do...while(0). This leaves two strange bits in the body of the function:

• the check if( ret == MBEDTLS_ERR_MPI_NOT_ACCEPTABLE ) followed by continue will jump to the end of the do/while body, thus equivalent to a break when the continuation condition is false. Is the intent in this case to restart the loop (as before the commit), or exit? (Ref C99 standard, section 6.8.6.2)
• the check if( count++ > 10 ) can never be true since backward control flow is impossible.

Steps to reproduce
n/a.

Credit to Synopsys Coverity for finding these issues.

---

Note: I'm happy to prepare a PR if it would be helpful, but not sure what the intent is.

[gilles-peskine-arm]

Thank you for reporting this! We made a mistake in a security fix.

The intent is, as before the fix, to go back to the call to mbedtls_mpi_fill_random and try again in case the randomly generated value is not suitable. This worked before because the loop was while(ret is not satisfactory) but no longer works with a while(0) loop.

This is a case where a goto would have been more readable than this technically-not-goto-but-still-not-well-structured-code. Not that I wouldn't favor rewriting the code with a clear while loop.

Fortunately the bug is hard to trigger: you have to control the RNG output, and all this lets you do is cause the RSA operation to fail. The density of unacceptable values is tiny (about 2/sqrt(N)) so this won't happen in practice without specially crafted value.