1343505 - (CVE-2017-5446) Firefox crashed when encounter immature HTTP/2 server send DATA frame padding length large than payload length

Status: VERIFIED FIXED

(Core :: Networking: HTTP, defect)

Description

[chhsiao90]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/56.0.2924.87 Safari/537.36

Steps to reproduce:

We had a test tool that focus on HTTP/2 client/browser testing.
You could reference https://github.com/summerwind/h2spec/pull/74#issuecomment-283273210

Long story short, when the server send a DATA frame defined padding length=6, but only had 5 bytes in payload(including padding length byte). Firefox would crash.

Steps to reproduce:
1. mkdir src
2. export GOPATH=path to the folder created at step.1
3. mkdir -p src/github.com/summerwind
4. cd src/github.com/summerwind
5. git clone https://github.com/chhsiao90/h2spec.git
6. git checkout h2specd
7. go run cmd/h2spec/h2specd.go -t -p 8080 -c server.crt -k server.key -v
8. open firefox to http://localhost:8080
9. find the third link under 6.1. DATA marked with desc: Sends a DATA frame with invalid pad length


Actual results:

Firefox crashed


Expected results:

Marked the http2 connection as connection error, send GOAWAY frame and close the connection

Comment 1

[:Gijs (he/him)]

Can you provide a crashreport ID for the crash you're seeing? Does this happen on nightly as well? (https://nightly.mozilla.org)

Comment 2

[Rueimin Jiang]

The following is the gdb backtrace from firefox-developer addition.
My system environment is linux mint arm64.

```
Thread 6 "Socket Thread" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fffe19ab700 (LWP 31742)]
0x00007fffe8007490 in mozilla::net::nsPreloadedStream::nsPreloadedStream(nsIAsyncInputStream*, char const*, unsigned int) () from /home/mike820324/Downloads/firefox-nightly/libxul.so
(gdb) bt
#0  0x00007fffe8007490 in mozilla::net::nsPreloadedStream::nsPreloadedStream(nsIAsyncInputStream*, char const*, unsigned int) () from /home/mike820324/Downloads/firefox-nightly/libxul.so
#1  0x00007fffe81950f1 in mozilla::net::nsHttpConnection::PushBack(char const*, unsigned int) () from /home/mike820324/Downloads/firefox-nightly/libxul.so
#2  0x00007fffe81a1aa2 in mozilla::net::nsHttpTransaction::ProcessData(char*, unsigned int, unsigned int*) [clone .cold.74] () from /home/mike820324/Downloads/firefox-nightly/libxul.so
#3  0x00007fffe9e937c1 in mozilla::net::nsHttpTransaction::WritePipeSegment(nsIOutputStream*, void*, char*, unsigned int, unsigned int, unsigned int*) () from /home/mike820324/Downloads/firefox-nightly/libxul.so
#4  0x00007fffe9e39144 in nsPipeOutputStream::WriteSegments(nsresult (*)(nsIOutputStream*, void*, char*, unsigned int, unsigned int, unsigned int*), void*, unsigned int, unsigned int*) ()
   from /home/mike820324/Downloads/firefox-nightly/libxul.so
#5  0x00007fffe81a17a3 in mozilla::net::nsHttpTransaction::WriteSegments(mozilla::net::nsAHttpSegmentWriter*, unsigned int, unsigned int*) [clone .cold.72] () from /home/mike820324/Downloads/firefox-nightly/libxul.so
#6  0x00007fffe8161265 in mozilla::net::Http2Stream::WriteSegments(mozilla::net::nsAHttpSegmentWriter*, unsigned int, unsigned int*) () from /home/mike820324/Downloads/firefox-nightly/libxul.so
#7  0x00007fffe816da83 in mozilla::net::Http2Session::WriteSegmentsAgain(mozilla::net::nsAHttpSegmentWriter*, unsigned int, unsigned int*, bool*) () from /home/mike820324/Downloads/firefox-nightly/libxul.so
#8  0x00007fffe8182fec in mozilla::net::nsHttpConnection::OnSocketReadable() [clone .cold.417] () from /home/mike820324/Downloads/firefox-nightly/libxul.so
#9  0x00007fffe9e9232d in mozilla::net::nsHttpConnection::OnInputStreamReady(nsIAsyncInputStream*) () from /home/mike820324/Downloads/firefox-nightly/libxul.so
#10 0x00007fffe9e7c198 in mozilla::net::nsSocketInputStream::OnSocketReady(nsresult) () from /home/mike820324/Downloads/firefox-nightly/libxul.so
#11 0x00007fffe9e7bd4c in mozilla::net::nsSocketTransport::OnSocketReady(PRFileDesc*, short) () from /home/mike820324/Downloads/firefox-nightly/libxul.so
#12 0x00007fffe9e7132e in mozilla::net::nsSocketTransportService::DoPollIteration(mozilla::BaseTimeDuration<mozilla::TimeDurationValueCalculator>*) () from /home/mike820324/Downloads/firefox-nightly/libxul.so
#13 0x00007fffe9e70b92 in mozilla::net::nsSocketTransportService::Run() () from /home/mike820324/Downloads/firefox-nightly/libxul.so
#14 0x00007fffe9e42da3 in nsThread::ProcessNextEvent(bool, bool*) () from /home/mike820324/Downloads/firefox-nightly/libxul.so
#15 0x00007fffe9e56eb9 in NS_ProcessNextEvent(nsIThread*, bool) () from /home/mike820324/Downloads/firefox-nightly/libxul.so
#16 0x00007fffe9e9bc5f in mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) () from /home/mike820324/Downloads/firefox-nightly/libxul.so
#17 0x00007fffea72c6c2 in MessageLoop::Run() () from /home/mike820324/Downloads/firefox-nightly/libxul.so
#18 0x00007fffea657cfb in nsThread::ThreadFunc(void*) () from /home/mike820324/Downloads/firefox-nightly/libxul.so
#19 0x00007ffff64d3008 in _pt_root.cold.6 () from /home/mike820324/Downloads/firefox-nightly/libnspr4.so
#20 0x00007ffff7bc16ba in start_thread (arg=0x7fffe19ab700) at pthread_create.c:333
#21 0x00007ffff6c4a82d in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:109
```

