Bug 1343505 (CVE-2017-5446)

Firefox crashed when encounter immature HTTP/2 server send DATA frame padding length large than payload length

VERIFIED FIXED in Firefox -esr45

Status

()

VERIFIED FIXED
2 years ago
a year ago

People

(Reporter: chhsiao90, Assigned: u408661)

Tracking

({sec-high})

51 Branch
mozilla55
sec-high
Points:
---
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(firefox-esr4553+ fixed, firefox52 wontfix, firefox-esr5253+ fixed, firefox53+ fixed, firefox54+ fixed, firefox55+ fixed)

Details

(Whiteboard: [necko-active][spdy][adv-main53+][adv-esr52.1+][adv-esr45.9+])

Attachments

(7 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
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

2 years ago
Can you provide a crashreport ID for the crash you're seeing? Does this happen on nightly as well? (https://nightly.mozilla.org)
Group: firefox-core-security → core-security
Component: Untriaged → Networking: HTTP
Flags: needinfo?(chhsiao90)
Product: Firefox → Core

Comment 2

2 years ago
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

2 years ago
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

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

Comment 5

2 years ago
Here is one of my crash report id on my MacOS: bp-02e0bae1-d562-423f-9cb9-429fb2170301
(Reporter)

Updated

2 years ago
Flags: needinfo?(chhsiao90)
Nick, Pat, how whats this one?
Flags: needinfo?(mcmanus)
Flags: needinfo?(hurley)
(Assignee)

Comment 7

2 years ago
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.
Assignee: nobody → hurley
Flags: needinfo?(mcmanus)
Flags: needinfo?(hurley)
Whiteboard: [necko-active][spdy]
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?
(Assignee)

Comment 9

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

Comment 10

2 years ago
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.
hard to exploit - but lets treat it as sec-high. too many unknowns.
Keywords: sec-high
(Assignee)

Comment 12

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

Comment 13

2 years ago
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.
Flags: needinfo?(chhsiao90)
Flags: sec-bounty?
(Reporter)

Comment 14

2 years ago
(In reply to Nicholas Hurley [:nwgh][:hurley] (also hurley@todesschaf.org) 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!
(Assignee)

Comment 15

2 years ago
Posted patch patch (obsolete) — Splinter Review
Attachment #8842659 - Flags: review?(mcmanus)
(Assignee)

Comment 16

2 years ago
Posted patch test patchSplinter Review
Attachment #8842660 - Flags: review?(mcmanus)
(Assignee)

Comment 17

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

Comment 18

2 years ago
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.
Flags: needinfo?(chhsiao90)
(Assignee)

Comment 19

2 years ago
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).
Attachment #8842659 - Flags: review?(mcmanus) → review+
Attachment #8842660 - Flags: review?(mcmanus) → review+
(Assignee)

Comment 20

2 years ago
Attachment #8842995 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8842659 - Attachment is obsolete: true
(Assignee)

Comment 21

2 years ago
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.
Attachment #8842995 - Flags: sec-approval?
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.
status-firefox52: --- → wontfix
status-firefox53: --- → affected
status-firefox54: --- → affected
status-firefox-esr45: --- → affected
status-firefox-esr52: --- → affected
tracking-firefox53: --- → +
tracking-firefox54: --- → +
tracking-firefox-esr45: --- → 53+
tracking-firefox-esr52: --- → 52+
Whiteboard: [necko-active][spdy] → [necko-active][spdy][checkin on 3/21]
tracking-firefox-esr52: 52+ → 53+
Attachment #8842995 - Flags: sec-approval? → sec-approval+
Group: core-security → network-core-security
(Assignee)

Comment 23

2 years ago
See original sec-approval request for reasoning.
Attachment #8849226 - Flags: sec-approval?
(Assignee)

Comment 24

2 years ago
See original request for rationale
Attachment #8849228 - Flags: sec-approval?
(Assignee)

Comment 25

2 years ago
Attachment #8849230 - Flags: sec-approval?
(Assignee)

Comment 26

2 years ago
Attachment #8849232 - Flags: sec-approval?
(Assignee)

Comment 27

2 years ago
Attachment #8849233 - Flags: sec-approval?
(Assignee)

Comment 28

2 years ago
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
Attachment #8849226 - Flags: sec-approval? → approval-mozilla-aurora?
(Assignee)

Comment 29

2 years ago
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
Attachment #8849228 - Flags: sec-approval? → approval-mozilla-beta?
(Assignee)

Updated

2 years ago
Attachment #8849230 - Attachment description: patch for release → patch for release (in case we want it later)
Attachment #8849230 - Flags: sec-approval?
(Assignee)

Comment 30

2 years ago
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.
Attachment #8849232 - Flags: sec-approval? → approval-mozilla-esr52?
(Assignee)

Comment 31

2 years ago
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.
Attachment #8849233 - Flags: sec-approval? → approval-mozilla-esr45?
Attachment #8849226 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8849228 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8849232 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Attachment #8849233 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
This can land now.
Keywords: checkin-needed
Whiteboard: [necko-active][spdy][checkin on 3/21] → [necko-active][spdy]
https://hg.mozilla.org/integration/mozilla-inbound/rev/dfa17169cf1f
Status: UNCONFIRMED → ASSIGNED
status-firefox55: --- → affected
tracking-firefox55: --- → ?
Ever confirmed: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/dfa17169cf1f
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
tracking-firefox55: ? → +
Group: network-core-security → core-security-release
Flags: sec-bounty? → sec-bounty+
Whiteboard: [necko-active][spdy] → [necko-active][spdy][adv-main53+][adv-esr52.1+][adv-esr45.9+]
Alias: CVE-2017-5446
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/
Flags: needinfo?(chhsiao90)
(Reporter)

Comment 40

2 years ago
After confirmed, it had been fixed at 53.0b9, thanks for your hard working!
Flags: needinfo?(chhsiao90)
Status: RESOLVED → VERIFIED
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.