1886929 - Subtraction underflow in neqo when receiving malformed (too short) network packet

Status: RESOLVED FIXED

(Core :: Networking, defect, P2)

Description

[Lars Eggert [:lars]]

Experimenting with cargo fuzz in https://github.com/mozilla/neqo/pull/1764 immediately revealed a possible subtraction underflow in this line

            let token = Self::opt(decoder.decode(decoder.remaining() - expansion))?;

when decoder.remaining(), i.e., the leftover bytes in the incoming packet, is less than expansion. One input that triggers this crash is [179, 255, 0, 0, 32, 0, 0], but there are many others.

A misbehaving server can use this as a packet-of-death to crash the neqo stack (and possibly Fx?) by sending a malformed packet in response to a QUIC Client Initial (for example).

The fix is trivial, i.e., an addition of

            if decoder.remaining() < expansion {
                return Err(Error::InvalidPacket);
            }

just before that line.

Comment 1

[Martin Thomson [:mt:]]

A checked_sub().map_err(|_| Error::Whatever)? might be better

Comment 2

[Lars Eggert [:lars]]

@mt ACK.

Do I just open a PR for the fix over on GitHub, or is there a process for security fixes? (Sorry, newbie.)

Comment 3

[Martin Thomson [:mt:]]

Given the nature of this attack, we can just fix it in the clear. But I might prefer if we hold that fix until close to an uplift so that we don't expose ourselves for too long. It's just DoS, but no point in leaving the window open for a long time. Code freeze is April 11 so let's aim for the week ahead of that.

This will need a test.

Comment 4

[Lars Eggert [:lars]]

The PR is here: https://github.com/mozilla/neqo-ghsa-hvhj-4r52-8568/pull/1 (private repo forked off this draft disclosure). We should land this for the next release.

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

has this landed?

Comment 6

[Kershaw Chang [:kershaw]]

(In reply to Randell Jesup [:jesup] (needinfo me) from comment #5)

has this landed?

Yes, it's landed in bug 1890988 and the autoland looks like still green.

Comment 7

[Kershaw Chang [:kershaw]]

This was fixed bug 1890988, so I think we can close this one.

Comment 8

[Valentin Gosu [:valentin]]

Thank you Lars and Kershaw!

Comment 9

[Daniel Veditz [:dveditz]]

(In reply to Martin Thomson [:mt:] from comment #3)

This will need a test.

Lars: Is there a test for this in bug 1890988 (or elsewhere)? Or should we reopen this bug as a placeholder for that?

Comment 10

[Lars Eggert [:lars]]

There is a test in https://github.com/mozilla/neqo/pull/1816 that got merged after landing the fix.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Given that this is rated sec-low, I'm inclined to wontfix this for ESR115. Is there an argument to be made for doing the backport beyond just the severity rating or is that an acceptable choice from your perspective too?

Comment 12

[Lars Eggert [:lars]]

I think that should be OK. We have no bug reports that this bug was triggered in deployment, it was found by the fuzzer.