starred/hickory-dns

Fork 0

mirror of https://github.com/hickory-dns/hickory-dns.git synced 2026-04-25 03:05:51 +03:00

[GH-ISSUE #263] Questionable implementation of DNSClass -> u16 conversion #423

New issue

Closed

opened 2026-03-15 22:25:55 +03:00 by kerem · 4 comments

kerem commented

2026-03-15 22:25:55 +03:00

Owner

Originally created by @briansmith on GitHub (Oct 25, 2017).
Original GitHub issue: https://github.com/hickory-dns/hickory-dns/issues/263

DNSClass::OPTs are constructed using code like this, that reads the value from the wire:

DNSClass::for_opt(try!(decoder.read_u16()))

Thus, the value within a DNSClass::OPT can be any value in [0, 65535], and the peer chooses that value, so it is attacker-controlled.

Now consider the implementation of DNSClasss -> u16 conversion:

impl From<DNSClass> for u16 {
    fn from(rt: DNSClass) -> Self {
        match rt {
            DNSClass::IN => 1,
            DNSClass::CH => 3,
            DNSClass::HS => 4,
            DNSClass::NONE => 254,
            DNSClass::ANY => 255,
            DNSClass::OPT(version) => version,
        }
    }
}

This has a collision for values {1, 3, 4, 254, 255} of version.

Now consider the Ord implementation for DNSClass:

impl Ord for DNSClass {
    fn cmp(&self, other: &Self) -> Ordering {
        u16::from(*self).cmp(&u16::from(*other))
    }
}

This is using the colliding DNSClass -> u16 conversion.

In turn, the Ord implementation is used for the purposes of determining the canonical order of records in a record set. DNSSEC requires us to sort records in the canonical order. However, there's no way that we're guaranteeing that things are in the canonical order here, because of the collision. Because of #200, this broken sorting logic is used not just for signing, but also for verifying DNSSEC-signed record sets.

I don't see any attack that can make use of this + #200, but this is definitely buggy.

The solution is probably to rewrite the Ord implementation of DNSClass to not use the buggy DNSClass -> u16 conversion. Probably we should also remove the buggy conversion function itself.

Originally created by @briansmith on GitHub (Oct 25, 2017). Original GitHub issue: https://github.com/hickory-dns/hickory-dns/issues/263 `DNSClass::OPT`s are constructed using code like this, that reads the value from the wire: ```rust DNSClass::for_opt(try!(decoder.read_u16())) ``` Thus, the value within a `DNSClass::OPT` can be any value in [0, 65535], and the peer chooses that value, so it is attacker-controlled. Now consider the implementation of `DNSClasss` -> u16 conversion: ```rust impl From<DNSClass> for u16 { fn from(rt: DNSClass) -> Self { match rt { DNSClass::IN => 1, DNSClass::CH => 3, DNSClass::HS => 4, DNSClass::NONE => 254, DNSClass::ANY => 255, DNSClass::OPT(version) => version, } } } ``` This has a collision for values {1, 3, 4, 254, 255} of version. Now consider the `Ord` implementation for `DNSClass`: ```rust impl Ord for DNSClass { fn cmp(&self, other: &Self) -> Ordering { u16::from(*self).cmp(&u16::from(*other)) } } ``` This is using the colliding `DNSClass` -> `u16` conversion. In turn, the `Ord` implementation is used for the purposes of determining the canonical order of records in a record set. DNSSEC requires us to sort records in the canonical order. However, there's no way that we're guaranteeing that things are in the canonical order here, because of the collision. Because of #200, this broken sorting logic is used not just for signing, but also for verifying DNSSEC-signed record sets. I don't see any attack that can make use of this + #200, but this is definitely buggy. The solution is probably to rewrite the `Ord` implementation of `DNSClass` to not use the buggy `DNSClass` -> `u16` conversion. Probably we should also remove the buggy conversion function itself.

kerem

2026-03-15 22:25:55 +03:00

closed this issue
added the
bug

trust

crate:proto
labels

kerem commented

2026-03-15 22:26:25 +03:00

Author

Owner

@briansmith commented on GitHub (Oct 25, 2017):

Note that test test_order() doesn't consider ORD. If it did, we would probably have discovered this bug at the time that test was written.

@briansmith commented on GitHub (Oct 25, 2017): Note that test `test_order()` doesn't consider `ORD`. If it did, we would probably have discovered this bug at the time that test was written.

kerem commented

2026-03-15 22:26:30 +03:00

Author

Owner

@bluejekyll commented on GitHub (Oct 25, 2017):

This is a great catch!

@bluejekyll commented on GitHub (Oct 25, 2017): This is a great catch!

kerem commented

2026-03-15 22:26:35 +03:00

Author

Owner

@bluejekyll commented on GitHub (Nov 21, 2017):

Ok, I reviewed the code, and usage.

This field is annoying, it's overloaded to mean the max payload length from the rfc: https://tools.ietf.org/html/rfc6891#section-6.1.2

Basically, OPT is only used in the EDNS context. Given that context, it represents the maximum length which is supported by the server. Most set this to 4,096. TRust-DNS actually defaults (it should be configurable) to 1500, which is generally a good network packet size. Though, this should really be 28 less for IPv4 or 48 less for IPv6.

@bluejekyll commented on GitHub (Nov 21, 2017): Ok, I reviewed the code, and usage. This field is annoying, it's overloaded to mean the max payload length from the rfc: https://tools.ietf.org/html/rfc6891#section-6.1.2 Basically, `OPT` is only used in the EDNS context. Given that context, it represents the maximum length which is supported by the server. Most set this to 4,096. TRust-DNS actually defaults (it should be configurable) to 1500, which is generally a good network packet size. Though, this should really be 28 less for IPv4 or 48 less for IPv6.

kerem commented

2026-03-15 22:26:40 +03:00

Author

Owner

@bluejekyll commented on GitHub (Nov 26, 2017):

Reopening... The Opt max_data_length should never be less than 512 (i.e. original DNS max packet length). This will actually fix the concern of this issue.

@bluejekyll commented on GitHub (Nov 26, 2017): Reopening... The Opt max_data_length should never be less than 512 (i.e. original DNS max packet length). This will actually fix the concern of this issue.

kerem referenced this issue

2026-03-15 22:44:51 +03:00

[GH-ISSUE #423] It should be possible to use a single DNS configuration/options object for multiple lookups #483

kerem referenced this issue

2026-03-16 02:05:57 +03:00

[PR #424] [CLOSED] use arc for ResolverConfig #1430