Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
937353002:
Adding method to create process using LowBox token in sandbox code. (Closed)
|
Created:
5 years, 10 months ago by Shrikant Kelkar Modified:
5 years, 9 months ago CC:
chromium-reviews, creis+watch_chromium.org, grt+watch_chromium.org, nasko+codewatch_chromium.org, jam, gavinp+memory_chromium.org, darin-cc_chromium.org, wfh+watch_chromium.org, erikwright+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionThis CL adds a method to create process using LowBox token on Windows. LowBox will help us tackle some of the escapes from Sandbox.
R=cpu,jschuh,rvargas,wfh,forshaw
BUG=455496
Committed: https://crrev.com/f7540af7428f4b146136ec19b781886693f8c03f
Cr-Commit-Position: refs/heads/master@{#318648}
Patch Set 1 #Patch Set 2 : Added platform checking #
Total comments: 34
Patch Set 3 : Changes as per review comments. #Patch Set 4 : Adjusted order of includes. #
Total comments: 2
Patch Set 5 : Fixed indent. #
Total comments: 10
Patch Set 6 : Disabled allowopenevent and denyopenevent as these tests don't seem to go through sandbox code path… #Patch Set 7 : Addressing comments on earlier patch. #
Total comments: 3
Patch Set 8 : Modified app container tests #Patch Set 9 : Removed sid.h/.cc #
Total comments: 6
Patch Set 10 : Separated out lowbox and startupinfoex paths for creating appcontainer. #Patch Set 11 : Fixed comment inside target_process. #
Total comments: 40
Patch Set 12 : Review changes #Patch Set 13 : Fixed comment casing. #
Total comments: 27
Patch Set 14 : Review changes.. #
Total comments: 4
Patch Set 15 : Review changes... #Patch Set 16 : Sync to TOT to see if ios_dbg_simulator_ninja errors go away. #
Messages
Total messages: 47 (10 generated)
ptal
Looking good with a few issues. I'll do some tests on it to see what additional security it's really giving us. https://codereview.chromium.org/937353002/diff/20001/base/win/object_watcher.cc File base/win/object_watcher.cc (right): https://codereview.chromium.org/937353002/diff/20001/base/win/object_watcher.... base/win/object_watcher.cc:39: wait_flags |= WT_TRANSFER_IMPERSONATION; What's the reason being this change? Currently it's leaving a handle to the original impersonation token lying around which is pretty dangerous. Also I'm not sure it would actually work because the threadpool wouldn't be able to reassign the impersonation token afterwards. If I remove the line I can't see any obvious problem occurring. https://codereview.chromium.org/937353002/diff/20001/content/common/sandbox_w... File content/common/sandbox_win.cc (right): https://codereview.chromium.org/937353002/diff/20001/content/common/sandbox_w... content/common/sandbox_win.cc:540: // Reviewers: This doesn't seem to be right place for installing Based on my original testing I don't believe you need to install the appcontainer sid on the system for it to work (at least when using NtCreateLowBoxToken). We should be able to generate a per-process appcontainer SID using a simple incrementing counter. For example something like this seems to work: static unsigned int per_process_rid = 0; std::wstring app_container_sid = base::StringPrintf( L"S-1-15-2-3251537155-1984446955-2931258699-841473695-1938553385-" L"924012148-%u", per_process_rid++); policy->SetAppContainer(app_container_sid.c_str()); Also how was the original appcontainer sid generated? We want to ensure it isn't likely to conflict with anything in use. https://codereview.chromium.org/937353002/diff/20001/sandbox/win/src/process_... File sandbox/win/src/process_thread_interception.cc (right): https://codereview.chromium.org/937353002/diff/20001/sandbox/win/src/process_... sandbox/win/src/process_thread_interception.cc:8: #include "sandbox/win/src/interceptors.h" Was there more changes to this file? Removing this include doesn't seem to impact the build. https://codereview.chromium.org/937353002/diff/20001/sandbox/win/src/target_p... File sandbox/win/src/target_process.cc (right): https://codereview.chromium.org/937353002/diff/20001/sandbox/win/src/target_p... sandbox/win/src/target_process.cc:139: if (!::CreateProcessAsUserW(lockdown_token_.Get(), There was some discussion about doing this trick for all platforms not just Win8 as that would greatly simplify the code. Did we test this out and find it didn't work? https://codereview.chromium.org/937353002/diff/20001/sandbox/win/src/target_p... sandbox/win/src/target_process.cc:211: if (!NT_SUCCESS(status)) { Missing a call to lockdown_token_.Close() for this execution path.
Do we have bugs filed to get combase and wininet removed? I'm a bit disturbed that they're being imported at all, and we really should get the responsible teams fixing that ASAP.
Missing sbox tests. I'd suggest doing the integration with chrome policy on a separate CL. https://codereview.chromium.org/937353002/diff/20001/base/memory/shared_memor... File base/memory/shared_memory_win.cc (right): https://codereview.chromium.org/937353002/diff/20001/base/memory/shared_memor... base/memory/shared_memory_win.cc:147: LPCWSTR shared_memory_name = NULL; nit: wchar_t* or char16* https://codereview.chromium.org/937353002/diff/20001/base/memory/shared_memor... base/memory/shared_memory_win.cc:152: PAGE_READWRITE, 0, static_cast<DWORD>(rounded_size), shared_memory_name); name_.empty() ? nullptr : name_.c_str() ? Separate cl? https://codereview.chromium.org/937353002/diff/20001/sandbox/win/src/app_cont... File sandbox/win/src/app_container.h (right): https://codereview.chromium.org/937353002/diff/20001/sandbox/win/src/app_cont... sandbox/win/src/app_container.h:40: ResultCode ShareForStartup( Looks like we can remove this code too. https://codereview.chromium.org/937353002/diff/20001/sandbox/win/src/broker_s... File sandbox/win/src/broker_services.cc (right): https://codereview.chromium.org/937353002/diff/20001/sandbox/win/src/broker_s... sandbox/win/src/broker_services.cc:411: if (base::win::GetVersion() >= base::win::VERSION_VISTA) { This is preventing the use of any appcontainer related stuff before vista, but I don't think this CL preserves that behavior (attempts to call CreateLowBox, which will crash). https://codereview.chromium.org/937353002/diff/20001/sandbox/win/src/broker_s... sandbox/win/src/broker_services.cc:413: const AppContainerAttributes* app_container = Remove this https://codereview.chromium.org/937353002/diff/20001/sandbox/win/src/nt_inter... File sandbox/win/src/nt_internals.h (right): https://codereview.chromium.org/937353002/diff/20001/sandbox/win/src/nt_inter... sandbox/win/src/nt_internals.h:676: typedef NTSTATUS(WINAPI *NtQueryInformationProcess)( I don't see this being used https://codereview.chromium.org/937353002/diff/20001/sandbox/win/src/sandbox_... File sandbox/win/src/sandbox_policy_base.cc (right): https://codereview.chromium.org/937353002/diff/20001/sandbox/win/src/sandbox_... sandbox/win/src/sandbox_policy_base.cc:485: TOKEN_ALL_ACCESS, Just curious, any idea what this does? https://codereview.chromium.org/937353002/diff/20001/sandbox/win/src/sandbox_... sandbox/win/src/sandbox_policy_base.cc:492: if (!NT_SUCCESS(status) && token_lowbox == NULL) { || !token_lowbox?. An && implies that if the function fails the contract is that there is no garbage in an out argument (which may be true, but is not the nt contract). In fact, I'd remove token_lowbox from the if, and DCHECK that is not null (if anything) on success. https://codereview.chromium.org/937353002/diff/20001/sandbox/win/src/target_p... File sandbox/win/src/target_process.cc (right): https://codereview.chromium.org/937353002/diff/20001/sandbox/win/src/target_p... sandbox/win/src/target_process.cc:154: // For Windows 8 and above we will first create process with default token nit: create the process with a default... later, after setting ... required for setting an app.... along with an
wfh@chromium.org changed reviewers: + wfh@chromium.org
Looks like the existing appcontainer tests needs removing or fixing - since the code has changed (specifically, the call in broker_services.cc) https://codereview.chromium.org/937353002/diff/20001/sandbox/win/src/target_p... File sandbox/win/src/target_process.cc (right): https://codereview.chromium.org/937353002/diff/20001/sandbox/win/src/target_p... sandbox/win/src/target_process.cc:211: if (!NT_SUCCESS(status)) { On 2015/02/20 11:38:02, forshaw wrote: > Missing a call to lockdown_token_.Close() for this execution path. This should get closed in TargetProcess descructor at exit of BrokerServicesBase::SpawnTarget as it's a scoped handle. I don't think we need any of the Close() calls tbh - rvargas?
https://codereview.chromium.org/937353002/diff/20001/sandbox/win/src/target_p... File sandbox/win/src/target_process.cc (right): https://codereview.chromium.org/937353002/diff/20001/sandbox/win/src/target_p... sandbox/win/src/target_process.cc:211: if (!NT_SUCCESS(status)) { On 2015/02/21 01:13:15, Will Harris wrote: > On 2015/02/20 11:38:02, forshaw wrote: > > Missing a call to lockdown_token_.Close() for this execution path. > > This should get closed in TargetProcess descructor at exit of > BrokerServicesBase::SpawnTarget as it's a scoped handle. I don't think we need > any of the Close() calls tbh - rvargas? Yes, not leaking. I thought that for some reason we didn't want an open handle after leaving this function... which is kind of true as the process may live a long time and an extra handle is just more resources waiting to be messed up with :). We should probably scope it to a local variable up there.
ptal https://codereview.chromium.org/937353002/diff/20001/base/memory/shared_memor... File base/memory/shared_memory_win.cc (right): https://codereview.chromium.org/937353002/diff/20001/base/memory/shared_memor... base/memory/shared_memory_win.cc:147: LPCWSTR shared_memory_name = NULL; On 2015/02/21 01:01:22, rvargas wrote: > nit: wchar_t* or char16* removed, as per second comment. https://codereview.chromium.org/937353002/diff/20001/base/memory/shared_memor... base/memory/shared_memory_win.cc:152: PAGE_READWRITE, 0, static_cast<DWORD>(rounded_size), shared_memory_name); On 2015/02/21 01:01:22, rvargas wrote: > name_.empty() ? nullptr : name_.c_str() ? > Separate cl? Done. https://codereview.chromium.org/937353002/diff/20001/base/win/object_watcher.cc File base/win/object_watcher.cc (right): https://codereview.chromium.org/937353002/diff/20001/base/win/object_watcher.... base/win/object_watcher.cc:39: wait_flags |= WT_TRANSFER_IMPERSONATION; On 2015/02/20 11:38:02, forshaw wrote: > What's the reason being this change? Currently it's leaving a handle to the > original impersonation token lying around which is pretty dangerous. Also I'm > not sure it would actually work because the threadpool wouldn't be able to > reassign the impersonation token afterwards. > > If I remove the line I can't see any obvious problem occurring. Removed.. During initial checks, I think chrome_elf or someone similar threw error here because I thought that time it ended up using process impersonation token and after this flag error went away, but will remove it if it is not causing issues and if one of our dlls fail here will fix that instead. https://codereview.chromium.org/937353002/diff/20001/content/common/sandbox_w... File content/common/sandbox_win.cc (right): https://codereview.chromium.org/937353002/diff/20001/content/common/sandbox_w... content/common/sandbox_win.cc:540: // Reviewers: This doesn't seem to be right place for installing On 2015/02/20 11:38:02, forshaw wrote: > Based on my original testing I don't believe you need to install the > appcontainer sid on the system for it to work (at least when using > NtCreateLowBoxToken). We should be able to generate a per-process appcontainer > SID using a simple incrementing counter. For example something like this seems > to work: > > static unsigned int per_process_rid = 0; > > std::wstring app_container_sid = base::StringPrintf( > L"S-1-15-2-3251537155-1984446955-2931258699-841473695-1938553385-" > L"924012148-%u", per_process_rid++); > > policy->SetAppContainer(app_container_sid.c_str()); > > Also how was the original appcontainer sid generated? We want to ensure it isn't > likely to conflict with anything in use. Appended GUID, ptal. https://codereview.chromium.org/937353002/diff/20001/sandbox/win/src/app_cont... File sandbox/win/src/app_container.h (right): https://codereview.chromium.org/937353002/diff/20001/sandbox/win/src/app_cont... sandbox/win/src/app_container.h:40: ResultCode ShareForStartup( On 2015/02/21 01:01:22, rvargas wrote: > Looks like we can remove this code too. Done. https://codereview.chromium.org/937353002/diff/20001/sandbox/win/src/broker_s... File sandbox/win/src/broker_services.cc (right): https://codereview.chromium.org/937353002/diff/20001/sandbox/win/src/broker_s... sandbox/win/src/broker_services.cc:411: if (base::win::GetVersion() >= base::win::VERSION_VISTA) { On 2015/02/21 01:01:22, rvargas wrote: > This is preventing the use of any appcontainer related stuff before vista, but I > don't think this CL preserves that behavior (attempts to call CreateLowBox, > which will crash). removed https://codereview.chromium.org/937353002/diff/20001/sandbox/win/src/broker_s... sandbox/win/src/broker_services.cc:413: const AppContainerAttributes* app_container = On 2015/02/21 01:01:22, rvargas wrote: > Remove this Done. https://codereview.chromium.org/937353002/diff/20001/sandbox/win/src/nt_inter... File sandbox/win/src/nt_internals.h (right): https://codereview.chromium.org/937353002/diff/20001/sandbox/win/src/nt_inter... sandbox/win/src/nt_internals.h:676: typedef NTSTATUS(WINAPI *NtQueryInformationProcess)( On 2015/02/21 01:01:22, rvargas wrote: > I don't see this being used removed https://codereview.chromium.org/937353002/diff/20001/sandbox/win/src/process_... File sandbox/win/src/process_thread_interception.cc (right): https://codereview.chromium.org/937353002/diff/20001/sandbox/win/src/process_... sandbox/win/src/process_thread_interception.cc:8: #include "sandbox/win/src/interceptors.h" On 2015/02/20 11:38:02, forshaw wrote: > Was there more changes to this file? Removing this include doesn't seem to > impact the build. There were more changes in here.. I was returning ThreadToken in OpenProcessToken (until we do a revert) to avoid some of the checking/failings in wininet, but I think that got resolved with SetInformationThread changes. https://codereview.chromium.org/937353002/diff/20001/sandbox/win/src/sandbox_... File sandbox/win/src/sandbox_policy_base.cc (right): https://codereview.chromium.org/937353002/diff/20001/sandbox/win/src/sandbox_... sandbox/win/src/sandbox_policy_base.cc:485: TOKEN_ALL_ACCESS, On 2015/02/21 01:01:22, rvargas wrote: > Just curious, any idea what this does? Not sure, but guess is that it will open and return new lowbox token with that access. https://codereview.chromium.org/937353002/diff/20001/sandbox/win/src/sandbox_... sandbox/win/src/sandbox_policy_base.cc:492: if (!NT_SUCCESS(status) && token_lowbox == NULL) { On 2015/02/21 01:01:22, rvargas wrote: > || !token_lowbox?. An && implies that if the function fails the contract is that > there is no garbage in an out argument (which may be true, but is not the nt > contract). > > In fact, I'd remove token_lowbox from the if, and DCHECK that is not null (if > anything) on success. Done. https://codereview.chromium.org/937353002/diff/20001/sandbox/win/src/target_p... File sandbox/win/src/target_process.cc (right): https://codereview.chromium.org/937353002/diff/20001/sandbox/win/src/target_p... sandbox/win/src/target_process.cc:139: if (!::CreateProcessAsUserW(lockdown_token_.Get(), On 2015/02/20 11:38:02, forshaw wrote: > There was some discussion about doing this trick for all platforms not just Win8 > as that would greatly simplify the code. Did we test this out and find it didn't > work? No, I haven't tested on other platforms. But to be on safer side I would like to experiment with Win8 on canary first and see if things are working okay in the field. Merging code will be easier CL later. https://codereview.chromium.org/937353002/diff/20001/sandbox/win/src/target_p... sandbox/win/src/target_process.cc:154: // For Windows 8 and above we will first create process with default token On 2015/02/21 01:01:22, rvargas wrote: > nit: create the process with a default... later, after setting ... required for > setting an app.... along with an Done. https://codereview.chromium.org/937353002/diff/20001/sandbox/win/src/target_p... sandbox/win/src/target_process.cc:211: if (!NT_SUCCESS(status)) { On 2015/02/20 11:38:02, forshaw wrote: > Missing a call to lockdown_token_.Close() for this execution path. Done. https://codereview.chromium.org/937353002/diff/20001/sandbox/win/src/target_p... sandbox/win/src/target_process.cc:211: if (!NT_SUCCESS(status)) { On 2015/02/21 01:26:01, rvargas wrote: > On 2015/02/21 01:13:15, Will Harris wrote: > > On 2015/02/20 11:38:02, forshaw wrote: > > > Missing a call to lockdown_token_.Close() for this execution path. > > > > This should get closed in TargetProcess descructor at exit of > > BrokerServicesBase::SpawnTarget as it's a scoped handle. I don't think we > need > > any of the Close() calls tbh - rvargas? > > Yes, not leaking. I thought that for some reason we didn't want an open handle > after leaving this function... which is kind of true as the process may live a > long time and an extra handle is just more resources waiting to be messed up > with :). > > We should probably scope it to a local variable up there. Done. https://codereview.chromium.org/937353002/diff/20001/sandbox/win/src/target_p... sandbox/win/src/target_process.cc:211: if (!NT_SUCCESS(status)) { On 2015/02/21 01:13:15, Will Harris wrote: > On 2015/02/20 11:38:02, forshaw wrote: > > Missing a call to lockdown_token_.Close() for this execution path. > > This should get closed in TargetProcess descructor at exit of > BrokerServicesBase::SpawnTarget as it's a scoped handle. I don't think we need > any of the Close() calls tbh - rvargas? Done.
do the existing app container tests need to be changed/removed? https://codereview.chromium.org/937353002/diff/60001/sandbox/win/src/target_p... File sandbox/win/src/target_process.cc (right): https://codereview.chromium.org/937353002/diff/60001/sandbox/win/src/target_p... sandbox/win/src/target_process.cc:159: cmd_line.get(), nit: align parameters
Cause of some of those appcontainer tests seem to be installing of same ID through earlier code, now that code has been removed. Indent fixed. ptal. https://codereview.chromium.org/937353002/diff/60001/sandbox/win/src/target_p... File sandbox/win/src/target_process.cc (right): https://codereview.chromium.org/937353002/diff/60001/sandbox/win/src/target_p... sandbox/win/src/target_process.cc:159: cmd_line.get(), On 2015/02/21 02:45:45, Will Harris wrote: > nit: align parameters Done.
On 2015/02/21 02:45:46, Will Harris wrote: > do the existing app container tests need to be changed/removed? Without looking at them I would guess that they should be fixed. We definitely need new tests to cover the new features.
On 2015/02/21 03:19:08, rvargas wrote: > On 2015/02/21 02:45:46, Will Harris wrote: > > do the existing app container tests need to be changed/removed? > > Without looking at them I would guess that they should be fixed. We definitely > need new tests to cover the new features. hmm app container tests (see ps4) seem to be unhappy still.
> hmm app container tests (see ps4) seem to be unhappy still. Well of the ones which are failing we no longer need a check for failure on impersonation as we've removed that limitation. The other failing tests, checking opening event objects, are failing because of the way in which the win32 apis behave when running under an appcontainer token. Specifically they try and lookup named objects under \Sessions\X\AppContainerNamedObjects\PackageSID\.... Other than adding that directory object for each process there's not a lot of ways around it when calling a win32 API such as OpenEvent. You could do it with the native APIs but that's just adding additional complexity. For a renderer the lack of this directory doesn't really matter as it's unlikely it'll be able to access resources under that location anyway, but if we ever want to apply appcontainers to slightly less restrictive processes we might need to fix it.
https://codereview.chromium.org/937353002/diff/80001/base/win/object_watcher.cc File base/win/object_watcher.cc (right): https://codereview.chromium.org/937353002/diff/80001/base/win/object_watcher.... base/win/object_watcher.cc:9: #include "base/win/windows_version.h" Unused include, removing doesn't affect the build. https://codereview.chromium.org/937353002/diff/80001/sandbox/win/src/sid.cc File sandbox/win/src/sid.cc (left): https://codereview.chromium.org/937353002/diff/80001/sandbox/win/src/sid.cc#o... sandbox/win/src/sid.cc:12: ::CopySid(SECURITY_MAX_SID_SIZE, sid_, const_cast<SID*>(sid)); nit: Obviously the original code didn't check the return of CopySid but should we are least check it using a DCHECK? https://codereview.chromium.org/937353002/diff/80001/sandbox/win/src/sid.cc File sandbox/win/src/sid.cc (right): https://codereview.chromium.org/937353002/diff/80001/sandbox/win/src/sid.cc#n... sandbox/win/src/sid.cc:15: : unique_sub_auth_sid_(NULL) { unique_sub_auth_sid_ never actually used. https://codereview.chromium.org/937353002/diff/80001/sandbox/win/src/sid.cc#n... sandbox/win/src/sid.cc:31: void Sid::GenerateUniqueSubAuthoritySid(std::wstring* unique_sid) { nit: Would it not make more sense to return the wstring rather than via a pointer? https://codereview.chromium.org/937353002/diff/80001/sandbox/win/src/sid.cc#n... sandbox/win/src/sid.cc:34: ::CoCreateGuid(&guid); Not sure I like creating random SIDs too much, the domain of GUID generation and SIDs is not the same and I guess there might be the odd time that they collide. I think it's very unlikely to occur but I personally think it would be better to pre-generate a fixed SID (which we can document) then add a some sort of unique RID to that.
https://codereview.chromium.org/937353002/diff/80001/base/win/object_watcher.cc File base/win/object_watcher.cc (right): https://codereview.chromium.org/937353002/diff/80001/base/win/object_watcher.... base/win/object_watcher.cc:9: #include "base/win/windows_version.h" On 2015/02/23 13:32:28, forshaw wrote: > Unused include, removing doesn't affect the build. Done. https://codereview.chromium.org/937353002/diff/80001/sandbox/win/src/sid.cc File sandbox/win/src/sid.cc (left): https://codereview.chromium.org/937353002/diff/80001/sandbox/win/src/sid.cc#o... sandbox/win/src/sid.cc:12: ::CopySid(SECURITY_MAX_SID_SIZE, sid_, const_cast<SID*>(sid)); On 2015/02/23 13:32:29, forshaw wrote: > nit: Obviously the original code didn't check the return of CopySid but should > we are least check it using a DCHECK? Done. https://codereview.chromium.org/937353002/diff/80001/sandbox/win/src/sid.cc File sandbox/win/src/sid.cc (right): https://codereview.chromium.org/937353002/diff/80001/sandbox/win/src/sid.cc#n... sandbox/win/src/sid.cc:15: : unique_sub_auth_sid_(NULL) { On 2015/02/23 13:32:29, forshaw wrote: > unique_sub_auth_sid_ never actually used. Done. https://codereview.chromium.org/937353002/diff/80001/sandbox/win/src/sid.cc#n... sandbox/win/src/sid.cc:31: void Sid::GenerateUniqueSubAuthoritySid(std::wstring* unique_sid) { On 2015/02/23 13:32:29, forshaw wrote: > nit: Would it not make more sense to return the wstring rather than via a > pointer? Done. https://codereview.chromium.org/937353002/diff/80001/sandbox/win/src/sid.cc#n... sandbox/win/src/sid.cc:34: ::CoCreateGuid(&guid); On 2015/02/23 13:32:29, forshaw wrote: > Not sure I like creating random SIDs too much, the domain of GUID generation and > SIDs is not the same and I guess there might be the odd time that they collide. > I think it's very unlikely to occur but I personally think it would be better to > pre-generate a fixed SID (which we can document) then add a some sort of unique > RID to that. Yes, I see your point, will discuss with other reviewers and will try to get a common resolve.
please separate the change in shared_memory_win.cc to its own CL send it to me. We can get this in today.
also separate the 3 /content/ files into another CL basically lets get appcontainer with the sandbox tests i, and then in a second CL we integrate with chrome.
https://codereview.chromium.org/937353002/diff/20001/sandbox/win/src/broker_s... File sandbox/win/src/broker_services.cc (right): https://codereview.chromium.org/937353002/diff/20001/sandbox/win/src/broker_s... sandbox/win/src/broker_services.cc:411: if (base::win::GetVersion() >= base::win::VERSION_VISTA) { On 2015/02/21 02:32:40, Shrikant Kelkar wrote: > On 2015/02/21 01:01:22, rvargas wrote: > > This is preventing the use of any appcontainer related stuff before vista, but > I > > don't think this CL preserves that behavior (attempts to call CreateLowBox, > > which will crash). > > removed ? To rephrase, I don't think the behavior regarding OS versions is correct, but I may be missing something. https://codereview.chromium.org/937353002/diff/20001/sandbox/win/src/sandbox_... File sandbox/win/src/sandbox_policy_base.cc (right): https://codereview.chromium.org/937353002/diff/20001/sandbox/win/src/sandbox_... sandbox/win/src/sandbox_policy_base.cc:485: TOKEN_ALL_ACCESS, On 2015/02/21 02:32:41, Shrikant Kelkar wrote: > On 2015/02/21 01:01:22, rvargas wrote: > > Just curious, any idea what this does? > Not sure, but guess is that it will open and return new lowbox token with that > access. Do we need ALL_ACCESS? https://codereview.chromium.org/937353002/diff/120001/sandbox/win/src/sandbox... File sandbox/win/src/sandbox_policy_base.cc (right): https://codereview.chromium.org/937353002/diff/120001/sandbox/win/src/sandbox... sandbox/win/src/sandbox_policy_base.cc:490: if (!NT_SUCCESS(status)) { nit: no {} https://codereview.chromium.org/937353002/diff/120001/sandbox/win/src/sid.cc File sandbox/win/src/sid.cc (right): https://codereview.chromium.org/937353002/diff/120001/sandbox/win/src/sid.cc#... sandbox/win/src/sid.cc:7: #include <objbase.h> ? https://codereview.chromium.org/937353002/diff/120001/sandbox/win/src/sid.cc#... sandbox/win/src/sid.cc:30: std::wstring Sid::GenerateUniqueSubAuthoritySid() { I don't think this code belongs here
ptal. I have modified App_container_tests for deny only. Here is what I found so far, If I create appcontainer through CreateLowbox way then certain parts like BNO are not setup as they are setup with StartupInfoEx path. I traced through kernelbase for openevent, it fails early on with patch to get BNO. In other case it successfully queries BNO specifically arranged for AppContainerNamedObjects/SID. I am guessing that last parameter to CreateLowbox could be potentially used to set things up, but we actually don't need to depend on that behavior. In case of Chrome, policy driven object creation seem to work fine through interception/duplicatehandle code.
https://codereview.chromium.org/937353002/diff/160001/sandbox/win/src/app_con... File sandbox/win/src/app_container.h (left): https://codereview.chromium.org/937353002/diff/160001/sandbox/win/src/app_con... sandbox/win/src/app_container.h:40: ResultCode ShareForStartup( I know I asked you to remove this, but after talking with cpu yesterday we concluded it was better to keep the old code as it was, at least for a while (especially after the realization that the lowbox token is not really related with a regular AppContainer). How about we separate the two concepts so that: SetAppContainer() is needed for a normal AppContainer HasAppcontainer() returns true for that normal AppContainer (and false for lowbox) SetLowBox(app_container_sid) is what we use for the lowbox path HasLowBox() as named GetCapabilities() works for both cases. ---------- This means that TargetPolicy gains a new method besides SetAppContainer: SetLowBox(sid)... and we enforce that lowbox and appcontainer policies are mutually exclusive. Another option would be not to add SetLowBox() and instead redefine TokenLevel to have something below USER_LOCKDOWN... that seems actually cleaner but a little more dangerous so we can just leave that for later. https://codereview.chromium.org/937353002/diff/160001/sandbox/win/src/app_con... File sandbox/win/src/app_container_test.cc (right): https://codereview.chromium.org/937353002/diff/160001/sandbox/win/src/app_con... sandbox/win/src/app_container_test.cc:107: EXPECT_EQ(SBOX_ALL_OK, runner.GetPolicy()->SetCapability(capability)); ... And tests that deal with LowBox don't require InstallAppContainer or SetCapability... just SetLowBox https://codereview.chromium.org/937353002/diff/160001/sandbox/win/src/policy_... File sandbox/win/src/policy_broker.cc (right): https://codereview.chromium.org/937353002/diff/160001/sandbox/win/src/policy_... sandbox/win/src/policy_broker.cc:90: // Interceptions provided by process_thread_policy, without actual policy. What's the change here? The first three interceptions are still handled by process_thread_policy, but not the others... https://codereview.chromium.org/937353002/diff/160001/sandbox/win/src/policy_... sandbox/win/src/policy_broker.cc:92: !INTERCEPT_NT(manager, NtOpenProcess, OPEN_PROCESS_ID, 20)) Nit: this requires {} (and below)
ptal. Modified most of the code again to be in sync with what we discussed. Idea is to keep earlier approach of launching process into appcontainer as it is and creating new interface to launch using lowbox token.
Almost there. https://codereview.chromium.org/937353002/diff/160001/sandbox/win/src/policy_... File sandbox/win/src/policy_broker.cc (right): https://codereview.chromium.org/937353002/diff/160001/sandbox/win/src/policy_... sandbox/win/src/policy_broker.cc:90: // Interceptions provided by process_thread_policy, without actual policy. On 2015/02/27 20:16:34, rvargas wrote: > What's the change here? The first three interceptions are still handled by > process_thread_policy, but not the others... Looks like you missed these comments. https://codereview.chromium.org/937353002/diff/200001/sandbox/win/src/app_con... File sandbox/win/src/app_container_test.cc (right): https://codereview.chromium.org/937353002/diff/200001/sandbox/win/src/app_con... sandbox/win/src/app_container_test.cc:145: // For convenience including LowBox tests inside this file. nit: remove lines 144-45 https://codereview.chromium.org/937353002/diff/200001/sandbox/win/src/app_con... sandbox/win/src/app_container_test.cc:146: // We expect TEST_DENIED because when we setup appcontainer through lowbox way nit: Actually, I think we can remove the whole comment. https://codereview.chromium.org/937353002/diff/200001/sandbox/win/src/app_con... sandbox/win/src/app_container_test.cc:163: } caveat: missing more tests for a future CL (as in, a test with USER_LOCKDOWN which is what this CL promises to provide). Maybe a TODO is in order :) https://codereview.chromium.org/937353002/diff/200001/sandbox/win/src/sandbox... File sandbox/win/src/sandbox_policy.h (right): https://codereview.chromium.org/937353002/diff/200001/sandbox/win/src/sandbox... sandbox/win/src/sandbox_policy.h:187: // with SetAppContainer method. So far there are two methods to start nit: I think we can drop the second sentence. https://codereview.chromium.org/937353002/diff/200001/sandbox/win/src/sandbox... File sandbox/win/src/sandbox_policy_base.cc (right): https://codereview.chromium.org/937353002/diff/200001/sandbox/win/src/sandbox... sandbox/win/src/sandbox_policy_base.cc:346: if (lowbox_sid_) nit: This should be an error. https://codereview.chromium.org/937353002/diff/200001/sandbox/win/src/sandbox... sandbox/win/src/sandbox_policy_base.cc:349: if (!ConvertStringSidToSid(sid, &lowbox_sid_)) The should be an OS version check here https://codereview.chromium.org/937353002/diff/200001/sandbox/win/src/sandbox... sandbox/win/src/sandbox_policy_base.cc:473: appcontainer_list_->HasAppContainer() && nit: send this to the previous line https://codereview.chromium.org/937353002/diff/200001/sandbox/win/src/sandbox... sandbox/win/src/sandbox_policy_base.cc:474: lowbox_sid_) nit : requires {} https://codereview.chromium.org/937353002/diff/200001/sandbox/win/src/sandbox... sandbox/win/src/sandbox_policy_base.cc:475: return SBOX_ERROR_UNEXPECTED_CALL; SBOX_ERROR_BAD_PARAMS ? https://codereview.chromium.org/937353002/diff/200001/sandbox/win/src/sandbox... sandbox/win/src/sandbox_policy_base.cc:505: // We are maintaining two approaches this time and making them mutually nit: this time? (aka, remove) https://codereview.chromium.org/937353002/diff/200001/sandbox/win/src/sandbox... sandbox/win/src/sandbox_policy_base.cc:505: // We are maintaining two approaches this time and making them mutually nit: we are not making them mutually exclusive... they are mutually exclusive :p https://codereview.chromium.org/937353002/diff/200001/sandbox/win/src/sandbox... sandbox/win/src/sandbox_policy_base.cc:506: // exclusive. One is to start appcontainer process through StartupInfoEx nit: start an AppContainer https://codereview.chromium.org/937353002/diff/200001/sandbox/win/src/sandbox... sandbox/win/src/sandbox_policy_base.cc:507: // and other is by attaching LowBox token after process creation. nit: We don't attach a token, we replace the token https://codereview.chromium.org/937353002/diff/200001/sandbox/win/src/sandbox... sandbox/win/src/sandbox_policy_base.cc:521: nit: remove empty line. In fact, move line 520 to 524 https://codereview.chromium.org/937353002/diff/200001/sandbox/win/src/sandbox... sandbox/win/src/sandbox_policy_base.cc:526: *lockdown, nit: we can fit more arguments per line here... https://codereview.chromium.org/937353002/diff/200001/sandbox/win/src/sandbox... sandbox/win/src/sandbox_policy_base.cc:534: if (!NT_SUCCESS(status)) { nit: no {} https://codereview.chromium.org/937353002/diff/200001/sandbox/win/src/target_... File sandbox/win/src/target_process.cc (right): https://codereview.chromium.org/937353002/diff/200001/sandbox/win/src/target_... sandbox/win/src/target_process.cc:146: if (!set_lockdown_token_after_create) { nit: if (set_lockdown_token_after_create) https://codereview.chromium.org/937353002/diff/200001/sandbox/win/src/target_... sandbox/win/src/target_process.cc:161: // Our approach is to first create process with a default token and then nit: remove "our approach" https://codereview.chromium.org/937353002/diff/200001/sandbox/win/src/target_... sandbox/win/src/target_process.cc:163: // for setting an appcontainer token along with an impersonation token. nit: AppContainer https://codereview.chromium.org/937353002/diff/200001/sandbox/win/src/target_... File sandbox/win/src/target_process.h (right): https://codereview.chromium.org/937353002/diff/200001/sandbox/win/src/target_... sandbox/win/src/target_process.h:48: // set_lockdown_token_after_create: When set, takes alternate path of nit: use running prose to describe args: when |var| is set, the lockdown token is replaced after the process is created. (or lockdown_token_). Mentioning alternate paths here just confuses things.
ptal. https://codereview.chromium.org/937353002/diff/160001/sandbox/win/src/policy_... File sandbox/win/src/policy_broker.cc (right): https://codereview.chromium.org/937353002/diff/160001/sandbox/win/src/policy_... sandbox/win/src/policy_broker.cc:90: // Interceptions provided by process_thread_policy, without actual policy. On 2015/02/28 01:10:05, rvargas wrote: > On 2015/02/27 20:16:34, rvargas wrote: > > What's the change here? The first three interceptions are still handled by > > process_thread_policy, but not the others... > > Looks like you missed these comments. Done. https://codereview.chromium.org/937353002/diff/200001/sandbox/win/src/app_con... File sandbox/win/src/app_container_test.cc (right): https://codereview.chromium.org/937353002/diff/200001/sandbox/win/src/app_con... sandbox/win/src/app_container_test.cc:145: // For convenience including LowBox tests inside this file. On 2015/02/28 01:10:06, rvargas wrote: > nit: remove lines 144-45 Done. https://codereview.chromium.org/937353002/diff/200001/sandbox/win/src/app_con... sandbox/win/src/app_container_test.cc:146: // We expect TEST_DENIED because when we setup appcontainer through lowbox way On 2015/02/28 01:10:06, rvargas wrote: > nit: Actually, I think we can remove the whole comment. Done. https://codereview.chromium.org/937353002/diff/200001/sandbox/win/src/app_con... sandbox/win/src/app_container_test.cc:163: } On 2015/02/28 01:10:06, rvargas wrote: > caveat: missing more tests for a future CL (as in, a test with USER_LOCKDOWN > which is what this CL promises to provide). Maybe a TODO is in order :) Done. https://codereview.chromium.org/937353002/diff/200001/sandbox/win/src/sandbox... File sandbox/win/src/sandbox_policy.h (right): https://codereview.chromium.org/937353002/diff/200001/sandbox/win/src/sandbox... sandbox/win/src/sandbox_policy.h:187: // with SetAppContainer method. So far there are two methods to start On 2015/02/28 01:10:06, rvargas wrote: > nit: I think we can drop the second sentence. Done. https://codereview.chromium.org/937353002/diff/200001/sandbox/win/src/sandbox... File sandbox/win/src/sandbox_policy_base.cc (right): https://codereview.chromium.org/937353002/diff/200001/sandbox/win/src/sandbox... sandbox/win/src/sandbox_policy_base.cc:346: if (lowbox_sid_) On 2015/02/28 01:10:06, rvargas wrote: > nit: This should be an error. Done. https://codereview.chromium.org/937353002/diff/200001/sandbox/win/src/sandbox... sandbox/win/src/sandbox_policy_base.cc:349: if (!ConvertStringSidToSid(sid, &lowbox_sid_)) On 2015/02/28 01:10:06, rvargas wrote: > The should be an OS version check here Done. https://codereview.chromium.org/937353002/diff/200001/sandbox/win/src/sandbox... sandbox/win/src/sandbox_policy_base.cc:473: appcontainer_list_->HasAppContainer() && On 2015/02/28 01:10:06, rvargas wrote: > nit: send this to the previous line Done. https://codereview.chromium.org/937353002/diff/200001/sandbox/win/src/sandbox... sandbox/win/src/sandbox_policy_base.cc:474: lowbox_sid_) On 2015/02/28 01:10:06, rvargas wrote: > nit : requires {} Done. https://codereview.chromium.org/937353002/diff/200001/sandbox/win/src/sandbox... sandbox/win/src/sandbox_policy_base.cc:475: return SBOX_ERROR_UNEXPECTED_CALL; On 2015/02/28 01:10:06, rvargas wrote: > SBOX_ERROR_BAD_PARAMS ? Done. https://codereview.chromium.org/937353002/diff/200001/sandbox/win/src/sandbox... sandbox/win/src/sandbox_policy_base.cc:505: // We are maintaining two approaches this time and making them mutually On 2015/02/28 01:10:06, rvargas wrote: > nit: we are not making them mutually exclusive... they are mutually exclusive :p Done. https://codereview.chromium.org/937353002/diff/200001/sandbox/win/src/sandbox... sandbox/win/src/sandbox_policy_base.cc:505: // We are maintaining two approaches this time and making them mutually On 2015/02/28 01:10:06, rvargas wrote: > nit: this time? (aka, remove) Done. https://codereview.chromium.org/937353002/diff/200001/sandbox/win/src/sandbox... sandbox/win/src/sandbox_policy_base.cc:506: // exclusive. One is to start appcontainer process through StartupInfoEx On 2015/02/28 01:10:06, rvargas wrote: > nit: start an AppContainer Done. https://codereview.chromium.org/937353002/diff/200001/sandbox/win/src/sandbox... sandbox/win/src/sandbox_policy_base.cc:507: // and other is by attaching LowBox token after process creation. On 2015/02/28 01:10:06, rvargas wrote: > nit: We don't attach a token, we replace the token Done. https://codereview.chromium.org/937353002/diff/200001/sandbox/win/src/sandbox... sandbox/win/src/sandbox_policy_base.cc:521: On 2015/02/28 01:10:06, rvargas wrote: > nit: remove empty line. In fact, move line 520 to 524 Done. https://codereview.chromium.org/937353002/diff/200001/sandbox/win/src/sandbox... sandbox/win/src/sandbox_policy_base.cc:526: *lockdown, On 2015/02/28 01:10:06, rvargas wrote: > nit: we can fit more arguments per line here... Done. https://codereview.chromium.org/937353002/diff/200001/sandbox/win/src/sandbox... sandbox/win/src/sandbox_policy_base.cc:534: if (!NT_SUCCESS(status)) { On 2015/02/28 01:10:06, rvargas wrote: > nit: no {} Done. https://codereview.chromium.org/937353002/diff/200001/sandbox/win/src/target_... File sandbox/win/src/target_process.cc (right): https://codereview.chromium.org/937353002/diff/200001/sandbox/win/src/target_... sandbox/win/src/target_process.cc:146: if (!set_lockdown_token_after_create) { On 2015/02/28 01:10:06, rvargas wrote: > nit: if (set_lockdown_token_after_create) Done. https://codereview.chromium.org/937353002/diff/200001/sandbox/win/src/target_... sandbox/win/src/target_process.cc:161: // Our approach is to first create process with a default token and then On 2015/02/28 01:10:07, rvargas wrote: > nit: remove "our approach" Done. https://codereview.chromium.org/937353002/diff/200001/sandbox/win/src/target_... sandbox/win/src/target_process.cc:163: // for setting an appcontainer token along with an impersonation token. On 2015/02/28 01:10:06, rvargas wrote: > nit: AppContainer Done. https://codereview.chromium.org/937353002/diff/200001/sandbox/win/src/target_... File sandbox/win/src/target_process.h (right): https://codereview.chromium.org/937353002/diff/200001/sandbox/win/src/target_... sandbox/win/src/target_process.h:48: // set_lockdown_token_after_create: When set, takes alternate path of On 2015/02/28 01:10:07, rvargas wrote: > nit: use running prose to describe args: when |var| is set, the lockdown token > is replaced after the process is created. (or lockdown_token_). Mentioning > alternate paths here just confuses things. Done.
lgtm with nits https://codereview.chromium.org/937353002/diff/240001/sandbox/win/src/policy_... File sandbox/win/src/policy_target.cc (left): https://codereview.chromium.org/937353002/diff/240001/sandbox/win/src/policy_... sandbox/win/src/policy_target.cc:91: can you remind me of why this removal? https://codereview.chromium.org/937353002/diff/240001/sandbox/win/src/sandbox... File sandbox/win/src/sandbox_policy_base.cc (right): https://codereview.chromium.org/937353002/diff/240001/sandbox/win/src/sandbox... sandbox/win/src/sandbox_policy_base.cc:159: ::LocalFree() or at least we seem to be consistent on this file about that. https://codereview.chromium.org/937353002/diff/240001/sandbox/win/src/sandbox... sandbox/win/src/sandbox_policy_base.cc:340: if (base::win::OSInfo::GetInstance()->version() < base::win::VERSION_WIN8) why the long form instead of the GetVersinon() one? https://codereview.chromium.org/937353002/diff/240001/sandbox/win/src/sandbox... sandbox/win/src/sandbox_policy_base.cc:341: return SBOX_ERROR_UNEXPECTED_CALL; return SBOX_ERROR_UNSUPPORTED https://codereview.chromium.org/937353002/diff/240001/sandbox/win/src/sandbox... sandbox/win/src/sandbox_policy_base.cc:476: lowbox_sid_) { is there a way to trigger this check while bypassing the checks above? https://codereview.chromium.org/937353002/diff/240001/sandbox/win/src/sandbox... sandbox/win/src/sandbox_policy_base.cc:520: NtCreateLowBoxToken CreateLowBox = NULL; function should be named CreateLowBoxToken, or is it clashing with something else? https://codereview.chromium.org/937353002/diff/240001/sandbox/win/src/sandbox... File sandbox/win/src/sandbox_policy_base.h (right): https://codereview.chromium.org/937353002/diff/240001/sandbox/win/src/sandbox... sandbox/win/src/sandbox_policy_base.h:164: PSID lowbox_sid_; instead pointer to sid? we don't use PHANDLE for example. https://codereview.chromium.org/937353002/diff/240001/sandbox/win/src/target_... File sandbox/win/src/target_process.cc (right): https://codereview.chromium.org/937353002/diff/240001/sandbox/win/src/target_... sandbox/win/src/target_process.cc:149: // an AppContainer token along with an impersonation token. nice!
one more thing please update the CL descrioption, is no longer about appcontainers. Also have a couple of sentences explaining the approach.
https://codereview.chromium.org/937353002/diff/240001/sandbox/win/src/sandbox... File sandbox/win/src/sandbox_policy_base.cc (right): https://codereview.chromium.org/937353002/diff/240001/sandbox/win/src/sandbox... sandbox/win/src/sandbox_policy_base.cc:341: return SBOX_ERROR_UNEXPECTED_CALL; I'm not sure what is the right thing to do, but note that SetAppContainer() returns SBOX_ALL_OK instead, which means that the caller doesn't need OS-version dependent code. https://codereview.chromium.org/937353002/diff/240001/sandbox/win/src/sandbox... sandbox/win/src/sandbox_policy_base.cc:345: return SBOX_ERROR_UNEXPECTED_CALL; On the other hand, unsupported looks like a good choice here. BTW, we should be consistent in returning the same error if the order of calls is inverted (setLowBox + SetAppContainer) https://codereview.chromium.org/937353002/diff/240001/sandbox/win/src/sandbox... sandbox/win/src/sandbox_policy_base.cc:509: // AppContainer process through StartupInfoEx and other is by replacing nit: remove "by"? https://codereview.chromium.org/937353002/diff/240001/sandbox/win/src/sandbox... sandbox/win/src/sandbox_policy_base.cc:526: nit: move the empty line above the previous line. https://codereview.chromium.org/937353002/diff/240001/sandbox/win/src/target_... File sandbox/win/src/target_process.h (right): https://codereview.chromium.org/937353002/diff/240001/sandbox/win/src/target_... sandbox/win/src/target_process.h:48: // |set_lockdown_token_after_create| is set, the lockdown token nit: "when |foo| is set"?, "if |foo| is set"?
ptal. https://codereview.chromium.org/937353002/diff/240001/sandbox/win/src/policy_... File sandbox/win/src/policy_target.cc (left): https://codereview.chromium.org/937353002/diff/240001/sandbox/win/src/policy_... sandbox/win/src/policy_target.cc:91: On 2015/02/28 02:13:53, cpu wrote: > can you remind me of why this removal? I forgot the exact number, but a call from wininet comes through from here fails at sizeof(token) check and ends up modifying token to IdentifyLevel and then onwards all other calls fail. https://codereview.chromium.org/937353002/diff/240001/sandbox/win/src/sandbox... File sandbox/win/src/sandbox_policy_base.cc (right): https://codereview.chromium.org/937353002/diff/240001/sandbox/win/src/sandbox... sandbox/win/src/sandbox_policy_base.cc:159: On 2015/02/28 02:13:53, cpu wrote: > ::LocalFree() or at least we seem to be consistent on this file about that. Done. https://codereview.chromium.org/937353002/diff/240001/sandbox/win/src/sandbox... sandbox/win/src/sandbox_policy_base.cc:340: if (base::win::OSInfo::GetInstance()->version() < base::win::VERSION_WIN8) On 2015/02/28 02:13:53, cpu wrote: > why the long form instead of the GetVersinon() one? Copied from earlier usage within this file. like line 315. https://codereview.chromium.org/937353002/diff/240001/sandbox/win/src/sandbox... sandbox/win/src/sandbox_policy_base.cc:341: return SBOX_ERROR_UNEXPECTED_CALL; On 2015/02/28 02:13:53, cpu wrote: > return SBOX_ERROR_UNSUPPORTED Done. https://codereview.chromium.org/937353002/diff/240001/sandbox/win/src/sandbox... sandbox/win/src/sandbox_policy_base.cc:341: return SBOX_ERROR_UNEXPECTED_CALL; On 2015/02/28 02:23:08, rvargas wrote: > I'm not sure what is the right thing to do, but note that SetAppContainer() > returns SBOX_ALL_OK instead, which means that the caller doesn't need OS-version > dependent code. Acknowledged. https://codereview.chromium.org/937353002/diff/240001/sandbox/win/src/sandbox... sandbox/win/src/sandbox_policy_base.cc:345: return SBOX_ERROR_UNEXPECTED_CALL; On 2015/02/28 02:23:08, rvargas wrote: > On the other hand, unsupported looks like a good choice here. > > BTW, we should be consistent in returning the same error if the order of calls > is inverted (setLowBox + SetAppContainer) Done. https://codereview.chromium.org/937353002/diff/240001/sandbox/win/src/sandbox... sandbox/win/src/sandbox_policy_base.cc:509: // AppContainer process through StartupInfoEx and other is by replacing On 2015/02/28 02:23:08, rvargas wrote: > nit: remove "by"? Done. https://codereview.chromium.org/937353002/diff/240001/sandbox/win/src/sandbox... sandbox/win/src/sandbox_policy_base.cc:520: NtCreateLowBoxToken CreateLowBox = NULL; On 2015/02/28 02:13:53, cpu wrote: > function should be named CreateLowBoxToken, or is it clashing with something > else? Done. https://codereview.chromium.org/937353002/diff/240001/sandbox/win/src/sandbox... sandbox/win/src/sandbox_policy_base.cc:526: On 2015/02/28 02:23:08, rvargas wrote: > nit: move the empty line above the previous line. Done. https://codereview.chromium.org/937353002/diff/240001/sandbox/win/src/sandbox... File sandbox/win/src/sandbox_policy_base.h (right): https://codereview.chromium.org/937353002/diff/240001/sandbox/win/src/sandbox... sandbox/win/src/sandbox_policy_base.h:164: PSID lowbox_sid_; On 2015/02/28 02:13:53, cpu wrote: > instead pointer to sid? we don't use PHANDLE for example. Sorry, didn't understand. We need PSID for CreateLowBoxToken. https://codereview.chromium.org/937353002/diff/240001/sandbox/win/src/target_... File sandbox/win/src/target_process.cc (right): https://codereview.chromium.org/937353002/diff/240001/sandbox/win/src/target_... sandbox/win/src/target_process.cc:149: // an AppContainer token along with an impersonation token. On 2015/02/28 02:13:53, cpu wrote: > nice! Acknowledged. https://codereview.chromium.org/937353002/diff/240001/sandbox/win/src/target_... File sandbox/win/src/target_process.h (right): https://codereview.chromium.org/937353002/diff/240001/sandbox/win/src/target_... sandbox/win/src/target_process.h:48: // |set_lockdown_token_after_create| is set, the lockdown token On 2015/02/28 02:23:08, rvargas wrote: > nit: "when |foo| is set"?, "if |foo| is set"? Done.
lgtm https://codereview.chromium.org/937353002/diff/240001/sandbox/win/src/sandbox... File sandbox/win/src/sandbox_policy_base.cc (right): https://codereview.chromium.org/937353002/diff/240001/sandbox/win/src/sandbox... sandbox/win/src/sandbox_policy_base.cc:526: On 2015/02/28 02:33:47, Shrikant Kelkar wrote: > On 2015/02/28 02:23:08, rvargas wrote: > > nit: move the empty line above the previous line. > > Done. Don't see it (the variable declaration should be together with the function that uses it as opposed to with the previous block of code) https://codereview.chromium.org/937353002/diff/260001/sandbox/win/src/sandbox... File sandbox/win/src/sandbox_policy_base.cc (right): https://codereview.chromium.org/937353002/diff/260001/sandbox/win/src/sandbox... sandbox/win/src/sandbox_policy_base.cc:345: return SBOX_ERROR_UNSUPPORTED; This is now inconsistent. https://codereview.chromium.org/937353002/diff/260001/sandbox/win/src/target_... File sandbox/win/src/target_process.h (right): https://codereview.chromium.org/937353002/diff/260001/sandbox/win/src/target_... sandbox/win/src/target_process.h:48: // when |set_lockdown_token_after_create| is set, the lockdown token nit: Capitalize after period.
https://codereview.chromium.org/937353002/diff/240001/sandbox/win/src/sandbox... File sandbox/win/src/sandbox_policy_base.cc (right): https://codereview.chromium.org/937353002/diff/240001/sandbox/win/src/sandbox... sandbox/win/src/sandbox_policy_base.cc:526: On 2015/02/28 02:48:05, rvargas wrote: > On 2015/02/28 02:33:47, Shrikant Kelkar wrote: > > On 2015/02/28 02:23:08, rvargas wrote: > > > nit: move the empty line above the previous line. > > > > Done. > > Don't see it (the variable declaration should be together with the function that > uses it as opposed to with the previous block of code) Done. https://codereview.chromium.org/937353002/diff/260001/sandbox/win/src/sandbox... File sandbox/win/src/sandbox_policy_base.cc (right): https://codereview.chromium.org/937353002/diff/260001/sandbox/win/src/sandbox... sandbox/win/src/sandbox_policy_base.cc:345: return SBOX_ERROR_UNSUPPORTED; On 2015/02/28 02:48:05, rvargas wrote: > This is now inconsistent. Let's discuss Monday. https://codereview.chromium.org/937353002/diff/260001/sandbox/win/src/target_... File sandbox/win/src/target_process.h (right): https://codereview.chromium.org/937353002/diff/260001/sandbox/win/src/target_... sandbox/win/src/target_process.h:48: // when |set_lockdown_token_after_create| is set, the lockdown token On 2015/02/28 02:48:05, rvargas wrote: > nit: Capitalize after period. Done.
The CQ bit was checked by shrikant@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from cpu@chromium.org, rvargas@chromium.org Link to the patchset: https://codereview.chromium.org/937353002/#ps280001 (title: "Review changes...")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/937353002/280001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
The CQ bit was checked by shrikant@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/937353002/280001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
The CQ bit was checked by shrikant@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/937353002/280001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
The CQ bit was checked by shrikant@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from cpu@chromium.org, rvargas@chromium.org Link to the patchset: https://codereview.chromium.org/937353002/#ps300001 (title: "Sync to TOT to see if ios_dbg_simulator_ninja errors go away.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/937353002/300001
Message was sent while issue was closed.
Committed patchset #16 (id:300001)
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/f7540af7428f4b146136ec19b781886693f8c03f Cr-Commit-Position: refs/heads/master@{#318648} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
