Skip to content

fix(gateway): don't route packets from expired NAT sessions#8124

Merged
thomaseizinger merged 7 commits into
mainfrom
chore/no-route-untranslated-packets
Feb 14, 2025
Merged

fix(gateway): don't route packets from expired NAT sessions#8124
thomaseizinger merged 7 commits into
mainfrom
chore/no-route-untranslated-packets

Conversation

@thomaseizinger
Copy link
Copy Markdown
Member

@thomaseizinger thomaseizinger commented Feb 14, 2025

When we receive an inbound packet from the TUN device on the Gateway, we make a lookup in the NAT table to see if it needs to be translated back to a DNS proxy IP.

At present, non-existence of such a NAT entry results in the packet being sent entirely unmodified because that is what needs to happen for CIDR resources. Whilst that is important, the same code path is currently being executed for DNS resources whose NAT session expired! Those packets should be dropped instead which is what we do with this PR.

To differentiate between not having a NAT session at all or whether a previous one existed but is expired now, we keep around all previous "outside" tuples of NAT sessions around. Those are only very small in their memory-footprint. The entire NAT table is scoped to a connection to the given peer and will thus eventually freed once the peer disconnects. This allows us to reliably and cheaply detect, whether a packet is using an expired NAT session. This check must be cheap because all traffic of CIDR resources and the Internet resource needs to perform this check such that we know that they don't have to be translated.

This might be the source of some of the "Source not allowed" errors we have been seeing in client logs.

@vercel
Copy link
Copy Markdown

vercel Bot commented Feb 14, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
firezone ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 14, 2025 7:19am

Copy link
Copy Markdown
Member

@jamilbk jamilbk left a comment

Choose a reason for hiding this comment

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

LGTM.

Another edge case is when the client is restarted, the server it was talking to will continue to send data back to it.

Since our tunnel IPs persist across sessions, is it possible that when the client re-authenticates, these packets are able to "route" back all the way to the client.

If that's true, I'm wondering if this is a security issue:

  1. Client has access to DNS Resource A
  2. Client signs out
  3. Admin removes Client accesss to DNS Resource A
  4. Client signs back in
  5. Packets from DNS Resource A make their way back to client

You think the above is possible?

@thomaseizinger thomaseizinger force-pushed the chore/no-route-untranslated-packets branch from a078809 to 8ccc91e Compare February 14, 2025 04:52
@thomaseizinger
Copy link
Copy Markdown
Member Author

Admin removes Client accesss to DNS Resource A

This will immediately deauthorize the resource on the Gateway.

Client signs back in

At this point, the client will have to send another connection intent which will get refused by the policy engine.

@thomaseizinger
Copy link
Copy Markdown
Member Author

@jamilbk Had to change the design a bit because it wasn't actually working (thank god I decided to write a unit-test !)

@jamilbk
Copy link
Copy Markdown
Member

jamilbk commented Feb 14, 2025

@jamilbk Had to change the design a bit because it wasn't actually working (thank god I decided to write a unit-test !)

One day I hope to be able to catch Rust bugs by eye-balling them 🥹

@thomaseizinger
Copy link
Copy Markdown
Member Author

To differentiate between not having a NAT session at all or whether a previous one existed but is expired now, we keep around all previous "outside" tuples of NAT sessions around. Those are only very small in their memory-footprint. The entire NAT table is scoped to a connection to the given peer and will thus eventually freed once the peer disconnects. This allows us to reliably and cheaply detect, whether a packet is using an expired NAT session. This check must be cheap because all traffic of CIDR resources and the Internet resource needs to perform this check such that we know that they don't have to be translated.

@jamilbk I added some more notes on the design here.

@thomaseizinger thomaseizinger changed the title fix(gateway): don't route packets from expired NAT session fix(gateway): don't route packets from expired NAT sessions Feb 14, 2025
@thomaseizinger thomaseizinger marked this pull request as ready for review February 14, 2025 07:21
@thomaseizinger thomaseizinger added this pull request to the merge queue Feb 14, 2025
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Feb 14, 2025
@thomaseizinger thomaseizinger added this pull request to the merge queue Feb 14, 2025
Merged via the queue into main with commit 9cce4fd Feb 14, 2025
@thomaseizinger thomaseizinger deleted the chore/no-route-untranslated-packets branch February 14, 2025 08:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants