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

Issue 1496093002: Add a restricted ACL to installer temp directory (Closed)

Created:
5 years ago by jschuh
Modified:
5 years ago
CC:
chromium-reviews, grt+watch_chromium.org, 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

Add a restricted ACL to installer temp directory This will prevent any sort of file squatting. BUG=565543 Committed: https://crrev.com/68e66bfceb698fefd4116e014365d23698291083 Cr-Commit-Position: refs/heads/master@{#365408}

Patch Set 1 #

Total comments: 8

Patch Set 2 : fixes #

Patch Set 3 : git cl format #

Patch Set 4 : nit #

Total comments: 9

Patch Set 5 : owner sid and nits #

Total comments: 8

Patch Set 6 : TokenOwner #

Total comments: 25

Patch Set 7 : feedback #

Patch Set 8 : trailing semicolons in sddl #

Total comments: 4

Patch Set 9 : feedback #

Total comments: 4

Patch Set 10 : feedback #

Total comments: 2

Patch Set 11 : nit #

Unified diffs Side-by-side diffs Delta from patch set Stats (+112 lines, -21 lines) Patch
M chrome/installer/mini_installer/exit_code.h View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
M chrome/installer/mini_installer/mini_installer.cc View 1 2 3 4 5 6 7 8 9 10 7 chunks +111 lines, -21 lines 0 comments Download

Messages

