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

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2015
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug-Security

Blocking:
issue 468933



Sign in to add a comment
link

Issue 468936: Pwn2own gpu bug

Reported by aarya@google.com, Mar 19 2015 Project Member

Issue description

Buffer Overflow via Race Condition
// src/gpu/command_buffer/service/gles2_cmd_decoder.cc
error::Error GLES2DecoderImpl::HandleGetUniformfv(uint32 immediate_data_size, const v
oid* cmd_data) {
const gles2::cmds::GetUniformfv& c = *static_cast<const gles2::cmds::GetUniformfv
*>(cmd_data);
GLuint program = c.program;
GLint fake_location = c.location;
GLuint service_id;
GLint real_location = -1;
Error error;
typedef cmds::GetUniformfv::Result Result;
Result* result;
GLenum result_type;
if (GetUniformSetup(
program, fake_location, c.params_shm_id, c.params_shm_offset,
&error, &real_location, &service_id,
reinterpret_case<void**>(&result), &result_type)) {
if (result_type == GL_BOOL || result_type == GL_BOOL_VEC2 || result_type == G
L_BOOL_VEC3 || result_type == GL_BOOL_VEC4) {
GLsizei num_values = result->GetNumResults();
scoped_ptr<GLint[]> temp(new GLint[num_values]);
glGetUniformiv(service_id, real_location, temp.get());
GLfloat* dst = result->GetData();
for (GLsizei ii = 0; ii < num_values; ++i) {
dst[ii] = (temp[ii] != 0);
}
} else {
glGetUniformfv(service_id, real_location, result->GetData());
}
}
return error;
}
Specifically, the following code is buggy.
GLsizei num_values = result->GetNumResults();
GetNumResults function simply returns result->size , which is validated and determined by
GetUniformSetup . However, the result is located in shared memory, so even if
GetUniformSetup validated the size, result->size can be manipulated afterwards to cause an
overflow.
 

Comment 1 by aarya@google.com, Mar 19 2015

Blocking: chromium:468933

Comment 2 by rsesek@chromium.org, Mar 19 2015

Labels: Restrict-View-SecurityTeam

Comment 3 by aarya@google.com, Mar 19 2015

Labels: -Type-Bug Type-Bug-Security

Comment 4 by infe...@chromium.org, Mar 19 2015

In order to understand this vulnerability, it is important to know how Native Client and GPU process
communicate to each other. As a mean of communication, shared memory is used for sending commands
(OpenGL API, etc.) as well as receiving return values.
To give an example, when glGetUniformfv (one of the OpenGL APIs) is called, the shared memory used
for commands is filled in with the function id and its arguments. Later on, GPU process takes and processes
this data, and writes to the shared memory used for return values.
Triggering the bug is rather simple. We prepare 2 threads and let one of them continues to call
glGetUniformfv while the other one constantly modifies the value at where result->size resides.

Comment 5 by infe...@chromium.org, Mar 19 2015

Cc: jfb@chromium.org

Comment 6 by wfh@chromium.org, Mar 19 2015

Speaking to the reporter, the suggested fix here is to add an extra out parameter to GetUniformSetup to return the number of results (num_values) instead of doing the additional result->GetNumValues() call. This avoids the TOCTOU.

Comment 7 by jfb@chromium.org, Mar 19 2015

Cc: sehr@chromium.org
Sync'ed with kbr, he's looking into it and will ping me / NaCl team if he needs help.

Comment 8 by timwillis@google.com, Mar 19 2015

thanks jfb!

Comment 9 by jln@chromium.org, Mar 19 2015

Cc: zmo@chromium.org
Adding zmo who I think wrote the code.

Comment 10 by jln@chromium.org, Mar 19 2015

In GLES2DecoderImpl::HandleGetUniformfv(), GLsizei num_values = result->GetNumResults(); has an obvious "Time of check vs. time of use" (TOCTU) issue given that |result| lives in untrusted shared memory.

Comment 11 by jln@chromium.org, Mar 19 2015

Cc: piman@chromium.org

Comment 12 by kbr@chromium.org, Mar 19 2015

