Http2Decompressor::DoLiteralInternal can loop infinitely

RESOLVED FIXED in Firefox 44

Status

()

Core
Networking: HTTP
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: keeler, Assigned: nwgh)

Tracking

({csectype-dos})

Trunk
mozilla45
csectype-dos
Points:
---

Firefox Tracking Flags

(firefox44+ fixed, firefox45 fixed)

Details

(Whiteboard: [spdy], URL)

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
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
(Reporter)

Updated

3 years ago
Blocks: 1197847
(Reporter)

Comment 1

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

Looks like this happened in 44.
status-firefox44: --- → affected
tracking-firefox44: --- → ?
Keywords: csectype-dos
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.
Assignee: nobody → hurley
Whiteboard: [spdy]
(Assignee)

Comment 3

3 years ago
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.
(Assignee)

Comment 4

3 years ago
Created attachment 8686828 [details] [diff] [review]
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.
Attachment #8686828 - Flags: review?(mcmanus)
Attachment #8686828 - Flags: review?(mcmanus) → review+
don't forget the aurora lift. thanks!
(Assignee)

Comment 7

3 years ago
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
Attachment #8686828 - Flags: approval-mozilla-aurora?

Comment 8

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/167f9422e399
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox45: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45

Comment 9

3 years ago
Tracked because of csectype DOS.
tracking-firefox44: ? → +
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+
Attachment #8686828 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 11

3 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/e68fb30045ae
status-firefox44: affected → fixed
You need to log in before you can comment on or make changes to this bug.