Comment 3

[Rueimin Jiang]

I have also tested on nightly and can confirm the problem occur in firefox nightly as well.
(In reply to :Gijs from comment #1)
> Can you provide a crashreport ID for the crash you're seeing? Does this
> happen on nightly as well? (https://nightly.mozilla.org)

Comment 4

[Rueimin Jiang]

(In reply to :Gijs from comment #1)
> Can you provide a crashreport ID for the crash you're seeing? Does this
> happen on nightly as well? (https://nightly.mozilla.org)

Sorry for the late reply, 
The following is my crash report id

Crash ID: bp-a4458a02-8155-42c0-9433-80da22170301

And I have also tested that the firefox 51 in windows also being affected.

Comment 5

[chhsiao90]

Here is one of my crash report id on my MacOS: bp-02e0bae1-d562-423f-9cb9-429fb2170301

Comment 6

[Dragana Damjanovic [:dragana]]

Nick, Pat, how whats this one?

Comment 7

[u408661]

Hrm, could've sworn there was handling for this case. A quick look at the code indicates there isn't, though. It should be a pretty easy fix. I'll take it on.

Comment 8

[Patrick McManus [:mcmanus]]

thanks nick.. testcase too please.

afaict the exposure here is an out of bound read (which can cause segv) - but if it doesn't cause a segv the data being read is just going to be ignored (perceived as padding) so there isn't an information leak.. so this probably isn't sec-high. agree?

Comment 9

[u408661]

(In reply to Patrick McManus [:mcmanus] from comment #8)
> thanks nick.. testcase too please.

Yep yep.

> afaict the exposure here is an out of bound read (which can cause segv) -
> but if it doesn't cause a segv the data being read is just going to be
> ignored (perceived as padding) so there isn't an information leak.. so this
> probably isn't sec-high. agree?

I don't know... in this case (looking at the code, not the repro) this seems to be excess padding on the last DATA frame. The transaction knows it's done, so it's trying to push stuff back into the connection buffer, and that's when the crash happens - so we're trying to write random data that came from the network into a memory buffer we allocated on the heap. Sounds like a recipe for potential disaster, to me.

Comment 10

[u408661]

Ah, though the SEGV is likely from reading too far into the buffer, yes. (Sorry, I'm still thinking through this as I type). Though with properly-formed padding, I could see some kind of strange injection into one stream from another. That would require either (1) a server trying to mess with itself, or (2) a malicious h2 proxy trying to do potentially nasty things to a CONNECT tunnel (though since that's encrypted, probably about the best they could accomplish without some friendly bug in the TLS layer is to shut down that tunnel - which they can already do). (I discount, of course, an h2 proxy trying to do nasty things to a non-CONNECT stream through itself, since it can already mess with those at will to begin with.)

So in short, yeah, probably not sec-high.

Comment 11

[Patrick McManus [:mcmanus]]

hard to exploit - but lets treat it as sec-high. too many unknowns.

Comment 12

[u408661]

Alright, as expected, the fix is very straightforward. The tricky bit comes now in adding the test :)

Comment 13

[u408661]

BTW, reporter, the test case should accept a FRAME_SIZE_ERROR as well as (or perhaps instead of) a PROTOCOL_ERROR. My initial fix attempt used FRAME_SIZE_ERROR (which is more appropriate for this case) and failed the h2spec test case. I will most likely be submitting my patch for firefox with FRAME_SIZE_ERROR even though it fails h2spec, so it would be good to get h2spec updated.

Comment 14

[chhsiao90]

(In reply to Nicholas Hurley [:nwgh][:hurley] (also [e-mail]) from comment #13)
> BTW, reporter, the test case should accept a FRAME_SIZE_ERROR as well as (or
> perhaps instead of) a PROTOCOL_ERROR. My initial fix attempt used
> FRAME_SIZE_ERROR (which is more appropriate for this case) and failed the
> h2spec test case. I will most likely be submitting my patch for firefox with
> FRAME_SIZE_ERROR even though it fails h2spec, so it would be good to get
> h2spec updated.

Thanks for the advise, I will get it updated!

Comment 15

[u408661]

Attachment: patch

Comment 16

[u408661]

Attachment: test patch

Comment 17

[u408661]

This would've gone a lot faster if I'd not used "validate" in the server and "verify" in the client, but here we are. Should be good to go.

Comment 18

[chhsiao90]

BTW, IMHO it still be better to marked as PROTOCOL_ERROR because rfc had said that.

In https://tools.ietf.org/html/rfc7540#section-6.1
> The total number of padding octets is determined by the value of the
> Pad Length field.  If the length of the padding is the length of the
> frame payload or greater, the recipient MUST treat this as a
> connection error (Section 5.4.1) of type PROTOCOL_ERROR.

But I found that I was doing a wrong assertion, that for this case it should be connection error, not stream error.
I will update for that.

Comment 19

[u408661]

Ah, indeed it does. I was looking at the wrong part of the spec when deciding to use FRAME_SIZE_ERROR :) I'll update my patch (mcmanus - this doesn't change the mechanics of the patch, just a few bytes of one line, so if you're mid-review, no need to wait for me to update).

Comment 20

[u408661]

Attachment: patch w/updated error code

Comment 21

[u408661]

Comment on attachment 8842995 [details] [diff] [review]
patch w/updated error code

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Patch exposes the exact error pretty straightforwardly (so easy to cause a crash at least), but exploitability beyond that still appears to be difficult at best.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? Not beyond the simplicity of the code.

Which older supported branches are affected by this flaw? All

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? No, but they should be very easy to create and very low-risk.

How likely is this patch to cause regressions; how much testing does it need? Very unlikely, hand testing that I've already done should be fine.

Comment 22

[Al Billings [:abillings - ex-MoCo]]

Sec-approval+ for checkin on March 21, two weeks into the new cycle. We're about to ship Firefox 52 in a couple of days.

At that time, we'll want branch patches made and nominated as well.

Comment 23

[u408661]

Attachment: patch for aurora

See original sec-approval request for reasoning.

Comment 24

[u408661]

Attachment: patch for beta

See original request for rationale

Comment 25

[u408661]

Attachment: patch for release (in case we want it later)

Comment 26

[u408661]

Attachment: patch for esr52

Comment 27

[u408661]

Attachment: patch for esr45

Comment 28

[u408661]

Comment on attachment 8849226 [details] [diff] [review]
patch for aurora

Approval Request Comment
[Feature/Bug causing the regression]: http/2
[User impact if declined]: potential security risk
[Is this code covered by automated tests?]: on trunk, yes
[Has the fix been verified in Nightly?]: no - waiting for approved landing date
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: simple, targeted fix
[String changes made/needed]: none

Comment 29

[u408661]

Comment on attachment 8849228 [details] [diff] [review]
patch for beta

Approval Request Comment
[Feature/Bug causing the regression]: http/2
[User impact if declined]: potential security risk
[Is this code covered by automated tests?]: on mozilla-central
[Has the fix been verified in Nightly?]: no (waiting on approved landing date), but has manual testing
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: small, targeted fix
[String changes made/needed]: none

Comment 30

[u408661]

Comment on attachment 8849232 [details] [diff] [review]
patch for esr52

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: 
Fix Landed on Version: will land on nightly 55
Risk to taking this patch (and alternatives if risky): low risk - simple targeted fix
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 31

[u408661]

Comment on attachment 8849233 [details] [diff] [review]
patch for esr45

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: 
Fix Landed on Version: will land on nightly 55
Risk to taking this patch (and alternatives if risky): low risk, simple targeted fix
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 32

[Al Billings [:abillings - ex-MoCo]]

This can land now.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

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

Comment 34

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/dfa17169cf1f

Comment 35

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/462fa21070f5

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/bd23131fffca

Comment 37

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/d98de46f8f272cea18456b01ce7411b8da5cd336

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr45/rev/290f10412f9291f49e74fe7ff32af3259c944e9a

Comment 39

[Matt Wobensmith [:mwobensmith][:matt:]]

Hi there, I was hoping to confirm that this bug was fixed, but ran into issues using your original steps.

Can you verify that the latest build of Firefox 53 beta fixes the crash? Thank you.

http://ftp.mozilla.org/pub/firefox/candidates/53.0b9-candidates/build1/

Comment 40

[chhsiao90]

After confirmed, it had been fixed at 53.0b9, thanks for your hard working!