Skip to content

tls: read the InnerPlaintext content type past the padding - #5171

Closed
itzzdev09 wants to merge 1 commit into
secdev:masterfrom
itzzdev09:fix/tls13-inner-plaintext-padding
Closed

tls: read the InnerPlaintext content type past the padding#5171
itzzdev09 wants to merge 1 commit into
secdev:masterfrom
itzzdev09:fix/tls13-inner-plaintext-padding

Conversation

@itzzdev09

Copy link
Copy Markdown

The bug

RFC 8446 §5.2 lays a TLS 1.3 InnerPlaintext out as:

content || content_type || zeros(padding)

TLSInnerPlaintext.pre_dissect() tries to skip that padding, but the guard can never fire:

if s[-1] != b"\x00":

s is bytes, so s[-1] is an int and the comparison against b"\x00" is always true. The padding branch is dead code and msg_len is always len(s) - 1, so the content type is read from the final byte — which for a padded record is a pad byte.

The dead branch is also inverted: it starts at n = 1 and advances while the byte is non-zero, so even if the comparison were fixed it would stop immediately on the first zero and strip exactly one byte regardless of how much padding there is.

Same input, dissected before and after:

type pad
master 0x00 b''
this PR 0x16 b'\x00\x00\x00'

(input b"HELLO" + b"\x16" + b"\x00" * 3"). On master the record reports content type 0, which is not a valid ContentType at all, and the padding is swallowed into the message list rather than landing in pad.

The fix

Scan back over the zero padding; the last non-zero byte is the content type.

  • An unpadded record stops the loop immediately and keeps msg_len = len(s) - 1, exactly as before.
  • An all-zero payload holds no valid content type, so it keeps the previous behaviour rather than raising. I tried raising first and it broke default-constructed packets, so the degenerate case is deliberately left alone — this PR only changes records that actually carry padding.

Tests

Three cases added to test/scapy/layers/tls/tls13.uts: the padded matrix (pad lengths 1/3/16 × content types 0x15/0x16/0x17), an unpadded record, and the all-zero payload. The padded test fails on master; the other two pass in both arms and are there to pin that I didn't change the unpadded or degenerate paths.

Run on 190b4ba3 with the branch applied, and the same suites on an unmodified master worktree:

suite master this PR
tls13 53 passed, 0 failed 56 passed, 0 failed
tls 91 passed, 0 failed 91 passed, 0 failed
cert 33 passed, 37 failed 33 passed, 37 failed
sslv2 1 passed, 27 failed 1 passed, 27 failed
tlsclientserver 9 passed, 26 failed 9 passed, 26 failed

cert, sslv2 and tlsclientserver fail identically in both arms — pre-existing in this checkout with a newer cryptography (50.0.1) than it supports. Flagging them rather than omitting them; they are not caused by this change. flake8 on the touched module is identical to master.


Disclosure per CONTRIBUTING: prepared with AI assistance (Claude Code, Claude Opus 5). Found by a build/dissect round-trip sweep over scapy's packet classes; every number above comes from running the suites in both arms rather than from inspection.

🤖 Generated with Claude Code

RFC 8446 sect. 5.2 lays a TLS 1.3 InnerPlaintext out as

    content || content_type || zeros(padding)

TLSInnerPlaintext.pre_dissect() tried to skip that padding, but the guard
could never fire:

    if s[-1] != b"\x00":

s is bytes, so s[-1] is an int and the comparison against b"\x00" is
always true. The padding branch was dead code, and msg_len was always
len(s) - 1 -- the type was read from the final byte, which for a padded
record is a pad byte, not the content type.

The dead branch was also inverted: it started at n = 1 and advanced while
the byte was non-zero, so had it run it would have stopped immediately on
the first zero and stripped one byte regardless of the padding length.

Scan back over the zero padding instead; the last non-zero byte is the
content type. An unpadded record stops the loop immediately and keeps
msg_len = len(s) - 1, exactly as before. An all-zero payload holds no
valid content type (0 is not a ContentType), so it keeps the previous
behaviour rather than raising, which leaves default-constructed packets
dissectable.

Before, on b"HELLO" + b"\x16" + b"\x00" * 3:

    type=0x00  pad=b''

After:

    type=0x16  pad=b'\x00\x00\x00'

Test suites, run against this checkout and against an unmodified master
worktree for comparison. cert/sslv2/tlsclientserver fail identically in
both arms -- pre-existing here with a newer `cryptography` than this
checkout supports -- and are listed for completeness:

    tls13            53 -> 56 passed, 0 failed  (3 new; 1 fails on master)
    tls              91 passed,  0 failed       unchanged
    cert             33 passed, 37 failed       unchanged
    sslv2             1 passed, 27 failed       unchanged
    tlsclientserver   9 passed, 26 failed       unchanged

flake8 on the touched module is identical to master.

AI-Assisted: yes (Claude Code, Claude Opus 5)

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@codecov

codecov Bot commented Sep 11, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.12%. Comparing base (190b4ba) to head (806236c).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5171      +/-   ##
==========================================
+ Coverage   80.81%   81.12%   +0.30%     
==========================================
  Files         390      391       +1     
  Lines       97736    97798      +62     
==========================================
+ Hits        78986    79336     +350     
+ Misses      18750    18462     -288     
Files with missing lines Coverage Δ
scapy/layers/tls/record_tls13.py 89.83% <100.00%> (+3.05%) ⬆️

... and 20 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@gpotter2

Copy link
Copy Markdown
Member

@itzzdev09 Please do not PR farm with AI. I do not understand what you're trying to fix, the summary is unreadable. Please reopen once you've written it manually.

@gpotter2 gpotter2 closed this Sep 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants