Closed Bug 1352926 Opened 2 years ago Closed 2 years ago

AddressSanitizer: heap-buffer-overflow on address in nsContentSink::ProcessLinkHeader(nsAString const&)

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox-esr45 53+ fixed
firefox52 --- wontfix
firefox-esr52 53+ fixed
firefox53 + fixed
firefox54 + fixed
firefox55 + fixed

People

(Reporter: bc, Assigned: bzbarsky)

References

(Blocks 1 open bug, )

Details

(Keywords: csectype-bounds, sec-high, Whiteboard: [post-critsmash-triage][adv-main53+][adv-esr45.9+][adv-esr52.1+])

Attachments

(2 files)

Attached file asan report
Linux Nightly

Load http://www.jednoslad.pl/tag/numer-vin

==12116==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x611000787080 at pc 0x7f5da0d779b7 bp 0x7ffd3ee47010 sp 0x7ffd3ee47008
READ of size 2 at 0x611000787080 thread T0
    #0 0x7f5da0d779b6 in nsContentSink::ProcessLinkHeader(nsAString const&) /home/worker/workspace/build/src/dom/base/nsContentSink.cpp:495:12
    #1 0x7f5da0d80164 in DoProcessLinkHeader /home/worker/workspace/build/src/dom/base/nsContentSink.cpp:363:3
Group: core-security → dom-core-security
I just tried loading that page, and so far far I haven't even seen nsContentSink::ProcessLinkHeader called.  :(

If this is at all reproducible, printing out the exact sequence of UTF-16 codepoints in the argument passed to ProcessLinkHeader that triggers that failure would be really helpful.
OK, bc showed me a way to reproduce.  The actual Link header being sent by the relevant page looks like this:

  Link: <http://www.jednoslad.pl/wp-json/>; rel="https://api.w.org/"   :

but a minimal testcase looks like this:

  Link:"" x

What happens here?  

1) We ensure that our string contains an extra null, so that the length of the buffer in this case is 6 (two quote chars, space, x, two nulls).  We point "start" to index 0.

2) We enter the outermost "while (*start != kNullCh)" loop.

3) There is no leading space.  We point "end" to index 0.

4) We enter the "semicolon or comma" loop, discover that "end" points to a quote, look for the end quote.

5) When we find the close quote, we set "end" to point to it (index 1 in this case).  Then we set "ch" to *(end + 1), which in this case is the space.

6) Since ch is not null or semicolon or comma, we increment "end" via "*(++end) = kNullCh;" and set ch to *(end+1) again, which is 'x'.  "end" now points to index 2.

7) We enter the "keep going until semi or comma" loop.  We increment "end" to point to index 3, set "ch" to *end, which is _still_ 'x'.  So we loop again and increment "end" to point to index 4.  At this point "ch" becomes null, so we break out of the loop, and leave the "quoted string" if block, ending up at the end of the main body of the "look for semicolon or comma" loop.  This does ++end, so "end" now points at index 5.

8) We come back to the loop header of the "look for semicolon or comma" loop.  *end is null, so we break out of that loop.  We set "endCh" to *end, which is the null char.  "start < end" tests true, so we do that whole big block.  Then we do "start = ++end" and go back to the overall loop over "start".  At this point "start" and "end" both point to index _6_, which is off the end of our string.

9) Now we dereference "start" to compare it to kNullCh, which is already kinda bogus.  ASAN might be OK with it if we overallocated the string buffer, of course...  But then we start walking "end" along until we find a null or comma or semicolon and walk off the end of our allocation.

