Bug 1194818 (CVE-2015-7218)

Firefox HTTP2 Malformed Header Frame DoS

RESOLVED FIXED in Firefox 43, Firefox OS master

Status

()

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

People

(Reporter: c0nrad, Assigned: mcmanus)

Tracking

({csectype-dos, sec-low})

39 Branch
mozilla43
csectype-dos, sec-low
Points:
---

Firefox Tracking Flags

(firefox42 wontfix, firefox43 fixed, firefox-esr38 wontfix, b2g-master fixed)

Details

(Whiteboard: [b2g-adv-main2.5?][post-critsmash-triage][adv-main43+])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

3 years ago
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/44.0.2403.155 Safari/537.36

Steps to reproduce:

Firefox HTTP2 Malformed Header Frame DoS

## Overview
A malformed HTTP2 header frame is sent to the browser. Normally a header frame consist of a pad length, stream dependency identifier, weight, header block fragement, and padding. But only a single byte is sent that eventually results in an integer underflow, which cases an nsCString to try to allocate nearly 2^32 bytes of memory. This is greater than firefox allows and an assert is tripped.

## Walkthrough
On line 1226 of file Http2Session.cpp, a call is made to append the decompressed frame onto a decompressed frame buffer:

self->mDecompressBuffer.Append(self->mInputFrameBuffer + kFrameHeaderBytes + paddingControlBytes + priorityLen,
	                           self->mInputFrameDataSize - paddingControlBytes - priorityLen - paddingLength);

At this point the variables are:

self->mDecompressBuffer.Append(nsAutoArrayPtr<char> (ptr) + uint_8(9) + uint8_t(0) + uint16_t(0),
	                           uint32_t(1) - uint8_t(0) - uint32_t(5) - uint16_t(0));

priorityLen is equal to five because the kFlag_PRIORITY is set on line 1186 of Http2Session.cpp.

The second argument is casted to a size_type (uint32), which results in an integer underflow of 4294967292.

A bunch of string checks are then performed to make sure the append is sane. Eventually in nsTSubstring.cpp:45 MutatePrep is called with:

MutatePrep(this=0x0000000121e52b80, aCapacity=4294967292, aOldData=0x000000010be8b8f0, aOldFlags=0x000000010be8b8ec).

In the function, kMaxCapacity for a string is determined to be:

const size_type kMaxCapacity = (size_type(-1) / 2 - sizeof(nsStringBuffer)) / sizeof(char_type) - 2;
// kMaxCapacity = 2147483637

kMaxCapacity is then compared to our capacity (4294967292). Since the capacity exceeds the max allowed, an error is thrown on line 531 of nsTSubstring.cpp.

AllocFailed(Length() - aCutLength + 1)

## Exploitability

To exploit we would need our integer underflow to result in a value less than 2147483637. Since we only partially control a uint8 and a uint16, this is not possible. Only an assert can be generated.

## Tested on 
Firefox 39.0. MacOS
42.0a1 (2015-07-21) MacOS

## POC
Code can be found here:
https://notr-bucket.s3.amazonaws.com/eabe6314-752b-4508-9ee1-bccf756c09e1

To use:
Extract the file and the certs
$ go run firefoxHeaderPoc.go
$ ./firefox https://localhost:8000

## Author
Stuart Larsen
Yahoo! Pentest Team
Patrick: is there more (and worse) like this in this library?
Component: Untriaged → Networking: HTTP
Flags: needinfo?(mcmanus)
Keywords: csectype-dos, sec-low
Product: Firefox → Core
(Assignee)

Comment 2

3 years ago
 I'm thrilled Stuart is combing through this! Thank you. Are you looking at anything else we should know about?

