Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1496093002:
Add a restricted ACL to installer temp directory (Closed)
DescriptionAdd 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 #
Messages
Total messages: 34 (8 generated)
jschuh@chromium.org changed reviewers: + grt@chromium.org
I need to run this through the bots, since I don't think my local test covered it, but it should be pretty straightforward.
looks good overall. it's worth testing w/ a component=static_library build to be sure rand_s doesn't pose a problem with /NODEFAULTLIB. after landing, please watch for a rise in UNABLE_TO_GET_WORK_DIRECTORY (109) errors on canary (for per-user) and dev (for per-machine) installs. https://codereview.chromium.org/1496093002/diff/1/chrome/installer/mini_insta... File chrome/installer/mini_installer/mini_installer.cc (right): https://codereview.chromium.org/1496093002/diff/1/chrome/installer/mini_insta... chrome/installer/mini_installer/mini_installer.cc:568: SECURITY_ATTRIBUTES sa = {0}; nit: i prefer {} since {0} doesn't mean "initialize everything to zero" like it looks. i won't force you to change it, though. https://codereview.chromium.org/1496093002/diff/1/chrome/installer/mini_insta... chrome/installer/mini_installer/mini_installer.cc:583: return false; how about falling through to the previous case if this unexpectedly fails? https://codereview.chromium.org/1496093002/diff/1/chrome/installer/mini_insta... chrome/installer/mini_installer/mini_installer.cc:589: rand_s(&id); we link with /NODEFAULTLIB. will this work? https://codereview.chromium.org/1496093002/diff/1/chrome/installer/mini_insta... chrome/installer/mini_installer/mini_installer.cc:594: if (!HexEncode(&id, 3, work_dir->get() + end, work_dir->capacity() - end)) { nit: omit braces
looks like the installer failed with STATUS_ACCESS_VIOLATION on the trybot. it'd be nice to have stack traces there, huh?
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_insta... File chrome/installer/mini_installer/mini_installer.cc (right): https://codereview.chromium.org/1496093002/diff/1/chrome/installer/mini_insta... chrome/installer/mini_installer/mini_installer.cc:568: SECURITY_ATTRIBUTES sa = {0}; On 2015/12/03 21:38:32, grt wrote: > nit: i prefer {} since {0} doesn't mean "initialize everything to zero" like it > looks. i won't force you to change it, though. Ah, good point. https://codereview.chromium.org/1496093002/diff/1/chrome/installer/mini_insta... chrome/installer/mini_installer/mini_installer.cc:583: return false; On 2015/12/03 21:38:32, grt wrote: > how about falling through to the previous case if this unexpectedly fails? This is one of those must-never-fail sort of things, so I'd prefer to keep it. https://codereview.chromium.org/1496093002/diff/1/chrome/installer/mini_insta... chrome/installer/mini_installer/mini_installer.cc:589: rand_s(&id); On 2015/12/03 21:38:32, grt wrote: > we link with /NODEFAULTLIB. will this work? Switched it to call RtlGenRandom() directly. https://codereview.chromium.org/1496093002/diff/1/chrome/installer/mini_insta... chrome/installer/mini_installer/mini_installer.cc:594: if (!HexEncode(&id, 3, work_dir->get() + end, work_dir->capacity() - end)) { On 2015/12/03 21:38:32, grt wrote: > nit: omit braces Switched it back to sizeof.
jschuh@chromium.org changed reviewers: + forshaw@chromium.org
+forshaw@, in case he wants to check my sddl.
On 2015/12/04 02:37:30, jschuh (very slow) wrote: > +forshaw@, in case he wants to check my sddl. So CREATOR OWNER doesn't do what you expect when you set it as an explicit ACE entry, it's only treated specially when calculating an inherited ACE. So your SDDL string in this case would set an explicit ACE for the CREATOR OWNER SID rather than giving access to the owner as you'd expect. This means if this runs as a normal user it would presumably cause problems as that user wouldn't be able to create new directories or files under the temp directory. You'll need to pull the owner SID manually from the current token using GetTokenInformation(TokenOwner) and plug it in the SDDL string. Also if I'm picking nits you don't need to specify the ID (inherited) ACE flag as it's not being inherited from anything, but it doesn't make much of a difference except to the security viewer dialog.
https://codereview.chromium.org/1496093002/diff/60001/chrome/installer/mini_i... File chrome/installer/mini_installer/mini_installer.cc (right): https://codereview.chromium.org/1496093002/diff/60001/chrome/installer/mini_i... chrome/installer/mini_installer/mini_installer.cc:579: wchar_t volume[MAX_PATH]; use PathString here rather than a wchar_t array: PathString volume; DWORD flags = 0; if (::GetVolumePathName(base_path, volume.capacity(), volume.get()) && ::GetVolumeInformation(volume.get(), ...)) (and prefix win32 calls with :: since that seems to be the style of the file) https://codereview.chromium.org/1496093002/diff/60001/chrome/installer/mini_i... chrome/installer/mini_installer/mini_installer.cc:582: GetVolumeInformation(volume, nullptr, 0, nullptr, nullptr, &flags, sadly, we can't use C++-11 yet in mini_installer since (last i checked) the signing bots are using an old toolchain. for now, use NULL everywhere rather than nullptr. mmoss is working on resolving this. https://codereview.chromium.org/1496093002/diff/60001/chrome/installer/mini_i... chrome/installer/mini_installer/mini_installer.cc:593: sddl, SDDL_REVISION, &sa.lpSecurityDescriptor, NULL)) { is it safer to explicitly use SDDL_REVISION_1? MSDN says StringSDRevision must be that value. i see that sddl.h defines SDDL_REVISION to SDDL_REVISION_1 for now, but would we necessarily want to pick up a change in a future version of the SDK automatically?
https://codereview.chromium.org/1496093002/diff/60001/chrome/installer/mini_i... File chrome/installer/mini_installer/mini_installer.cc (right): https://codereview.chromium.org/1496093002/diff/60001/chrome/installer/mini_i... chrome/installer/mini_installer/mini_installer.cc:561: bool CreateWorkDir(const wchar_t* base_path, PathString* work_dir) { regarding testing, i think it's appropriate to move this utility function into a new .{cc,h} pair (mini_installer_util?) and create a corresponding _unittest.cc file. these new sources and the test can then be linked into setup_unittests.exe. be warned that you'll have to land a change to the signing script that adds the new files to |extra_files|, |input_headers|, and |input_files| so that the official build doesn't tip over.
Getting the owner SID without RAII cleanup is a massive pain, so I broke it out into a number of helper functions. The rest of the responses are inline, and I'm open to ignoring the failure to set the ACL. https://codereview.chromium.org/1496093002/diff/60001/chrome/installer/mini_i... File chrome/installer/mini_installer/mini_installer.cc (right): https://codereview.chromium.org/1496093002/diff/60001/chrome/installer/mini_i... chrome/installer/mini_installer/mini_installer.cc:561: bool CreateWorkDir(const wchar_t* base_path, PathString* work_dir) { On 2015/12/04 16:51:43, grt wrote: > regarding testing, i think it's appropriate to move this utility function into a > new .{cc,h} pair (mini_installer_util?) and create a corresponding _unittest.cc > file. these new sources and the test can then be linked into > setup_unittests.exe. be warned that you'll have to land a change to the signing > script that adds the new files to |extra_files|, |input_headers|, and > |input_files| so that the official build doesn't tip over. In principle I agree. In practice, this was how I spent my one day of coding for the next few weeks, so it's going to be quite a while before I can get back to it and do that. How about I file a bug on it and promise to get it taken care of? https://codereview.chromium.org/1496093002/diff/60001/chrome/installer/mini_i... chrome/installer/mini_installer/mini_installer.cc:579: wchar_t volume[MAX_PATH]; On 2015/12/04 15:37:35, grt wrote: > use PathString here rather than a wchar_t array: > PathString volume; > DWORD flags = 0; > if (::GetVolumePathName(base_path, volume.capacity(), volume.get()) && > ::GetVolumeInformation(volume.get(), ...)) > > (and prefix win32 calls with :: since that seems to be the style of the file) Done. https://codereview.chromium.org/1496093002/diff/60001/chrome/installer/mini_i... chrome/installer/mini_installer/mini_installer.cc:582: GetVolumeInformation(volume, nullptr, 0, nullptr, nullptr, &flags, On 2015/12/04 15:37:35, grt wrote: > sadly, we can't use C++-11 yet in mini_installer since (last i checked) the > signing bots are using an old toolchain. for now, use NULL everywhere rather > than nullptr. mmoss is working on resolving this. Done. https://codereview.chromium.org/1496093002/diff/60001/chrome/installer/mini_i... chrome/installer/mini_installer/mini_installer.cc:593: sddl, SDDL_REVISION, &sa.lpSecurityDescriptor, NULL)) { On 2015/12/04 15:37:35, grt wrote: > is it safer to explicitly use SDDL_REVISION_1? MSDN says StringSDRevision must > be that value. i see that sddl.h defines SDDL_REVISION to SDDL_REVISION_1 for > now, but would we necessarily want to pick up a change in a future version of > the SDK automatically? Done. https://codereview.chromium.org/1496093002/diff/80001/chrome/installer/mini_i... File chrome/installer/mini_installer/mini_installer.cc (right): https://codereview.chromium.org/1496093002/diff/80001/chrome/installer/mini_i... chrome/installer/mini_installer/mini_installer.cc:642: return false; @grt - I can make this continue on failure if you find it preferable.
https://codereview.chromium.org/1496093002/diff/80001/chrome/installer/mini_i... File chrome/installer/mini_installer/mini_installer.cc (right): https://codereview.chromium.org/1496093002/diff/80001/chrome/installer/mini_i... chrome/installer/mini_installer/mini_installer.cc:571: ::GetTokenInformation(token, TokenUser, &user, 0, &size); Technically speaking there's a distinction between Owner and User from a DACL perspective. However it'll probably only be an issue in UAC scenarios, where normal user is their own SID but elevated is admin group SID. As this directory is ephemeral anyway and we probably don't care too much about UAC issues no reason to change.
https://codereview.chromium.org/1496093002/diff/80001/chrome/installer/mini_i... File chrome/installer/mini_installer/mini_installer.cc (right): https://codereview.chromium.org/1496093002/diff/80001/chrome/installer/mini_i... chrome/installer/mini_installer/mini_installer.cc:571: ::GetTokenInformation(token, TokenUser, &user, 0, &size); On 2015/12/05 10:26:31, forshaw wrote: > Technically speaking there's a distinction between Owner and User from a DACL > perspective. However it'll probably only be an issue in UAC scenarios, where > normal user is their own SID but elevated is admin group SID. As this directory > is ephemeral anyway and we probably don't care too much about UAC issues no > reason to change. This is why I said from the beginning that I *hate* the Windows security descriptor APIs. It's like they're intentionally designed to be error prone and painful to use. Accepting that, you did use TokenOwner in your original comment, and it's a trivial switch. So, I'll fix it.
https://codereview.chromium.org/1496093002/diff/80001/chrome/installer/mini_i... File chrome/installer/mini_installer/mini_installer.cc (right): https://codereview.chromium.org/1496093002/diff/80001/chrome/installer/mini_i... chrome/installer/mini_installer/mini_installer.cc:571: ::GetTokenInformation(token, TokenUser, &user, 0, &size); On 2015/12/05 18:37:38, jschuh (very slow) wrote: > On 2015/12/05 10:26:31, forshaw wrote: > > Technically speaking there's a distinction between Owner and User from a DACL > > perspective. However it'll probably only be an issue in UAC scenarios, where > > normal user is their own SID but elevated is admin group SID. As this > directory > > is ephemeral anyway and we probably don't care too much about UAC issues no > > reason to change. > > This is why I said from the beginning that I *hate* the Windows security > descriptor APIs. It's like they're intentionally designed to be error prone and > painful to use. > > Accepting that, you did use TokenOwner in your original comment, and it's a > trivial switch. So, I'll fix it. Just to clarify -- since I've mucked this up a few times already -- using TokenOwner means that under UAC elevation the ACL grants access to the admin but not the normal user, correct? So, it seems worth making that small change to ensure the directory is always restricted to the most privileged context (even if that is redundant in the scenarios we care about).
On 2015/12/05 18:49:14, jschuh (very slow) wrote: > https://codereview.chromium.org/1496093002/diff/80001/chrome/installer/mini_i... > File chrome/installer/mini_installer/mini_installer.cc (right): > > https://codereview.chromium.org/1496093002/diff/80001/chrome/installer/mini_i... > chrome/installer/mini_installer/mini_installer.cc:571: > ::GetTokenInformation(token, TokenUser, &user, 0, &size); > On 2015/12/05 18:37:38, jschuh (very slow) wrote: > > On 2015/12/05 10:26:31, forshaw wrote: > > > Technically speaking there's a distinction between Owner and User from a > DACL > > > perspective. However it'll probably only be an issue in UAC scenarios, where > > > normal user is their own SID but elevated is admin group SID. As this > > directory > > > is ephemeral anyway and we probably don't care too much about UAC issues no > > > reason to change. > > > > This is why I said from the beginning that I *hate* the Windows security > > descriptor APIs. It's like they're intentionally designed to be error prone > and > > painful to use. > > > > Accepting that, you did use TokenOwner in your original comment, and it's a > > trivial switch. So, I'll fix it. > > Just to clarify -- since I've mucked this up a few times already -- using > TokenOwner means that under UAC elevation the ACL grants access to the admin but > not the normal user, correct? So, it seems worth making that small change to > ensure the directory is always restricted to the most privileged context (even > if that is redundant in the scenarios we care about). Yes it means exactly that, new objects will be owned by the administrator group rather that the specific user SID. lgtm.
https://codereview.chromium.org/1496093002/diff/60001/chrome/installer/mini_i... File chrome/installer/mini_installer/mini_installer.cc (right): https://codereview.chromium.org/1496093002/diff/60001/chrome/installer/mini_i... chrome/installer/mini_installer/mini_installer.cc:561: bool CreateWorkDir(const wchar_t* base_path, PathString* work_dir) { On 2015/12/05 01:04:39, jschuh (very slow) wrote: > On 2015/12/04 16:51:43, grt wrote: > > regarding testing, i think it's appropriate to move this utility function into > a > > new .{cc,h} pair (mini_installer_util?) and create a corresponding > _unittest.cc > > file. these new sources and the test can then be linked into > > setup_unittests.exe. be warned that you'll have to land a change to the > signing > > script that adds the new files to |extra_files|, |input_headers|, and > > |input_files| so that the official build doesn't tip over. > > In principle I agree. In practice, this was how I spent my one day of coding for > the next few weeks, so it's going to be quite a while before I can get back to > it and do that. How about I file a bug on it and promise to get it taken care > of? sgtm provided that you satisfy my request below. https://codereview.chromium.org/1496093002/diff/80001/chrome/installer/mini_i... File chrome/installer/mini_installer/mini_installer.cc (right): https://codereview.chromium.org/1496093002/diff/80001/chrome/installer/mini_i... chrome/installer/mini_installer/mini_installer.cc:571: ::GetTokenInformation(token, TokenUser, &user, 0, &size); On 2015/12/05 18:49:14, jschuh (very slow) wrote: > On 2015/12/05 18:37:38, jschuh (very slow) wrote: > > On 2015/12/05 10:26:31, forshaw wrote: > > > Technically speaking there's a distinction between Owner and User from a > DACL > > > perspective. However it'll probably only be an issue in UAC scenarios, where > > > normal user is their own SID but elevated is admin group SID. As this > > directory > > > is ephemeral anyway and we probably don't care too much about UAC issues no > > > reason to change. > > > > This is why I said from the beginning that I *hate* the Windows security > > descriptor APIs. It's like they're intentionally designed to be error prone > and > > painful to use. > > > > Accepting that, you did use TokenOwner in your original comment, and it's a > > trivial switch. So, I'll fix it. > > Just to clarify -- since I've mucked this up a few times already -- using > TokenOwner means that under UAC elevation the ACL grants access to the admin but > not the normal user, correct? So, it seems worth making that small change to > ensure the directory is always restricted to the most privileged context (even > if that is redundant in the scenarios we care about). I'm far from an expert at the NT security model. Be aware that there are three or four ways the installer can be run: 1. as a normal user for per-user installs 2. as an elevated user for per-machine installs 3. as SYSTEM for per-machine installs and updates 4. as whatever msiexec uses (SYSTEM? something else?) when it runs us for per-machine installs https://codereview.chromium.org/1496093002/diff/80001/chrome/installer/mini_i... chrome/installer/mini_installer/mini_installer.cc:642: return false; On 2015/12/05 01:04:39, jschuh (very slow) wrote: > @grt - I can make this continue on failure if you find it preferable. i'm okay with this as-is, but please rejigger so that there's a distinct ExitCode value for "failed to set security descriptor on work dir" so that we can see the extent to which this is happening in the wild. https://codereview.chromium.org/1496093002/diff/100001/chrome/installer/mini_... File chrome/installer/mini_installer/mini_installer.cc (right): https://codereview.chromium.org/1496093002/diff/100001/chrome/installer/mini_... chrome/installer/mini_installer/mini_installer.cc:556: ::GetVolumeInformation(volume.get(), NULL, 0, NULL, NULL, &flags, NULL, did "git cl format" do this indentation? i would expect either six spaces (four more than the previous line) or aligned with ::GetVolumePathName. https://codereview.chromium.org/1496093002/diff/100001/chrome/installer/mini_... chrome/installer/mini_installer/mini_installer.cc:557: 0) || (flags & FILE_PERSISTENT_ACLS); || -> && https://codereview.chromium.org/1496093002/diff/100001/chrome/installer/mini_... chrome/installer/mini_installer/mini_installer.cc:576: if (GetTokenInformation(token, TokenOwner, owner, size, &size)) nit: ::GetTokenInformation https://codereview.chromium.org/1496093002/diff/100001/chrome/installer/mini_... chrome/installer/mini_installer/mini_installer.cc:585: // Sets an ACL allowing only the current owner, admin, and system. suggested comment (assuming it's correct): // Populates |sd| suitable for use when creating directories within |path| with ACLs allowing access to only the current owner, admin, and system. https://codereview.chromium.org/1496093002/diff/100001/chrome/installer/mini_... chrome/installer/mini_installer/mini_installer.cc:587: bool SetSecurityDescriptor(PSECURITY_DESCRIPTOR* sd, const wchar_t* path) { swap sd and path since in params should precede out params https://codereview.chromium.org/1496093002/diff/100001/chrome/installer/mini_... chrome/installer/mini_installer/mini_installer.cc:598: bool result = sddl.append(L"D:PAI(A;ID;FA;;;BA)" // Admin: Full control. nit: break this line just before the open paren so that only the D:PAI is on this line, and all subsequent lines contain the individual ACE strings. then the comment for this line could be "Protected, auto-inherited DACL" https://codereview.chromium.org/1496093002/diff/100001/chrome/installer/mini_... chrome/installer/mini_installer/mini_installer.cc:598: bool result = sddl.append(L"D:PAI(A;ID;FA;;;BA)" // Admin: Full control. i think the "ID" flag should be removed here and below. msdn says it "Indicates that the ACE was inherited. The system sets this bit when it propagates an inherited ACE to a child object." https://codereview.chromium.org/1496093002/diff/100001/chrome/installer/mini_... chrome/installer/mini_installer/mini_installer.cc:598: bool result = sddl.append(L"D:PAI(A;ID;FA;;;BA)" // Admin: Full control. to be utterly pedantic, each of these should have a trailing semicolon to fit the format shown in https://msdn.microsoft.com/library/windows/desktop/aa374928.aspx: "ace_type;ace_flags;rights;object_guid;inherit_object_guid;account_sid;(resource_attribute)" https://codereview.chromium.org/1496093002/diff/100001/chrome/installer/mini_... chrome/installer/mini_installer/mini_installer.cc:599: L"(A;OICIIOID;GA;;;BA)" nit: each of these should be four-space indented https://codereview.chromium.org/1496093002/diff/100001/chrome/installer/mini_... chrome/installer/mini_installer/mini_installer.cc:599: L"(A;OICIIOID;GA;;;BA)" IIUC: OI - files in this directory will inherit the ACE CI - directories in this directory will inherit the ACE IO - the ace does not apply to this directory (only to the contained files and directories) is this what you desire? should the ACE to apply to |path| itself? https://codereview.chromium.org/1496093002/diff/100001/chrome/installer/mini_... chrome/installer/mini_installer/mini_installer.cc:647: RtlGenRandom(&id, sizeof(id)); nit: ::RtlGenRandom
Patchset #7 (id:120001) has been deleted
And, hopefully this is the last round. https://codereview.chromium.org/1496093002/diff/80001/chrome/installer/mini_i... File chrome/installer/mini_installer/mini_installer.cc (right): https://codereview.chromium.org/1496093002/diff/80001/chrome/installer/mini_i... chrome/installer/mini_installer/mini_installer.cc:571: ::GetTokenInformation(token, TokenUser, &user, 0, &size); On 2015/12/07 14:56:22, grt (no reviews until Dec 14) wrote: > On 2015/12/05 18:49:14, jschuh (very slow) wrote: > > On 2015/12/05 18:37:38, jschuh (very slow) wrote: > > > On 2015/12/05 10:26:31, forshaw wrote: > > > > Technically speaking there's a distinction between Owner and User from a > > DACL > > > > perspective. However it'll probably only be an issue in UAC scenarios, > where > > > > normal user is their own SID but elevated is admin group SID. As this > > > directory > > > > is ephemeral anyway and we probably don't care too much about UAC issues > no > > > > reason to change. > > > > > > This is why I said from the beginning that I *hate* the Windows security > > > descriptor APIs. It's like they're intentionally designed to be error prone > > and > > > painful to use. > > > > > > Accepting that, you did use TokenOwner in your original comment, and it's a > > > trivial switch. So, I'll fix it. > > > > Just to clarify -- since I've mucked this up a few times already -- using > > TokenOwner means that under UAC elevation the ACL grants access to the admin > but > > not the normal user, correct? So, it seems worth making that small change to > > ensure the directory is always restricted to the most privileged context (even > > if that is redundant in the scenarios we care about). > > I'm far from an expert at the NT security model. Be aware that there are three > or four ways the installer can be run: > 1. as a normal user for per-user installs > 2. as an elevated user for per-machine installs > 3. as SYSTEM for per-machine installs and updates > 4. as whatever msiexec uses (SYSTEM? something else?) when it runs us for > per-machine installs Yup, we should be good on all cases now. The redundant scenario I was referring to is the normal user case, where this basically duplicates the existing ACL assuming that %TEMP% lives under the user's profile. https://codereview.chromium.org/1496093002/diff/80001/chrome/installer/mini_i... chrome/installer/mini_installer/mini_installer.cc:642: return false; On 2015/12/07 14:56:22, grt (no reviews until Dec 14) wrote: > On 2015/12/05 01:04:39, jschuh (very slow) wrote: > > @grt - I can make this continue on failure if you find it preferable. > > i'm okay with this as-is, but please rejigger so that there's a distinct > ExitCode value for "failed to set security descriptor on work dir" so that we > can see the extent to which this is happening in the wild. Done. https://codereview.chromium.org/1496093002/diff/100001/chrome/installer/mini_... File chrome/installer/mini_installer/mini_installer.cc (right): https://codereview.chromium.org/1496093002/diff/100001/chrome/installer/mini_... chrome/installer/mini_installer/mini_installer.cc:556: ::GetVolumeInformation(volume.get(), NULL, 0, NULL, NULL, &flags, NULL, On 2015/12/07 14:56:23, grt (no reviews until Dec 14) wrote: > did "git cl format" do this indentation? i would expect either six spaces (four > more than the previous line) or aligned with ::GetVolumePathName. VS keeps trying to cleverly reformat things after git-cl-format. And I'm moving between machines often enough that I don't have them all set up correctly. https://codereview.chromium.org/1496093002/diff/100001/chrome/installer/mini_... chrome/installer/mini_installer/mini_installer.cc:557: 0) || (flags & FILE_PERSISTENT_ACLS); On 2015/12/07 14:56:23, grt (no reviews until Dec 14) wrote: > || -> && Done. Forgot to reverse all the conditionals when I moved it out to its own function that. https://codereview.chromium.org/1496093002/diff/100001/chrome/installer/mini_... chrome/installer/mini_installer/mini_installer.cc:576: if (GetTokenInformation(token, TokenOwner, owner, size, &size)) On 2015/12/07 14:56:23, grt (no reviews until Dec 14) wrote: > nit: ::GetTokenInformation Done. https://codereview.chromium.org/1496093002/diff/100001/chrome/installer/mini_... chrome/installer/mini_installer/mini_installer.cc:585: // Sets an ACL allowing only the current owner, admin, and system. On 2015/12/07 14:56:22, grt (no reviews until Dec 14) wrote: > suggested comment (assuming it's correct): > // Populates |sd| suitable for use when creating directories within |path| with > ACLs allowing access to only the current owner, admin, and system. Done. https://codereview.chromium.org/1496093002/diff/100001/chrome/installer/mini_... chrome/installer/mini_installer/mini_installer.cc:587: bool SetSecurityDescriptor(PSECURITY_DESCRIPTOR* sd, const wchar_t* path) { On 2015/12/07 14:56:22, grt (no reviews until Dec 14) wrote: > swap sd and path since in params should precede out params Done. https://codereview.chromium.org/1496093002/diff/100001/chrome/installer/mini_... chrome/installer/mini_installer/mini_installer.cc:598: bool result = sddl.append(L"D:PAI(A;ID;FA;;;BA)" // Admin: Full control. On 2015/12/07 14:56:23, grt (no reviews until Dec 14) wrote: > i think the "ID" flag should be removed here and below. msdn says it "Indicates > that the ACE was inherited. The system sets this bit when it propagates an > inherited ACE to a child object." D'oh. cargo cult. Done. https://codereview.chromium.org/1496093002/diff/100001/chrome/installer/mini_... chrome/installer/mini_installer/mini_installer.cc:598: bool result = sddl.append(L"D:PAI(A;ID;FA;;;BA)" // Admin: Full control. On 2015/12/07 14:56:22, grt (no reviews until Dec 14) wrote: > nit: break this line just before the open paren so that only the D:PAI is on > this line, and all subsequent lines contain the individual ACE strings. then the > comment for this line could be "Protected, auto-inherited DACL" Done. https://codereview.chromium.org/1496093002/diff/100001/chrome/installer/mini_... chrome/installer/mini_installer/mini_installer.cc:598: bool result = sddl.append(L"D:PAI(A;ID;FA;;;BA)" // Admin: Full control. On 2015/12/07 14:56:23, grt (no reviews until Dec 14) wrote: > to be utterly pedantic, each of these should have a trailing semicolon to fit > the format shown in > https://msdn.microsoft.com/library/windows/desktop/aa374928.aspx: > > "ace_type;ace_flags;rights;object_guid;inherit_object_guid;account_sid;(resource_attribute)" The documentation is a lie! At least, it went all kablooey when I tried to leave the trailing semicolons. https://codereview.chromium.org/1496093002/diff/100001/chrome/installer/mini_... chrome/installer/mini_installer/mini_installer.cc:599: L"(A;OICIIOID;GA;;;BA)" On 2015/12/07 14:56:22, grt (no reviews until Dec 14) wrote: > nit: each of these should be four-space indented "Visual Studio!!!!!!!" https://codereview.chromium.org/1496093002/diff/100001/chrome/installer/mini_... chrome/installer/mini_installer/mini_installer.cc:599: L"(A;OICIIOID;GA;;;BA)" On 2015/12/07 14:56:23, grt (no reviews until Dec 14) wrote: > IIUC: > OI - files in this directory will inherit the ACE > CI - directories in this directory will inherit the ACE > IO - the ace does not apply to this directory (only to the contained files and > directories) > is this what you desire? should the ACE to apply to |path| itself? Oops. My brain keeps thinking "temp directory" rather than "directory inside system temp". So, the IO is wrong. https://codereview.chromium.org/1496093002/diff/100001/chrome/installer/mini_... chrome/installer/mini_installer/mini_installer.cc:647: RtlGenRandom(&id, sizeof(id)); On 2015/12/07 14:56:23, grt (no reviews until Dec 14) wrote: > nit: ::RtlGenRandom Done.
And now it works locally but fails on the bot. Awesome.
On 2015/12/11 17:00:45, jschuh (very slow) wrote: > And now it works locally but fails on the bot. Awesome. Oops. Looks like I git'd those trailing semicolons back into the SDDL. we'll see if it works on the bots now.
Just to confirm, it works fine locally and on the bots now. The bad run was because I pushed the wrong version of the patch (the one with trailing semicolons) in that previous CL.
https://codereview.chromium.org/1496093002/diff/100001/chrome/installer/mini_... File chrome/installer/mini_installer/mini_installer.cc (right): https://codereview.chromium.org/1496093002/diff/100001/chrome/installer/mini_... chrome/installer/mini_installer/mini_installer.cc:598: bool result = sddl.append(L"D:PAI(A;ID;FA;;;BA)" // Admin: Full control. On 2015/12/11 04:07:24, jschuh (very slow) wrote: > On 2015/12/07 14:56:22, grt (no reviews until Dec 14) wrote: > > nit: break this line just before the open paren so that only the D:PAI is on > > this line, and all subsequent lines contain the individual ACE strings. then > the > > comment for this line could be "Protected, auto-inherited DACL" > > Done. Ah, I meant like this: bool result = sddl.append(L"D:PAI" // Protected, auto-inherited DACL. L"(A;;FA;;;BA)" // Admin: Full control. so that all lines but the first specify a single ACE string each https://codereview.chromium.org/1496093002/diff/100001/chrome/installer/mini_... chrome/installer/mini_installer/mini_installer.cc:599: L"(A;OICIIOID;GA;;;BA)" On 2015/12/11 04:07:24, jschuh (very slow) wrote: > On 2015/12/07 14:56:22, grt (no reviews until Dec 14) wrote: > > nit: each of these should be four-space indented > > "Visual Studio!!!!!!!" Give up and use a real editor. :-) https://codereview.chromium.org/1496093002/diff/150001/chrome/installer/mini_... File chrome/installer/mini_installer/mini_installer.cc (right): https://codereview.chromium.org/1496093002/diff/150001/chrome/installer/mini_... chrome/installer/mini_installer/mini_installer.cc:604: L"(A;OICI;GA;;;CO)" // Owner: Full control. I haven't digested the nuances around "Creator owner" vs the current owner, so I'll take this detail on faith + forshaw's stamp of approval. https://codereview.chromium.org/1496093002/diff/150001/chrome/installer/mini_... chrome/installer/mini_installer/mini_installer.cc:707: *exit_code = ProcessExitResult(SUCCESS_EXIT_CODE); I don't think this is quite right since CreateWorkDir has codepaths that return false without modifying |exit_code|. I think CreateWorkDir needs to be adjusted so that it populates a value in all cases. For example, the first "return false;" up there should probably be "*exit_code = ProcessExitResult(PATH_STRING_OVERFLOW); return false;" Maybe it's simpler for CreateWorkDir (and this function?) to return a ProcessExitResult rather than a bool. WDYT?
https://codereview.chromium.org/1496093002/diff/190001/chrome/installer/mini_... File chrome/installer/mini_installer/mini_installer.cc (right): https://codereview.chromium.org/1496093002/diff/190001/chrome/installer/mini_... chrome/installer/mini_installer/mini_installer.cc:679: ++id; // Try a different name. wow, i just noticed how subtle this is. it only works on little-endian machines due to the truncation on line 666, no? yikes! https://codereview.chromium.org/1496093002/diff/190001/chrome/installer/mini_... chrome/installer/mini_installer/mini_installer.cc:684: return result; remove line 654 and change this to: return exit_code->IsSuccess();
Patchset #11 (id:210001) has been deleted
Patchset #9 (id:170001) has been deleted
https://codereview.chromium.org/1496093002/diff/100001/chrome/installer/mini_... File chrome/installer/mini_installer/mini_installer.cc (right): https://codereview.chromium.org/1496093002/diff/100001/chrome/installer/mini_... chrome/installer/mini_installer/mini_installer.cc:598: bool result = sddl.append(L"D:PAI(A;ID;FA;;;BA)" // Admin: Full control. On 2015/12/14 18:59:26, grt wrote: > On 2015/12/11 04:07:24, jschuh (very slow) wrote: > > On 2015/12/07 14:56:22, grt (no reviews until Dec 14) wrote: > > > nit: break this line just before the open paren so that only the D:PAI is on > > > this line, and all subsequent lines contain the individual ACE strings. then > > the > > > comment for this line could be "Protected, auto-inherited DACL" > > > > Done. > > Ah, I meant like this: > bool result = sddl.append(L"D:PAI" // Protected, auto-inherited DACL. > L"(A;;FA;;;BA)" // Admin: Full control. > so that all lines but the first specify a single ACE string each I've now adjusted the first line, but I think it makes sense to keep the inherit and non-inherit permissions grouped by the principal, as it currently is. https://codereview.chromium.org/1496093002/diff/150001/chrome/installer/mini_... File chrome/installer/mini_installer/mini_installer.cc (right): https://codereview.chromium.org/1496093002/diff/150001/chrome/installer/mini_... chrome/installer/mini_installer/mini_installer.cc:604: L"(A;OICI;GA;;;CO)" // Owner: Full control. On 2015/12/14 18:59:26, grt wrote: > I haven't digested the nuances around "Creator owner" vs the current owner, so > I'll take this detail on faith + forshaw's stamp of approval. Thinking about it, I'm going to keep the inheritable ACEs inherit-only, which forshaw@ signed off on earlier. The truth is that they're redundant to the other ACEs, and that the way I have it now is the normal pattern used throughout Windows. Since forshaw@ is on vacation, hopefully I can clarify. The subtlety of the creator owner relationship is that UAC elevated users and members of the administrators group will not be granted explicit access by the creator owner ACEs. Rather, the appropriate privileged account is going to be set as the creator/owner in those scenarios. The idea here being that when elevated you don't want to leak a privileged object to your unelevated context. The other obnoxiously confusing detail (that forshaw@ explained to me earlier) is that the well-known SID for creator owner doesn't actually have any meaning in the context of explicit ACEs (it's used only for inheritance). Hence, all the annoying work to extract the owner from the user's token. https://codereview.chromium.org/1496093002/diff/150001/chrome/installer/mini_... chrome/installer/mini_installer/mini_installer.cc:707: *exit_code = ProcessExitResult(SUCCESS_EXIT_CODE); On 2015/12/14 18:59:26, grt wrote: > I don't think this is quite right since CreateWorkDir has codepaths that return > false without modifying |exit_code|. I think CreateWorkDir needs to be adjusted > so that it populates a value in all cases. For example, the first "return > false;" up there should probably be "*exit_code = > ProcessExitResult(PATH_STRING_OVERFLOW); return false;" Maybe it's simpler for > CreateWorkDir (and this function?) to return a ProcessExitResult rather than a > bool. WDYT? I think I checked that all code paths return correctly, but it is ugly and very fragile. So, yeah, you're right, I'll fix it up. Also, I totally missed PATH_STRING_OVERFLOW, which is definitely more informative. https://codereview.chromium.org/1496093002/diff/190001/chrome/installer/mini_... File chrome/installer/mini_installer/mini_installer.cc (right): https://codereview.chromium.org/1496093002/diff/190001/chrome/installer/mini_... chrome/installer/mini_installer/mini_installer.cc:679: ++id; // Try a different name. On 2015/12/15 20:08:10, grt wrote: > wow, i just noticed how subtle this is. it only works on little-endian machines > due to the truncation on line 666, no? yikes! I know, right? I was originally going to leave it alone, now that you've made me publicly acknowledge it, I'm compelled to just repopulate from the system random pool. https://codereview.chromium.org/1496093002/diff/190001/chrome/installer/mini_... chrome/installer/mini_installer/mini_installer.cc:684: return result; On 2015/12/15 20:08:10, grt wrote: > remove line 654 and change this to: > return exit_code->IsSuccess(); Done (clever).
lgtm w/ a nit https://codereview.chromium.org/1496093002/diff/230001/chrome/installer/mini_... File chrome/installer/mini_installer/mini_installer.cc (right): https://codereview.chromium.org/1496093002/diff/230001/chrome/installer/mini_... chrome/installer/mini_installer/mini_installer.cc:653: ::RtlGenRandom(&id, sizeof(id)); nit: move these two lines into the top of the loop and get rid of line 678.
The CQ bit was checked by jschuh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from forshaw@chromium.org, grt@chromium.org Link to the patchset: https://codereview.chromium.org/1496093002/#ps250001 (title: "nit")
https://codereview.chromium.org/1496093002/diff/230001/chrome/installer/mini_... File chrome/installer/mini_installer/mini_installer.cc (right): https://codereview.chromium.org/1496093002/diff/230001/chrome/installer/mini_... chrome/installer/mini_installer/mini_installer.cc:653: ::RtlGenRandom(&id, sizeof(id)); On 2015/12/15 21:12:16, grt wrote: > nit: move these two lines into the top of the loop and get rid of line 678. This is why it's a dumb idea to write code in 15 minute increments between meetings.
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
Message was sent while issue was closed.
Committed patchset #11 (id:250001)
Message was sent while issue was closed.
Description was changed from ========== Add a restricted ACL to installer temp directory This will prevent any sort of file squatting. BUG=565543 ========== to ========== 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} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/68e66bfceb698fefd4116e014365d23698291083 Cr-Commit-Position: refs/heads/master@{#365408} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
