Added protection against mapping image sections between processes.

Added protection against mapping image sections between processes.
This patch adds guards to stop SharedMemory objects from mapping Windows
image sections which could come from untrusted sources.

BUG=564238
Committed: https://crrev.com/0474abea8469d78ce3988364ee273984ac49a9f2
Cr-Commit-Position: refs/heads/master@{#365984}

[forshaw, 2015-12-16 15:06:32]

forshaw@chromium.org changed reviewers:
+ nasko@chromium.org, thestig@chromium.org, wfh@chromium.org

[forshaw, 2015-12-16 15:06:32]

thestig@ could you review the changes to base for me.
wfh@ could you look at this from a Windows security perspective.
nasko@ could you look at the change to sandbox_init_win.cc.

If you need more context on the changes which aren't present in the issue
tracker let me know.

Thanks.

[Will Harris, 2015-12-16 18:02:38]

On 2015/12/16 15:06:32, forshaw wrote:
> thestig@ could you review the changes to base for me.
> wfh@ could you look at this from a Windows security perspective.
> nasko@ could you look at the change to sandbox_init_win.cc.
> 
> If you need more context on the changes which aren't present in the issue
> tracker let me know.
> 
> Thanks.

it seems there are quite a lot of deprecated functions here, see
crbug.com/345734 in particular the name, and some of the methods here don't even
seem to be called from anywhere in Chrome.

I wonder if it's worth clearing up crbug.com/345734 in particular the code
around anonymous sections, before embarking on this? It might make this CL
easier?

[Lei Zhang, 2015-12-16 19:57:48]

On 2015/12/16 18:02:38, Will Harris wrote:
> On 2015/12/16 15:06:32, forshaw wrote:
> > thestig@ could you review the changes to base for me.
> > wfh@ could you look at this from a Windows security perspective.
> > nasko@ could you look at the change to sandbox_init_win.cc.
> > 
> > If you need more context on the changes which aren't present in the issue
> > tracker let me know.
> > 
> > Thanks.
> 
> it seems there are quite a lot of deprecated functions here, see
> crbug.com/345734 in particular the name, and some of the methods here don't
even
> seem to be called from anywhere in Chrome.
> 
> I wonder if it's worth clearing up crbug.com/345734 in particular the code
> around anonymous sections, before embarking on this? It might make this CL
> easier?

I think only one of the ctors being modified might be deprecated. Everything
else (MapAt, Open, etc) are here to stay. Why don't we just go ahead and review
it? It's not that big of a CL and it's all written already.

[Lei Zhang, 2015-12-16 20:44:11]

https://codereview.chromium.org/1501003002/diff/40001/base/memory/shared_memo...
File base/memory/shared_memory_unittest.cc (right):

https://codereview.chromium.org/1501003002/diff/40001/base/memory/shared_memo...
base/memory/shared_memory_unittest.cc:577: const char* kTestSectionName =
"UnsafeImageSection";
const char kFoo[] ?

https://codereview.chromium.org/1501003002/diff/40001/base/memory/shared_memo...
base/memory/shared_memory_unittest.cc:607: // Check that a handle without
SECTION_QUERY also can't be mapped as it can't
This should not happen with IPC under normal operation, right? I suppose if we
hit this case with this CL landed, we'll know where we lack test coverage.

https://codereview.chromium.org/1501003002/diff/40001/base/memory/shared_memo...
base/memory/shared_memory_unittest.cc:616: ::GetCurrentProcess(),
&handle_no_query, FILE_MAP_READ, FALSE, 0));
If you don't need FILE_MAP_READ, you can just call
SharedMemory::DuplicateHandle().

https://codereview.chromium.org/1501003002/diff/40001/base/memory/shared_memo...
File base/memory/shared_memory_win.cc (right):

https://codereview.chromium.org/1501003002/diff/40001/base/memory/shared_memo...
base/memory/shared_memory_win.cc:50: DCHECK(nt_query_section_func != nullptr);
omit: != nullptr