This is all rather scary, because this code writes null chars into the string it's walking.  So if it can be made to walk off the end of it, it can start writing null chars into random memory it does not own!
Is this a regression of some sort?  Would like to get it on the radar for 54 if so.  (53 would be better, but that's going out the door soon...)
Flags: needinfo?(bzbarsky)
The fundamental bug is in step 7, where we set "ch" to *end.  We've already looked at that character.  So we end up double-incrementing "end".

It looks to me like this bug was introduced in the refactoring done by bug 168371.  Before that, this bit (in content/html/document/src/nsHTMLContentSink.cpp at the time):

             ch = *(end + 1);
 
             // keep going until semi or comma
             while (ch != kNullCh && ch != kSemiCh && ch != kCommaCh) {
               ++end;
 
               ch = *end;
             }

used to look like this:


             while ((kNullCh != *(end + 1)) && (kSemiCh != *(end + 1)) &&
                    (kCommaCh != *(end + 1))) { // keep going until semi or comma
               end++;
             }

and the latter is equivalent to the loop body of the former doing "ch = *(end + 1)".  Then bug 218837 merge the HTML and XML content sink code into nsContentSink.cpp, but kept the prettier-yet-buggy HTML version.

So yes, a regression, but 14+ years old...
Flags: needinfo?(bzbarsky)
See comment 2 and comment 4 for some context that might be helpful in review
Attachment #8854569 - Flags: review?(afarre)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
[Tracking Requested - why for this release]: Security bugs are bad.  I'm assuming we're not going to track for 52 and 53.
Comment on attachment 8854569 [details] [diff] [review]
Make sure to check the right character for being semicolor or comma

[Security approval request comment]
How easily could an exploit be constructed based on the patch?  In terms of overrunning the buffer, I would expect someone who sees "changing a function called ProcessLinkHeader" to just start fuzzing Link headers and I would be extremely shocked if that did not quickly create a buffer overrun.  Using that to construct an actual exploit, I'm not sure.  Seems like it should be pretty doable.  :(

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?  No, and in particular I avoided saying that what matters is the "kNullCh" comparison, no the semicolon/comma ones.  But I don't expect that to help much against anyone who can write a fuzzer, per above.

Which older supported branches are affected by this flaw?  All of them.

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?  I expect this patch will apply as-is to all the branches we might care about.

How likely is this patch to cause regressions; how much testing does it need?  Hard to say.  I _think_ this doesn't introduce any new security bugs or functionality regressions, but confidence is low.
Attachment #8854569 - Flags: sec-approval?
Really, we should rewrite all this in some less error-prone style, but I wanted an easily backportable spot-fix...
Also, we should clearly do some fuzzing of this code ourselves....  Tyson, do you know who might have some cycles for that?
Flags: needinfo?(twsmith)
Raymond: Doesn't Peach have a "pit" for the http protocol? Are we using it?
Flags: needinfo?(rforbes)
Comment on attachment 8854569 [details] [diff] [review]
Make sure to check the right character for being semicolor or comma

Review of attachment 8854569 [details] [diff] [review]:
-----------------------------------------------------------------

Nit: Spelling, should be s/color/colon/ in commit message.

Tried to step through ProcessLinkHeader with a couple of different, similar looking inputs to see if I could come up with something that would break, but couldn't. Totally agree that we should fuzz this.
Attachment #8854569 - Flags: review?(afarre) → review+
yes. I haven't run it in a while, I can do it again.
Flags: needinfo?(rforbes)
> Nit: Spelling, should be s/color/colon/ in commit message.

Good catch.  Fixed.
Comment on attachment 8854569 [details] [diff] [review]
Make sure to check the right character for being semicolor or comma

sec-approval=dveditz

This seems like the kind of thing that would catch someone's attention, and it's a small fix. We should uplift this everywhere. a=dveditz for all the branches.
Attachment #8854569 - Flags: sec-approval?
Attachment #8854569 - Flags: sec-approval+
Attachment #8854569 - Flags: approval-mozilla-esr52+
Attachment #8854569 - Flags: approval-mozilla-beta+
Attachment #8854569 - Flags: approval-mozilla-aurora+
Attachment #8854569 - Flags: approval-mozilla-esr45+
The push in comment 15 didn't address the review comment.  :(  Now I guess that will propagate over to all the branches too....
> Fixed on the uplifts anyway

Thank you!
https://hg.mozilla.org/mozilla-central/rev/383e9b300834
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Group: dom-core-security → core-security-release
Flags: qe-verify?
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main53+][adv-esr45.9+][adv-esr52.1+]
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #9)
> Also, we should clearly do some fuzzing of this code ourselves....  Tyson,
> do you know who might have some cycles for that?

Sorry missed clearing this. rforbes is looking at this at the moment.
Flags: needinfo?(twsmith)
Group: core-security-release
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.