1709952 - Potential OBR parsing malicious ODoH configs

Status: RESOLVED FIXED

(Core :: Networking: DNS, defect, P2)

Description

[Daniel Veditz [:dveditz]]

The parsing of ODoH config files was switched from a raw buffer to a Span<T> in bug 1698015. This is good because Span<> has built-in Rust-like bounds checking, but the parser uses DNSParser's get16bit() which does not. There are a couple of places where the buffer length is checked to make sure there's more data, but a buffer that gives its size correctly as having enough length for the length data, but not enough for a full structure will end up reading garbage bytes during the while (length > 0) loop.

Likely saving us from easy exploitation is that length is unsigned, so when you keep decrementing it will underflow. Undefined so an optimizer might do something crazy, but usually wraps around into a huge number and the loop will keep going until it gets an access violation, or eventually the garbage its reading has a length interpreted as bigger than what we think is left.

without thinking too hard I think changing the while condition to be bigger than the size of the fixed part of the config structure would do it. There's a version in here, though. Will we try to support different versions, with potentially different sizes, in the future? that could complicate it a bit. In that case maybe more checking of length along the way: we have to know length is big enough before decrementing it at each step, or in sections where we decrement in a row of known sizes.

    while (length > 12) {

I Don't care for magic values like 12 so if the structures are actually defined somewhere (I assume so?) you could do some kind of sizeof(). But then your size could include compiler-added padding. Not sure that would happen with such simple structures, but maybe safer to just use a commented #define

If length != 0 at the end of the loop then there's bogus extra data. Not sure if that's worth logging. You log an invalid config, but not when you take the early return false bailouts so I leave that up to you.

I saw this because I happened to notice bug 1698015 (summary mentioned downloading configs) so I looked at that patch. I have not had time to look more widely at the code in this file but it's probably worth checking for similar patterns.

Comment 1

[Daniel Veditz [:dveditz]]

We don't check for overflow on index, either, but since it's the same uint16 as length that should be fine as long as we keep length from wrapping.

Comment 2

[Daniel Veditz [:dveditz]]

If you're going to use Span you should take advantage of it's Rust-like safety and let it do the indexing. If instead of get16bit(aData.Elements(), index) you did get16bit(aData[index], 0) it would be safer. That could still read one byte too far so get16bit(aData.FromTo(index, index+2), 0) would be better. If you want it prettier you could create a version of get16bit() that takes a Span explicitly and hides the repeating arguments.

If you do that the MOZ_RELEASE_ASSERT() bounds checks will trigger. You still have to fix the length checking in the loop also, but at least it would be memory-safe if people muck with this in the future and mess that part up.

Comment 3

[Dragana Damjanovic [:dragana]]

Kershaw, can you take a look?

Comment 4

[Kershaw Chang [:kershaw]]

Attachment: Bug 1709952 - Improve parsing ODoHConfig, r=#necko

Comment 5

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Improve parsing ODoHConfig, r=necko-reviewers,dragana
https://hg.mozilla.org/integration/autoland/rev/d93550b352f4c235509b856de95cdad5b6eee2c5
https://hg.mozilla.org/mozilla-central/rev/d93550b352f4