Skip to content

DNS #465

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

Merged
merged 16 commits into from
May 19, 2022
Merged

DNS #465

merged 16 commits into from
May 19, 2022

Conversation

Dirbaio
Copy link
Member

@Dirbaio Dirbaio commented Apr 13, 2021

Requires #573

DNS in smoltcp! Overall design matches the rest of smoltcp, there should be no surprises :)

The DNS state machine is implemented as a DnsSocket, instead of using UdpSocket, because:

  • It can send and receive UDP packets on the fly, avoiding the problems we ran into with DHCP (need big buffers, if they're full packets get dropped...).
  • The UDP source port has to be randomized per query, so running N queries in parallel would require N UdpSockets.

wire is complete, socket works with a few missing things:

  • Remove hardcoded IP addresses in socket.
  • Support multiple servers, retry queries with other servers when one doesn't work.
  • Add timeouts
  • Allow specifying DNS names in human-friendly string format: "rust-lang.org" -> b"\x09rust-lang\x03org\x00";
  • Add protection against CNAME loops
  • Handle NXDOMAIN and other DNS server errors
  • Handle when query completes succesfully but there are no A/AAAA records.
  • Add support for AAAA queries. -- will do later
  • Perhaps add a way to query A+AAAA at the same time? -- will do later
  • Proper random DONE pending Basic rand infrastructure. #547
  • Fix ipv6-less build

Sorry, something went wrong.

@ryan-summers
Copy link
Contributor

@Dirbaio Given that DNS is purely above UDP, what are some of your thoughts on moving DNS into an external crate layered above the embedded-nal? This would allow DNS to be used for devices that aren't necessarily using smoltcp as well.

@Dirbaio
Copy link
Member Author

Dirbaio commented May 10, 2021

@ryan-summers How would you address the problems outlined in this PR's description?

  • Source port must be randomized for each query to have acceptable entropy. This'd require recreating the UdpSocket every time, and require N UdpSockets to do N parallel queries.
  • It'd require buffers to hold the DNS request and response packets, wasting RAM. This is the same problem that the previous DHCP impl had.

These apply whether the stack is "smoltcp UdpSocket -> Dns" or "smoltcp UdpSocket -> embedded-nal UdpSocket -> Dns".

@ryan-summers
Copy link
Contributor

ryan-summers commented May 10, 2021

@ryan-summers How would you address the problems outlined in this PR's description?

  • Source port must be randomized for each query to have acceptable entropy. This'd require recreating the UdpSocket every time, and require N UdpSockets to do N parallel queries.

This isn't required by RFC 1035 as far as I can tell - where's the need to randomize the source port each time coming from?

  • It'd require buffers to hold the DNS request and response packets, wasting RAM. This is the same problem that the previous DHCP impl had.

Fundamentally, smoltcp already buffers these packets (in the socket buffer) for deserialization, so depending on how it is written, we could implement external DNS with zero (additional) buffering. There's already a lot of interest/discussion around using smoltcp sockets in a zero-copy API (#359, #422), so I don't see why we couldn't go that approach to eliminate the need for buffering packets.

@Dirbaio
Copy link
Member Author

Dirbaio commented May 10, 2021

This isn't required by RFC 1035 as far as I can tell - where's the need to randomize the source port each time coming from?

If source port is not random, an attacker can spoof a response by guessing just the transaction ID. The TXID is 16 bits, which is considered too easy to guess. See here, here, here. These talk mostly about caching servers, which are more exposed since their IP is publicly known and a successful poisoning's impact is amplified due to the caching server serving many clients, but it still applies to end-user DNS clients too. I'd consider it a bad practice to ship a DNS client without mitigations for that in 2021.

Fundamentally, smoltcp already buffers these packets (in the socket buffer) for deserialization.

The only buffer usage in DnsSocket will be the phy's buffers, the socket itself won't need any additional buffers.

If DNS worked on top of UdpSocket it'd need buffers for 1 rx packet and 1 tx packet, so 2*MTU = 3kb (or maybe less if you can get away with smaller DNS packets).

There's already a lot of interest/discussion around using smoltcp sockets in a zero-copy API (#359, #422), so I don't see why we couldn't go that approach to eliminate the need for buffering packets.

Yep, agreed! If there was a way for the user to add a socket that gets callbacks for process and emit (something like dyn CustomSocket maybe?) then it'd be possible to impl DNS on top of that without extra buffering. The process and emit callbacks would get buffers from the PHY directly, like current in-tree sockets do.

However we don't have that today, so the only way to get zero-buffer DNS is with a custom smoltcp DnsSocket.


With the current code, everything from wire can be used directly to build a DNS client (such as one working on top of embedded-nal). The missing bits is the "state machine" logic to execute a DNS query (retry on timeouts, query again if we got a CNAME response with no A record, etc).

Maybe this state machine could be a separate public struct so that it can be used directly to build a custom DNS client on top of some UdpSocket such as embedded-nal? Wiring it up to the socket, handling udp source ports and multiple queries would be the responsibility of the user in this case. Smoltcp would still have the UdpSocket that "just works" with zero buffering, using the state machine struct.

@ryan-summers
Copy link
Contributor

Maybe this state machine could be a separate public struct so that it can be used directly to build a custom DNS client on top of some UdpSocket such as embedded-nal? Wiring it up to the socket, handling udp source ports and multiple queries would be the responsibility of the user in this case. Smoltcp would still have the UdpSocket that "just works" with zero buffering, using the state machine struct.

Yeah, I was giving this some thought to. I think this would be a great way to go, as it would support the "high performance" case, but also provide the tools needed to go without smoltcp (as much as I like it, sometimes hardware constraints me). Ultimately I think these de/serialize and/or state machine logic could live outside of smoltcp in a separate crate, but that's not necessarily a requirement on my end either.

@Dirbaio
Copy link
Member Author

Dirbaio commented May 10, 2021

👍

I'll give that approach a go then! If it turns out good we can do the same with DHCP, I remember a similar conversation about using DHCP on top of non-smoltcp RawSockets.

@Dirbaio
Copy link
Member Author

Dirbaio commented May 10, 2021

Ultimately I think these de/serialize and/or state machine logic could live outside of smoltcp in a separate crate

That'd make sense, but that'd apply to the wire module too then. We'd have 3-4 or more separate crates, and we'd have to forward Cargo features between them, as if we didn't have enough Cargo feature mess already :)

@Dirbaio Dirbaio force-pushed the dns branch 4 times, most recently from 89544b4 to 5ce1f1e Compare October 7, 2021 22:47
@Dirbaio
Copy link
Member Author

Dirbaio commented Oct 7, 2021

Rebased, it's now fully working! TODO in PR description.

/// The time-to-live (IPv4) or hop limit (IPv6) value used in outgoing packets.
hop_limit: Option<u8>,

// TEMPORARY rand implementation. TODO
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the eventual idea to expose Rand as a public trait and add a type parameter to DnsSocket, allowing users to bring their own implementation (or fall back on this default implementation)? Or is there already an appropriate abstraction in the embedded Rust ecosystem?

Copy link
Contributor

@datdenkikniet datdenkikniet Oct 10, 2021

Choose a reason for hiding this comment

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

I would highly recommend that the rand_core or rand (which is mostly an extension to rand_core) crate/traits are used here! They're quite widely used.

RngCore is implemented for the hardware RNG in stm32f4xx-hal, stm32f7xx-hal and stm32l4xx-hal crates (and others?).

Copy link
Member Author

@Dirbaio Dirbaio Oct 10, 2021

Choose a reason for hiding this comment

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

I'm not sure what to do about this. A related item is TCP initial sequence numbers should be randomized too, they're currently constant. So whatever we do about RNG for DNS, we want TCP to be able to use it too.

  • Option 1: Own an instance of T: RngCore. This would mean adding a generic param to at least Interface, InterfaceBuilder, TcpSocket, DnsSocket, SocketSet. I really don't like dirtying the API with more generics than necessary...
  • Option 2: use dyn. Annoying because you need some kind of alloc container to own a dyn RngCore. Borrowing a &mut dyn RngCore could work but forcing borrowing for std targets is annoying too. Also the vtable overhead is not nice.
  • Option 3: Be opinionated and bundle a particular PRNG. Have the user set the seed. It doesn't really have to be cryptographically secure, so a weak small PRNG would do. It's rude to rule out using a CSPRNG though.
  • Option 4: Have cargo features so the user can choose between a simple PRNG or a big fat CSPRNG. If the user is not careful this could cause bloat because they'll end up with two CSPRNGS in the binary, though.
  • Option 5: Default to a weak PRNG, add a Cargo feature to delegate to a extern "Rust" fn _smoltcp_rand() -> u32 so the user can hook up their own CSPRNG/HWRNG if wanted.

Thoughts?

Copy link

Choose a reason for hiding this comment

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

OEMs rarely change anything that's not possible to avoid changing, so how about an Option 6. Default to as secure as possible, but document quite clearly how to trim away security and bytes.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that we should be "secure by default". Devs will follow the path of least resistance, and if we write it unsecure, then it's very likely most applications will be insecure out of ignorance (likely including my own). That being said, I don't see any reason we shouldn't allow an opt-out? Alternatively, we can force them to provide an (rng: impl RngCore) in any function when requesting a new DNS request, which would allow randomization of sockets (e.g. get random generator only when needing random numbers). This only addresses DNS though and not TCP.

Copy link
Member Author

@Dirbaio Dirbaio Oct 11, 2021

Choose a reason for hiding this comment

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

These RNG uses are not that critical for security though. They (DNS txid, DNS source port, TCP initial sequence numbers) aim to make blind spoofing attacks harder by adding a bit more entropy (the key here is blind: it's an attacker who can spoof packets, but can't sniff the legit packets between the smoltcp host and the peer. If it could, it no longer offers protection: the attacker can simply see the values for port/seq/txid and spoof the right response packet).

Also, they only offer 32bits of entropy at best which is nowhere near enough to be considered secure anyway. if you care about security you should be using legit authenticated encryption such as TLS. If you are, such attacks become DoS's at best, they can't compromise you.

Someone in your LAN can DoS you in lots of different ways already, such as ARP spoofing, or flooding you at 100mbps line rate. Someone outside your LAN will have a much harder time spoofing packets in the first place: they have to get past NAT/firewall.

Also, bundling our own CSPRNG by default still doesn't make it secure-by-default: the user has to seed it securely, which is necessarily hardware-dependent. I don't see an easy way to get past that...

Copy link
Member Author

Choose a reason for hiding this comment

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

Something we can do is simply not bundle a weak PRNG by default, forcing the user to supply some impl. If they decide to supply a weak one, it's their problem.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've opened a separate PR for the rand infra: #547

What do you guys think? Feedback/reviews welcome!

bors bot added a commit that referenced this pull request Oct 14, 2021
547: Basic rand infrastructure. r=Dirbaio a=Dirbaio

See [previous discussion](#465 (review)). Opening a separate PR so it can be discussed separately.

- Add `smoltcp::rand`.
  - On `std` targets, `OsRng` is used by default.
  - The user can supply a custom impl by enabling the `rand-custom-impl` Cargo feature and using the `rand_custom_impl!()` macro.
  - Specifying a custom impl is mandatory when `std` is not enabled.
- Make TCP initial sequence numbers actually random.


Co-authored-by: Dario Nieuwenhuis <dirbaio@dirbaio.net>
@korken89
Copy link
Contributor

While testing this our I ran across one issue, if one does not enable IPv6 the PR fails to build.
Nothing major, but seems like feature gating needs some looking into. :)

@Dirbaio Dirbaio marked this pull request as ready for review May 19, 2022 19:45
@Dirbaio Dirbaio changed the title WIP: DNS DNS May 19, 2022
@Dirbaio Dirbaio merged commit 7728660 into master May 19, 2022
@Dirbaio Dirbaio deleted the dns branch May 19, 2022 23:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants