double-free leading to crash everytime it find chunked stream ending with zero and sending data after that also

RESOLVED FIXED in Firefox 64

Status

()

defect
P2
normal
RESOLVED FIXED
10 months ago
9 months ago

People

(Reporter: saikumarssn.cse, Assigned: kershaw)

Tracking

(6 keywords)

unspecified
mozilla65
Points:
---
Dependency tree / graph
Bug Flags:
sec-bounty -
qe-verify -

Firefox Tracking Flags

(firefox-esr60 wontfix, firefox63 wontfix, firefox64+ fixed, firefox65+ fixed)

Details

(Whiteboard: [reporter-external] [client-bounty-form] [verif?][necko-triaged][sg:dos][adv-main64-], crash signature)

Attachments

(3 attachments)

Posted file fireforcrash.txt
Firefox is being crashed, when it tried to process the chunked data. when Transfer-Encoding header is used, we suppose to send the data in the chunks form. When all the chunks being transferred, we need to tell the client with chunk length zero. so that it understands chunked stream completed. 

But if we send the data after sending zero length, then firefox not able to understand it, and it being crashed.

Example:

5
Don't
5
be af
5
firef
5
ox is
0      
crash(after saying it is end, we still sending some data)    
\r\n


 ---->here we are saying no chunked data after this.so that it should end with         \r\n. But we  are sending some data after this also. Like

steps to reproduce the crash
1)download the firefoxcrash.txt
2)nc -lp 8000 < firefoxcrash.txt
3)browe the url from firefox like 
http://localhost:8000
4)then close the netcat client. then you will obserev the firefox crash.

Windows 10
Firefox version:63.0.1
Flags: sec-bounty?
Duplicate of this bug: 1504510
And there is one more same kind of bug is there.

When we using chunked stream, and terminating chunk is a regular chunk, with the exception that its length is zero. IT should be end with \r\n\r\n(only these should be at the end). But if we provide \n\r\r\n as a ending control characters, then also firefox getting crashed.
Posted file uaf_log.txt
Group: firefox-core-security → network-core-security
Status: UNCONFIRMED → NEW
Component: Security → Networking
Ever confirmed: true
Keywords: csectype-uaf
Product: Firefox → Core
Cautiously calling this sec-high, but mccr8 notes the free and re-use appear to have the same stack so maybe there's no time for an exploitable re-use.
Flags: needinfo?(kershaw)
Keywords: sec-high
The UAF is happening in ~nsHttpChunkedDecoder, which looks like this:
  ~nsHttpChunkedDecoder() { delete mTrailers; }

The type of mTrailers is:
  nsAutoPtr<nsHttpHeaderArray>  mTrailers;