Owner: vmi...@chromium.org
vmiura@ and I looked at this. Agree that we need to use the size that the command decoder computed, not the one that we previously stored into shared memory. This can be fixed with a localized change to the signature of GetUniformSetup and the callers. vmiura@ said he'd take this; thanks.

Comment 13 by jln@chromium.org, Mar 19 2015

Besides the immediate bug, we should try and make sure there are not more instance of trusting the values in shared memory. It's tricky to get this right.

We should probably prefix all variables in shared memory with untrusted_ or something along these lines.

Comment 14 by piman@chromium.org, Mar 19 2015

That's the only one caller of GetNumResults on the service side, so at least there's that.

Comment 15 by piman@chromium.org, Mar 19 2015

We should probably rename GetNumResults to GetNumResultsFromClient or something to make it clear that this is not something we should call service-side.

Comment 16 by wfh@chromium.org, Mar 19 2015

Labels: -Pri-0 Pri-1 Security_Severity-High Security_Impact-Stable

Comment 17 by ClusterFuzz, Mar 19 2015

Project Member
Labels: M-41

Comment 18 by bugdroid1@chromium.org, Mar 20 2015

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/181c7400b2bf50ba02ac77149749fb419b4d4797

commit 181c7400b2bf50ba02ac77149749fb419b4d4797
Author: vmiura <vmiura@chromium.org>
Date: Fri Mar 20 01:32:51 2015

gpu: Use GetUniformSetup computed result size.

R=piman@chromium.org
BUG= 468936 

Review URL: https://codereview.chromium.org/1016193003

Cr-Commit-Position: refs/heads/master@{#321489}

[modify] http://crrev.com/181c7400b2bf50ba02ac77149749fb419b4d4797/gpu/command_buffer/service/gles2_cmd_decoder.cc

Comment 19 by infe...@chromium.org, Mar 20 2015

Status: Fixed

Comment 20 by ClusterFuzz, Mar 20 2015

Project Member
Labels: M-42 Merge-Triage
Adding Merge-Triage label for tracking purposes.

Once your fix had sufficient bake time (on canary, dev as appropriate), please nominate your fix for merge by adding the Merge-Requested label.

When your merge is approved by the release manager, please start merging with higher milestone label first. Make sure to re-request merge for every milestone in the label list. You can get branch information on omahaproxy.appspot.com.

- Your friendly ClusterFuzz

Comment 21 by ianbeer@chromium.org, Mar 20 2015

Cc: jannewger@google.com
cc:ing jannewger@ who worked on a dynamorio tool to detect shared memory TOCTOU bugs in chrome

Comment 22 by ianbeer@chromium.org, Mar 20 2015

Cc: thomasdu...@google.com

Comment 23 by thomasdu...@google.com, Mar 20 2015

Hey all,

so yeah, jannewger@ put a lot of work into writing a shared memory TOCTOU detector for Chrome last year (based on dynamic binary instrumentation).

Unfortunately, it turned out to be a pretty horrible ordeal -- Chrome's Windows Sandbox plays very very rough, and interacting with any DBI tools turned out to be a nightmare of epic proportions (dozens of DynamoRIO bugs fixed over a period of more than 6 months, and even then we could never get it to work right).

I'd love to revive this project, but do it "right" this time -- e.g. work with a full-system emulator (QEMU or Bochs or the like).

If nobody objects violently, jan & ian & me are going to look at Bochspwn again and then draft a design on how this stuff can be taken care of.

Cheers,
Thomas

Comment 24 by jfb@chromium.org, Mar 20 2015

Cc: kcc@chromium.org
I'm not sure the fix [1] is fully correct. The code does:

  SizedResult<GLint>* result;
  result = GetSharedMemoryAs<SizedResult<GLint>*>(...);
  ...
  result->size = size; // Shared memory store.
  *result_size = size; // Store on the current local stack.

SizedResult [2] is just a struct with POD members. GetSharedMemoryAs [3] just gets the shared memory pointer. If I'm reading this code correctly there's no synchronization anywhere. Without telling the compiler that the memory can be externally modified this code is wrong: the compiler is allowed to delay the store indefinitely, reuse the stored value and not use result_size, or even use result->size as temporary storage because nobody else is accessing it as far as it knows. I don't want to be alarmist: you've probably fixed the big race condition window, but there's still a window that's compiler dependent. Using volatile would be the simplest fix. I'd be wary of using non-volatile atomics because I'm pretty sure I could legally teach the optimizer to outsmart you :-)

