New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 788306 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Dec 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac , Fuchsia
Pri: 1
Type: Bug



Sign in to add a comment

Sandbox escape due to miss check in TracingHostMsg_TriggerBackgroundTrace IPC message

Reported by zhouat2...@gmail.com, Nov 24 2017

Issue description

UserAgent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/62.0.3202.75 Safari/537.36

Steps to reproduce the problem:
1. route the normal other ipc message (render to browser ) to TracingHostMsg_TriggerBackgroundTrace handler. 

2. see the SEGV_MAPERR

3. the ipc message handler is here: https://cs.chromium.org/chromium/src/content/browser/tracing/trace_message_filter.cc?l=43

What is the expected behavior?
the Chromium process Received signal 11 SEGV_MAPERR due to null pointer access .
i can reproduce both at chromium 63 and chromium 64.

What went wrong?
1. missing check the config_ at source code: https://cs.chromium.org/chromium/src/content/browser/tracing/background_tracing_manager_impl.cc?type=cs&q=OnHistogramTrigger&sq=package:chromium&l=326 

Did this work before? N/A 

Chrome version: 64.0.3277.0  Channel: n/a
OS Version: Ubuntu 16.04.3 LTS \n \l
Flash Version: Disabled
 
Labels: Needs-Feedback
Thanks for reporting. Could you upload plain poc file, instead of rar?

Comment 2 Deleted

Project Member

Comment 3 by sheriffbot@chromium.org, Nov 25 2017

Cc: jialiul@chromium.org
Labels: -Needs-Feedback
Thank you for providing more feedback. Adding requester "jialiul@chromium.org" to the cc list and removing "Needs-Feedback" label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
zhouat2017, could you explain how this can be used in a (non-local) attack? 
If so, could you provide a better trigger? e.g. a crafted web-page etc
Cc: simonhatch@chromium.org
Components: Internals>Core
I suspect the precondition of this attack is that you already have compromised the render process via a different RCE vulnerability in the Renderer process. You then use the issue described in this bug to attempt to compromise the Browser process.

Comment 6 Deleted

Cc: awhalley@chromium.org
awhalley@, re#6, how do we usually treat this kind of reporting?

Comment 8 Deleted

Comment 9 by wfh@chromium.org, Nov 27 2017

Cc: wfh@chromium.org
hi - is it possible for you to upload a renderer patch that would call the IPC and cause the issue to happen in the browser? This would make reproducing this easier for us. How are you reproducing this yourself?

Comment 10 Deleted

Cc: primiano@chromium.org
Components: Speed>Tracing
Labels: -Pri-2 Security_Severity-High Security_Impact-Stable OS-Android OS-Chrome OS-Fuchsia OS-Mac OS-Windows Pri-1
Owner: oysteine@chromium.org
Status: Assigned (was: Unconfirmed)
oysteine: Looks like you own the relevant code. Could you please take a look? Thanks!
Components: -Internals>Core
Status: Started (was: Assigned)
Labels: M-63
oysteine@ knows more about this code and looks like he is already on it.

I have doubts on the severity of this. I think this is a bug and should be fixed, but IMO not even worth merging back more than beta.
I don't think this is a sandbox escape though.

I took a look at the code at it seems to me that what's going on is the following:
if you manage to forge an TracingHostMsg_TriggerBackgroundTrace IPC to the browser too early (or I think even without the finch experiment running, where the config_ might be never set), the browser will dereference an empty unique_ptr and crash. 

I am not a security expert, but causing the browser to crash because of hitting a nullptr, doesn't look like a sandbox escape to me. I mean, you are escaping into sure death, because all chrome processes will then die.

Comment 16 Deleted

RE #15: Denial-of-service via NULL dereference was my analysis of the issue as well; we do not typically treat such issues as security issues (https://chromium.googlesource.com/chromium/src/+/master/docs/security/faq.md#Are-denial-of-service-issues-considered-security-bugs).

