Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(599)

Issue 5580002: Mac: Tell the GPU sandbox to deny a few things. (Closed)

Created:
10 years ago by Nico
Modified:
9 years, 7 months ago
CC:
chromium-reviews, Paweł Hajdan Jr., pam+watch_chromium.org, ben+cc_chromium.org
Visibility:
Public.

Description

Mac: Tell the GPU sandbox to deny a few things. It's now no longer allowed to do network requests and can't access most files. Here are the stacks that it prints if I patch in http://codereview.chromium.org/1765005/show: http://codepad.org/6zrJfnlB BUG=48607 TEST=GPU stuff still works. When you run with --enable-sandbox-logging, quite a bunch of stuff is logged as "denied". Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=68321

Patch Set 1 #

Patch Set 2 : unit test compiles and passes #

Patch Set 3 : more restrictive #

Patch Set 4 : remove debug code #

Patch Set 5 : '' #

Patch Set 6 : works in release #

Total comments: 14

Patch Set 7 : comments #

Patch Set 8 : browser tests pass #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+61 lines, -16 lines) Patch
M chrome/browser/gpu.sb View 1 2 3 4 5 6 7 1 chunk +11 lines, -2 lines 1 comment Download
M chrome/common/chrome_switches.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/common/sandbox_init_wrapper_mac.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/common/sandbox_mac.h View 1 2 3 4 5 6 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/common/sandbox_mac.mm View 1 2 3 4 5 6 7 3 chunks +45 lines, -3 lines 0 comments Download
M chrome/common/sandbox_mac_unittest_helper.mm View 1 2 chunks +1 line, -7 lines 0 comments Download

Messages

Total messages: 11 (0 generated)
Nico
jeremy: please review kbr: fyi
10 years ago (2010-12-03 03:00:23 UTC) #1
Nico
Hrm, has issues in release builds. Please hold off from reviewing for now.
10 years ago (2010-12-03 03:47:57 UTC) #2
Nico
Note to self: In Release builds, it dies here: #0 0x93abaef6 in __kill () #1 ...
10 years ago (2010-12-03 04:01:17 UTC) #3
Nico
Now it works in release mode, too. There are still issues with a few tests ...
10 years ago (2010-12-03 04:07:02 UTC) #4
jeremy
This generally looks great, hope we can get this landed quickly. Did you test this ...
10 years ago (2010-12-03 14:22:09 UTC) #5
Ken Russell (switch to Gerrit)
On 2010/12/03 14:22:09, jeremy wrote: > Did you test this on 10.5 as well or ...
10 years ago (2010-12-03 18:59:12 UTC) #6
Nico
Thanks for the comments. I haven't tested on 10.5 yet, but will do so once ...
10 years ago (2010-12-04 00:26:10 UTC) #7
Nico
http://codereview.chromium.org/5580002/diff/12001/chrome/browser/gpu.sb File chrome/browser/gpu.sb (right): http://codereview.chromium.org/5580002/diff/12001/chrome/browser/gpu.sb#newcode11 chrome/browser/gpu.sb:11: (allow file-read* file-write* (regex "^/(private/)?(tmp|var)(/|$)")) On 2010/12/04 00:26:10, Nico ...
10 years ago (2010-12-04 01:58:36 UTC) #8
Nico
I tested a few webgl demos on 10.5, and it works just as well as ...
10 years ago (2010-12-04 02:42:54 UTC) #9
Ken Russell (switch to Gerrit)
On 2010/12/04 02:42:54, Nico wrote: > I tested a few webgl demos on 10.5, and ...
10 years ago (2010-12-04 02:47:57 UTC) #10
jeremy
10 years ago (2010-12-05 08:04:50 UTC) #11
LGTM with the caveat that I think we should remove the shmem hole before calling
this done.

Also, you can remove the mach lookup hole if it only prints a warning.

http://codereview.chromium.org/5580002/diff/12001/chrome/browser/gpu.sb
File chrome/browser/gpu.sb (right):

http://codereview.chromium.org/5580002/diff/12001/chrome/browser/gpu.sb#newco...
chrome/browser/gpu.sb:15: ; That's probably harmless, but allowing the look-up
is also probably harmless.
Are you sure you need this?  Does this cause things to not work, or just print a
warning?

Apologies for pressing the point, just wanted to make sure this is as minimal as
possible.

http://codereview.chromium.org/5580002/diff/25001/chrome/browser/gpu.sb#newco...
chrome/browser/gpu.sb:13: ; Without this, the GPU process prints
I think giving the GPU process full access to /tmp and /var may be a pretty big
security hole.

I'm OK with you landing this in the interim, but I think we should plumb the
shared memory creation through the browser process and remove this line asap.

Could you reword the commend to this effect?

Powered by Google App Engine
This is Rietveld 408576698