CVPN-2559: Handle expresslane flags and session id tampering#425
Open
kp-mariappan-ramasamy wants to merge 13 commits into
Open
CVPN-2559: Handle expresslane flags and session id tampering#425kp-mariappan-ramasamy wants to merge 13 commits into
kp-mariappan-ramasamy wants to merge 13 commits into
Conversation
Looks like, cargo lock update is missed in previous PR
|
Code coverage summary for d45bf53: ✅ Region coverage 69% passes |
kp-mariappan-ramasamy
force-pushed
the
expresslane-flags
branch
from
70ae078 to
ab36d05
Compare
Make EXPRESSLANE_KEYS_ROTATION_INTERVAL to a per-connection setting and exposed in client and server configs.
kp-mariappan-ramasamy
force-pushed
the
expresslane-flags
branch
from
ab36d05 to
c254a6f
Compare
kp-mariappan-ramasamy
changed the title
kp-mariappan-ramasamy
changed the title
Forged UDP packet with the same session id but a different
protocol version byte could tear down an established session. Drop
the packet instead.
There are two distinct error paths:
* ConnectionError::InvalidProtocolVersion
Header parsed successfully but its version does not match the
session's tunnel_protocol_version. Was fatal in the Datagram arm.
* WireError(FromWireError::InvalidProtocolVersion(..))
Header parse itself rejects the version bytes as not a known
protocol version at all. Previously caught by the WireError(_)
catch-all, which is fatal. An attacker who garbles the version
bytes (e.g. 99.99) instead of just mismatching them would still
have killed the session via this path.
Header bytes are plaintext on the wire by design (the server needs
session id to route to the right Connection before DTLS decryption),
so any destructive action keyed off header content alone is an
unauthenticated-input DoS. Both paths now drop the packet and keep
the session alive.
The test currently fails to demonstrate the issue. Will be fixed in next commit
The flags field is transmitted in clear on the wire. Without including it in the AEAD associated data, an on-path attacker could flip the encoded bit without invalidating the auth tag, steering the receiver down the wrong packet-codec path. Extend auth_vec from 16 to 18 bytes to cover session_id || wire_counter || flags.
kp-mariappan-ramasamy
force-pushed
the
expresslane-flags
branch
from
c254a6f to
31f8eba
Compare
Comment on lines
234
to
273
| @@ -249,7 +245,6 @@ impl ConnectionError { | |||
| match self { | |||
| TimedOut => true, | |||
| Unauthorized => true, | |||
| InvalidProtocolVersion => true, | |||
| InvalidMode => true, | |||
| InvalidConnectionType => true, | |||
| NoAvailableClientIp => true, | |||
| @@ -259,7 +254,6 @@ impl ConnectionError { | |||
| PacketCodecDoesNotExist => true, | |||
| PacketCodecError(_) => true, | |||
| Disconnected => true, | |||
| EncodingReqRetransmitCbDoesNotExist => true, | |||
| PathMtuDiscoveryRequired { .. } => true, | |||
| Tls(crate::tls::Error::Fatal(ErrorKind::DomainNameMismatch)) => true, | |||
| Tls(crate::tls::Error::Fatal(ErrorKind::DuplicateMessage)) => true, | |||
| @@ -270,8 +264,10 @@ impl ConnectionError { | |||
| WireError(wire::FromWireError::InsufficientData) => false, | |||
| WireError(wire::FromWireError::InvalidExpressData) => false, | |||
| WireError(wire::FromWireError::ReplayedExpressData) => false, | |||
| WireError(wire::FromWireError::InvalidProtocolVersion(..)) => false, | |||
| WireError(_) => true, | |||
|
|
|||
| InvalidProtocolVersion => false, | |||
| InvalidState => false, // Can be due to out of order or repeated messages | |||
| InvalidInsideIo => false, // Can be used for test only test control plane | |||
| UnknownSessionID => false, | |||
Contributor
There was a problem hiding this comment.
I'm a bit concerned about this one - does this mean older Lightway client can now connect to a newer server without any issue in the future if we bumped Lightway version to a newer one?
Contributor
Author
There was a problem hiding this comment.
Yes, it is a general rule to make server backward compatible with client. Even now, without expresslane older clients 1.2 will still be able to connect to 1.3 server.
This change should not impact that, and is targeted to ignore tampered protocol version.
I did not understand your exact concern though, can you explain a bit more
Replace the strict equality check 'config.version != tunnel_protocol_version'
with a forward-compatible negotiation:
* peer advertised our local max or below → take min(local_max, peer).
* peer advertised an Unknown byte (byte 0, or any byte we do not
recognise — e.g. a future version we have not seen yet) → fall
back to our local max.
Net effect: V_n peer talking to V_m build (n != m) negotiates to
min(n, m) in both directions. Lets us ship new protocol versions
later without breaking expresslane interop with the binaries
already in the field.
A peer that genuinely sends byte 0 (never advertised) is treated
the same as a future version: we reply with our local max. The
peer won't be able to reciprocate, expresslane simply won't
establish — same outcome as a reject.
Bump the expresslane wire version to Version2. V2 binds the flags field into the AEAD AAD so an on-path attacker cannot flip flags without invalidating the auth tag. V1 keeps the original layout (no flags in AAD) for interoperability with legacy V1 peers that predate the AAD change. Outgoing config now advertises whatever version was negotiated with the peer instead of always advertising LOCAL_MAX. A V2 server talking to a V1 client thus downgrades its own outgoing config to V1, and the V1 wire path stays compatible with the legacy peer. The initial outgoing config (before any peer reply) still uses LOCAL_MAX. Net effect: V2 endpoints talking to each other get full flag protection. V2 endpoints talking to legacy V1 peers continue to interoperate (no protection on flags for those sessions, same posture as before the AAD change). No silent AEAD failures between old and new endpoints; mismatches surface at config exchange time.
kp-mariappan-ramasamy
force-pushed
the
expresslane-flags
branch
2 times, most recently
from
6082d8c to
c6eb712
Compare
Found when running clippy enabling all features.
kp-mariappan-ramasamy
force-pushed
the
expresslane-flags
branch
from
c6eb712 to
2adb982
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Changes:
Motivation and Context
Since expresslane flags are not included in AAD, a onpath attacker can modfiy expresslane flags without being detected and close the connection. We got notified by a researcher about this bug.
Also the protocol version in UDP lightway packets can be tampered by on-path attacker and make server close the client connection. Ignore protocol version error too since it is a plaintext header.
How Has This Been Tested?
Added unit test to verify this tampering
Types of changes
Checklist:
main