Closed
Description
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’
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?