Skip to content

Use TLS record limit for REALITY target buffer#35

Open
M13-Perfect wants to merge 1 commit into
XTLS:mainfrom
M13-Perfect:use-tls-record-limit-for-target-buffer
Open

Use TLS record limit for REALITY target buffer#35
M13-Perfect wants to merge 1 commit into
XTLS:mainfrom
M13-Perfect:use-tls-record-limit-for-target-buffer

Conversation

@M13-Perfect

Copy link
Copy Markdown

Related to XTLS/Xray-core#6356.

Summary

REALITY currently uses a hardcoded 8192-byte buffer size for target TLS records.

This patch replaces the hardcoded value with the existing TLS record bound:

recordHeaderLen + maxCiphertextTLS13

This keeps the existing size variable and all downstream logic unchanged,
while deriving the bound from existing TLS constants instead of using an
arbitrary value.

Rationale

The linked Xray-core issue shows that www.microsoft.com can return a TLS
Certificate record with total length 8273 bytes, which exceeds the previous
8192-byte limit.

maxCiphertextTLS13 is already defined as 16384 + 256, and recordHeaderLen
accounts for the TLS record header. Using these existing constants keeps the
buffer bounded and aligned with the TLS 1.3 record boundary.

Relation to #33

This is an alternative implementation to #33. Instead of using 17 * 1024,
this patch reuses the existing TLS constants and keeps the change to a single
line.

Validation
Ran gofmt -w tls.go
Ran git diff --check
Verified in Docker

@M13-Perfect

Copy link
Copy Markdown
Author

Hi maintainers, this is my first contribution to REALITY.

I kept the change intentionally small by reusing the existing TLS constants instead of introducing another literal buffer size.

If you prefer a dedicated constant name, I am happy to adjust the patch.

@lArtiquel

Copy link
Copy Markdown

Reviewed this against #33 on current main, and this is the more precise fix — I'd suggest merging it.

The steal-handshake reader parses the target's response one TLS record at a time: handshakeLen = recordHeaderLen + Value(s2cSaved[3:5]...) (the record-length field) at tls.go:329, then trims s2cSaved = s2cSaved[handshakeLen:] after each record. So size only ever has to hold a single record, and the largest a TLS 1.3 record can be is exactly recordHeaderLen + maxCiphertextTLS13. That's the value this PR uses, which makes it not merely "big enough" but the tightest bound that is provably sufficient for any conformant target — and it can't be outgrown by a larger certificate chain, since multi-record chains are already consumed record-by-record.

It's also the ceiling REALITY already enforces on its own record reader (conn.go:707: n > maxCiphertextTLS13), so deriving size from the same constant keeps the two paths consistent instead of introducing a second independent literal.

By comparison, #33's 17 * 1024 (17408) also clears the 16645 bound and works, but it's a loose literal — its value had to be revised up from 16384 once TLS 1.3 record overhead was accounted for, which is exactly the guesswork that deriving from maxCiphertextTLS13 avoids.

Builds cleanly against current main. The Microsoft case in #6356 (Certificate record 8273 B) sits well under the new bound.

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