Fix TLS 1.3 record padding handling in _nx_secure_tls_process_record (RFC 8446 §5.4)

[EdouardMALOT]

Summary

A TLS 1.3 peer that pads its records cannot talk to NetX Duo: the very first padded record is rejected and the connection is torn down with a fatal alert.

Per RFC 8446 §5.4, the decrypted TLS 1.3 record is:

struct {
    opaque content[TLSPlaintext.length];
    ContentType type;
    uint8 zeros[length_of_padding];
} TLSInnerPlaintext;

The receiver must scan from the end of the plaintext, skipping zero bytes, to find the inner content type. _nx_secure_tls_process_record() instead reads the literal last byte as the content type and strips exactly one byte. For any padded record the extracted type is 0x00 (invalid, later rejected as an unrecognized message type), and the padding bytes would remain inside the message content.

Record padding is not exotic: the Java JDK (11+) TLS 1.3 implementation sends padded records (encountered in the field with a Java client), and OpenSSL produces them with -record_padding N.

Changes

• nx_secure/src/nx_secure_tls_process_record.c — after decryption of a TLS 1.3 record, scan backward from the end of the plaintext skipping zero bytes; the first non-zero byte is the inner content type, and the message length is set to exclude the type byte and the padding. A plaintext consisting only of zeros is rejected (NX_SECURE_TLS_INVALID_PACKET), as required by §5.4 ("If a receiving implementation does not find a non-zero octet in the cleartext, it MUST terminate the connection").

Unpadded records take the same path as before: the first byte inspected (the last plaintext byte) is non-zero and the resulting length is unchanged.

Note: for the all-zeros case, RFC 8446 prescribes an unexpected_message alert; NX_SECURE_TLS_INVALID_PACKET currently maps to the internal-error alert group in _nx_secure_tls_map_error_to_alert(). The connection is still terminated with a fatal alert; happy to adjust the mapping if a stricter alert code is preferred.

Test plan

• TLS 1.3 session against a peer sending padded records (Java 11+ HTTPS client): previously failed on the first encrypted record, now completes
• OpenSSL with -record_padding N: handshake and application data exchange succeed
• Unpadded TLS 1.3 traffic (OpenSSL/Mosquitto defaults): unchanged
• TLS 1.2 record processing: not affected (code path is TLS 1.3-only)

[fdesbiens]

Thank you — this is a genuine interoperability bug, well identified, and the RFC citation is on point. What I verified before the findings:

The bug is real and the diagnosis is exact. The old code read the literal last byte of the decrypted plaintext as the inner content type and stripped exactly one byte. For any record with padding that yields message_type == 0, and the padding bytes stay inside the message content. Your note that this is not an exotic case matches my expectation: JDK 11+ pads by default and OpenSSL will on request.

The scan logic is correct. scan_offset starts at nx_packet_length and is decremented only after the scan_offset > 0 test, so the indices visited are length-1 down to 0 with no underflow and no read before the start of the plaintext. message_length = scan_offset is exactly the content length, since the content occupies [0, scan_offset) with the type byte at scan_offset. For an unpadded record the first byte inspected is non-zero, giving message_length = length - 1 — identical to the old arithmetic, so existing traffic is provably unaffected.

Both degenerate cases are handled. An all-zeros plaintext leaves message_type == 0 and is rejected, as §5.4 requires. A zero-length plaintext skips the loop entirely and is also rejected, which is right — a TLSInnerPlaintext with no content type is malformed.

The error path is ordered safely. I checked this specifically because message_type == 0 would be dangerous if it escaped: the if (error_status != NX_SECURE_TLS_SUCCESS) return(error_status); at :362 runs before message_type is used for message dispatch at :383, so a failed scan always returns before the type is acted on.

Three things to change:

The de-padding loop is more expensive than it looks. nx_packet_data_extract_offset() traverses the packet chain from the head on every call (common/src/nx_packet_data_extract_offset.c:37-62), and because the scan starts at the end of the record, every call walks the entire chain. The loop calls it once per padding byte. Details and a fix on :291.