Bug 1413999 changed mTrailers from a raw pointer to an auto pointer. I suspect that what is happening is that mTrailers is being explicitly deleted, but then it gets automatically deleted after that, and somehow that causes a UAF. It doesn't seem exploitable at all. The fix would be to change the dtor to a default dtor.
I think attacker can run DOS attacks on firefox using this bug. Please correct me if i am wrong.
(In reply to Andrew McCreight [:mccr8] from comment #5)
> The UAF is happening in ~nsHttpChunkedDecoder, which looks like this:
>   ~nsHttpChunkedDecoder() { delete mTrailers; }
> 
> The type of mTrailers is:
>   nsAutoPtr<nsHttpHeaderArray>  mTrailers;
> 
> Bug 1413999 changed mTrailers from a raw pointer to an auto pointer. I
> suspect that what is happening is that mTrailers is being explicitly
> deleted, but then it gets automatically deleted after that, and somehow that
> causes a UAF. It doesn't seem exploitable at all. The fix would be to change
> the dtor to a default dtor.

Thanks for pointing this out.
Changing to a deault dtor really fixes this crash.
Assignee: nobody → kershaw
Flags: needinfo?(kershaw)
Priority: -- → P2
Whiteboard: [reporter-external] [client-bounty-form] [verif?] → [reporter-external] [client-bounty-form] [verif?][necko-triaged]
Posted patch patchSplinter Review
Attachment #9023908 - Flags: review?(valentin.gosu)
Comment on attachment 9023908 [details] [diff] [review]
patch

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

Unit tests would be nice. :)
Attachment #9023908 - Flags: review?(valentin.gosu) → review+
Keywords: checkin-needed
Being a sec-high affecting more than trunk (going off #c0 and code inspection), this needs sec-approval before it can land.
https://wiki.mozilla.org/Security/Bug_Approval_Process
Comment on attachment 9023908 [details] [diff] [review]
patch

[Security Approval Request]

How easily could an exploit be constructed based on the patch?: I would say it's easy, if someone knows that mTrailers ns nsAutoPtr.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes

Which older supported branches are affected by this flaw?: esr60

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

Do you have backports for the affected branches?: No

If not, how different, hard to create, and risky will they be?: Base on comment #4, I think the risk is low.

How likely is this patch to cause regressions; how much testing does it need?: The risk of regression is low, since this is just a simple and correct fix.
Manual test according to comment #0 should be enough.
Flags: needinfo?(kershaw)
Attachment #9023908 - Flags: sec-approval?
Kershaw, can you figure out why not one of the existing checks in test_chunked_responses.js hit this problem (or perhaps what's necessary to change in them to trigger it)? From my quick glance on these tests, some of them do not end with a clean "0\r\n\r\n" sequence.
(In reply to Daniel Stenberg [:bagder] from comment #12)
> Kershaw, can you figure out why not one of the existing checks in
> test_chunked_responses.js hit this problem (or perhaps what's necessary to
> change in them to trigger it)? From my quick glance on these tests, some of
> them do not end with a clean "0\r\n\r\n" sequence.

Since none of the checks in test_chunked_responses.js matches the following two conditions at the same time.
1. Create |mTrailers|.
2. End the chunked data without "\r\n".

The detailed steps of this crash is:
1. |mTrailers| is created at [1].
2. Since the end of the chunked data is not "\r\n", |mReachedEOF| will never be set to true at [2].
3. Since the eof is never reached, TakeTrailers() is not called [3]. 
4. |mTrailers| will be deleted twice.



[1] https://searchfox.org/mozilla-central/rev/7f7c353e969e61a6a85201cc8ad3c3de12ac30d8/netwerk/protocol/http/nsHttpChunkedDecoder.cpp#121
[2] https://searchfox.org/mozilla-central/rev/7f7c353e969e61a6a85201cc8ad3c3de12ac30d8/netwerk/protocol/http/nsHttpChunkedDecoder.cpp#137
[3] https://searchfox.org/mozilla-central/rev/7f7c353e969e61a6a85201cc8ad3c3de12ac30d8/netwerk/protocol/http/nsHttpTransaction.cpp#1834-1837
(In reply to Andrew McCreight [:mccr8] from comment #5)
> mTrailers is being explicitly deleted, but then it gets
> automatically deleted after that, and somehow that causes
> a UAF. It doesn't seem exploitable at all.

A double-free is technically a use-after-free, and if the two frees are spaced out such that the chunk can be re-allocated the second free can cause problems with the code that now owns the new object. When they happen with no intervening code like this it can't be exploited other than as a denial of service. ASAN builds detect this because they track all allocs/frees and instrument memory chunks.

This crash does appear to happen in the wild: bp-03154728-5949-47b5-b635-7a6f30181105

DOS bugs usually don't need to be hidden.
Blocks: 1413999
Group: network-core-security
Crash Signature: [@ mozilla::net::nsHttpChunkedDecoder::~nsHttpChunkedDecoder ]
Summary: Firefox is being crashed, everytime it find chunked stream ending with zero and sending data after that also → double-free leading to crash everytime it find chunked stream ending with zero and sending data after that also
Whiteboard: [reporter-external] [client-bounty-form] [verif?][necko-triaged] → [reporter-external] [client-bounty-form] [verif?][necko-triaged][sg:dos]
Comment on attachment 9023908 [details] [diff] [review]
patch

No longer need sec-approval.

Consistent with bug 1413999 being the regressing bug, crash-stats doesn't report any crashes with this signature in versions before Firefox 59.
Attachment #9023908 - Flags: sec-approval?
Keywords: checkin-needed
Pushed by ebalazs@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6604d308037
Do not delete mTrailers explicitly, r=valentin
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e6604d308037
Status: NEW → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Please request Beta approval on this when you get a chance. It grafts cleanly as-landed.
Flags: needinfo?(kershaw)
Comment on attachment 9023908 [details] [diff] [review]
patch

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1413999

User impact if declined: Firefox could crash if it gets more string after the end of trailers.

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): This patch is really simple and is proved to be able to fix the crash.

String changes made/needed: none
Flags: needinfo?(kershaw)
Attachment #9023908 - Flags: approval-mozilla-beta?
Comment on attachment 9023908 [details] [diff] [review]
patch

fix double free, approved for 64.0b11
Attachment #9023908 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Unfortunately denial of service bugs are excluded from our bounty program
Flags: sec-bounty? → sec-bounty-
Can i apply for CVE from the following website?
https://cveform.mitre.org/
Flags: needinfo?(kershaw)
dveditz is a better person to answer this question than Selena. He can give more of a definitive answer to this than me, but CVEs for security issues found in Firefox are assigned by Mozilla itself, not the reporter. In this particular case, as described in comment 14, we decided this does not appear to be an exploitable security issue, so it will probably not be assigned a CVE.
Flags: needinfo?(sdeckelmann)
We don't assign CVEs for Denial of Service bugs in client products (they may qualify in a server product). You may appeal that to MITRE through the form mentioned in comment 24, but they usually defer to the CNA/vendor.
Flags: needinfo?(dveditz)
That's great. Thank you or your cooperation.
Whiteboard: [reporter-external] [client-bounty-form] [verif?][necko-triaged][sg:dos] → [reporter-external] [client-bounty-form] [verif?][necko-triaged][sg:dos][adv-main64-]
You need to log in before you can comment on or make changes to this bug.