https://codereview.chromium.org/1501003002/diff/40001/base/memory/shared_memo...
base/memory/shared_memory_win.cc:237: if (mapped_file_ == nullptr)
if (!mapped_file_)

[forshaw, 2015-12-17 11:38:20]

thestig@ PTAL, done the fixes from the review.

https://codereview.chromium.org/1501003002/diff/40001/base/memory/shared_memo...
File base/memory/shared_memory_unittest.cc (right):

https://codereview.chromium.org/1501003002/diff/40001/base/memory/shared_memo...
base/memory/shared_memory_unittest.cc:577: const char* kTestSectionName =
"UnsafeImageSection";
On 2015/12/16 20:44:10, Lei Zhang wrote:
> const char kFoo[] ?

Done.

https://codereview.chromium.org/1501003002/diff/40001/base/memory/shared_memo...
base/memory/shared_memory_unittest.cc:607: // Check that a handle without
SECTION_QUERY also can't be mapped as it can't
On 2015/12/16 20:44:10, Lei Zhang wrote:
> This should not happen with IPC under normal operation, right? I suppose if we
> hit this case with this CL landed, we'll know where we lack test coverage.

Yes this shouldn't happen as long as I've caught all the places where section
handles are duplicated. But I think it's valuable to test.

https://codereview.chromium.org/1501003002/diff/40001/base/memory/shared_memo...
base/memory/shared_memory_unittest.cc:616: ::GetCurrentProcess(),
&handle_no_query, FILE_MAP_READ, FALSE, 0));
On 2015/12/16 20:44:10, Lei Zhang wrote:
> If you don't need FILE_MAP_READ, you can just call
> SharedMemory::DuplicateHandle().

It needs to be explicitly duplicated as SharedMemory::DuplicateHandle will
create a new handle with all the same access rights as the original one. This
needs to explicitly remove the SECTION_QUERY right to do the test.

https://codereview.chromium.org/1501003002/diff/40001/base/memory/shared_memo...
File base/memory/shared_memory_win.cc (right):

https://codereview.chromium.org/1501003002/diff/40001/base/memory/shared_memo...
base/memory/shared_memory_win.cc:50: DCHECK(nt_query_section_func != nullptr);
On 2015/12/16 20:44:10, Lei Zhang wrote:
> omit: != nullptr

Done.

https://codereview.chromium.org/1501003002/diff/40001/base/memory/shared_memo...
base/memory/shared_memory_win.cc:237: if (mapped_file_ == nullptr)
On 2015/12/16 20:44:10, Lei Zhang wrote:
> if (!mapped_file_)

Done.

[Lei Zhang, 2015-12-17 19:16:30]

lgtm if wfh@ is happy.

[Will Harris, 2015-12-17 23:34:04]

lgtm

[nasko, 2015-12-18 00:15:25]

LGTM based on wfh@'s review.

[forshaw, 2015-12-18 00:18:49]

The CQ bit was checked by forshaw@chromium.org

[forshaw, 2015-12-18 00:19:26]

On 2015/12/18 00:15:25, nasko wrote:
> LGTM based on wfh@'s review.

Thanks everyone for the reviews, it's very much appreciated.

[commit-bot: I haz the power, 2015-12-18 00:19:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1501003002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1501003002/60001

[commit-bot: I haz the power, 2015-12-18 02:17:06]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-12-18 02:18:04]

Description was changed from

==========
Added protection against mapping image sections between processes.
This patch adds guards to stop SharedMemory objects from mapping Windows
image sections which could come from untrusted sources.

BUG=564238
==========

to

==========
Added protection against mapping image sections between processes.
This patch adds guards to stop SharedMemory objects from mapping Windows
image sections which could come from untrusted sources.

BUG=564238

Committed: https://crrev.com/0474abea8469d78ce3988364ee273984ac49a9f2
Cr-Commit-Position: refs/heads/master@{#365984}
==========

[commit-bot: I haz the power, 2015-12-18 02:18:05]

Patchset 4 (id:??) landed as
https://crrev.com/0474abea8469d78ce3988364ee273984ac49a9f2
Cr-Commit-Position: refs/heads/master@{#365984}