Per RFC 9000, a Destination Connection ID in a Retry packet is of valid
length as long as it is 20 bytes or shorter. However, when validating
tokens for Retry packets, our server currently makes an assumption that
it will only deal with a Destination Connection ID that is exactly 20
bytes long. As a result, a misbehaving or malicious client can cause our
server to panic.
When our server validates a token for a Retry packet, the nonce is
partially constructed from the Destination Connection ID. Due to our
server's bad assumption, a shorter-than-expected Destination Connection
ID will result in cipher.AEAD.Open being given a nonce of bad length. We
currently use XChaCha20-Poly1305 from x/crypto for our AEAD
implementation, which will panic when given a nonce of bad length.
Fixesgolang/go#78292
Change-Id: Ieb65d8b84bcb5c531bdb46d5c75b13dad9c76eb2
Reviewed-on: https://go-review.googlesource.com/c/net/+/758360
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Nicholas Husin <husin@google.com>
Reviewed-by: Damien Neil <dneil@google.com>
Avoid a state where we can have a MAX_STREAMS frame to send,
but do not send the frame for an indefinite amount of time.
Conn.appendStreamFrames writes stream-related frames to
the current packet. It also handles removing streams
from the Conn when we no longer need to track their state.
Removing streams can affect the frames we want to send.
In particular, we may want to send a MAX_STREAMS to the
peer indicating that it can open more streams because
we've closed out some of the existing ones.
Add MAX_STREAMS after removing streams, to ensure we
pick up any changes to the sent value before adding it.
This case doesn't show up in tests, because the test harness's
idleness detection causes the Conn's event loop to run and notice
the pending MAX_STREAMS frame. Changing tests to use
testing/synctest (a followup CL) causes the problem to
appear, because the event loop isn't run while the Conn
is idle.
Change-Id: Ia7394891317dae6ecfd529a9b3501ac082cb453e
Reviewed-on: https://go-review.googlesource.com/c/net/+/714481
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Damien Neil <dneil@google.com>
Reviewed-by: Nicholas Husin <husin@google.com>
Reviewed-by: Nicholas Husin <nsh@golang.org>
testConn.handshake runs through the initial QUIC handshake
and verifies that the connection under test sends an expected
sequence of handshake messages.
The last datagram in the handshake is sent by the client,
and contains an ACK for the last datagram sent by the server.
The client sends this ACK after max_ack_delay (25ms) passes, minus
the timer granularity (1ms). The timer granularity is a constant
containing the expected maximum delay between a timer event's
scheduled time and the timer actually firing.
The expected handshake datagram used by testConn.handshake contains
an ACK with an ACK Delay value of 25ms (max_ack_delay).
This doesn't account for the timer granularity adjustment.
However, since testConn.handshake advances time by 25ms rather than 24ms,
the test connection sends the ACK at the later time and includes a
larger ACK Delay value.
Fix testConn.handshake to sleep for the expected delay (24ms).
Fix the expected handshake datagram accordingly.
This all avoids test failures after switching this package to use
testing/synctest's fake clock, under which the connection sends
this ACK at the scheduled time rather than a time under direct
control of the test.
Change-Id: I1af6e02e02f6493758e41db45a46d06a65441a7b
Reviewed-on: https://go-review.googlesource.com/c/net/+/714480
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Nicholas Husin <husin@google.com>
Auto-Submit: Damien Neil <dneil@google.com>
Reviewed-by: Nicholas Husin <nsh@golang.org>
An "optimistic ACK attack" involves an attacker sending ACKs
for packets it hasn't received, causing the victim's
congestion controller to improperly send at a higher rate.
The standard defense against this attack is to skip the occasional
packet number, and to close the connection with an error if the
peer ACKs an unsent packet.
Implement this defense, increasing the gap between skipped
packet numbers as a connection's lifetime grows and correspondingly
the amount of work required on the part of the attacker.
Change-Id: I01f44f13367821b86af6535ffb69d380e2b4d7b7
Reviewed-on: https://go-review.googlesource.com/c/net/+/664298
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
Auto-Submit: Damien Neil <dneil@google.com>
The sentPacket type tracks the state of a packet sent to the peer.
Packets can be in progress, acknowledged, or lost.
Track this state with an enum rather than a set of bools,
to avoid the possibility of nonsensical states such as a packet
being both acknowledged and lost.
This also simplifies a following change to add an "unsent" state
for intentionally skipped packet numbers.
Change-Id: I87c8fc399c72337c033ab7ec5ec8db2c56c732f9
Reviewed-on: https://go-review.googlesource.com/c/net/+/664297
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
Auto-Submit: Damien Neil <dneil@google.com>
When discarding keys for a packet number space, we remove any
in-flight packets in that space from our count of bytes in flight.
Skip any packets in the space that were already acked or declared lost.
Also skip over acked/lost packets when discarding state in response
to a Reset message. This change shouldn't have any effect, since we
never declare packets acked/lost until we receive an ACK, and we
never handle a Reset after receiving an ACK.
Change-Id: I8842f03aa06e57a834a211f1284a48fe5d469db3
Reviewed-on: https://go-review.googlesource.com/c/net/+/664335
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
Auto-Submit: Damien Neil <dneil@google.com>
Decoding QUIC packet numbers requires keeping track of the largest
packet number received so far from the peer. Our tests haven't
bothered doing that so far, so tests can't work with packet
numbers past 255.
Fix that so we can write tests that use more packets.
Change-Id: Icb795e5cf69794381c12a3a03b0da6bcf47a69c0
Reviewed-on: https://go-review.googlesource.com/c/net/+/664296
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
Auto-Submit: Damien Neil <dneil@google.com>
Add a NewEndpoint function to create an *Endpoint using a net.PacketConn.
We rely on a number of features of *net.UDPConn which aren't provided
by PacketConn, so an Endpoint using anything other than a UDPConn will
be limited. The main use case for providing a non-UDPConn connection
is testing, in particular tests which use testing/synctest and cannot
use a concrete network connection.
Change-Id: I9e62cb8d7d545f64d99103beb9a32f149d4119bf
Reviewed-on: https://go-review.googlesource.com/c/net/+/641498
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
Auto-Submit: Damien Neil <dneil@google.com>
Keep track of which peer-provided connection ID sequence numbers
we have sent a RETIRE_CONNECTION_ID for, received an ack, and
removed from our set of tracked IDs.
Rework how we track retired connection IDs in general:
Rather than keeping all state for retired IDs, just keep a
set of the sequence numbers of IDs which we're in the process
of retiring.
Change-Id: I14da8b5295d5fbe8318c8afe556cbd2c8a56d856
Reviewed-on: https://go-review.googlesource.com/c/net/+/635717
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
When dropping packet protection keys for a number space:
Check to see if there is unused CRYPTO data received from the peer
in the space. If so, close the connection with an error. This can
only happen if the peer has sent us data with a gap in it. We
can never read the data that fills that gap (because we're dropping
the key it would be encrypted with), and this situation cannot
happen without the peer sending invalid TLS handshake data.
Drop any buffered CRYPTO data being sent to the peer.
Under normal operations, we may have data that was sent to the peer
but which we haven't received an ACK for yet. The peer has
received the data (or we wouldn't be dropping the number space)
and we will never see the ACK (because we're dropping the key it
would be encrypted with).
Fixesgolang/go#70704
Change-Id: I53380169cb59a2a6f87e69b38522ba81ad38c2b0
Reviewed-on: https://go-review.googlesource.com/c/net/+/634617
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
Auto-Submit: Damien Neil <dneil@google.com>
The QUIC interop runner "keyrotate" test requires that the client
initiate a key rotation early in the connection. With our current
ack frequency, it seems that we need to rotate within the first
300-400 packets for the test to pass.
Reduce the initial key rotation from 1000 to 100 packets.
Rotating earlier shouldn't have any real downsides
(rotation is cheap and generally done once per connection,
except for very long-lived connections), and this is simpler
than providing a way to tune the rotation interval in one
specific test.
For golang/go#67138
Change-Id: I33d47ea35ed39f0a13c171adb2b0698f8c93050e
Reviewed-on: https://go-review.googlesource.com/c/net/+/582855
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
