Skip to content

Comments

Tolerate a bad record version in TLSv1.3 plaintext records#19058

Closed
mattcaswell wants to merge 3 commits intoopenssl:masterfrom
mattcaswell:tolerate-bad-version
Closed

Tolerate a bad record version in TLSv1.3 plaintext records#19058
mattcaswell wants to merge 3 commits intoopenssl:masterfrom
mattcaswell:tolerate-bad-version

Conversation

@mattcaswell
Copy link
Member

When a server responds to a second TLSv1.3 ClientHello it is required to
set the legacy_record_version to 0x0303 (TLSv1.2). The client is required
to ignore that field even if it is wrong. The recent changes to the read
record layer in PR #18132 made the record layer stricter and it was
checking that the legacy_record_version was the correct value. This
caused connection failures when talking to buggy servers that set the
wrong legacy_record_version value.

We make us more tolerant again.

Fixes #19051

When a server responds to a second TLSv1.3 ClientHello it is required to
set the legacy_record_version to 0x0303 (TLSv1.2). The client is required
to ignore that field even if it is wrong. The recent changes to the read
record layer in PR openssl#18132 made the record layer stricter and it was
checking that the legacy_record_version was the correct value. This
caused connection failures when talking to buggy servers that set the
wrong legacy_record_version value.

We make us more tolerant again.

Fixes openssl#19051
The RFC requires us to ignore this field in plaintext records - so even
if it is set incorrectly we should tolerate it.
@mattcaswell mattcaswell added branch: master Applies to master branch approval: review pending This pull request needs review by a committer approval: otc review pending labels Aug 24, 2022
@t8m t8m added triaged: bug The issue/pr is/fixes a bug and removed approval: otc review pending labels Aug 24, 2022
@Tarnum-tst
Copy link

Tarnum-tst commented Aug 26, 2022

Tested/working. Ping @openssl/contractors for second review. :-)

@mattcaswell
Copy link
Member Author

Ping @openssl/committers for second review

@ghost

This comment was marked as spam.

Copy link
Member

@beldmit beldmit left a comment

Choose a reason for hiding this comment

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

LGTM

@beldmit beldmit added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Aug 26, 2022
@openssl-machine
Copy link
Collaborator

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

@t8m
Copy link
Member

t8m commented Aug 29, 2022

Merged to master branch. Thank you.

@t8m t8m closed this Aug 29, 2022
openssl-machine pushed a commit that referenced this pull request Aug 29, 2022
When a server responds to a second TLSv1.3 ClientHello it is required to
set the legacy_record_version to 0x0303 (TLSv1.2). The client is required
to ignore that field even if it is wrong. The recent changes to the read
record layer in PR #18132 made the record layer stricter and it was
checking that the legacy_record_version was the correct value. This
caused connection failures when talking to buggy servers that set the
wrong legacy_record_version value.

We make us more tolerant again.

Fixes #19051

Reviewed-by: Dmitry Belyavskiy <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from #19058)
openssl-machine pushed a commit that referenced this pull request Aug 29, 2022
The RFC requires us to ignore this field in plaintext records - so even
if it is set incorrectly we should tolerate it.

Reviewed-by: Dmitry Belyavskiy <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from #19058)
sftcd pushed a commit to sftcd/openssl that referenced this pull request Sep 24, 2022
When a server responds to a second TLSv1.3 ClientHello it is required to
set the legacy_record_version to 0x0303 (TLSv1.2). The client is required
to ignore that field even if it is wrong. The recent changes to the read
record layer in PR openssl#18132 made the record layer stricter and it was
checking that the legacy_record_version was the correct value. This
caused connection failures when talking to buggy servers that set the
wrong legacy_record_version value.

We make us more tolerant again.

Fixes openssl#19051

Reviewed-by: Dmitry Belyavskiy <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#19058)
sftcd pushed a commit to sftcd/openssl that referenced this pull request Sep 24, 2022
The RFC requires us to ignore this field in plaintext records - so even
if it is set incorrectly we should tolerate it.

Reviewed-by: Dmitry Belyavskiy <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#19058)
beldmit pushed a commit to beldmit/openssl that referenced this pull request Dec 26, 2022
When a server responds to a second TLSv1.3 ClientHello it is required to
set the legacy_record_version to 0x0303 (TLSv1.2). The client is required
to ignore that field even if it is wrong. The recent changes to the read
record layer in PR openssl#18132 made the record layer stricter and it was
checking that the legacy_record_version was the correct value. This
caused connection failures when talking to buggy servers that set the
wrong legacy_record_version value.

We make us more tolerant again.

Fixes openssl#19051

Reviewed-by: Dmitry Belyavskiy <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#19058)
beldmit pushed a commit to beldmit/openssl that referenced this pull request Dec 26, 2022
The RFC requires us to ignore this field in plaintext records - so even
if it is set incorrectly we should tolerate it.

Reviewed-by: Dmitry Belyavskiy <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#19058)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approval: done This pull request has the required number of approvals branch: master Applies to master branch triaged: bug The issue/pr is/fixes a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unable to connect via TLS 1.3

5 participants