-
Notifications
You must be signed in to change notification settings - Fork 619
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.
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
relax handling of TLS 1.3 plaintext alerts pre-encrypted data exchange #1392
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1392 +/- ##
==========================================
+ Coverage 96.32% 96.34% +0.01%
==========================================
Files 64 64
Lines 14734 14810 +76
==========================================
+ Hits 14193 14269 +76
Misses 541 541
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
03319bd
to
9e3ecf2
Compare
I've flipped this out of draft status. I think it's ready for review. The updated branch replaces the clumsy unit testing approach w/ a better integration testing approach. It also drops the usage of |
This change moves the members of the `RecordLayer` type to match preferred convention: * Ordered by visibility, `pub(crate)` parts first, private parts last. * Ordered by complexity, more involved functions first, trivial functions later.
This commit updates the `RecordLayer` to maintain state about whether a decryption has ever occurred. This is maintained separate from the `read_seq` state that can be overwritten when a rekey occurs. The net effect is that once a `RecordLayer` has decrypted a message successfully it will remember this fact across any number of rekeys. This information is useful for the deframer layer where we want to make a determination about how to handle a received plaintext alert based on whether any decryptions have occurred prior to the alert.
The described behaviour isn't happening at the location of the comment, it happens later on after some additional processing. At the comment site we're only considering `ChangeCipherSpec` for early return, not returning an err for non-handshake messages while joining. This commit updates the comment to better reflect the current reality.
Some TLS 1.3 implementations send plaintext alerts (e.g. for an unknown certificate issuer) early in the handshake. Trying to decrypt these messages will produce a decrypt error (because they're plaintext!). We also don't want to allow plaintext alerts to be received after encrypted records have been exchanged, since this could allow an active adversary to inject alerts. As a compromise to support clients that send a plaintext alert before any encrypted data, we adjust the deframer in this commit to pass through plaintext alerts iff: * The message type is alert, (e.g. not application data, etc) * There have been no encrypted records received yet. * The message payload is no more than 2 bytes in size (matching an expected plaintext alert payload). * The negotiated protocol version is TLS 1.3 - in TLS 1.2 the CCS messages make whether to expect plaintext or not unambiguous. It's only for TLS 1.3 that we need the heuristics mentioned above. This retains protection against plaintext alerts being sent after encrypted content while still allowing the server to log the correct alert in the early-handshake condition, instead of a decrypt error.
9e3ecf2
to
4ae022c
Compare
Description
This branch addresses #1369 while still maintaining protection against unexpected plaintext alerts.
record_layer: reorder members.
This change moves the members of the
RecordLayer
type to match preferred convention:pub(crate)
parts first, private parts last.record_layer: track whether decryption has occurred.
This commit updates the
RecordLayer
to maintain state about whether a decryption has ever occurred. This is maintained separate from theread_seq
state that can be overwritten when a rekey occurs.The net effect is that once a
RecordLayer
has decrypted a message successfully it will remember this fact across any number of rekeys. This information is useful for the deframer layer where we want to make a determination about how to handle a received plaintext alert based on whether any decryptions have occurred prior to the alert.deframer: remove stale comment.
The described behaviour in the removed part of the comment isn't happening at the location of the comment, but later in the code. We're only considering
ChangeCipherSpec
for early return, not returning an err for non-handshake messages while joining.deframer: allow plaintext alerts in early 1.3 HS.
Some TLS 1.3 implementations send plaintext alerts (e.g. for an unknown certificate issuer) early in the handshake.
Trying to decrypt these messages will produce a decrypt error (because they're plaintext!). We also don't want to allow plaintext alerts to be received after encrypted records have been exchanged, since this could allow an active adversary to inject alerts.
As a compromise to support clients that send a plaintext alert before any encrypted data, we adjust the deframer in this commit to pass through plaintext alerts iff:
This retains protection against plaintext alerts being sent after encrypted content while still allowing the server to log the correct
alert in the early-handshake condition, instead of a decrypt error.
Manual testing
In addition to the unit test coverage, you can reproduce the situation from #1369 with this branch manually using
tlsserver-mio
and anopenssl s_client
client.openssl s_client
:openssl \ s_client \ -connect localhost:8080 \ --verify_return_error \ -tls1_3 </dev/null
tlsserver-mio
from the code inmain
. You should observe different server output, notably:Resolves #1369