Unsigned integer underflow in s2n_record_parse_cbc() stuffer wipe calculation

[cpsource]

Summary

In tls/s2n_record_read_cbc.c line 121, an unsigned integer underflow can occur:

POSIX_GUARD(s2n_stuffer_wipe_n(&conn->in, s2n_stuffer_data_available(&conn->in) - payload_length));

If payload_length exceeds s2n_stuffer_data_available(&conn->in), the subtraction wraps to a very large value, causing s2n_stuffer_wipe_n() to attempt wiping far more data than intended.

The issue chain:

1. Line 58: uint16_t payload_length = encrypted_length;
2. Line 85-87: s2n_sub_overflow() computes into uint32_t out, but the result is truncated back to uint16_t payload_length = out;
3. Line 112: s2n_stuffer_reread(&conn->in) resets the read cursor
4. Line 121: The subtraction s2n_stuffer_data_available() - payload_length is unguarded and can underflow

Git History

Prior work has addressed related issues in this file but not this specific bug:

• Commit 3e8e6bb8b (Oct 2020, "Adding underflow checks Adding underflow checks #2322") added s2n_sub_overflow() for the padding subtraction at line 86 — a related fix, but introduced the uint32_t → uint16_t truncation at line 87 that feeds into this bug.
• Commit 4220a4d24 (Apr 2024, "fix: Wipe conn->in on all record parse failures fix: Wipe conn->in on all record parse failures #4499") removed a s2n_stuffer_wipe(&conn->in) from the error path at line 105, but did not address line 121.

The bug at line 121 remains present as of current main.

Suggested Fix

Add an underflow guard before the subtraction:

uint32_t available = s2n_stuffer_data_available(&conn->in);
POSIX_ENSURE_GTE(available, payload_length);
POSIX_GUARD(s2n_stuffer_wipe_n(&conn->in, available - payload_length));

Also consider fixing the uint32_t → uint16_t truncation at line 87:

uint32_t out = 0;
POSIX_GUARD(s2n_sub_overflow(payload_length, en.data[en.size - 1] + 1, &out));
POSIX_ENSURE_LTE(out, UINT16_MAX);
payload_length = out;

Impact

Could cause memory corruption or denial of service when processing crafted CBC-encrypted TLS records.

Prior Art Search

• Searched GitHub issues for: s2n_record_read_cbc underflow, CBC record underflow, stuffer_wipe_n payload_length, record_read_cbc overflow — no existing reports found.
• Searched git log for related fixes — found the two commits above, neither addresses this specific bug.

Found during code review.

[mizunoyuuki]

Hi, I'd like to work on this issue.

I've implemented the suggested fix and confirmed that all existing unit tests pass.

Changes made in tls/s2n_record_read_cbc.c:

1. Line 87 — uint32_t → uint16_t truncation guard:
   Added POSIX_ENSURE_LTE(out, UINT16_MAX) after s2n_sub_overflow() to guard
   against data loss when assigning the result back to uint16_t payload_length.

2. Line 121 — underflow guard before subtraction:
   Replaced the unguarded subtraction with an explicit POSIX_ENSURE_GTE(available, payload_length) check before computing the wipe length.

I've already opened a PR: #5807