1194818 - (CVE-2015-7218) Firefox HTTP2 Malformed Header Frame DoS

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[c0nrad]

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

Comment 1

[Daniel Veditz [:dveditz]]

Patrick: is there more (and worse) like this in this library?

Comment 2

[Patrick McManus [:mcmanus]]

 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?

Comment 3

[Patrick McManus [:mcmanus]]

Attachment: h2 header priority handling

Comment 4

[Patrick McManus [:mcmanus]]

patch passes the PoC included in bug and regression xpcshell h2 testing run locally.

Comment 5

[Patrick McManus [:mcmanus]]

bug 1194820 is essentially a twin to this one - one is for the request side and one is for the response side.

Comment 6

[u408661]

(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

Comment 7

[Patrick McManus [:mcmanus]]

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.

Comment 8

[Daniel Veditz [:dveditz]]

If you're satisfied that there's not a more severe security problem lurking in here then please just land any time.

Comment 9

[Patrick McManus [:mcmanus]]

(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.

Comment 10

[u408661]

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.

Comment 11

[Patrick McManus [:mcmanus]]

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.

Comment 12

[Patrick McManus [:mcmanus]]

Attachment: 0001-bug-1194818-h2-header-priority-handling-r-hurley.patch

Comment 13

[u408661]

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 :)

Comment 14

[Patrick McManus [:mcmanus]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/af935eae9bed

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Backed out for Windows build bustage.
https://treeherder.mozilla.org/logviewer.html#?job_id=13512265&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/d773e076ec4a

Comment 16

[Patrick McManus [:mcmanus]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c9f33fb9436

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9c9f33fb9436

Comment 18

[c0nrad]

Just checking up, will a MFSA be released for this?

Comment 19

[Christiane Ruetten [INACTIVE]]

Patrick, do you know if anyone checked whether this also affects previous versions of Firefox?

Comment 20

[Patrick McManus [:mcmanus]]

nick, can you figure out (and request) any necessary backports for this and 1194820?

Comment 21

[c0nrad]

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.

Comment 22

[u408661]

(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 23

[u408661]

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.

Comment 24

[Patrick McManus [:mcmanus]]

(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 25

[Sylvestre Ledru [:Sylvestre]]

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.