Skip to content
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

Jump to bottom

QUIC TLS conformance updates #21686

Closed
wants to merge 7 commits into from
Closed

QUIC TLS conformance updates #21686

wants to merge 7 commits into from

Conversation

mattcaswell
Copy link
Member

Updates for a few issues related to conformance in the QUIC TLS implementation.

@mattcaswell mattcaswell added branch: master Merge to master branch approval: review pending This pull request needs review by a committer approval: otc review pending This pull request needs review by an OTC member tests: present The PR has suitable tests present labels Aug 8, 2023
@mattcaswell mattcaswell linked an issue Aug 8, 2023 that may be closed by this pull request
Adapt QUIC TLS module to process post-handshake TLS handshake messages openssl/project#121
Closed
@mattcaswell
Copy link
Member Author

New commit pushed addressing a pre-existing use-after-free bug that the new tests highlighted in the CIs.

@t8m
Copy link
Member

t8m commented Aug 8, 2023

There are conflicts now, please rebase.

@mattcaswell
Copy link
Member Author

There are conflicts now, please rebase.

Rebased.

@t8m t8m added the triaged: bug The issue/pr is/fixes a bug label Aug 8, 2023
@t8m t8m removed the approval: otc review pending This pull request needs review by an OTC member label Aug 8, 2023
@mattcaswell
Copy link
Member Author

Ping for second review?

hlandau
hlandau approved these changes Aug 9, 2023
View reviewed changes
Copy link
Member

@hlandau hlandau left a comment

Choose a reason for hiding this comment

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

Great stuff. My approval stands either way with regards to the below comment.

If you do choose to renumber my approval can be assumed to stand for any trivial renumbering of the tests.

test/quic_multistream_test.c Outdated Show resolved Hide resolved
@hlandau hlandau 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 9, 2023
@mattcaswell
Copy link
Member Author

Force pushed an update containing a trivial renumbering of the scripts only as per @hlandau suggestion.

@t8m - please reconfirm? I assume the 24-hour timer does not need to reset?

t8m
t8m approved these changes Aug 10, 2023
View reviewed changes
Copy link
Member

@t8m t8m left a comment

Choose a reason for hiding this comment

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

Approved with the renumbering. IMO the merge waiting window does not need to reset.

@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.

@mattcaswell mattcaswell added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Aug 11, 2023
@mattcaswell
Unexpected QUIC post-handshake CertificateRequests are a PROTOCOL_VIO…
b75f90d 
…LATION

An OpenSSL QUIC client does not send the post_handshake_auth extension.
Therefore if a server sends a post-handsahke CertificateRequest then this
would be treated as a TLS protocol violation with an "unexpected message"
alert code. However RFC 9001 specifically requires us to treat this as
QUIC PROTOCOL_VIOLATION. So we have to translate the "unexpected message"
alert code in this one instance.
@mattcaswell
Don't forget we are doing QUIC if we clear the QUIC TLS data
1cac469 
We should retain the TLS1_FLAGS_QUIC setting in in s3.flags even after a
"clear" operation.
@mattcaswell
NewSessionTickets with an early_data extension must have a valid max …
8696faa 
…value

The max_early_data value must be 0xffffffff if the extension is present in
a NewSessionTicket message in QUIC. Otherwise it is a PROTOCOL_VIOLATION.
@mattcaswell
TLS KeyUpdate messages are not allowed in QUIC
b44abad 
We already disallowed the sending of TLS KeyUpdate messages. We also treat
the receipt of a TLS KeyUpdate message as an unexpected message.

RFC 9001 section 6:
Endpoints MUST treat the receipt of a TLS KeyUpdate message as a connection
error of type 0x010a, equivalent to a fatal TLS alert of unexpected_message;
see Section 4.8.
@mattcaswell
Add the ability to send NewSessionTicket messages when we want them
a525a92
@mattcaswell
Add a test for receiving a post-handshake CertificateRequest
3818734 
This should result in a QUIC PROTOCOL_VIOLATION

We also add tests for a post-handshake KeyUpdate, and a NewSessionTicket
with an invalid max_early_data value.
@mattcaswell
Fix a use-after-free in quic_tls.c
14dac16 
The comments in quic_tls.c claimed that the dummybio was never used by
us. In fact that is not entirely correct since we set and cleared the
retry flags on it. This means that we have to manage it properly, and update
it in the event of set1_bio() call on the record layer method.
@mattcaswell mattcaswell added approval: review pending This pull request needs review by a committer approval: otc review pending This pull request needs review by an OTC member and removed approval: ready to merge The 24 hour grace period has passed, ready to merge labels Aug 11, 2023
@mattcaswell
Copy link
Member Author

