Null pointer dereference in s2n_stuffer_write_network_order

[dougch]

Security issue notifications

If you discover a potential security issue in s2n we ask that you notify
AWS Security via our vulnerability reporting page. Please do not create a public github issue.

Problem:

Pr-requisite to #1031

When running gcc-11 with the -fanalyzer flag, a null pointer dereference is reported:

/home/dougch/gitrepos/dougch/s2n-tls/stuffer/s2n_stuffer_network_order.c: In function ‘s2n_stuffer_write_network_order’:
/home/dougch/gitrepos/dougch/s2n-tls/stuffer/s2n_stuffer_network_order.c:31:17: error: dereference of NULL ‘0’ [CWE-476] [-Werror=analyzer-null-dereference]
   31 |         data[i] = (input >> (shift)) & UINT8_MAX;
      |                 ^
  ‘s2n_stuffer_write_network_order’: event 1
    |
    |/home/dougch/gitrepos/dougch/s2n-tls/utils/s2n_ensure.h:35:12:
    |   35 |         if (!(cond)) {             \
    |      |            ^
    |      |            |
    |      |            (1) following ‘false’ branch (when ‘length <= 8’)...
/home/dougch/gitrepos/dougch/s2n-tls/utils/s2n_safety_macros.h:214:63: note: in expansion of macro ‘__S2N_ENSURE’
    |  214 | #define POSIX_ENSURE(condition, error)                        __S2N_ENSURE((condition), POSIX_BAIL(error))
    |      |                                                               ^~~~~~~~~~~~
/home/dougch/gitrepos/dougch/s2n-tls/stuffer/s2n_stuffer_network_order.c:24:5: note: in expansion of macro ‘POSIX_ENSURE’
    |   24 |     POSIX_ENSURE(length <= sizeof(input), S2N_ERR_SAFETY);
    |      |     ^~~~~~~~~~~~
    |
  ‘s2n_stuffer_write_network_order’: event 2
    |
    |/home/dougch/gitrepos/dougch/s2n-tls/utils/s2n_safety_macros.h:207:66:
    |  207 | #define POSIX_BAIL(error)                                     do { _S2N_ERROR((error)); return S2N_FAILURE; } while (0)
    |      |                                                                  ^
    |      |                                                                  |
    |      |                                                                  (2) ...to here
/home/dougch/gitrepos/dougch/s2n-tls/utils/s2n_ensure.h:36:13: note: in definition of macro ‘__S2N_ENSURE’
    |   36 |             action;                \
    |      |             ^~~~~~
/home/dougch/gitrepos/dougch/s2n-tls/utils/s2n_safety_macros.h:214:89: note: in expansion of macro ‘POSIX_BAIL’
    |  214 | #define POSIX_ENSURE(condition, error)                        __S2N_ENSURE((condition), POSIX_BAIL(error))
    |      |                                                                                         ^~~~~~~~~~
/home/dougch/gitrepos/dougch/s2n-tls/stuffer/s2n_stuffer_network_order.c:24:5: note: in expansion of macro ‘POSIX_ENSURE’
    |   24 |     POSIX_ENSURE(length <= sizeof(input), S2N_ERR_SAFETY);
    |      |     ^~~~~~~~~~~~
    |
  ‘s2n_stuffer_write_network_order’: event 3
    |
    |/home/dougch/gitrepos/dougch/s2n-tls/utils/s2n_ensure.h:35:12:
    |   35 |         if (!(cond)) {             \
    |      |            ^
    |      |            |
    |      |            (3) following ‘false’ branch...
/home/dougch/gitrepos/dougch/s2n-tls/utils/s2n_safety_macros.h:383:63: note: in expansion of macro ‘__S2N_ENSURE’
    |  383 | #define POSIX_GUARD(result)                                   __S2N_ENSURE((result) > S2N_FAILURE, return S2N_FAILURE)
    |      |                                                               ^~~~~~~~~~~~
/home/dougch/gitrepos/dougch/s2n-tls/stuffer/s2n_stuffer_network_order.c:25:5: note: in expansion of macro ‘POSIX_GUARD’
    |   25 |     POSIX_GUARD(s2n_stuffer_skip_write(stuffer, length));
    |      |     ^~~~~~~~~~~
    |
  ‘s2n_stuffer_write_network_order’: event 4
    |
    |/home/dougch/gitrepos/dougch/s2n-tls/utils/s2n_ensure.h:34:8:
    |   34 |     do {                           \
    |      |        ^
    |      |        |
    |      |        (4) ...to here
/home/dougch/gitrepos/dougch/s2n-tls/utils/s2n_safety_macros.h:383:63: note: in expansion of macro ‘__S2N_ENSURE’
    |  383 | #define POSIX_GUARD(result)                                   __S2N_ENSURE((result) > S2N_FAILURE, return S2N_FAILURE)
    |      |                                                               ^~~~~~~~~~~~
/home/dougch/gitrepos/dougch/s2n-tls/stuffer/s2n_stuffer_network_order.c:25:5: note: in expansion of macro ‘POSIX_GUARD’
    |   25 |     POSIX_GUARD(s2n_stuffer_skip_write(stuffer, length));
    |      |     ^~~~~~~~~~~
    |
  ‘s2n_stuffer_write_network_order’: events 5-9
    |
    |   26 |     uint8_t *data = (stuffer->blob.data) ? (stuffer->blob.data + stuffer->write_cursor - length) : NULL;
    |      |              ~~~~                                                                                ^
    |      |              |                                                                                   |
    |      |              (6) ...to here                                                     (5) following ‘false’ branch...
    |   27 | 
    |   28 |     for (int i = 0; i < length; i++) {
    |      |                     ~~~~~~~~~~                                                                    
    |      |                       |
    |      |                       (7) following ‘true’ branch...
    |   29 |         S2N_INVARIANT(i <= length);
    |      |                                   ~                                                               
    |      |                                   |
    |      |                                   (8) ...to here
    |   30 |         uint8_t shift = (length - i - 1) * CHAR_BIT;
    |   31 |         data[i] = (input >> (shift)) & UINT8_MAX;
    |      |             ~                                                                                     
    |      |             |
    |      |             (9) ‘0’ is NULL
    |
  ‘s2n_stuffer_write_network_order’: event 10
    |
    |   31 |         data[i] = (input >> (shift)) & UINT8_MAX;
    |      |                 ^
    |      |                 |
    |      |                 (10) dereference of NULL ‘iftmp.0_20 = PHI <iftmp.0_27(6), 0B(5)> + (sizetype)i’

CWE-476

Solution:

A description of the possible solution in terms of S2N architecture. Highlight and explain any potentially controversial design decisions taken.

• Does this change what S2N sends over the wire? If yes, explain.
• Does this change any public APIs? If yes, explain.
• Which versions of TLS will this impact?

Requirements / Acceptance Criteria:

What must a solution address in order to solve the problem? How do we know the solution is complete?

• RFC links: Links to relevant RFC(s)
• Related Issues: Link any relevant issues
• Will the Usage Guide or other documentation need to be updated?
• Testing: How will this change be tested? Call out new integration tests, functional tests, or particularly interesting/important unit tests.
  • Will this change trigger SAW changes? Changes to the state machine, the s2n_handshake_io code that controls state transitions, the DRBG, or the corking/uncorking logic could trigger SAW failures.
  • Should this change be fuzz tested? Will it handle untrusted input? Create a separate issue to track the fuzzing work.

Out of scope:

Is there anything the solution will intentionally NOT address?

[jmayclin]

This was resolved in #4436