Repository navigation
device: do not update peer endpoint on cookie replies - #497
Open
miguelcorderopamphile wants to merge 1 commit into
Open
miguelcorderopamphile wants to merge 1 commit into
miguelcorderopamphile wants to merge 1 commit into
Conversation
Cookie replies are not authenticated peer traffic: the cookie is encrypted with a key derived from public values and its AAD is the mac1 of an observed handshake, so an on-path attacker can forge or replay them from any source. The device loop currently updates the peer endpoint (roaming) and creates a connected socket based on the source address of any packet that did not error, including cookie replies. A single forged cookie reply therefore redirects all of the peer's subsequent traffic to an arbitrary address (persistent tunnel blackhole). Only update the endpoint and create the connected socket for authenticated traffic. Parity with wireguard-go and the Linux kernel; Mullvad's gotatun removed this behavior for the same reason. Verified with a live PoC against the daemon: before the change a forged cookie reply sent from a different address replaced endpoint=127.0.0.1:51999 with the attacker address and redirected the client handshake traffic; after the change the endpoint is unchanged and traffic keeps flowing to the configured peer. Signed-off-by: Miguel Cordero Pamphile <miguelcorderopamphile@gmail.com>
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
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.
Fixes #496.
What
The device UDP handler updated the peer endpoint (roaming) and created a connected socket based on the source address of any packet that did not return an error, including cookie replies. Cookie replies are not authenticated peer traffic (the cookie is encrypted with a key derived from public values and its AAD is the mac1 of an observed handshake), so an on-path attacker could forge or replay one from an arbitrary address and redirect all of the peer traffic (handshakes, keepalives, data) to that address.
This PR only updates the endpoint / creates the connected socket for authenticated traffic.
Why
Parity with wireguard-go and the Linux kernel; Mullvad gotatun removed the same behavior for the same reason ("Do not update peer endpoint/roam on received cookie replies. This change is made to be consistent with Linux kernel and wireguard-go").
Verification
cargo test -p boringtun --lib: 10 passed.endpoint=127.0.0.1:51999with the attacker address (endpoint=127.0.0.2:55505) and the next handshake init went to the attacker.127.0.0.1:51999and traffic keeps flowing to the configured peer.Note: the behavior is exercised through the device event loop, so the live PoC (raw UDP packets) was used instead of a unit test; happy to add an integration test under
device/integration_testsif you prefer.