1224328 - Http2Decompressor::DoLiteralInternal can loop infinitely

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Http2Decompressor::DoLiteralInternal(...)
{
...
  int32_t newline = 0;
  while ((newline = value.FindChar('\n', newline)) != -1) {
    if (value[newline + 1] == ' ' || value[newline + 1] == '\t') {
      LOG(("Http2Decompressor::Disallowing folded header value %s",
           value.BeginReading()));
      return NS_ERROR_ILLEGAL_VALUE;
    }
  }

FindChar starts searching at the index specified, so if a newline is in the string, the first loop will find it and then subsequent loops will keep finding that exact same newline, leading to an infinite loop. The fun part is that this prevents any other networking.

By the way, there's also the potential out-of-bounds reads on the next line that should be fixed.

This is happening in the wild at https://9point6.com

Comment 1

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

[Tracking Requested - why for this release]:
This DOSes the browser.

Looks like this happened in 44.

Comment 2

[Patrick McManus [:mcmanus]]

yeah - nick and I were talking about this on irc, but it looks like you beat us to filing the bug.

there is some other weirdness going on in that header too (null, tab, etc..) so we should evaluate the whole thing as well.

Comment 3

[u408661]

The infinite loop should be an easy fix - no problem there. I don't believe the possible out-of-bounds read is actually a possible out-of-bounds read, given it's an nsACstring, so there should always at least be a \0 after any \n we find.

I'll take a look at the header in question and see what else I can find wrong with it/our code's handling of it (it sounds like a doozy), but a patch to fix the loop will be incoming as soon as I validate my fix.

Comment 4

[u408661]

Attachment: Don't infinite loop when parsing headers with newlines

Here we go, works fine. Now to look at the other bits in that cookie and see if there's anything else nasty going on.

Comment 5

[Patrick McManus [:mcmanus]]

don't forget the aurora lift. thanks!

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/167f9422e399

Comment 7

[u408661]

Comment on attachment 8686828 [details] [diff] [review]
Don't infinite loop when parsing headers with newlines

Approval Request Comment
[Feature/regressing bug #]: bug 1197847
[User impact if declined]: possible DoS when visiting a site served via http/2
[Describe test coverage new/current, TreeHerder]: local testing to verify fix
[Risks and why]: low/none
[String/UUID change made/needed]: none

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/167f9422e399

Comment 9

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracked because of csectype DOS.

Comment 10

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8686828 [details] [diff] [review]
Don't infinite loop when parsing headers with newlines

This is a bad one, let's fix soon! Aurora44+

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e68fb30045ae