CSRSS lockdown: destroy CSRSS heap

CSRSS lockdown: destroy CSRSS heap

A heap is created by CSR client that is used to share memory between the client and csrss.exe (the server). We destroy this heap to ensure all heaps are valid (that any callers of GetProcessHeaps() receive valid heaps).


BUG=464430
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng

Review-Url: https://codereview.chromium.org/2726733003
Cr-Commit-Position: refs/heads/master@{#469175}
Committed: https://chromium.googlesource.com/chromium/src/+/6a8fe629a1676d9de015dbac090145119e2c62ef

[liamjm (20p), 2017-02-28 23:58:32]

Description was changed from

==========
Merge branch 'master' of https://chromium.googlesource.com/chromium/src into
destroy_heap


use heapdestroy() not rtldestroyheap(), plus some cleanups


destroy heap POC with test


destroy heap with hack POC

BUG=
==========

to

==========
Merge branch 'master' of https://chromium.googlesource.com/chromium/src into
destroy_heap


use heapdestroy() not rtldestroyheap(), plus some cleanups


destroy heap POC with test


destroy heap with hack POC

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

[liamjm (20p), 2017-03-01 00:10:49]

Description was changed from

==========
Merge branch 'master' of https://chromium.googlesource.com/chromium/src into
destroy_heap


use heapdestroy() not rtldestroyheap(), plus some cleanups


destroy heap POC with test


destroy heap with hack POC

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

to

==========
A heap is created by CSR client that is used to share memory between the client
and csrss.exe (the server). We destroy this heap to ensure all heaps are valid
(that any callers of GetProcessHeaps() receive valid heaps).


BUG=464430
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

[liamjm (20p), 2017-03-22 18:48:21]

liamjm@chromium.org changed reviewers:
+ pennymac@chromium.org, wfh@chromium.org

[liamjm (20p), 2017-03-22 18:48:21]

Hi,

Please have a look at this code, now with some tests of the heap functions

[Will Harris, 2017-03-22 19:21:49]

https://codereview.chromium.org/2726733003/diff/100001/sandbox/win/src/heap_h...
File sandbox/win/src/heap_helper.cc (right):

https://codereview.chromium.org/2726733003/diff/100001/sandbox/win/src/heap_h...
sandbox/win/src/heap_helper.cc:24: _HEAP* heap =
reinterpret_cast<_HEAP*>(handle);
is there any validation that can be done here that the code is actually dealing
with a valid heap handle?

https://codereview.chromium.org/2726733003/diff/100001/sandbox/win/src/heap_h...
sandbox/win/src/heap_helper.cc:34: // Let's search for the CSR port heap handle,
which we identify purely based
nit: in chromium convention is to avoid use of 'us' and 'we' and prefer to just
state the operation happening

e.g.

"Search for the CSR port heap handle, identified purely based on flags."

or something like that. (also elsewhere)

https://codereview.chromium.org/2726733003/diff/100001/sandbox/win/src/lpc_po...
File sandbox/win/src/lpc_policy_test.cc (right):

https://codereview.chromium.org/2726733003/diff/100001/sandbox/win/src/lpc_po...
sandbox/win/src/lpc_policy_test.cc:194: TEST(LpcPolicyTest, TestHeapFlags) {
nice test!

https://codereview.chromium.org/2726733003/diff/100001/sandbox/win/src/target...
File sandbox/win/src/target_services.cc (right):

https://codereview.chromium.org/2726733003/diff/100001/sandbox/win/src/target...
sandbox/win/src/target_services.cc:55: PVOID csr_port_heap =
sandbox::FindCsrPortHeap();
implicit cast from HANDLE to PVOID - is this what you intend?

https://codereview.chromium.org/2726733003/diff/100001/sandbox/win/src/target...
sandbox/win/src/target_services.cc:55: PVOID csr_port_heap =
sandbox::FindCsrPortHeap();
are we not already in sandbox namespace. hmm? strange

https://codereview.chromium.org/2726733003/diff/100001/sandbox/win/src/target...
sandbox/win/src/target_services.cc:56: if (nullptr == csr_port_heap) {
!csr_port_heap

https://codereview.chromium.org/2726733003/diff/100001/sandbox/win/src/target...
sandbox/win/src/target_services.cc:57: LOG(ERROR) << "Failed to find CSR Port
heap handle" return false;
win\src\target_services.cc(57): error C2143: syntax error: missing ';' before
'return'

[liamjm (20p), 2017-04-14 17:27:20]

thanks. PTAL.

https://codereview.chromium.org/2726733003/diff/100001/sandbox/win/src/heap_h...
File sandbox/win/src/heap_helper.cc (right):

https://codereview.chromium.org/2726733003/diff/100001/sandbox/win/src/heap_h...
sandbox/win/src/heap_helper.cc:24: _HEAP* heap =
reinterpret_cast<_HEAP*>(handle);
On 2017/03/22 19:21:49, Will Harris wrote:
> is there any validation that can be done here that the code is actually
dealing
> with a valid heap handle?

I've added nullptr checks.

We could use ntdll!RtlValidateHeap to check if this a valid heap?

Or we could add a check of the signature (ie magic bytes) (something similiar to
ntdll!RtlpCheckHeapSignature, which is called by ntdll!RtlValidateHeap)

Thoughts?

https://codereview.chromium.org/2726733003/diff/100001/sandbox/win/src/heap_h...
sandbox/win/src/heap_helper.cc:34: // Let's search for the CSR port heap handle,
which we identify purely based
On 2017/03/22 19:21:49, Will Harris wrote:
> nit: in chromium convention is to avoid use of 'us' and 'we' and prefer to
just
> state the operation happening
> 
> e.g.
> 
> "Search for the CSR port heap handle, identified purely based on flags."
> 
> or something like that. (also elsewhere)

Done.

https://codereview.chromium.org/2726733003/diff/100001/sandbox/win/src/lpc_po...
File sandbox/win/src/lpc_policy_test.cc (right):

https://codereview.chromium.org/2726733003/diff/100001/sandbox/win/src/lpc_po...
sandbox/win/src/lpc_policy_test.cc:194: TEST(LpcPolicyTest, TestHeapFlags) {
On 2017/03/22 19:21:49, Will Harris wrote:
> nice test!

Acknowledged.

https://codereview.chromium.org/2726733003/diff/100001/sandbox/win/src/target...
File sandbox/win/src/target_services.cc (right):

https://codereview.chromium.org/2726733003/diff/100001/sandbox/win/src/target...
sandbox/win/src/target_services.cc:55: PVOID csr_port_heap =
sandbox::FindCsrPortHeap();
On 2017/03/22 19:21:49, Will Harris wrote:
> are we not already in sandbox namespace. hmm? strange

Yeah... 
Just in an unnamed namespace at this point.

We could move a couple of functions into the namespace, if desired.

https://codereview.chromium.org/2726733003/diff/100001/sandbox/win/src/target...
sandbox/win/src/target_services.cc:55: PVOID csr_port_heap =
sandbox::FindCsrPortHeap();
On 2017/03/22 19:21:49, Will Harris wrote:
> implicit cast from HANDLE to PVOID - is this what you intend?

No. 
changed to HANDLE.

Thanks.

https://codereview.chromium.org/2726733003/diff/100001/sandbox/win/src/target...
sandbox/win/src/target_services.cc:56: if (nullptr == csr_port_heap) {
On 2017/03/22 19:21:49, Will Harris wrote:
> !csr_port_heap

Done.

https://codereview.chromium.org/2726733003/diff/100001/sandbox/win/src/target...
sandbox/win/src/target_services.cc:57: LOG(ERROR) << "Failed to find CSR Port
heap handle" return false;
On 2017/03/22 19:21:49, Will Harris wrote:
> win\src\target_services.cc(57): error C2143: syntax error: missing ';' before
> 'return'

Done.

[Will Harris, 2017-05-01 18:33:16]

this is looking good, but the tests still appear to be failing, can you take
another look?

https://codereview.chromium.org/2726733003/diff/100001/sandbox/win/src/target...
File sandbox/win/src/target_services.cc (right):

https://codereview.chromium.org/2726733003/diff/100001/sandbox/win/src/target...
sandbox/win/src/target_services.cc:55: PVOID csr_port_heap =
sandbox::FindCsrPortHeap();
On 2017/04/14 17:27:20, liamjm (20p) wrote:
> On 2017/03/22 19:21:49, Will Harris wrote:
> > are we not already in sandbox namespace. hmm? strange
> 
> Yeah... 
> Just in an unnamed namespace at this point.
> 
> We could move a couple of functions into the namespace, if desired.

yes it seems to make sense to move all these functions to the sandbox anonymous
namespace, and then remove the sandbox:: parts below

[liamjm (20p), 2017-05-03 17:50:15]

On 2017/05/01 18:33:16, Will Harris wrote:
> this is looking good, but the tests still appear to be failing, can you take
> another look?
> 
>
https://codereview.chromium.org/2726733003/diff/100001/sandbox/win/src/target...
> File sandbox/win/src/target_services.cc (right):
> 
>
https://codereview.chromium.org/2726733003/diff/100001/sandbox/win/src/target...
> sandbox/win/src/target_services.cc:55: PVOID csr_port_heap =
> sandbox::FindCsrPortHeap();
> On 2017/04/14 17:27:20, liamjm (20p) wrote:
> > On 2017/03/22 19:21:49, Will Harris wrote:
> > > are we not already in sandbox namespace. hmm? strange
> > 
> > Yeah... 
> > Just in an unnamed namespace at this point.
> > 
> > We could move a couple of functions into the namespace, if desired.
> 
> yes it seems to make sense to move all these functions to the sandbox
anonymous
> namespace, and then remove the sandbox:: parts below

Blarg. Broke the compile. Fixed this and move the namespace inside sandbox in
target_services.cc.

Still need to ensure that tests pass on win version < 10. Please wait until this
before reviewing.

[liamjm (20p), 2017-05-03 19:33:11]

On 2017/05/03 17:50:15, liamjm (20p) wrote:
> On 2017/05/01 18:33:16, Will Harris wrote:
> > this is looking good, but the tests still appear to be failing, can you take
> > another look?
> > 
> >
>
https://codereview.chromium.org/2726733003/diff/100001/sandbox/win/src/target...
> > File sandbox/win/src/target_services.cc (right):
> > 
> >
>
https://codereview.chromium.org/2726733003/diff/100001/sandbox/win/src/target...
> > sandbox/win/src/target_services.cc:55: PVOID csr_port_heap =
> > sandbox::FindCsrPortHeap();
> > On 2017/04/14 17:27:20, liamjm (20p) wrote:
> > > On 2017/03/22 19:21:49, Will Harris wrote:
> > > > are we not already in sandbox namespace. hmm? strange
> > > 
> > > Yeah... 
> > > Just in an unnamed namespace at this point.
> > > 
> > > We could move a couple of functions into the namespace, if desired.
> > 
> > yes it seems to make sense to move all these functions to the sandbox
> anonymous
> > namespace, and then remove the sandbox:: parts below
> 
> Blarg. Broke the compile. Fixed this and move the namespace inside sandbox in
> target_services.cc.
> 
> Still need to ensure that tests pass on win version < 10. Please wait until
this
> before reviewing.

Trybots passed across range of OSes.
 PTAL, thanks.

[Will Harris, 2017-05-03 19:35:30]

Description was changed from

==========
A heap is created by CSR client that is used to share memory between the client
and csrss.exe (the server). We destroy this heap to ensure all heaps are valid
(that any callers of GetProcessHeaps() receive valid heaps).


BUG=464430
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

to

==========
CSRSS lockdown: destroy CSRSS heap

A heap is created by CSR client that is used to share memory between the client
and csrss.exe (the server). We destroy this heap to ensure all heaps are valid
(that any callers of GetProcessHeaps() receive valid heaps).


BUG=464430
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

[Will Harris, 2017-05-03 19:36:27]

lgtm

[liamjm (20p), 2017-05-03 19:37:38]

The CQ bit was checked by liamjm@chromium.org

[commit-bot: I haz the power, 2017-05-03 19:38:21]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[liamjm (20p), 2017-05-03 20:33:54]

The CQ bit was checked by liamjm@chromium.org

[liamjm (20p), 2017-05-03 20:33:54]

The patchset sent to the CQ was uploaded after l-g-t-m from wfh@chromium.org
Link to the patchset: https://codereview.chromium.org/2726733003/#ps240001
(title: "Merge branch 'master' of https://chromium.googlesource.com/chromium/src
into destroy_heap")

[commit-bot: I haz the power, 2017-05-03 20:34:39]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-03 22:40:51]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1493843634293140,
"parent_rev": "f51689179bfbf2a29e2870e5d5081b51fa49a6a1", "commit_rev":
"6a8fe629a1676d9de015dbac090145119e2c62ef"}

[commit-bot: I haz the power, 2017-05-03 22:41:05]

Description was changed from

==========
CSRSS lockdown: destroy CSRSS heap

A heap is created by CSR client that is used to share memory between the client
and csrss.exe (the server). We destroy this heap to ensure all heaps are valid
(that any callers of GetProcessHeaps() receive valid heaps).


BUG=464430
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

to

==========
CSRSS lockdown: destroy CSRSS heap

A heap is created by CSR client that is used to share memory between the client
and csrss.exe (the server). We destroy this heap to ensure all heaps are valid
(that any callers of GetProcessHeaps() receive valid heaps).


BUG=464430
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng

Review-Url: https://codereview.chromium.org/2726733003
Cr-Commit-Position: refs/heads/master@{#469175}
Committed:
https://chromium.googlesource.com/chromium/src/+/6a8fe629a1676d9de015dbac0901...
==========

[commit-bot: I haz the power, 2017-05-03 22:41:06]

Committed patchset #13 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/6a8fe629a1676d9de015dbac0901...

[dcheng, 2017-05-03 23:26:04]

dcheng@chromium.org changed reviewers:
+ dcheng@chromium.org

[dcheng, 2017-05-03 23:26:05]

drive-by

https://codereview.chromium.org/2726733003/diff/240001/sandbox/win/src/heap_h...
File sandbox/win/src/heap_helper.cc (right):

https://codereview.chromium.org/2726733003/diff/240001/sandbox/win/src/heap_h...
sandbox/win/src/heap_helper.cc:51: LOG(ERROR) << "Unable to get flags for this
heap";
Is there a reason these are LOG(ERROR) instead of DLOG? It doesn't seem
particularly actionable to end users and takes up binary space (logging strings
aren't dropped, even in official builds).

[Will Harris, 2017-05-03 23:28:24]

On 2017/05/03 23:26:05, dcheng wrote:
> drive-by
> 
>
https://codereview.chromium.org/2726733003/diff/240001/sandbox/win/src/heap_h...
> File sandbox/win/src/heap_helper.cc (right):
> 
>
https://codereview.chromium.org/2726733003/diff/240001/sandbox/win/src/heap_h...
> sandbox/win/src/heap_helper.cc:51: LOG(ERROR) << "Unable to get flags for this
> heap";
> Is there a reason these are LOG(ERROR) instead of DLOG? It doesn't seem
> particularly actionable to end users and takes up binary space (logging
strings
> aren't dropped, even in official builds).

This is true, I wonder why presubmit didn't pick this up...

The reason why LOG(ERROR) is used is because sandbox/win uses this a lot. We
probably need a bug to just clean these all up.

[Bret, 2017-05-06 00:00:13]

I just pulled and this is causing all my tabs to be sad as soon as they start
up. Replacing "return false" with "return true" in CsrssDisconnectCleanup makes
it stop.

[Will Harris, 2017-05-06 02:29:30]

On 2017/05/06 00:00:13, Bret Sepulveda wrote:
> I just pulled and this is causing all my tabs to be sad as soon as they start
> up. Replacing "return false" with "return true" in CsrssDisconnectCleanup
makes
> it stop.

which is strange, because the code has been in since 60.0.3089.0. Are you
running with --enable-features=EnableCsrssLockdown?

[Bret, 2017-05-09 23:31:46]

On 2017/05/06 02:29:30, Will Harris wrote:
> On 2017/05/06 00:00:13, Bret Sepulveda wrote:
> > I just pulled and this is causing all my tabs to be sad as soon as they
start
> > up. Replacing "return false" with "return true" in CsrssDisconnectCleanup
> makes
> > it stop.
> 
> which is strange, because the code has been in since 60.0.3089.0. Are you
> running with --enable-features=EnableCsrssLockdown?

I hadn't pulled in a while, and I wasn't running with any flags.

I pulled again and it's not happening now, but I noticed you didn't actually
change anything, so I'm very confused. At least it's not causing me trouble any
more.