Total messages: 34 (8 generated)
jschuh
I need to run this through the bots, since I don't think my local test ...
5 years ago (2015-12-03 20:47:00 UTC) #2
grt (UTC plus 2)
looks good overall. it's worth testing w/ a component=static_library build to be sure rand_s doesn't ...
5 years ago (2015-12-03 21:38:33 UTC) #3
grt (UTC plus 2)
looks like the installer failed with STATUS_ACCESS_VIOLATION on the trybot. it'd be nice to have ...
5 years ago (2015-12-03 21:41:18 UTC) #4
jschuh
I tested manually and worked out at least one problem. So, maybe it's good. https://codereview.chromium.org/1496093002/diff/1/chrome/installer/mini_installer/mini_installer.cc ...
5 years ago (2015-12-04 00:40:28 UTC) #5
jschuh
+forshaw@, in case he wants to check my sddl.
5 years ago (2015-12-04 02:37:30 UTC) #7
forshaw
On 2015/12/04 02:37:30, jschuh (very slow) wrote: > +forshaw@, in case he wants to check ...
5 years ago (2015-12-04 12:09:02 UTC) #8
grt (UTC plus 2)
https://codereview.chromium.org/1496093002/diff/60001/chrome/installer/mini_installer/mini_installer.cc File chrome/installer/mini_installer/mini_installer.cc (right): https://codereview.chromium.org/1496093002/diff/60001/chrome/installer/mini_installer/mini_installer.cc#newcode579 chrome/installer/mini_installer/mini_installer.cc:579: wchar_t volume[MAX_PATH]; use PathString here rather than a wchar_t ...
5 years ago (2015-12-04 15:37:35 UTC) #9
grt (UTC plus 2)
https://codereview.chromium.org/1496093002/diff/60001/chrome/installer/mini_installer/mini_installer.cc File chrome/installer/mini_installer/mini_installer.cc (right): https://codereview.chromium.org/1496093002/diff/60001/chrome/installer/mini_installer/mini_installer.cc#newcode561 chrome/installer/mini_installer/mini_installer.cc:561: bool CreateWorkDir(const wchar_t* base_path, PathString* work_dir) { regarding testing, ...
5 years ago (2015-12-04 16:51:44 UTC) #10
jschuh
Getting the owner SID without RAII cleanup is a massive pain, so I broke it ...
5 years ago (2015-12-05 01:04:39 UTC) #11
forshaw
https://codereview.chromium.org/1496093002/diff/80001/chrome/installer/mini_installer/mini_installer.cc File chrome/installer/mini_installer/mini_installer.cc (right): https://codereview.chromium.org/1496093002/diff/80001/chrome/installer/mini_installer/mini_installer.cc#newcode571 chrome/installer/mini_installer/mini_installer.cc:571: ::GetTokenInformation(token, TokenUser, &user, 0, &size); Technically speaking there's a ...
5 years ago (2015-12-05 10:26:32 UTC) #12
jschuh
https://codereview.chromium.org/1496093002/diff/80001/chrome/installer/mini_installer/mini_installer.cc File chrome/installer/mini_installer/mini_installer.cc (right): https://codereview.chromium.org/1496093002/diff/80001/chrome/installer/mini_installer/mini_installer.cc#newcode571 chrome/installer/mini_installer/mini_installer.cc:571: ::GetTokenInformation(token, TokenUser, &user, 0, &size); On 2015/12/05 10:26:31, forshaw ...
5 years ago (2015-12-05 18:37:38 UTC) #13
jschuh
https://codereview.chromium.org/1496093002/diff/80001/chrome/installer/mini_installer/mini_installer.cc File chrome/installer/mini_installer/mini_installer.cc (right): https://codereview.chromium.org/1496093002/diff/80001/chrome/installer/mini_installer/mini_installer.cc#newcode571 chrome/installer/mini_installer/mini_installer.cc:571: ::GetTokenInformation(token, TokenUser, &user, 0, &size); On 2015/12/05 18:37:38, jschuh ...
5 years ago (2015-12-05 18:49:14 UTC) #14
forshaw
On 2015/12/05 18:49:14, jschuh (very slow) wrote: > https://codereview.chromium.org/1496093002/diff/80001/chrome/installer/mini_installer/mini_installer.cc > File chrome/installer/mini_installer/mini_installer.cc (right): > > ...
5 years ago (2015-12-07 13:50:19 UTC) #15
grt (UTC plus 2)
https://codereview.chromium.org/1496093002/diff/60001/chrome/installer/mini_installer/mini_installer.cc File chrome/installer/mini_installer/mini_installer.cc (right): https://codereview.chromium.org/1496093002/diff/60001/chrome/installer/mini_installer/mini_installer.cc#newcode561 chrome/installer/mini_installer/mini_installer.cc:561: bool CreateWorkDir(const wchar_t* base_path, PathString* work_dir) { On 2015/12/05 ...
5 years ago (2015-12-07 14:56:23 UTC) #16
jschuh
And, hopefully this is the last round. https://codereview.chromium.org/1496093002/diff/80001/chrome/installer/mini_installer/mini_installer.cc File chrome/installer/mini_installer/mini_installer.cc (right): https://codereview.chromium.org/1496093002/diff/80001/chrome/installer/mini_installer/mini_installer.cc#newcode571 chrome/installer/mini_installer/mini_installer.cc:571: ::GetTokenInformation(token, TokenUser, ...
5 years ago (2015-12-11 04:07:24 UTC) #18
jschuh
And now it works locally but fails on the bot. Awesome.
5 years ago (2015-12-11 17:00:45 UTC) #19
jschuh
On 2015/12/11 17:00:45, jschuh (very slow) wrote: > And now it works locally but fails ...
5 years ago (2015-12-11 19:38:46 UTC) #20
jschuh
Just to confirm, it works fine locally and on the bots now. The bad run ...
5 years ago (2015-12-14 17:59:45 UTC) #21
grt (UTC plus 2)
https://codereview.chromium.org/1496093002/diff/100001/chrome/installer/mini_installer/mini_installer.cc File chrome/installer/mini_installer/mini_installer.cc (right): https://codereview.chromium.org/1496093002/diff/100001/chrome/installer/mini_installer/mini_installer.cc#newcode598 chrome/installer/mini_installer/mini_installer.cc:598: bool result = sddl.append(L"D:PAI(A;ID;FA;;;BA)" // Admin: Full control. On ...
5 years ago (2015-12-14 18:59:26 UTC) #22
grt (UTC plus 2)
https://codereview.chromium.org/1496093002/diff/190001/chrome/installer/mini_installer/mini_installer.cc File chrome/installer/mini_installer/mini_installer.cc (right): https://codereview.chromium.org/1496093002/diff/190001/chrome/installer/mini_installer/mini_installer.cc#newcode679 chrome/installer/mini_installer/mini_installer.cc:679: ++id; // Try a different name. wow, i just ...
5 years ago (2015-12-15 20:08:10 UTC) #23
jschuh
https://codereview.chromium.org/1496093002/diff/100001/chrome/installer/mini_installer/mini_installer.cc File chrome/installer/mini_installer/mini_installer.cc (right): https://codereview.chromium.org/1496093002/diff/100001/chrome/installer/mini_installer/mini_installer.cc#newcode598 chrome/installer/mini_installer/mini_installer.cc:598: bool result = sddl.append(L"D:PAI(A;ID;FA;;;BA)" // Admin: Full control. On ...
5 years ago (2015-12-15 20:58:59 UTC) #26
grt (UTC plus 2)
lgtm w/ a nit https://codereview.chromium.org/1496093002/diff/230001/chrome/installer/mini_installer/mini_installer.cc File chrome/installer/mini_installer/mini_installer.cc (right): https://codereview.chromium.org/1496093002/diff/230001/chrome/installer/mini_installer/mini_installer.cc#newcode653 chrome/installer/mini_installer/mini_installer.cc:653: ::RtlGenRandom(&id, sizeof(id)); nit: move these ...
5 years ago (2015-12-15 21:12:16 UTC) #27
jschuh
https://codereview.chromium.org/1496093002/diff/230001/chrome/installer/mini_installer/mini_installer.cc File chrome/installer/mini_installer/mini_installer.cc (right): https://codereview.chromium.org/1496093002/diff/230001/chrome/installer/mini_installer/mini_installer.cc#newcode653 chrome/installer/mini_installer/mini_installer.cc:653: ::RtlGenRandom(&id, sizeof(id)); On 2015/12/15 21:12:16, grt wrote: > nit: ...
5 years ago (2015-12-15 23:25:25 UTC) #30
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1496093002/250001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1496093002/250001
5 years ago (2015-12-15 23:27:46 UTC) #31
commit-bot: I haz the power
Committed patchset #11 (id:250001)
5 years ago (2015-12-16 00:48:46 UTC) #32
commit-bot: I haz the power
5 years ago (2015-12-16 00:49:55 UTC) #34
Message was sent while issue was closed.
Patchset 11 (id:??) landed as
https://crrev.com/68e66bfceb698fefd4116e014365d23698291083
Cr-Commit-Position: refs/heads/master@{#365408}

Powered by Google App Engine
This is Rietveld 408576698