This points at a wider problem: if I understand correctly *we need to audit all uses of shared memory*. Can you confirm my understanding is correct? If so this should be a separate bug.



  [1]: https://codereview.chromium.org/1016193003/diff/20001/gpu/command_buffer/service/gles2_cmd_decoder.cc
  [2]: https://code.google.com/p/chromium/codesearch#chromium/src/gpu/command_buffer/common/gles2_cmd_format.h&q=sizedresult&sq=package:chromium&type=cs&l=85
  [3]: https://code.google.com/p/chromium/codesearch#chromium/src/gpu/command_buffer/service/common_decoder.h&q=GetSharedMemoryAs&sq=package:chromium&type=cs&l=140

Comment 25 by kcc@chromium.org, Mar 20 2015

In C++ volatile has no relation to synchronization, so it should *not* be used 
as such. On the contrary, atomics are properly respected by compilers.

Comment 26 by jfb@chromium.org, Mar 20 2015

@kcc: volatile means it can be externally modified.

My thinking with atomic is that using LTO I can get the compiler to see all the uses of the buffer, and see that the result is written but never read. I'm pretty sure that allows the compiler to do as it wishes with the store.

Comment 27 by kcc@chromium.org, Mar 20 2015

>> @kcc: volatile means it can be externally modified.
Correct. So, for something that is part of shared memory it's probably a good idea.

Comment 28 by jfb@chromium.org, Mar 20 2015

Just to document the validity of using volatile here:

  http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2006/n2016.html

  volatile may be used when variables may be "externally modified", but the modification in fact is triggered synchronously by the thread itself, e.g. because the underlying memory is mapped at multiple locations.

Note that atomics can also be marked as volatile.

Comment 29 by vmi...@chromium.org, Mar 20 2015

@jfb: Good points.  This case seems particularly fragile to optimizations treating result->size and result_size as common expressions.

There are many places where we are dealing with shared memory pointers that are not marked volatile.  E.g. every GLES2 command handler has "const void* cmd_data" pointer to shared memory, which is cast to "const Struct&" e.g. https://code.google.com/p/chromium/codesearch#chromium/src/gpu/command_buffer/service/gles2_cmd_decoder.cc&q=handledraweleme&sq=package:chromium&l=7058

Should I update this patch then, and leave other cases for an audit?

Comment 30 by jfb@chromium.org, Mar 20 2015

@vmiura: yes, I would patch this and leave the other code to be audited. I file 469215 for this.

Comment 31 by jfb@chromium.org, Mar 20 2015

@vmiura: actually hold that patch. I'll fix all of the shared memory volatile issues at the same time instead. It's better for all the fixes to be the same, and to avoid disclosing this issue in this area but leaving others open.

Comment 32 by vmi...@chromium.org, Mar 20 2015

Labels: Merge-Requested
@jfb: Sounds good.

Then, merge request to M-42.

Comment 33 by amin...@google.com, Mar 21 2015

Labels: -Merge-Requested Merge-Review Hotlist-Merge-Review
[Automated comment] Request affecting a post-stable build (M41), manual review required.

Comment 34 by amin...@google.com, Mar 21 2015

Labels: Merge-Approved Hotlist-Merge-Approved
Approved for M42 (branch: 2311)

Comment 35 by amineer@chromium.org, Mar 23 2015

Labels: OS-All

Comment 36 by amineer@chromium.org, Mar 23 2015

Labels: -Merge-Review

Comment 37 by timwillis@google.com, Mar 23 2015

@vmiura - please merge to M42 (branch 2311).

Comment 38 by timwillis@google.com, Mar 24 2015

wfh is going to merge this shortly.

Comment 39 by bugdroid1@chromium.org, Mar 24 2015

Project Member
Labels: -Merge-Approved merge-merged-2311
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/eff0c488e38e0fbfb43145fb86bf39c814496a95

commit eff0c488e38e0fbfb43145fb86bf39c814496a95
Author: Will Harris <wfh@chromium.org>
Date: Tue Mar 24 22:09:22 2015