Unfortunately, as I suspected, after #21565 went in the resulting merge conflicts were significant and non-trivial to resolve.

I've now rebased and resolved all of those conflicts, but this now needs another round of reconfirmations.

@hlandau hlandau removed the approval: otc review pending This pull request needs review by an OTC member label Aug 11, 2023
@mattcaswell
Copy link
Member Author

@t8m - please reconfirm?

@t8m t8m 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 14, 2023
@mattcaswell mattcaswell added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Aug 15, 2023
@mattcaswell
Copy link
Member Author

Pushed. Thanks.

openssl-machine pushed a commit that referenced this pull request Aug 15, 2023
@mattcaswell
Unexpected QUIC post-handshake CertificateRequests are a PROTOCOL_VIO…
b644a93 
…LATION

An OpenSSL QUIC client does not send the post_handshake_auth extension.
Therefore if a server sends a post-handsahke CertificateRequest then this
would be treated as a TLS protocol violation with an "unexpected message"
alert code. However RFC 9001 specifically requires us to treat this as
QUIC PROTOCOL_VIOLATION. So we have to translate the "unexpected message"
alert code in this one instance.

Reviewed-by: Hugo Landau <hlandau@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #21686)
openssl-machine pushed a commit that referenced this pull request Aug 15, 2023
@mattcaswell
Don't forget we are doing QUIC if we clear the QUIC TLS data
0f2add9 
We should retain the TLS1_FLAGS_QUIC setting in in s3.flags even after a
"clear" operation.

Reviewed-by: Hugo Landau <hlandau@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #21686)
openssl-machine pushed a commit that referenced this pull request Aug 15, 2023
@mattcaswell
NewSessionTickets with an early_data extension must have a valid max …
04c7fb5 
…value

The max_early_data value must be 0xffffffff if the extension is present in
a NewSessionTicket message in QUIC. Otherwise it is a PROTOCOL_VIOLATION.

Reviewed-by: Hugo Landau <hlandau@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #21686)
openssl-machine pushed a commit that referenced this pull request Aug 15, 2023
@mattcaswell
TLS KeyUpdate messages are not allowed in QUIC
50a0af2 
We already disallowed the sending of TLS KeyUpdate messages. We also treat
the receipt of a TLS KeyUpdate message as an unexpected message.

RFC 9001 section 6:
Endpoints MUST treat the receipt of a TLS KeyUpdate message as a connection
error of type 0x010a, equivalent to a fatal TLS alert of unexpected_message;
see Section 4.8.

Reviewed-by: Hugo Landau <hlandau@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #21686)
openssl-machine pushed a commit that referenced this pull request Aug 15, 2023
@mattcaswell
Add the ability to send NewSessionTicket messages when we want them
614c08c 
Reviewed-by: Hugo Landau <hlandau@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #21686)
openssl-machine pushed a commit that referenced this pull request Aug 15, 2023
@mattcaswell
Add a test for receiving a post-handshake CertificateRequest
644ef0b 
This should result in a QUIC PROTOCOL_VIOLATION

We also add tests for a post-handshake KeyUpdate, and a NewSessionTicket
with an invalid max_early_data value.

Reviewed-by: Hugo Landau <hlandau@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #21686)
openssl-machine pushed a commit that referenced this pull request Aug 15, 2023
@mattcaswell
Fix a use-after-free in quic_tls.c
643f542 
The comments in quic_tls.c claimed that the dummybio was never used by
us. In fact that is not entirely correct since we set and cleared the
retry flags on it. This means that we have to manage it properly, and update
it in the event of set1_bio() call on the record layer method.

Reviewed-by: Hugo Landau <hlandau@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #21686)
@mattcaswell mattcaswell mentioned this pull request Sep 4, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hlandau hlandau hlandau approved these changes

@t8m t8m t8m approved these changes

Assignees
No one assigned
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch tests: present The PR has suitable tests present triaged: bug The issue/pr is/fixes a bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Adapt QUIC TLS module to process post-handshake TLS handshake messages
4 participants
@mattcaswell @t8m @openssl-machine @hlandau