Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(46)

Issue 810083002: Added a new process mitigation to harden process token IL policy. (Closed)

Created:
5 years, 11 months ago by forshaw
Modified:
5 years, 10 months ago
CC:
chromium-reviews, darin-cc_chromium.org, jam, wfh+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Added a new process mitigation to harden process token IL policy. This adds a new process mitigation policy to harden the current process token's integrity level policy. What this actually means is the token's IL policy in its SACL is modified to add no-read-up and no-execute-up which is not the default. This prevents a lower privilege process from opening the token object with rights such as duplicate and impersonation which could be used to circumvent sandbox restrictions and elevate privileges. While the policy is only enabled on the browser process by making it a general mitigation policy it could be applied to all process levels such as the GPU process to provide a similar effect. BUG=440692 Committed: https://crrev.com/19669894e93f9279e860d9fff6a54f6cd042acd7 Cr-Commit-Position: refs/heads/master@{#313099}

Patch Set 1 #

Total comments: 10

Patch Set 2 : Changed earliest version to Windows 7 for policy, fixed minor style issues. #

Total comments: 2

Patch Set 3 : Changed comments to correctly indicate Windows 7 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+93 lines, -2 lines) Patch
M content/app/startup_helper_win.cc View 1 chunk +2 lines, -1 line 0 comments Download
M sandbox/win/src/process_mitigations.cc View 1 3 chunks +10 lines, -1 line 0 comments Download
M sandbox/win/src/restricted_token_utils.h View 1 2 1 chunk +13 lines, -0 lines 0 comments Download
M sandbox/win/src/restricted_token_utils.cc View 1 1 chunk +63 lines, -0 lines 0 comments Download
M sandbox/win/src/security_level.h View 1 chunk +5 lines, -0 lines 0 comments Download

Messages

Total messages: 21 (4 generated)
forshaw
Please can you review this CL.
5 years, 11 months ago (2014-12-17 17:59:03 UTC) #2
cpu_(ooo_6.6-7.5)
first question. This has the chance to break something subtle, justin submarine style. Do we ...
5 years, 11 months ago (2014-12-17 22:35:38 UTC) #3
forshaw
Well it shouldn't affect anything in theory. You shouldn't be able to access the token ...
5 years, 11 months ago (2014-12-17 22:39:05 UTC) #4
cpu_(ooo_6.6-7.5)
https://codereview.chromium.org/810083002/diff/1/sandbox/win/src/process_mitigations.cc File sandbox/win/src/process_mitigations.cc (right): https://codereview.chromium.org/810083002/diff/1/sandbox/win/src/process_mitigations.cc#newcode63 sandbox/win/src/process_mitigations.cc:63: if (version >= base::win::VERSION_VISTA && if you don't mind ...
5 years, 11 months ago (2014-12-18 21:05:36 UTC) #5
forshaw
If you can just have another quick look it would be appreciated. https://codereview.chromium.org/810083002/diff/1/sandbox/win/src/process_mitigations.cc File sandbox/win/src/process_mitigations.cc ...
5 years, 11 months ago (2014-12-19 08:30:00 UTC) #6
jschuh
Sorry, didn't realize anyone was waiting on me. lgtm
5 years, 11 months ago (2014-12-20 01:01:49 UTC) #7
Will Harris
On 2014/12/20 01:01:49, jschuh wrote: > Sorry, didn't realize anyone was waiting on me. lgtm ...
5 years, 11 months ago (2014-12-20 01:07:23 UTC) #8
cpu_(ooo_6.6-7.5)
lgtm with two nits. "best submarine is justin's submarine" https://codereview.chromium.org/810083002/diff/20001/sandbox/win/src/restricted_token_utils.h File sandbox/win/src/restricted_token_utils.h (right): https://codereview.chromium.org/810083002/diff/20001/sandbox/win/src/restricted_token_utils.h#newcode85 sandbox/win/src/restricted_token_utils.h:85: ...
5 years, 11 months ago (2014-12-20 01:10:05 UTC) #9
jschuh
I just noticed this is still sitting. Any reason it didn't land?
5 years, 10 months ago (2015-01-22 04:33:10 UTC) #10
Will Harris
my gift is some free try jobs.
5 years, 10 months ago (2015-01-22 04:53:31 UTC) #11
forshaw
I think it was just there's one file which needs owner approval from agl and ...
5 years, 10 months ago (2015-01-22 06:46:35 UTC) #12
jschuh
It's just a flag change in that file. And I'm happy to nag agl@ to ...
5 years, 10 months ago (2015-01-22 19:55:07 UTC) #13
Will Harris
On 2015/01/22 19:55:07, jschuh wrote: > It's just a flag change in that file. And ...
5 years, 10 months ago (2015-01-26 18:12:39 UTC) #14
agl
LGTM Sorry, if I haven't responded within 1 business day then I've missed the email ...
5 years, 10 months ago (2015-01-26 18:24:06 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/810083002/40001
5 years, 10 months ago (2015-01-26 18:28:54 UTC) #19
commit-bot: I haz the power
Committed patchset #3 (id:40001)
5 years, 10 months ago (2015-01-26 19:28:01 UTC) #20
commit-bot: I haz the power
5 years, 10 months ago (2015-01-26 19:29:13 UTC) #21
Message was sent while issue was closed.
Patchset 3 (id:??) landed as
https://crrev.com/19669894e93f9279e860d9fff6a54f6cd042acd7
Cr-Commit-Position: refs/heads/master@{#313099}

Powered by Google App Engine
This is Rietveld 408576698