Use of uninitialized memory in cert_app for empty commandline

[pascal-cuoq]

Summary

When cert_app is invoked with an empty commandline, it eventually relies on the value of ctx->accumulator_started in the function mbedtls_entropy_free at line 85:

mbedtls/library/entropy.c

Line 85 in a942b37

if( ctx->accumulator_started == -1 )

ctx is pointing to entropy, local variable of main. Because the commandline is empty, no part of that variable was written to. Reading the member accumulator_started, which is uninitialized, is more or less undefined behavior in itself (depending on the version of the C standard and the level of bad faith argumentation one is willing to develop about the meaning of words in the C standard) and should be avoided.

System information

Mbed TLS version (number or commit id): commit a942b37
Operating system and version: abstract POSIX system with implementation-defined choices of GCC for IA32
Configuration (if not default, please attach mbedtls_config.h):

mbedtls_config.h from the repo.

Compiler and options (if you used a pre-built binary, please indicate how you obtained it): abstract compiler with the implementation-defined choices of GCC targeting IA32.

Expected behavior

This use of uninitalized memory should be avoided because nothing prevents the condition ctx->accumulator_started == -1 from having the least desirable possible truth value. In addition, Mbed TLS is exactly the sort of code for which it is often dangerous to involve uninitialized variables in computations, because of the risk of leaking cryptographic secrets. Even if this particular use of uninitialized memory is arguably harmless, fixing it would facilitate the application of the sort of tool that guarantees the absence of uses of uninitialized memory for all executions, without having to waste time again on this one.

Actual behavior

The uninitialized contents of the variable entropy, local variable of main, are used.

Steps to reproduce

Invoke cert_app with empty commandline (argc==1)

Additional information

I follow the development of enough open-source projects to be familiar with the usual reports from well-meaning users of the project who have applied a static analyzer to the source code without being able to do the classification of warnings between false positives and bugs. I would like to emphasize that this is not such a report: the situation I described certainly happens for the provided input (namely argc==1).

This bug was found using TrustInSoft Analyzer during the REDOCS'22 event.

[ronald-cron-arm]

Thanks for this. Looking at it, it seems that the initialization of entropy (call to mbedtls_entropy_init( &entropy )) is missing at the beginning of the main function, together with the initialization of server_fd, ctr_drbg, ssl ... Would you be able to fix that in a PR ?

[m3g4d1v3r]

Hello there!

I've tried to reproduce the defect by invoking cert_app, with no success.
After debugging the executable, I've found that with no arguments the program flow reaches the cert_app.c:463: goto usage; statement before reaching cert_app.c:475:mbedtls_entropy_free( &entropy );.
Thus I'm not able to easily reproduce the bug.

With this in mind: can someone help me which steps can I take to improve the code?

Kind regards,
m3g4d1v3r

[pascal-cuoq]

Please try these steps to reproduce:

• make sure you are at commit a942b37 with no modifications
• Apply the patch below

diff --git a/library/entropy.c b/library/entropy.c
index 1e0d9d328..c800bfcf6 100644
--- a/library/entropy.c
+++ b/library/entropy.c
@@ -80,6 +80,13 @@ void mbedtls_entropy_init( mbedtls_entropy_context *ctx )
 
 void mbedtls_entropy_free( mbedtls_entropy_context *ctx )
 {
+    /* 'F' is a value we used for a local variable that
+     * was originally uninitialized.
+     * If we observe it, it is likely we are using uninitialized memory
+     * when accessing ctx->accumulator_started next. */
+  if( !memcmp(&ctx->accumulator_started, "FFFF", 4) )
+        fprintf(stderr, "'F' seen\n");
+
     /* If the context was already free, don't call free() again.
      * This is important for mutexes which don't allow double-free. */
     if( ctx->accumulator_started == -1 )
diff --git a/programs/x509/cert_app.c b/programs/x509/cert_app.c
index 00d563fc7..cf0ae8605 100644
--- a/programs/x509/cert_app.c
+++ b/programs/x509/cert_app.c
@@ -135,7 +135,7 @@ int main( int argc, char *argv[] )
     int exit_code = MBEDTLS_EXIT_FAILURE;
     mbedtls_net_context server_fd;
     unsigned char buf[1024];
-    mbedtls_entropy_context entropy;
+    mbedtls_entropy_context entropy; memset(&entropy, 'F', sizeof entropy); // originally uninitialized so any value is possible
     mbedtls_ctr_drbg_context ctr_drbg;
     mbedtls_ssl_context ssl;
     mbedtls_ssl_config conf;

The second chunk initializes a local variable that was originally uninitialized. The first chunk inserts a check that shows that the memory is uninitialized at the time we are about to use it.
When I compile and run, I get:

$ make
…
$ ./programs/x509/cert_app 
  . Loading the CA root certificate ... ok (1 skipped)

 usage: cert_app param=<>...

 acceptable parameters:
    mode=file|ssl       default: none
    filename=%s         default: cert.crt
    ca_file=%s          The single file containing the top-level CA(s) you fully trust
                        default: "" (none)
    crl_file=%s         The single CRL file you want to use
                        default: "" (none)
    ca_path=%s          The path containing the top-level CA(s) you fully trust
                        default: "" (none) (overrides ca_file)
    server_name=%s      default: localhost
    server_port=%d      default: 4433
    debug_level=%d      default: 0 (disabled)
    permissive=%d       default: 0 (disabled)

'F' seen

The last line shows that ctx->accumulator_started is somewhere in the local variable entropy and that it hasn't been written to when it is used by the line if( ctx->accumulator_started == -1 ). In the original code, it would have been uninitialized.