Merge: gpu: Use GetUniformSetup computed result size.

R=piman@chromium.org
BUG= 468936 

Review URL: https://codereview.chromium.org/1016193003

Cr-Commit-Position: refs/heads/master@{#321489}
(cherry picked from commit 181c7400b2bf50ba02ac77149749fb419b4d4797)

Review URL: https://codereview.chromium.org/1021803003

Cr-Commit-Position: refs/branch-heads/2311@{#345}
Cr-Branched-From: 09b7de5dd7254947cd4306de907274fa63373d48-refs/heads/master@{#317474}

[modify] http://crrev.com/eff0c488e38e0fbfb43145fb86bf39c814496a95/gpu/command_buffer/service/gles2_cmd_decoder.cc

Comment 40 by vmi...@chromium.org, Mar 24 2015

Sorry about the delay, wfh@ thanks for doing the merge.

Comment 41 by timwillis@google.com, Mar 25 2015

Labels: Merge-Requested
Merge-Requested to M41

Comment 42 by amin...@google.com, Mar 25 2015

Labels: -Merge-Requested Merge-Review
[Automated comment] Request affecting a post-stable build (M41), manual review required.

Comment 43 by pennymac@google.com, Mar 25 2015

Labels: -Hotlist-Merge-Review -Merge-Review Merge-Approved
Merge approved for M41 branch 2272.

Comment 44 by timwillis@google.com, Mar 25 2015

Cc: wfh@chromium.org
@wfh - would you be so kind and please perform the merge to M41 (branch 2272)?

Comment 45 by bugdroid1@chromium.org, Mar 25 2015

Project Member
Labels: -Merge-Approved merge-merged-2272
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fa0ba4065d7fb494c515ae507e1acdee22e6cb08

commit fa0ba4065d7fb494c515ae507e1acdee22e6cb08
Author: Will Harris <wfh@chromium.org>
Date: Wed Mar 25 23:37:03 2015

Merge: gpu: Use GetUniformSetup computed result size.

R=piman@chromium.org
BUG= 468936 

Review URL: https://codereview.chromium.org/1016193003

Cr-Commit-Position: refs/heads/master@{#321489}
(cherry picked from commit 181c7400b2bf50ba02ac77149749fb419b4d4797)

Review URL: https://codereview.chromium.org/1034093002

Cr-Commit-Position: refs/branch-heads/2272@{#451}
Cr-Branched-From: 827a380cfdb31aa54c8d56e63ce2c3fd8c3ba4d4-refs/heads/master@{#310958}

[modify] http://crrev.com/fa0ba4065d7fb494c515ae507e1acdee22e6cb08/gpu/command_buffer/service/gles2_cmd_decoder.cc

Comment 46 by timwillis@google.com, Mar 27 2015

Labels: Release-2-M41

Comment 47 by timwillis@google.com, Mar 30 2015

Cc: zdi.disc...@gmail.com
Labels: CVE-2015-1234
@ZDI - can you please let me know what the ZDI-CAN is for this issue? 

The CVE reference is CVE-2015-1234 (yes, I know that sounds fake, but it's real).

Comment 48 by timwillis@google.com, Mar 30 2015

...aaaaanndd it's on the original attachment. Nevermind.

ZDI-CAN-2833

Comment 49 Deleted

Comment 50 by timwillis@google.com, Apr 8 2015

Labels: -Merge-Triage

Comment 51 by ClusterFuzz, Jun 26 2015

Project Member
Labels: -Restrict-View-SecurityTeam
Bulk update: removing view restriction from closed bugs.

Comment 52 by sheriffbot@chromium.org, Oct 1 2016

Project Member
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

Comment 53 by sheriffbot@chromium.org, Oct 1 2016

Project Member
Labels: Restrict-View-SecurityNotify

Comment 54 by sheriffbot@chromium.org, Oct 2 2016

Project Member
Labels: -Restrict-View-SecurityNotify
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

Comment 55 by mbarbe...@chromium.org, Oct 2 2016

Labels: allpublic

Comment 56 by awhalley@chromium.org, Apr 25 2018

Labels: CVE_description-submitted

Sign in to add a comment