RustSec advisory for trust-dns-proto affecting Server, Resolver and Client

announcements
1

The trust-dns-proto library contains the underpinning DNS protocol implementations, including the serialization and deserialization primitives for names and labels, where a name is www.example.com and a label is www. DNS being a space conscious protocol has always had a name/label compression technique for sticking references back to previously written names/labels in a packet to reduce the DNS packet size.

The deserialization code has a bug related to this in which the reference to another name/label in the packet would be trusted in such a way that a malicious packet could cause a stackoverflow.

All versions of trust-dns-resolver, trust-dns and trust-dns-server are affected. This has been fixed in the current release of trust-dns-proto 0.4.3, which aligns with the trust-dns-resolver 0.9 release, as well as trust-dns (client) and trust-dns-server 0.14 series.

The corresponding fix in the alpha series is trust-dns-proto 0.5.0-alpha.3, which is used in trust-dns-resolver 0.10.0-alpha, as well as trust-dns (client) and trust-dns-server 0.15.0-alpha series.

A huge thanks goes to @oherrala who found and reported this issue, as well as reviewed and even patched the initial fix. I’m very grateful to his excellent work and detailed report on the issue.

Link to the rustsec advisory: Trust-DNS: Stack overflow when parsing DNS packet by oherrala · Pull Request #62 · rustsec/advisory-db · GitHub

EDIT: To make this clear, this is a DoS potential mainly effecting the trust-dns-server, and much more difficult to target the resolver and client (requiring a MITM, man-in-the-middle targeted attack). The result is a crash, with no known memory visibility issues.

As an addendum to this issue: I'm now aware that we currently don't have a decent process for reporting security advisories in the trust-dns project. I'm going to be researching a process for both reporting issues, and coordinating responses to issues with an early alert system to notify potentially impacted projects. This was a good learning exercise for the project. If anyone has a decent process they're aware of and can point me to it, I would appreciate it.

14 Likes
2

Excellent handling! :heart:

Since you ask for comparison material on policies:

NLnetLabs (research arm of [s]the .nl registrar/s] Europe's first public internet provider) authors the "NSD" DNS server and the "Unbound" Resolver, which have their (almost identical) policies documented here:

Edit: getting my facts straight about SODN/NLnet, thanks @jasper

2 Likes
3

Thank you! This is very helpful.

4

You're welcome!
And thank YOU for doing this. DNS is one of those unglamorous, oft-forgotten, dusty corners of the internet stack, but it needs love!

You are using Rust in exactly the use-case I envisioned when I first learned about Rust: writing more-than-rock-solid foundations of the computing world.
For that, you will always have my respect! :bowing_man: :heart:

2 Likes
5

:blush:

6

Note: this was assigned RUSTSEC-2018-0007

3 Likes
7

As a response to this issue, we've introduced a new Restrict type, https://github.com/bluejekyll/trust-dns/pull/586, to make it far less likely encounter overflowing operations during the parsing of the Record Data sections of the trust-dns-proto library.

https://crates.io/crates/trust-dns-proto/0.5.0-alpha.5

After making this change, potential overflow issues in CAA, TLSA, NSEC, and NSEC3 record types were discovered (and fixed). This can't be used to actively DOS a server, unless that is set to allow dynamic update. Similarly risk to the Resolver from those issues should be very low.

I don't currently plan to backport these changes to the 0.9 releases, as they are fairly extensive. The 0.10 release is ready to go, there should be no concerns with breaking public APIs before 0.10 Resolver is released.

3 Likes
8

Great stuff!

I've had a quick look at it, and I detect parallels to Perl's "taint mode", which plays a similar role in "tainting" untrusted user input, and many "dangerous" functions will error out when they detect they are called with unverified stuff.

Taint mode in action (from stackoverflow):

$ echo '`rm -rf /`' | perl -Te 'eval while <>'
Insecure dependency in eval while running with -T switch at -e line 1, <> line 1.

I really like how the unverified() call draws attention to the fact you're working with untrusted input :slight_smile:

impl RestrictedMath for Restrict<u8> { ... }
(source)

I see there is no Trait for checked_add, though there is one for normal Add;
What do you think? Is this a missing trait that std should export?

1 Like
9

I've never looked at Perl's "taint mode", but yes, this is very similar to tainting, as @Manishearth informed me the other day. I have thought about it in the past, and this change felt necessary to help make sure we don't continue having similar issues. It started as an experiment to see how it would work, and I think turned into something reasonable. There are still some things to think about I think in future proofing it (i.e. there are a lot of unverified uses). It's significantly better than before, and I think I'll be threading it through more interfaces over time.

I was thinking that, yes. It seems like it would be useful. I worked around that with the RestrictedMath trait you identified. I'd like to go back and revisit this when I have time with this, or another similar library: https://crates.io/crates/checked

10

I'm glad I'm not the only one seeing the similarity. I do think that with Rust's stronger type system, there is a lot more potential in the approach than Perl could ever enforce :+1:

I was thinking, if your Restricted type shows its worth, if it would be worthwhile to extract it into a separate API crate, so that others can use it too. (Similar to e.g. the http crate).
It's a sky castle at the moment, but I can see a future where parser crates hand of an interoperable Restricted<T>. :sunny::rainbow::cloud:

P.s. name bikeshedding: why "restricted" (which sounds descriptive of the usage), and not something like "unverified" or "untrusted" (which would be descriptive of the content)?

1 Like
11

Dependent types have been explored for this purpose. See TS*'s un type:

1 Like
12

Yes. I thought the same. It’s very focused on what the trust-dns libraries need at the moment, and I want to see if over time we can get rid of some of the unverified uses. Maybe at that point?

As to the name Restrict is the name of the type, and RestrictedMath for the trait and math functions. Is this the best name? I’m sure not. You have some good other options. I didn’t use Untrusted because I didn’t want confusion with the untrusted library, which I plan to make more extensive use of. Unverified is definitely an option Verified is currently the type name of the return from verify. We could change it to that, but I wanted a name that really got the idea across that you should be wary of the data contained in the type.

So in that sense Restrict Is more about the fact that the data is actively being restricted in its usage. That is, it’s safe usage is limited.

2 Likes
13

Minor detail and somewhat tangential, but NLnet Labs is not the research arm of the .nl registrar. That would be SIDN Labs (https://www.sidnlabs.nl/?language_id=2). NLnet Labs originates in one of the first commercial Internet Providers in the world.

14

You are absolutely correct! I've corrected it in the original post too.