New issue
Advanced search Search tips

Issue 648620 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Sep 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 2
Type: Bug-Security



Sign in to add a comment

CRASH() writes to a fixed mappable address

Reported by jannhorn@googlemail.com, Sep 20 2016

Issue description

UserAgent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/53.0.2785.113 Safari/537.36

Steps to reproduce the problem:
1. On Linux, trigger a CRASH() somehow.
2. Look at 'dmesg | grep segfault'.

What is the expected behavior?
Chrome should terminate safely.

What went wrong?
In dmesg:

[87568.538035] chrome[23293]: segfault at fbadbeef ip 000055e98e040812 sp 00007ffcc2e99bb0 error 6 in chrome[55e98a195000+631d000]

This is triggered by CRASH():

#ifndef CRASH
#if COMPILER(MSVC)
#define CRASH() (__debugbreak(), IMMEDIATE_CRASH())
#else
#define CRASH() \
    (WTFReportBacktrace(), (*(int*)0xfbadbeef = 0), IMMEDIATE_CRASH())
#endif
#endif

CRASH() first calls WTFReportBacktrace(), then attempts to kill the process by writing to 0xfbadbeef, and if that wasn't enough, kills the process through __builtin_trap() via IMMEDIATE_CRASH().

There are two issues with this:

 - With 32-bit Chromium on a 64-bit kernel (and with x32, which I think Chromium doesn't support
   yet, and theoretically also in a normal 64-bit binary on a 64-bit kernel), 0xfbadbeef can be
   a valid userland address. (On a 64-bit Linux kernel, the limit for allocations by 32-bit binaries is
   0xFFFFe000 unless the ADDR_LIMIT_3GB personality flag is active, see IA32_PAGE_OFFSET in the
   kernel sources.)
 - The access is a write, not just a read.

This means that four bytes of random memory might be zeroed before the process exits. If there are other threads, this could theoretically permit an attacker to exploit this as a memory corruption on another thread, especially when you keep in mind that the operating system might need some time to stop all threads after a crash.

Did this work before? N/A 

Chrome version: 53.0.2785.113  Channel: stable
OS Version: 
Flash Version: Shockwave Flash 23.0 r0

I think that it would make sense to change `(*(int*)0xfbadbeef = 0)` to `(*(volatile int*)((sizeof(int*)==8) ? 0xFFFFFFFFFFFFFBAD : 0xFFFFFBAD))`. This way, the crash address doesn't depend on whether something happens to be mapped at 0xfbadbeef, and even if something can somehow be mapped at the magic address, at least it won't be overwritten. `volatile` prevents the compiler from compiling out the dereference.

This issue is probably not exploitable on many machines because:
 - 32-bit Chromium on a 64-bit kernel is probably a relatively uncommon
   configuration, at least on Linux. (Although maybe it happens if 32-bit
   Android apps on 64-bit devices use webviews or so?)
 - On 64-bit Linux, I don't think anything is allocated below 0x550000000000 or
   so, at least unless all the address space above that address is filled up
   first.
 
Components: Internals>CrashReporting
Labels: Security_Severity-Low Security_Impact-Stable
Owner: thakis@chromium.org
Status: Assigned (was: Unconfirmed)
In 2013, the target address was changed by https://chromium.googlesource.com/chromium/src/+/740d638df08fa4a6e8aff981f84e96eef389b42a to avoid a location where Windows may've mapped, but there's no discussion of other platforms.

As a write, Triage guidelines suggest a maximum of Sev-Medium, but the mitigating factors identified significantly limit the exploitability so setting Security_Severity-Low.

Comment 2 by thakis@chromium.org, Sep 20 2016

We no longer ship 32-bit linux or mac builds. (We do ship 32-bit android on 64-bit kernels.)

jannhorn: what do you think about 

(WTFReportBacktrace(), (*(int*)0x2000 = 0), IMMEDIATE_CRASH())

The first page should be read-only everywhere these days.


But going just to IMMEDIATE_CRASH() is probably even better.
Cc: mbarbe...@chromium.org
It would be helpful (not necessary) if we kept something like this when ASan is enabled. We started relying on it in CF for certain cases, but maybe we just shouldn't.

I do agree that just going to IMMEDIATE_CRASH in production builds is better.
I agree - if it doesn't break things, IMMEDIATE_CRASH() is probably the best option. In case that doesn't work, I agree with thakis that a write to 0x2000 should also be fine.

> We no longer ship 32-bit linux or mac builds.

Debian does ship a 32-bit x86 chromium build though: https://packages.debian.org/jessie/chromium

Comment 6 Deleted

Comment 7 by thakis@chromium.org, Sep 22 2016

Cc: primiano@chromium.org mark@chromium.org

Comment 8 by thakis@chromium.org, Sep 22 2016

Status: Fixed (was: Assigned)
Project Member

Comment 9 by bugdroid1@chromium.org, Sep 22 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cf2b885f31494c8006875e56cddef2840fffc9fa

commit cf2b885f31494c8006875e56cddef2840fffc9fa
Author: thakis <thakis@chromium.org>
Date: Thu Sep 22 16:57:39 2016

Don't write to an arbitrary address before crashing in blink's CRASH macro

We could write to the first page which is write-protected everywhere, but
let's see if we can just remove this.  (If this breaks anyone's workflow,
please let me know.)

BUG= 648620 

Review-Url: https://codereview.chromium.org/2361843002
Cr-Commit-Position: refs/heads/master@{#420374}

[modify] https://crrev.com/cf2b885f31494c8006875e56cddef2840fffc9fa/third_party/WebKit/Source/wtf/Assertions.h

Project Member

Comment 10 by sheriffbot@chromium.org, Sep 23 2016

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Project Member

Comment 11 by sheriffbot@chromium.org, Dec 30 2016

Labels: -Restrict-View-SecurityNotify allpublic
This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Sign in to add a comment