The fact that it brings down the browser process makes it a more severe bug, but the fact that it requires a renderer compromise as a precondition in turn lowers the severity.
root cause:

(please refer to the decompiled code below for more detail info )
1. chrome receive  SIGSEGV  when visit `.text:0000000002C5122A       mov     rbx, [rax+10h]`,         <--- rax(config_) is 0x0 now, here cause the issue, which try to access [rax+0x10] (rules_).  
2. rax come from `.text:0000000002C51226   mov     rax, [r14+10h]`,                                   <--- rax come form [r14+0x10h], which is rax(config_),now it is 0x0 .
3. r14 come from `.text:0000000002C51214   mov     r14, rdi`,                                         <--- rdi is `this pointer`, and saved to r14(this pointer)

r14(this pointer) content is below, you see the pointer is not null, just the offset from [r14+0x10] is 0x0, which is only init at one place(here it try to access uninitialized memory, this can lead to Serious security issues). 
link address: https://cs.chromium.org/chromium/src/content/browser/tracing/background_tracing_manager_impl.cc?gsn=config_&l=180

gef➤  x/16gx $r14
0x55555d509310 <_ZN7content12_GLOBAL__N_128g_background_tracing_managerE+8>:	0x000055555cf76190	0x000004dc8bd2c6c0
0x55555d509320 <_ZN7content12_GLOBAL__N_128g_background_tracing_managerE+24>:	0x0000000000000000	0x000055555d509330
0x55555d509330 <_ZN7content12_GLOBAL__N_128g_background_tracing_managerE+40>:	0x0000000000000000	0x0000000000000000
0x55555d509340 <_ZN7content12_GLOBAL__N_128g_background_tracing_managerE+56>:	0x0000000000000000	0x0000000000000000
0x55555d509350 <_ZN7content12_GLOBAL__N_128g_background_tracing_managerE+72>:	0x0000000000000000	0x0000000000010000
0x55555d509360 <_ZN7content12_GLOBAL__N_128g_background_tracing_managerE+88>:	0x00000000ffffffff	0x000004dc8bd2d8a0
0x55555d509370 <_ZN7content12_GLOBAL__N_128g_background_tracing_managerE+104>:	0x000004dc8bd2d8a0	0x0000000000000001
0x55555d509380 <_ZN7content12_GLOBAL__N_128g_background_tracing_managerE+120>:	0x000004dc8c42ec60	0x000004dc8c42ec60


/* decompiled code */

.text:0000000002C51200 ; content::BackgroundTracingManagerImpl::OnHistogramTrigger(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&)
.text:0000000002C51200 _ZN7content28BackgroundTracingManagerImpl18OnHistogramTriggerERKNSt3__112basic_stringIcNS1_11char_traitsIcEENS1_9allocatorIcEEEE proc near
.text:0000000002C51200                                         ; CODE XREF: content::TraceMessageFilter::OnTriggerBackgroundTrace(std::__1::basic_string<char,std::__1::char_traits<char>,std::__1::allocator<char>> const&)+1Aj
.text:0000000002C51200                                         ; DATA XREF: content::BackgroundTracingManagerImpl::OnHistogramTrigger(std::__1::basic_string<char,std::__1::char_traits<char>,std::__1::allocator<char>> const&)+C4o
.text:0000000002C51200
.text:0000000002C51200 var_60          = byte ptr -60h
.text:0000000002C51200 var_40          = byte ptr -40h
.text:0000000002C51200 var_38          = byte ptr -38h
.text:0000000002C51200 var_30          = qword ptr -30h
.text:0000000002C51200
.text:0000000002C51200                 push    rbp
.text:0000000002C51201                 mov     rbp, rsp
.text:0000000002C51204                 push    r15
.text:0000000002C51206                 push    r14
.text:0000000002C51208                 push    r13
.text:0000000002C5120A                 push    r12
.text:0000000002C5120C                 push    rbx
.text:0000000002C5120D                 sub     rsp, 38h
.text:0000000002C51211                 mov     r12, rsi
.text:0000000002C51214                 mov     r14, rdi         <---
.text:0000000002C51217                 xor     edi, edi
.text:0000000002C51219                 call    _ZN7content13BrowserThread11CurrentlyOnENS0_2IDE ; content::BrowserThread::CurrentlyOn(content::BrowserThread::ID)
.text:0000000002C5121E                 test    al, al
.text:0000000002C51220                 jz      short loc_2C51282
.text:0000000002C51222                 mov     [rbp+var_30], r14
.text:0000000002C51226                 mov     rax, [r14+10h]   <---
.text:0000000002C5122A                 mov     rbx, [rax+10h]   <---
.text:0000000002C5122E                 mov     r14, [rax+18h]