Your open question about the alert has a clean answer — a one-token change gives exactly the RFC-required unexpected_message. Details on :307.

There is no regression test, and this one is straightforward to write because an existing test already invokes this function directly. Details in finding 3.

[fdesbiens]

The logic is right, but the primitive is the wrong one to call in a loop. nx_packet_data_extract_offset() walks the packet chain from the head on every invocation to locate the offset — see the traversal at common/src/nx_packet_data_extract_offset.c:37-62. Since this scan starts at the end of the record, every call walks the chain to its last fragment before copying a single byte.

So the cost is one full chain traversal plus one function call, per padding byte. NX_SECURE_TLS_MAX_PLAINTEXT_LENGTH is 16384 (nx_secure/inc/nx_secure_tls.h:626), and §5.4 permits a record to be almost entirely padding, so a peer can make NetX Duo perform on the order of 16000 chain-walking calls to extract one content-type byte — and repeat it for every record it sends. On the class of device this stack targets that is a meaningful amount of work handed to whoever is on the other end of the socket.

The decrypted packet is produced locally by the decryption routine just above, so its layout is known and it can be scanned directly in one pass: take the last fragment, walk its bytes backward from nx_packet_append_ptr toward nx_packet_prepend_ptr, and only step to an earlier fragment when an entire fragment turns out to be zeros. That is O(bytes examined) with no per-byte call, and it keeps the same semantics.

There is a second reason to prefer a single pass. The current loop's runtime is proportional to the padding length, which makes the padding length observable through timing. In most threat models that is not sensitive — but record padding exists precisely to frustrate traffic analysis, so leaking the quantity of padding through processing time undercuts the feature being implemented. Worth noting that this codebase already carries an explicit warning about exactly this class of issue: the comment at :342-346 in this same function insists the TLS 1.2 MAC check run regardless of decryption state, specifically to avoid a padding-related timing channel. A data-independent scan here would be consistent with that stance.

[fdesbiens]

You asked whether the mapping should be adjusted for a stricter alert code. It should, and it is simpler than you feared — no change to _nx_secure_tls_map_error_to_alert() is needed at all.

NX_SECURE_TLS_UNEXPECTED_MESSAGE already maps to exactly what RFC 8446 §5.4 requires: nx_secure/src/nx_secure_tls_map_error_to_alert.c:83-86 sets alert_number = NX_SECURE_TLS_ALERT_UNEXPECTED_MESSAGE (10) at NX_SECURE_TLS_ALERT_LEVEL_FATAL. NX_SECURE_TLS_INVALID_PACKET, by contrast, is in the large fall-through group ending at :261-265 that resolves to NX_SECURE_TLS_ALERT_INTERNAL_ERROR.

So returning NX_SECURE_TLS_UNEXPECTED_MESSAGE here is a one-token change that makes the behaviour spec-conformant.

Please keep the two failure cases distinct, though. The all-zeros case is a peer protocol violation and should be UNEXPECTED_MESSAGE. The extract-failure case at :296 is our own packet handling going wrong rather than anything the peer did, so INVALID_PACKET — and its internal_error alert — is the honest signal there. Right now both report the same thing, which loses that distinction.

[fdesbiens]

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.

[fdesbiens]

Two small things.

The change introduces a bare nested block solely to scope ULONG scan_offset, whereas this file declares all locals at the top of the function (:83-92). Declaring scan_offset alongside message_length and bytes_copied and dropping the extra braces would match the surrounding code and remove a level of indentation from an already deeply nested region.

The comment carries commit-message content: "Reading the literal last byte broke any padded record (Java JDK HttpClient pads by default) — scan back, skip the zeros." The first half explains what the code used to do wrong, which belongs in the commit message and the PR description where you have already written it well. A source comment stating what the code does now — the inner content type is the last non-zero byte, everything after it is padding — is what a future reader needs.