|
|
Created:
5 years, 10 months ago by Will Harris Modified:
5 years, 10 months ago CC:
chromium-reviews, kalyank, sadrul, rickyz+watch_chromium.org, wfh+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionReplace handles that the handle closer closes with dummy Events.
This is because on Windows, we close some handles opened by Windows DLLs to prevent them from leaking into sandboxed processes, and this can cause INVALID_HANDLE_EXCEPTION to be raised if "bad handle detection" gflag is enabled, as it currently is by default on Windows 10.
Only replace File and Event handles at the moment, as File handles are the ones causing the issues on Windows 10.
BUG=452613
TEST=sbox_integration_tests
TEST=Run Chrome on Windows 10 with crash reporting enabled, and check there are no crashes listed in chrome://crashes
Committed: https://crrev.com/b21f85ae8442e9fd831b70427c1c1919d8116af6
Cr-Commit-Position: refs/heads/master@{#317016}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : will put sandbox change in other CL #Patch Set 4 : only change win #
Total comments: 4
Patch Set 5 : address nits #Patch Set 6 : consume invalid handle exceptions on shutdown of sandboxed process #Patch Set 7 : Stuff closed handles with an empty Event #Patch Set 8 : only stuff events and file handles #
Total comments: 15
Patch Set 9 : address comments. add test. #Patch Set 10 : add max count #Patch Set 11 : remaining nits #
Total comments: 1
Patch Set 12 : add comment #
Messages
Total messages: 48 (14 generated)
wfh@chromium.org changed reviewers: + cpu@chromium.org
wfh@chromium.org changed reviewers: + rsesek@chromium.org
+rsesek for views on linux/mac changes.
On Mac and Linux, this will be adding uncalled code, since those platforms don't use the DLLLoader to start Chrome. I'm not sure if it's worth adding in calls on those platforms, since we don't have the issue being seen.
On 2015/02/12 18:59:28, Robert Sesek wrote: > On Mac and Linux, this will be adding uncalled code, since those platforms don't > use the DLLLoader to start Chrome. I'm not sure if it's worth adding in calls on > those platforms, since we don't have the issue being seen. Thanks Robert, I see now that client_util.cc is excluded from non Windows builds so I can isolate the changes to Windows. PTAL
LGTM https://codereview.chromium.org/919893002/diff/60001/components/crash/app/bre... File components/crash/app/breakpad_win.h (right): https://codereview.chromium.org/919893002/diff/60001/components/crash/app/bre... components/crash/app/breakpad_win.h:20: // Shuts down Breakpad. "Should be called immediately before exiting, after which crashes will not be caught." https://codereview.chromium.org/919893002/diff/60001/components/crash/app/bre... components/crash/app/breakpad_win.h:21: void ShutdownCrashReporter(); nit: blank line after
New patchsets have been uploaded after l-g-t-m from rsesek@chromium.org
Thanks for the review, Robert. https://codereview.chromium.org/919893002/diff/60001/components/crash/app/bre... File components/crash/app/breakpad_win.h (right): https://codereview.chromium.org/919893002/diff/60001/components/crash/app/bre... components/crash/app/breakpad_win.h:20: // Shuts down Breakpad. On 2015/02/12 19:18:04, Robert Sesek wrote: > "Should be called immediately before exiting, after which crashes will not be > caught." Done. https://codereview.chromium.org/919893002/diff/60001/components/crash/app/bre... components/crash/app/breakpad_win.h:21: void ShutdownCrashReporter(); On 2015/02/12 19:18:04, Robert Sesek wrote: > nit: blank line after Done.
cpu@chromium.org changed reviewers: + erikwright@chromium.org, siggi@chromium.org
will is going to add more context in the bug but I need to have siggi and erik opinionate on this CL. My current take is that is a no-go as is, but I do think we need to do something about the particular case we are hitting in w10. Somehow w10 is forcing our processes (maybe all processes) to have something like PROCESS_CREATION_MITIGATION_POLICY_STRICT_HANDLE_CHECKS_ALWAYS_ON
I commented on the bug. I think the solution of having a call into breakpad that happens on shutdown that just causes these exceptions to be eaten works for me. After all, there really isn't anything we can do about these exceptions on systems that have the strict handle checking enabled.
I retested patch set 5 and I am getting TERMINATION_STATUS_PROCESS_CRASHED from child sandboxed processes (as cpu/jschuh predicted) because INVALID_HANDLE_EXCEPTION changes the return code. patch set 6 fixes this by consuming the exception inside breakpad. I've tested this and both the crashes in chrome://crash and any UMA histograms for https://breakpad.appspot.com/7794002/ go away. This needs a corresponding CL in breakpad - https://breakpad.appspot.com/7794002/ PTAL
hmm vmware cut 'n' paste error - I meant ChildProcess.Crashed2 for this histogram for crashed process.
PTAL - I have the LGTM on the Breakpad CL - https://breakpad.appspot.com/7794002/ now, but won't commit until this approach has an LGTM here. Thanks!
On 2015/02/15 22:58:41, Will Harris wrote: > PTAL - I have the LGTM on the Breakpad CL - > https://breakpad.appspot.com/7794002/ now, but won't commit until this approach > has an LGTM here. Thanks! This LGTM after a fashion. It does have the problem that this'll obscure real handle issues happening during static uninitialization and the like, if the system's taking to enabling handle exceptions (which'd IMHO be a good thing). There's also the problem that closing these system handles could lead to double frees if system libraries take to managing these handles at unexpected times (I know nothing of what this is used/done for). It would IMHO be much preferable to stuff the handle slots we're closing - if we can. If these rude CloseHandle calls are happening early in the process lifetime (while it's still single threaded) you should be able to stuff the table slots with a DuplicateHandle loop? Something like bool StuffHandleSlot(HANDLE to_stuff) { HANDLE dummy = ::CreateEvent(...); // or similar std::vector<HANDLE> to_close; while (reinterpret_cast<uintptr_t>(dummy) < reinterpret_cast<uintptr_t>(to_stuff)) { to_close.push_back(dummy); dummy = ::DuplicateHandle(dummy, ...); } if (dummy != to_stuff) to_close.push_back(dummy); for (auto h : to_close) ::CloseHandle(h); return dummy == to_stuff; } might do the trick?
Also the CL description needs updating.
NVM, I guess the email thread retains an earlier description as subject. On Tue Feb 17 2015 at 10:23:32 AM <siggi@chromium.org> wrote: > Also the CL description needs updating. > > https://codereview.chromium.org/919893002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
New patchsets have been uploaded after l-g-t-m from siggi@chromium.org
wfh@chromium.org changed reviewers: + forshaw@chromium.org
Turns out, stuffing the handles with dummy Events works really well, and causes all EXCEPTION_INVALID_HANDLE to go away. I think an Event is the right object to choose, even though we're mostly closing File objects. Reasoning for this: - Events can't provide any means of sandbox escape - Handle closer is running post-lockdown so opening a File requires finding something that is not secured e.g. under a device namespace. I think this will give a net stability improvement because currently the next HANDLE that is opened will replace the one we closed, and so e.g. 3rd party DLLs that do handle opens might be using the same handle as the system DLLs - with this change, we can be sure that there can be no handle overlap. The only added risk is that previously operations such as WaitForSingleObject would be failing, while now they might hang.
PTAL. I've also only whitelisted handles of type File or Event for the handle stuffing.
> The only added > risk is that previously operations such as WaitForSingleObject would be failing, > while now they might hang. You could get around this by duplicating the event handle with no access rights (pass 0 for desired access and 0 for options). Without SYNCHRONIZE access right on the event handle WaitForSingleObject etc. will immediately fail with WAIT_FAILED return code.
https://codereview.chromium.org/919893002/diff/140001/sandbox/win/src/handle_... File sandbox/win/src/handle_closer_agent.cc (right): https://codereview.chromium.org/919893002/diff/140001/sandbox/win/src/handle_... sandbox/win/src/handle_closer_agent.cc:40: HANDLE dummy = ::CreateEvent(NULL, FALSE, FALSE, NULL); Can we just use one event object for all stuffed handles in a process? Each new event does consume some pool in the kernel for the event object which while not a massive burden especially on x64 there seems no reason to waste memory unnecessarily. The memory footprint of a process handle entry is ultimately lower than an event object. I can't see a security/compatibility risk of doing this considering what we're already breaking. https://codereview.chromium.org/919893002/diff/140001/sandbox/win/src/handle_... sandbox/win/src/handle_closer_agent.cc:42: if (dummy == INVALID_HANDLE_VALUE) While unlikely to fail CreateEvent actually returns NULL on error not INVALID_HANDLE_VALUE.
lgtm modulo a nit. https://codereview.chromium.org/919893002/diff/140001/sandbox/win/src/handle_... File sandbox/win/src/handle_closer_agent.cc (right): https://codereview.chromium.org/919893002/diff/140001/sandbox/win/src/handle_... sandbox/win/src/handle_closer_agent.cc:46: DWORD options = DUPLICATE_SAME_ACCESS; looks pretty reasonable - you may want to document the assumptions this relies on, namely what the handle reuse policy is, and the reliance on single-threadedness for this to work reliably.
looks good, I think it might desirable to set the security descriptor of the object so that waits fail with access denied. that is, remove the synchronize permission. https://codereview.chromium.org/919893002/diff/140001/sandbox/win/src/handle_... File sandbox/win/src/handle_closer_agent.cc (right): https://codereview.chromium.org/919893002/diff/140001/sandbox/win/src/handle_... sandbox/win/src/handle_closer_agent.cc:33: bool AttemptToStuffHandleSlot(HANDLE to_stuff, const base::string16& type) { to_stuff -> closed_handle ? https://codereview.chromium.org/919893002/diff/140001/sandbox/win/src/handle_... sandbox/win/src/handle_closer_agent.cc:40: HANDLE dummy = ::CreateEvent(NULL, FALSE, FALSE, NULL); On 2015/02/18 10:50:10, forshaw wrote: > Can we just use one event object for all stuffed handles in a process? Each new > event does consume some pool in the kernel for the event object which while not > a massive burden especially on x64 there seems no reason to waste memory > unnecessarily. The memory footprint of a process handle entry is ultimately > lower than an event object. I can't see a security/compatibility risk of doing > this considering what we're already breaking. given that is single threaded we can indeed use a function static and make it once. The just dup dup dup https://codereview.chromium.org/919893002/diff/140001/sandbox/win/src/handle_... sandbox/win/src/handle_closer_agent.cc:50: HANDLE dup_dummy; move dup_dummy below 51 https://codereview.chromium.org/919893002/diff/140001/sandbox/win/src/handle_... sandbox/win/src/handle_closer_agent.cc:177: // Attempt to stuff this handle with a new empty Event. "new dummy event"
oh one more, we need to limit the dup while loop to a reasonable number of loops, like 16 or 32.
> I think it might desirable to set the security descriptor of the object so that > waits fail with access denied. that is, remove the synchronize permission. Events are another one of those objects where the security wouldn't be honored if the object has no name, but as I mentioned a few comments ago you can dup with 0 access permissions and you'll lose SYNCHRONIZE. From what I can tell WaitForSingleObject etc. don't try and re-duplicate to get back a working handle.
On 2015/02/18 18:04:26, forshaw wrote: > Events are another one of those objects where the security wouldn't be honored > if the object has no name, but as I mentioned a few comments ago you can dup > with 0 access permissions and you'll lose SYNCHRONIZE. From what I can tell > WaitForSingleObject etc. don't try and re-duplicate to get back a working > handle. I'll be adding a sandbox test to make sure that target processes can't wait on the stuffed handles.
https://codereview.chromium.org/919893002/diff/140001/sandbox/win/src/handle_... File sandbox/win/src/handle_closer_agent.cc (right): https://codereview.chromium.org/919893002/diff/140001/sandbox/win/src/handle_... sandbox/win/src/handle_closer_agent.cc:55: break; come to think of it, another check you may want to add here is that you're making forward progress - e.g. you expect dup_dummy > dummy. That way you won't be caught infinitely looping, no matter what...
New patchsets have been uploaded after l-g-t-m from siggi@chromium.org
Patchset #9 (id:160001) has been deleted
PTAL - I think I've addressed everyone's comment, and I added a test that shows that it's all working, including not being able to wait on the new handle. https://codereview.chromium.org/919893002/diff/140001/sandbox/win/src/handle_... File sandbox/win/src/handle_closer_agent.cc (right): https://codereview.chromium.org/919893002/diff/140001/sandbox/win/src/handle_... sandbox/win/src/handle_closer_agent.cc:33: bool AttemptToStuffHandleSlot(HANDLE to_stuff, const base::string16& type) { On 2015/02/18 18:00:00, cpu wrote: > to_stuff -> closed_handle ? Done. https://codereview.chromium.org/919893002/diff/140001/sandbox/win/src/handle_... sandbox/win/src/handle_closer_agent.cc:40: HANDLE dummy = ::CreateEvent(NULL, FALSE, FALSE, NULL); we can't create the template handle static in this function because then it will be likely to be the same handle ID as the one we just closed, so instead I create it in constructor, and then close it again after we're done with the handle closer agent. https://codereview.chromium.org/919893002/diff/140001/sandbox/win/src/handle_... sandbox/win/src/handle_closer_agent.cc:42: if (dummy == INVALID_HANDLE_VALUE) On 2015/02/18 10:50:10, forshaw wrote: > While unlikely to fail CreateEvent actually returns NULL on error not > INVALID_HANDLE_VALUE. Done. https://codereview.chromium.org/919893002/diff/140001/sandbox/win/src/handle_... sandbox/win/src/handle_closer_agent.cc:46: DWORD options = DUPLICATE_SAME_ACCESS; changed this to 0 access as per other comments. https://codereview.chromium.org/919893002/diff/140001/sandbox/win/src/handle_... sandbox/win/src/handle_closer_agent.cc:50: HANDLE dup_dummy; On 2015/02/18 18:00:00, cpu wrote: > move dup_dummy below 51 Acknowledged. https://codereview.chromium.org/919893002/diff/140001/sandbox/win/src/handle_... sandbox/win/src/handle_closer_agent.cc:55: break; On 2015/02/18 19:02:47, Sigurður Ásgeirsson wrote: > come to think of it, another check you may want to add here is that you're > making forward progress - e.g. you expect dup_dummy > dummy. > That way you won't be caught infinitely looping, no matter what... We can't do this because we don't know if the handle we are trying to close might have been created before the handle closer initialized the dummy template handle, and thus already be lower. In experiments I've found that the first DuplicateHandle always dups into the target handle ID. https://codereview.chromium.org/919893002/diff/140001/sandbox/win/src/handle_... sandbox/win/src/handle_closer_agent.cc:177: // Attempt to stuff this handle with a new empty Event. On 2015/02/18 18:00:00, cpu wrote: > "new dummy event" Done.
lgtm https://codereview.chromium.org/919893002/diff/220001/sandbox/win/src/handle_... File sandbox/win/src/handle_closer_agent.cc (right): https://codereview.chromium.org/919893002/diff/220001/sandbox/win/src/handle_... sandbox/win/src/handle_closer_agent.cc:47: write a comment here about the technique including the dup with 0 access.
New patchsets have been uploaded after l-g-t-m from cpu@chromium.org
The CQ bit was checked by wfh@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/919893002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by wfh@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/919893002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
this is a flake caused by 2f7a8e80b83aa581aa6de86117a692093d16c217 which has already been reverted. I suppose I keep trying to land this. I want to get tomorrow's Canary.
The CQ bit was checked by wfh@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/919893002/240001
Message was sent while issue was closed.
Committed patchset #12 (id:240001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/b21f85ae8442e9fd831b70427c1c1919d8116af6 Cr-Commit-Position: refs/heads/master@{#317016} |