(In reply to Daniel Veditz [:dveditz] from comment #1)
> Patrick: is there more (and worse) like this in this library?

barring additional information, I think we're basically ok. There are 3 places where a frame has an optional member like this that creates funky offsets - the other one is DATA and that seems ok from reading it again just now as it is processed with a state machine rather than arithmetic. Most of the other frame types have fixed lengths which are checked prior to parsing at all. RST_STREAM has an optional member at the end, but we ignore that and enforce a fixed length check on the required members at the front.

The code is pretty careful about checking that copies fit in its allocations (which is why append() is used there and it fails reasonably at least.) and bounds checking accesses to simpler header members.

I can put forward simple patches for this and bug 1194820 - nick do you have enough node-http2 jujitsu to get testcases up?
Flags: needinfo?(mcmanus) → needinfo?(hurley)
(Assignee)

Comment 3

3 years ago
Created attachment 8649462 [details] [diff] [review]
h2 header priority handling
Attachment #8649462 - Flags: review?(hurley)
(Assignee)

Updated

3 years ago
Assignee: nobody → mcmanus
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Comment 4

3 years ago
patch passes the PoC included in bug and regression xpcshell h2 testing run locally.
(Assignee)

Comment 5

3 years ago
bug 1194820 is essentially a twin to this one - one is for the request side and one is for the response side.
(In reply to Patrick McManus [:mcmanus] from comment #2)
> I can put forward simple patches for this and bug 1194820 - nick do you have
> enough node-http2 jujitsu to get testcases up?

I'll see what I can pull together - it's been a while since I've delved deep in the internals of node-http2
Flags: needinfo?(hurley)
Attachment #8649462 - Flags: review?(hurley) → review+
(Assignee)

Comment 7

3 years ago
dan, let me know how you want this and 1194820 landed. I presume that one will be sec-low too and they can just land and ask for backports, but I'll wait for you to say.
Flags: needinfo?(dveditz)
If you're satisfied that there's not a more severe security problem lurking in here then please just land any time.
Flags: needinfo?(dveditz)
(Assignee)

Comment 9

3 years ago
(In reply to Daniel Veditz [:dveditz] from comment #8)
> If you're satisfied that there's not a more severe security problem lurking
> in here then please just land any time.

Nick - do you want to sign off here too? I read the padding code for the data frame again and it lgtm, but another read makes sense.
Flags: needinfo?(hurley)
I just did a once-over of this, and while I'm confident these patches fix all the issues, I did notice one last padding-related error in Http2Session::ParsePadding - if (paddingLength > mInputFrameDataSize) should be if (paddingLength + paddingControlBytes > mInputFrameDataSize). Again, I believe that error is taken care of/made irrelevant by these patches, but we may as well fix that while we're in here doing padding-related stuff.
Flags: needinfo?(hurley)
(Assignee)

Comment 11

3 years ago
updated based on comment 10. Also initialized outParams within the function in the case where there is no padding. They were already set outside the function so this doesn't change anything, but it makes it easier to use correctly.
(Assignee)

Comment 12

3 years ago
Created attachment 8654833 [details] [diff] [review]
0001-bug-1194818-h2-header-priority-handling-r-hurley.patch
Attachment #8649462 - Attachment is obsolete: true
Attachment #8654833 - Flags: review?(hurley)
Comment on attachment 8654833 [details] [diff] [review]
0001-bug-1194818-h2-header-priority-handling-r-hurley.patch

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

LGTM. I'm still working on tests for this and its PUSH_PROMISE cousin, FWIW. Just having some trouble debugging my issues since I'm still unable to get wireshark to decode any h2 session on any OS I try :)
Attachment #8654833 - Flags: review?(hurley) → review+
https://hg.mozilla.org/mozilla-central/rev/9c9f33fb9436
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-b2g-master: --- → fixed
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43

Updated

3 years ago
Group: core-security → core-security-release
(Reporter)

Comment 18

3 years ago
Just checking up, will a MFSA be released for this?
Patrick, do you know if anyone checked whether this also affects previous versions of Firefox?
Flags: needinfo?(mcmanus)
Whiteboard: [b2g-adv-main2.5?]
(Assignee)

Comment 20

3 years ago
nick, can you figure out (and request) any necessary backports for this and 1194820?
Flags: needinfo?(mcmanus)
(Reporter)

Comment 21

3 years ago
Hey Patrick, A co-worker and I are speaking on all the HTTP2 bugs we found at PacSec next week. (https://pacsec.jp/speakers.html). We were planning on speaking about these two bugs, just a heads up.
(In reply to Patrick McManus [:mcmanus] from comment #20)
> nick, can you figure out (and request) any necessary backports for this and
> 1194820?

These patches have already ridden the trains all the way to beta, so the only supported version that this and 1194820 affect is esr38. I'll request approval, but given the severity of these, I won't push hard.
Comment on attachment 8654833 [details] [diff] [review]
0001-bug-1194818-h2-header-priority-handling-r-hurley.patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: the best case for consideration is that this bug is going to be presented on at an upcoming security conference (https://pacsec.jp/speakers.html) 
User impact if declined: possible DoS
Fix Landed on Version: 43
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8654833 - Flags: approval-mozilla-esr38?
(Assignee)

Comment 24

3 years ago
(In reply to c0nrad from comment #21)
> Hey Patrick, A co-worker and I are speaking on all the HTTP2 bugs we found
> at PacSec next week. (https://pacsec.jp/speakers.html). We were planning on
> speaking about these two bugs, just a heads up.

thanks for your help with this!
Comment on attachment 8654833 [details] [diff] [review]
0001-bug-1194818-h2-header-priority-handling-r-hurley.patch

As it doesn't match the ESR criteria (sec-high or critical), not taking it.
Attachment #8654833 - Flags: approval-mozilla-esr38? → approval-mozilla-esr38-
Whiteboard: [b2g-adv-main2.5?] → [b2g-adv-main2.5?][post-critsmash-triage]
status-firefox42: --- → wontfix
status-firefox-esr38: --- → wontfix
Whiteboard: [b2g-adv-main2.5?][post-critsmash-triage] → [b2g-adv-main2.5?][post-critsmash-triage][adv-main43+]
Alias: CVE-2015-7218
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.