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

Status: RESOLVED FIXED

(Core :: Networking, defect, P2)

Description

[SAIKUMAR]

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

Comment 2

[SAIKUMAR]

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.

Comment 3

[Tyson Smith [:tsmith]]

Attachment: uaf_log.txt

Comment 4

[Daniel Veditz [:dveditz]]

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.

Comment 5

[Andrew McCreight [:mccr8]]

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.

Comment 6

[SAIKUMAR]

I think attacker can run DOS attacks on firefox using this bug. Please correct me if i am wrong.

Comment 7

[Kershaw Chang [:kershaw]]

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

Comment 8

[Kershaw Chang [:kershaw]]

Attachment: patch

Comment 9

[Valentin Gosu [:valentin]]

Comment on attachment 9023908 [details] [diff] [review]
patch

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

Unit tests would be nice. :)

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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 11

[Kershaw Chang [:kershaw]]

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.

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.

Comment 13

[Kershaw Chang [:kershaw]]

(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

Comment 14

[Daniel Veditz [:dveditz]]

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

Comment 15

[Daniel Veditz [:dveditz]]

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.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6604d308037
Do not delete mTrailers explicitly, r=valentin

Comment 17

[Raul Gurzau (:RaulG)]

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

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this when you get a chance. It grafts cleanly as-landed.

Comment 19

[Kershaw Chang [: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

Comment 20

[Julien Cristau [:jcristau] (back August 18)]

Comment on attachment 9023908 [details] [diff] [review]
patch

fix double free, approved for 64.0b11

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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

Comment 22

[Daniel Veditz [:dveditz]]

Unfortunately denial of service bugs are excluded from our bounty program

Comment 23

[SAIKUMAR]

Hi all,

when would i get the CVE number for this bug?

Comment 24

[SAIKUMAR]

Can i apply for CVE from the following website?
https://cveform.mitre.org/

Comment 25

[Kershaw Chang [:kershaw]]

(In reply to SAIKUMAR from comment #24)
> Can i apply for CVE from the following website?
> https://cveform.mitre.org/

Sorry that I have no idea about this.
Maybe someone from security team can answer your question.

Comment 26

[Andrew McCreight [:mccr8]]

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.

Comment 27

[Daniel Veditz [:dveditz]]

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.

Comment 28

[SAIKUMAR]

That's great. Thank you or your cooperation.