1288482 - (CVE-2016-5292) URL parsing causes crash

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Tyson Smith [:tsmith]]

Attachment: linux_asan_log.txt

+++ This bug was initially created as a clone of Bug #1288123 +++

NOTE: This clone was created because the description of the initial bug was crashing browsers. The original was logged by [e-mail].

WARNING: Opening the Bug #1288123 may trigger crash.

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

Steps to reproduce:

Create an html page with the following contents, access it, then "reload this page" one time.

Actual results:

It crashes for version 46+ on OS X, including FirefoxNightly 50.0a1 (2016-07-15). Older versions and other platforms work. (45 on OS X works, 47 on Linux works, etc.)

Process:               firefox [38048]
Path:                  /Applications/NightlyDebug.app/Contents/MacOS/firefox
Identifier:            org.mozilla.nightlydebug
Version:               47.0.2 (4716.7.2)
Code Type:             X86-64 (Native)
Parent Process:        ??? [1]
Responsible:           firefox [38048]
User ID:               25628395

Date/Time:             2016-07-20 08:00:17.778 -0700
OS Version:            Mac OS X 10.11.5 (15F34)
Report Version:        11
Anonymous UUID:        4F065409-6410-EED7-4352-470B9B7EA9DD

Sleep/Wake UUID:       1B10C1A4-591F-4DAC-AD77-30B47ECCF792

Time Awake Since Boot: 400000 seconds
Time Since Wake:       2900 seconds

System Integrity Protection: enabled

Crashed Thread:        0  Dispatch queue: com.apple.main-thread

Exception Type:        EXC_BAD_ACCESS (SIGSEGV)
Exception Codes:       KERN_INVALID_ADDRESS at 0x0000000213393047
Exception Note:        EXC_CORPSE_NOTIFY

VM Regions Near 0x213393047:
    mapped file            000000019de00000-000000019e082000 [ 2568K] r--/rwx SM=COW  
--> 
    STACK GUARD            0000700000000000-0000700000001000 [    4K] ---/rwx SM=NUL  stack guard for thread 62

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   libsystem_platform.dylib      	0x00007fff83382360 _platform_strncmp + 320
1   XUL                           	0x00000001052a5db7 0x105093000 + 2174391
2   XUL                           	0x00000001086e0f64 0x105093000 + 56942436
3   XUL                           	0x00000001051b695a 0x105093000 + 1194330
4   XUL                           	0x00000001051b1b9d 0x105093000 + 1174429
5   XUL                           	0x000000010866a5ae 0x105093000 + 56456622
6   XUL                           	0x000000010623009f 0x105093000 + 18469023
7   XUL                           	0x00000001062fb175 0x105093000 + 19300725
[...]


Expected results:

No crash.

Comment 1

[Tyson Smith [:tsmith]]

Attachment: test_case.html

Comment 3

[Andrew McCreight [:mccr8]]

Valentin, could you look at this? The top of the stack at least is in a strcmp of some URL thing, I believe.

Comment 4

[Daniel Veditz [:dveditz]]

Members of the triage team observed a crash in Nightly (50) on Mac and Linux, but not on Windows. No crash on Dev Edition (49). Since it appears to be a bad access from strcmp() whether it crashes may depend on build differences and what happens to be allocated at the incorrect pointer. Testing in an ASAN build may reveal this to go back much much further than the reporter's 46+ (though we don't have to care about anything earlier than ESR-45 at this point).

Comment 5

[Valentin Gosu [:valentin] (he/him)]

The issue seems to have been introduced by bug 1042347.
It happens because nsStandardURL::CoalescePath is expecting net_CoalesceDirs to only normalize the directory, not the entire filepath - which screws up the offsets.

Comment 6

[Patrick McManus [:mcmanus]]

thanks valentin

Comment 7

[Valentin Gosu [:valentin] (he/him)]

Attachment: Backout bug 1288482

MozReview-Commit-ID: J3LmMfQ854V

Comment 8

[Valentin Gosu [:valentin] (he/him)]

Attachment: Backout bug 1042347

MozReview-Commit-ID: J3LmMfQ854V

Comment 9

[Valentin Gosu [:valentin] (he/him)]

The fix for this was getting to be quite complicated - something I wouldn't feel confident to uplift.
So I think the right decision is to backout bug 1042347, and fix this corner case in that bug.

Comment 11

[Patrick McManus [:mcmanus]]

Comment on attachment 8774154 [details] [diff] [review]
Backout bug 1042347

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

just to note that this isn't a straight backout of https://hg.mozilla.org/mozilla-central/rev/f3925f5ffdbb because of a change in test coverage.. not sure if that will impact cleanliness of uplifts.

Comment 12

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8774154 [details] [diff] [review]
Backout bug 1042347

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Not easily.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No, but bugs 1288481, 1288123, 1288481 are public and contain examples of URLs that crash the browsers.

Which older supported branches are affected by this flaw?
Starting with Mozilla 47

If not all supported branches, which bug introduced the flaw?
Bug 1042347.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Easy to backport. Only involves a backout of bug 1042347.

How likely is this patch to cause regressions; how much testing does it need?
Unlikely. Only a backout.

Comment 13

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

I'm giving this sec-approval+ for trunk checkin now since the URLs are visible in bugs.

Comment 14

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

Comment on attachment 8774154 [details] [diff] [review]
Backout bug 1042347

We'll want patches for other affected branches made and nominated after this is on trunk.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/405ec1364547

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/405ec1364547

Comment 17

[Valentin Gosu [:valentin] (he/him)]

Attachment: Backout bug 1042347 [aurora]

Comment 18

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8776409 [details] [diff] [review]
Backout bug 1042347 [aurora]

Approval Request Comment
[Feature/regressing bug #]: Bug 1042347
[User impact if declined]: Crash with specially crafter URL. Security risk.
[Describe test coverage new/current, TreeHerder]: Some unit tests. Previous handling of %2e is known to be safe.
[Risks and why]: Low. This is just a backout.
[String/UUID change made/needed]: None.

Comment 19

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8776409 [details] [diff] [review]
Backout bug 1042347 [aurora]

Fix a crash and sec-high, taking it to aurora.