pqc-thunderbird / libgcrypt

GNU General Public License v2.0
1 stars 0 forks source link

-fanalyzer warning for mlkem public_key_size_bytes #60

Closed falko-strenzke closed 6 months ago

falko-strenzke commented 6 months ago
cipher/mlkem.c|528 col 18| warning: use of uninitialized value ‘public_key_size_bytes’ [CWE-457] [-Wanalyzer-use-of-uninitialized-value]
||   528 |   bit_strength = bitstrength_from_public_size_bytes (public_key_size_bytes);
||       |                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
||   ‘mlkem_encap’: events 1-4
||     |
||     |  509 | mlkem_encap (gcry_sexp_t *r_ciph,
||     |      | ^~~~~~~~~~~
||     |      | |
||     |      | (1) entry to ‘mlkem_encap’
||     |......
||     |  516 |   size_t public_key_size_bytes;
||     |      |          ~~~~~~~~~~~~~~~~~~~~~
||     |      |          |
||     |      |          (2) region created on stack here
||     |      |          (3) capacity: 8 bytes
||     |......
||     |  522 |   ec = public_key_from_sexp (
||     |      |        ~~~~~~~~~~~~~~~~~~~~~~
||     |      |        |
||     |      |        (4) calling ‘public_key_from_sexp’ from ‘mlkem_encap’
||     |  523 |       keyparms, &public_key, &public_key_size_bytes, NULL);
||     |      |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
||     |
||     +--> ‘public_key_from_sexp’: events 5-8
||            |
||            |  342 | public_key_from_sexp (const gcry_sexp_t keyparms,
||            |      | ^~~~~~~~~~~~~~~~~~~~
||            |      | |
||            |      | (5) entry to ‘public_key_from_sexp’
||            |......
||            |  348 |   return extract_opaque_mpi_from_sexp (keyparms,
||            |      |          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
||            |      |          |
||            |      |          (6) following ‘false’ branch (when ‘param’ is NULL)...
||            |      |          (7) ...to here
||            |      |          (8) calling ‘extract_opaque_mpi_from_sexp’ from ‘public_key_from_sexp’
||            |  349 |                                        "/y",
||            |      |                                        ~~~~~
||            |  350 |                                        pk_p,
||            |      |                                        ~~~~~
||            |  351 |                                        pk_len_p,
||            |      |                                        ~~~~~~~~~
||            |  352 |                                        param != NULL ? &param->public_key_bytes
||            |      |                                        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
||            |  353 |                                                      : (u16 *)NULL,
||            |      |                                                      ~~~~~~~~~~~~~~
||            |  354 |                                        _gcry_malloc);
||            |      |                                        ~~~~~~~~~~~~~
||            |
||            +--> ‘extract_opaque_mpi_from_sexp’: events 9-10
||                   |
||                   |  233 | extract_opaque_mpi_from_sexp (const gcry_sexp_t keyparms,
||                   |      | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
||                   |      | |
||                   |      | (9) entry to ‘extract_opaque_mpi_from_sexp’
||                   |......
||                   |  249 |   if (ec)
||                   |      |      ~
||                   |      |      |
||                   |      |      (10) following ‘false’ branch (when ‘ec == 0’)...
||                   |
||                 ‘extract_opaque_mpi_from_sexp’: event 11
||                   |
|../src/gcrypt-int.h|554| 32:
||                   |  554 | #define mpi_get_nbits(a)       _gcry_mpi_get_nbits ((a))
||                   |      |                                ^~~~~~~~~~~~~~~~~~~~~~~~~
||                   |      |                                |
||                   |      |                                (11) ...to here
cipher/mlkem.c|253 col 14| note: in expansion of macro ‘mpi_get_nbits’
||                   |  253 |   data_len = mpi_get_nbits (sk);
||                   |      |              ^~~~~~~~~~~~~
||                   |
||                 ‘extract_opaque_mpi_from_sexp’: events 12-19
||                   |
||                   |  254 |   if (data_len % 8)
||                   |      |      ^
||                   |      |      |
||                   |      |      (12) following ‘false’ branch...
||                   |......
||                   |  258 |   data_len /= 8;
||                   |      |   ~~~~~~~~~~~~~
||                   |      |            |
||                   |      |            (13) ...to here
||                   |  259 |   if (expected_size_bytes_opt
||                   |      |      ~
||                   |      |      |
||                   |      |      (14) following ‘false’ branch (when ‘expected_size_bytes_opt’ is NULL)...
||                   |......
||                   |  265 |   *data_p = alloc_func (data_len);
||                   |      |             ~~~~~~~~~~~~~~~~~~~~~
||                   |      |             |
||                   |      |             (15) ...to here
||                   |  266 |   if (*data_p == NULL)
||                   |      |      ~
||                   |      |      |
||                   |      |      (16) following ‘true’ branch...
||                   |  267 |     {
||                   |  268 |       ec = gpg_err_code_from_syserror ();
||                   |      |            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
||                   |      |            |
||                   |      |            (17) ...to here
||                   |......
||                   |  288 |   if (ec)
||                   |      |      ~
||                   |      |      |
||                   |      |      (18) following ‘false’ branch (when ‘ec == 0’)...
||                   |......
||                   |  293 |   return ec;
||                   |      |          ~~
||                   |      |          |
||                   |      |          (19) ...to here
||                   |
||            <------+
||            |
||          ‘public_key_from_sexp’: event 20
||            |
||            |  348 |   return extract_opaque_mpi_from_sexp (keyparms,
||            |      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
||            |      |          |
||            |      |          (20) returning to ‘public_key_from_sexp’ from ‘extract_opaque_mpi_from_sexp’
||            |  349 |                                        "/y",
||            |      |                                        ~~~~~
||            |  350 |                                        pk_p,
||            |      |                                        ~~~~~
||            |  351 |                                        pk_len_p,
||            |      |                                        ~~~~~~~~~
||            |  352 |                                        param != NULL ? &param->public_key_bytes
||            |      |                                        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
||            |  353 |                                                      : (u16 *)NULL,
||            |      |                                                      ~~~~~~~~~~~~~~
||            |  354 |                                        _gcry_malloc);
||            |      |                                        ~~~~~~~~~~~~~
||            |
||     <------+
||     |
||   ‘mlkem_encap’: events 21-24
||     |
||     |  522 |   ec = public_key_from_sexp (
||     |      |        ^~~~~~~~~~~~~~~~~~~~~~
||     |      |        |
||     |      |        (21) returning to ‘mlkem_encap’ from ‘public_key_from_sexp’
||     |  523 |       keyparms, &public_key, &public_key_size_bytes, NULL);
||     |      |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
||     |  524 |   if (ec)
||     |      |      ~  
||     |      |      |
||     |      |      (22) following ‘false’ branch (when ‘ec == 0’)...
||     |......
||     |  528 |   bit_strength = bitstrength_from_public_size_bytes (public_key_size_bytes);
||     |      |                  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
||     |      |                  |
||     |      |                  (23) ...to here
||     |      |                  (24) use of uninitialized value ‘public_key_size_bytes’ here
||     |
|| mlkem.c: In function ‘mlkem_decrypt’:
falko-strenzke commented 6 months ago

This is false positive: the analyzer doesn't know that the line

      ec = gpg_err_code_from_syserror ();

will assign a non-zero value to ec in case of memory exhaustion. In its further anaylsis, it thus thinks that the failing memory allocation will lead to null pointer deref since it misses that the leave is being jumped to.