Skip to content

Fixed TLS 1.3 record de-padding in _nx_secure_tls_process_record (RFC 8446 §5.4) #401

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
fdesbiens merged 6 commits into from
Aug 13, 2026
Merged

Fixed TLS 1.3 record de-padding in _nx_secure_tls_process_record (RFC 8446 §5.4) #401

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4570970
Fix TLS 1.3 record padding strip in _nx_secure_tls_process_record (RF…
EdouardMALOT Jun 25, 2026
c54255b
Move scan_offset to top-of-function locals, drop scoped block
EdouardMALOT Aug 4, 2026
ac7a967
Return unexpected_message when the plaintext has no inner content type
EdouardMALOT Aug 4, 2026
eeff2d4
Scan the plaintext directly instead of calling extract per byte
EdouardMALOT Aug 4, 2026
7dd2711
Add regression tests for the TLS 1.3 record de-padding
EdouardMALOT Aug 4, 2026
485adac
Declared the TLS 1.3 de-padding helper in the header, as its siblings…
fdesbiens Aug 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions nx_secure/inc/nx_secure_tls.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1407,6 +1407,8 @@ typedef struct NX_SECURE_TLS_SESSION_STRUCT

#if (NX_SECURE_TLS_TLS_1_3_ENABLED)
UINT _nx_secure_tls_1_3_crypto_init(NX_SECURE_TLS_SESSION *tls_session);
UINT _nx_secure_tls_1_3_strip_padding(NX_PACKET *decrypted_packet, USHORT *message_type_ptr,
UINT *message_length_ptr);
UINT _nx_secure_tls_1_3_client_handshake(NX_SECURE_TLS_SESSION *tls_session, UCHAR *packet_buffer,
UINT data_length, ULONG wait_option);
UINT _nx_secure_tls_1_3_server_handshake(NX_SECURE_TLS_SESSION *tls_session, UCHAR *packet_buffer,
Expand Down
94 changes: 78 additions & 16 deletions nx_secure/src/nx_secure_tls_process_record.c
View file
Open in desktop

@fdesbiens fdesbiens Jul 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need a matching regression test. This is worth having as a real test rather than just coverage: the bug is fully deterministic, so a padded-record test fails reliably on dev and passes with your fix.

test/regression/nx_secure_test/nx_secure_tls_process_record_test.c is the natural place. It already calls _nx_secure_tls_process_record() directly with hand-constructed packets and header buffers (:94, :126, :147, :175, :201, :229), which is exactly the shape needed. The one piece of setup it does not currently do is activate a remote session with a cipher, which the TLS 1.3 branch requires to be reached — nx_secure_tls_record_decrypt_coverage_test.c shows how that is arranged, so between the two files the pattern exists.

Three cases would cover the change:

  1. A padded record — inner type followed by N zero bytes — yields the correct message_type and a message_length that excludes both the type byte and the padding.
  2. An all-zeros plaintext is rejected, with the alert code from finding 2 asserted.
  3. An unpadded record behaves exactly as before, so the regression risk to existing traffic is pinned down.

Case 3 is the one I would most like to see, since "unpadded records take the same path as before" is currently an argument rather than a test.

@EdouardMALOT EdouardMALOT Aug 4, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 7dd2711. Added four cases in nx_secure_tls_process_record_test.c: padded record, all-zero plaintext, unpadded (the "existing traffic unaffected" assertion you asked for), and chained fragments. To keep the setup light, the de-padding scan was extracted into _nx_secure_tls_1_3_strip_padding — _nx_secure_tls_process_record now delegates to it, so the tests exercise the exact same code the runtime uses.

Original file line number Diff line number Diff line change
Expand Up @@ -276,22 +276,9 @@ NX_PACKET *decrypted_packet;
if (status == NX_SECURE_TLS_SUCCESS)
{

/* In TLS 1.3, encrypted records have a single byte at the
record that contains the message type (e.g. application data,
ect.), which is now the ACTUAL message type. */
status = nx_packet_data_extract_offset(decrypted_packet,
decrypted_packet -> nx_packet_length - 1,
&message_type, 1, &bytes_copied);
if (status || (bytes_copied != 1))
{
error_status = NX_SECURE_TLS_INVALID_PACKET;
}

/* Remove the content type byte from the data length to process. */
message_length = message_length - 1;

/* Adjust packet length. */
decrypted_packet -> nx_packet_length = message_length;
error_status = _nx_secure_tls_1_3_strip_padding(decrypted_packet,
&message_type,
&message_length);

/* Increment the sequence number. This is done in the MAC verify
step for 1.2 and earlier, but AEAD includes the MAC so we don't
Expand Down Expand Up @@ -646,3 +633,78 @@ NX_PACKET *current_ptr;
message_length -= (ULONG)(current_ptr -> nx_packet_append_ptr - current_ptr -> nx_packet_prepend_ptr);
}
}


#if (NX_SECURE_TLS_TLS_1_3_ENABLED)
/**************************************************************************/
/* */
/* FUNCTION RELEASE */
/* */
/* _nx_secure_tls_1_3_strip_padding PORTABLE C */
/* */
/* AUTHOR */
/* */
/* Edouard MALOT */
/* */
/* DESCRIPTION */
/* */
/* Recover the inner content type from a decrypted TLS 1.3 record and */
/* strip the trailing zero-byte padding. Per RFC 8446 §5.4, the inner */
/* content type is the last non-zero byte of the plaintext; all bytes */
/* after it are padding. The scan visits every plaintext byte, so its */
/* cost is independent of how much padding the peer added. */
/* */
/* INPUT */
/* */
/* decrypted_packet Decrypted record (possibly */
/* spanning chained fragments) */
/* message_type_ptr Set to the inner content type */
/* message_length_ptr Set to the length excluding */
/* the type byte and padding */
/* */
/* OUTPUT */
/* */
/* NX_SECURE_TLS_SUCCESS Inner type recovered */
/* NX_SECURE_TLS_UNEXPECTED_MESSAGE Plaintext had no non-zero */
/* byte (§5.4 violation) */
/* */
/**************************************************************************/
UINT _nx_secure_tls_1_3_strip_padding(NX_PACKET *decrypted_packet,
USHORT *message_type_ptr,
UINT *message_length_ptr)
{
NX_PACKET *scan_fragment;
UCHAR *scan_ptr;
ULONG running_offset = 0;
ULONG last_nonzero_offset = 0;
UCHAR last_nonzero_byte = 0;

for (scan_fragment = decrypted_packet; scan_fragment != NX_NULL; scan_fragment = scan_fragment -> nx_packet_next)
{
for (scan_ptr = scan_fragment -> nx_packet_prepend_ptr;
scan_ptr < scan_fragment -> nx_packet_append_ptr;
scan_ptr++)
{
if (*scan_ptr != 0)
{
last_nonzero_byte = *scan_ptr;
last_nonzero_offset = running_offset;
}
running_offset++;
}
}

if (last_nonzero_byte == 0)
{
*message_type_ptr = 0;
*message_length_ptr = 0;
decrypted_packet -> nx_packet_length = 0;
return(NX_SECURE_TLS_UNEXPECTED_MESSAGE);
}

*message_type_ptr = (USHORT)last_nonzero_byte;
*message_length_ptr = last_nonzero_offset;
decrypted_packet -> nx_packet_length = last_nonzero_offset;
return(NX_SECURE_TLS_SUCCESS);
}
#endif /* NX_SECURE_TLS_TLS_1_3_ENABLED */
101 changes: 101 additions & 0 deletions test/regression/nx_secure_test/nx_secure_tls_process_record_test.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,107 @@ UCHAR packet_buffer[100];
status = _nx_secure_tls_process_record(&tls_session, packet, &bytes_processed, 0);
EXPECT_EQ(NX_SECURE_TLS_INVALID_PACKET, status);

#if (NX_SECURE_TLS_TLS_1_3_ENABLED)
/* Regression tests for the TLS 1.3 record de-padding (RFC 8446 §5.4).
Before the fix, the code read the literal last byte of the plaintext
as the inner content type; any padded record from a compliant peer
(JDK 11+, OpenSSL with padding on) was mishandled. */
{
NX_PACKET *decrypted;
USHORT out_type;
UINT out_length;
UCHAR inner_plaintext[16];

/* Case 1: padded record. Plaintext = "hello" + type byte + 5 zeros.
The inner type must come back and the length must exclude both
the type byte and the padding. */
tls_session.nx_secure_record_queue_header = NX_NULL;
status = nx_packet_allocate(&pool_0, &decrypted, NX_IPv4_TCP_PACKET, NX_WAIT_FOREVER);
EXPECT_EQ(NX_SUCCESS, status);

memset(inner_plaintext, 0, sizeof(inner_plaintext));
memcpy(inner_plaintext, "hello", 5);
inner_plaintext[5] = NX_SECURE_TLS_APPLICATION_DATA;
status = nx_packet_data_append(decrypted, inner_plaintext, 11, &pool_0, NX_WAIT_FOREVER);
EXPECT_EQ(NX_SUCCESS, status);

out_type = 0;
out_length = 0;
status = _nx_secure_tls_1_3_strip_padding(decrypted, &out_type, &out_length);
EXPECT_EQ(NX_SECURE_TLS_SUCCESS, status);
EXPECT_EQ((USHORT)NX_SECURE_TLS_APPLICATION_DATA, out_type);
EXPECT_EQ((UINT)5, out_length);
EXPECT_EQ((ULONG)5, decrypted -> nx_packet_length);

status = nx_packet_release(decrypted);
EXPECT_EQ(NX_SUCCESS, status);

/* Case 2: all-zero plaintext. §5.4 says this is a peer protocol
violation and must yield unexpected_message. */
status = nx_packet_allocate(&pool_0, &decrypted, NX_IPv4_TCP_PACKET, NX_WAIT_FOREVER);
EXPECT_EQ(NX_SUCCESS, status);

memset(inner_plaintext, 0, sizeof(inner_plaintext));
status = nx_packet_data_append(decrypted, inner_plaintext, 10, &pool_0, NX_WAIT_FOREVER);
EXPECT_EQ(NX_SUCCESS, status);

out_type = 0xff;
out_length = 42;
status = _nx_secure_tls_1_3_strip_padding(decrypted, &out_type, &out_length);
EXPECT_EQ(NX_SECURE_TLS_UNEXPECTED_MESSAGE, status);
EXPECT_EQ((USHORT)0, out_type);
EXPECT_EQ((UINT)0, out_length);

status = nx_packet_release(decrypted);
EXPECT_EQ(NX_SUCCESS, status);

/* Case 3: unpadded record. Pre-fix arithmetic gave length - 1; the
new scan must return the exact same values so existing traffic is
unaffected. This is the regression-risk assertion. */
status = nx_packet_allocate(&pool_0, &decrypted, NX_IPv4_TCP_PACKET, NX_WAIT_FOREVER);
EXPECT_EQ(NX_SUCCESS, status);

memcpy(inner_plaintext, "hello", 5);
inner_plaintext[5] = NX_SECURE_TLS_APPLICATION_DATA;
status = nx_packet_data_append(decrypted, inner_plaintext, 6, &pool_0, NX_WAIT_FOREVER);
EXPECT_EQ(NX_SUCCESS, status);

out_type = 0;
out_length = 0;
status = _nx_secure_tls_1_3_strip_padding(decrypted, &out_type, &out_length);
EXPECT_EQ(NX_SECURE_TLS_SUCCESS, status);
EXPECT_EQ((USHORT)NX_SECURE_TLS_APPLICATION_DATA, out_type);
EXPECT_EQ((UINT)5, out_length);

status = nx_packet_release(decrypted);
EXPECT_EQ(NX_SUCCESS, status);

/* Case 4: padded record that straddles a fragment boundary. The scan
must walk through the chained NX_PACKETs, not just the head. */
status = nx_packet_allocate(&pool_0, &decrypted, NX_IPv4_TCP_PACKET, NX_WAIT_FOREVER);
EXPECT_EQ(NX_SUCCESS, status);

/* Payload > single-packet capacity (1536) forces chaining. Fill with
a non-zero content byte, then the type, then trailing zeros — the
trailing zeros will land in a later fragment. */
memset(data_buffer, 0xAB, 2000);
data_buffer[1999] = NX_SECURE_TLS_APPLICATION_DATA;
memset(&data_buffer[2000], 0, 500);
status = nx_packet_data_append(decrypted, data_buffer, 2500, &pool_0, NX_WAIT_FOREVER);
EXPECT_EQ(NX_SUCCESS, status);

out_type = 0;
out_length = 0;
status = _nx_secure_tls_1_3_strip_padding(decrypted, &out_type, &out_length);
EXPECT_EQ(NX_SECURE_TLS_SUCCESS, status);
EXPECT_EQ((USHORT)NX_SECURE_TLS_APPLICATION_DATA, out_type);
EXPECT_EQ((UINT)1999, out_length);

status = nx_packet_release(decrypted);
EXPECT_EQ(NX_SUCCESS, status);
}
#endif /* NX_SECURE_TLS_TLS_1_3_ENABLED */

printf("SUCCESS!\n");
test_control_return(0);
}
Expand Down