The code at 
https://cs.chromium.org/chromium/src/content/browser/tracing/background_tracing_manager_impl.cc?type=cs&q=OnHistogramTrigger&sq=package:chromium&l=326 
will  try to visit uninitialized memory, this lead to Serious security issues.

To fix this problem, you can refer to the patch i offer here , should add check at two place.


/* patch file start */

--- /home/zhouat/chrome_linux_2017_11_24/src/src-64.0.3276.2/content/browser/tracing/background_tracing_manager_impl.cc	2017-11-23 19:09:46.000000000 -0800
+++ background_tracing_manager_impl.cc	2017-11-23 20:09:50.238165886 -0800
@@ -322,6 +322,9 @@
                        base::Unretained(this), histogram_name));
     return;
   }
+ 
+  if(!config_ || config_->rules().empty())
+    return;
 
   for (const auto& rule : config_->rules()) {
     if (rule->ShouldTriggerNamedEvent(histogram_name))


/* patch file  end */


Thanks.

Project Member

Comment 19 by bugdroid1@chromium.org, Dec 7 2017

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

commit 07fad9439e5b1ab4b8f458963c553601c1e16ff1
Author: Oystein Eftevaag <oysteine@chromium.org>
Date: Thu Dec 07 18:59:01 2017

Background tracing: Ensure config is present on histogram trigger IPCs

R=ssid@chromium.org

Bug:  788306 
Change-Id: I2e81ab0c1836aebeb2ed36e02c38e39309557057
Reviewed-on: https://chromium-review.googlesource.com/793897
Reviewed-by: Siddhartha S <ssid@chromium.org>
Commit-Queue: oysteine <oysteine@chromium.org>
Cr-Commit-Position: refs/heads/master@{#522492}
[modify] https://crrev.com/07fad9439e5b1ab4b8f458963c553601c1e16ff1/content/browser/tracing/background_tracing_manager_impl.cc

Status: Fixed (was: Started)
Project Member

Comment 21 by sheriffbot@chromium.org, Dec 8 2017

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Labels: reward-topanel
Labels: -Type-Bug-Security -reward-topanel -Security_Impact-Stable -Security_Severity-High reward-0 Type-Bug
The VRP panel took a look at his, and concluded that this is an unexploitable null pointer dereference, sorry!

Comment 24 Deleted

> is there some specification can ensure that, the _config must be null? 

Yes: |config_| is a std::unique_ptr, not a raw pointer. As per [1] the default empty constructor for std::unique_ptr zero-initializes the pointer.

> even though the _config must be null, some times null pointer dereference can lead to privilege escalation, you can refer here for info

That requires that you are already able to execute arbitrary code to perform a mmap(MAP_FIXED) @ address 0. But if you can execute arbitrary code to do that mmap you won already, without needing to exploit a null pointer dereference.

[1] http://en.cppreference.com/w/cpp/memory/unique_ptr/unique_ptr

Comment 26 Deleted

Comment 27 Deleted

Hello - I'm afraid since this isn't being classed as a security bug, it won't be getting a CVE, sorry.
Project Member

Comment 29 by sheriffbot@chromium.org, Mar 16 2018

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