Correctly test for canonicalized path in the CreateNamedPipe policy engine.

Correctly test for canonicalized path in the CreateNamedPipe policy engine.

BUG=334897
TEST=sbox_integration_tests.exe

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=247511

[Will Harris, 2014-01-24 22:44:01]

Changed the implementation to use the \\?\ syntax.

On Windows XP CreatePipe calls inside the sandbox do not even get intercepted,
so all the tests have to be disabled, but I reenabled them on Vista+

[jschuh, 2014-01-24 22:53:32]

https://codereview.chromium.org/145553007/diff/50001/sandbox/win/src/named_pi...
File sandbox/win/src/named_pipe_dispatcher.cc (right):

https://codereview.chromium.org/145553007/diff/50001/sandbox/win/src/named_pi...
sandbox/win/src/named_pipe_dispatcher.cc:62: 
I thought we discussed having the dispatcher also look for traversal sequences,
just in case there's some crazy corner case we're not aware of when using the
prefix? I was thinking you'd just use SplitString and iterate over the resulting
vector to catch any ".." strings.

[Will Harris, 2014-01-24 22:56:26]

On 2014/01/24 22:53:32, Justin Schuh wrote:
>
https://codereview.chromium.org/145553007/diff/50001/sandbox/win/src/named_pi...
> File sandbox/win/src/named_pipe_dispatcher.cc (right):
> 
>
https://codereview.chromium.org/145553007/diff/50001/sandbox/win/src/named_pi...
> sandbox/win/src/named_pipe_dispatcher.cc:62: 
> I thought we discussed having the dispatcher also look for traversal
sequences,
> just in case there's some crazy corner case we're not aware of when using the
> prefix? I was thinking you'd just use SplitString and iterate over the
resulting
> vector to catch any ".." strings.

could do that as well, but if I blacklist ".." strings then there is no way to
test that the \\?\ sequence is actually having any effect.  So I suppose it's a
question of whether we prefer stronger tests, or extra checks against known bad
sequences.  Happy to go either way.

[jschuh, 2014-01-24 23:06:01]

On 2014/01/24 22:56:26, Will Harris wrote:
> On 2014/01/24 22:53:32, Justin Schuh wrote:
> >
>
https://codereview.chromium.org/145553007/diff/50001/sandbox/win/src/named_pi...
> > File sandbox/win/src/named_pipe_dispatcher.cc (right):
> > 
> >
>
https://codereview.chromium.org/145553007/diff/50001/sandbox/win/src/named_pi...
> > sandbox/win/src/named_pipe_dispatcher.cc:62: 
> > I thought we discussed having the dispatcher also look for traversal
> sequences,
> > just in case there's some crazy corner case we're not aware of when using
the
> > prefix? I was thinking you'd just use SplitString and iterate over the
> resulting
> > vector to catch any ".." strings.
> 
> could do that as well, but if I blacklist ".." strings then there is no way to
> test that the \\?\ sequence is actually having any effect.  So I suppose it's
a
> question of whether we prefer stronger tests, or extra checks against known
bad
> sequences.  Happy to go either way.

You can write an in-process test that just verifies the namespacing behavior,
and have the sandbox test just verify failure.

[Will Harris, 2014-01-27 18:31:20]

PTAL - Added checks for ".." in the policy engine, and added an extra in-process
test to validate that \\?\ does what it should do (even on WinXP!)

[jschuh, 2014-01-27 20:29:47]

https://codereview.chromium.org/145553007/diff/120001/sandbox/win/src/named_p...
File sandbox/win/src/named_pipe_dispatcher.cc (right):

https://codereview.chromium.org/145553007/diff/120001/sandbox/win/src/named_p...
sandbox/win/src/named_pipe_dispatcher.cc:48: ipc->return_info.handle =
INVALID_HANDLE_VALUE;
Never put INVALID_HANDLE_VALUE in anything that may be passed cross-process (due
to the own-process pseudo-handle being the same as INVALID_HANDLE_VALUE).

https://codereview.chromium.org/145553007/diff/120001/sandbox/win/src/named_p...
sandbox/win/src/named_pipe_dispatcher.cc:63: for
(std::vector<base::string16>::const_iterator iter = paths.begin();
Don't you want to nest this inside the above loop, and split the substrings to
catch cases like "\../"?

[Will Harris, 2014-01-27 23:47:17]

added better / and \ checks.  Added a few more tests.

PTAL.  Thanks.

https://codereview.chromium.org/145553007/diff/120001/sandbox/win/src/named_p...
File sandbox/win/src/named_pipe_dispatcher.cc (right):

https://codereview.chromium.org/145553007/diff/120001/sandbox/win/src/named_p...
sandbox/win/src/named_pipe_dispatcher.cc:48: ipc->return_info.handle =
INVALID_HANDLE_VALUE;
On 2014/01/27 20:29:47, Justin Schuh wrote:
> Never put INVALID_HANDLE_VALUE in anything that may be passed cross-process
(due
> to the own-process pseudo-handle being the same as INVALID_HANDLE_VALUE).

Done.

https://codereview.chromium.org/145553007/diff/120001/sandbox/win/src/named_p...
sandbox/win/src/named_pipe_dispatcher.cc:63: for
(std::vector<base::string16>::const_iterator iter = paths.begin();
On 2014/01/27 20:29:47, Justin Schuh wrote:
> Don't you want to nest this inside the above loop, and split the substrings to
> catch cases like "\../"?

Done.

[jschuh, 2014-01-28 01:32:25]

lgtm

[commit-bot: I haz the power, 2014-01-28 06:29:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wfh@chromium.org/145553007/180001

[commit-bot: I haz the power, 2014-01-28 18:36:25]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2014-01-28 18:39:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wfh@chromium.org/145553007/200001

[commit-bot: I haz the power, 2014-01-28 21:27:11]

Change committed as 247511

[rvargas (doing something else), 2014-01-28 22:26:46]

https://codereview.chromium.org/145553007/diff/200001/sandbox/win/src/named_p...
File sandbox/win/src/named_pipe_dispatcher.cc (right):

https://codereview.chromium.org/145553007/diff/200001/sandbox/win/src/named_p...
sandbox/win/src/named_pipe_dispatcher.cc:54: for
(std::vector<base::string16>::const_iterator iter = paths.begin();
why not search ".." and fail right away if that is present? We don't have to
support arbitrary policy patterns.

https://codereview.chromium.org/145553007/diff/200001/sandbox/win/src/named_p...
sandbox/win/src/named_pipe_dispatcher.cc:58: iter2 != innerpaths.end(); ++iter2)
{
nit: indent one space less

https://codereview.chromium.org/145553007/diff/200001/sandbox/win/src/named_p...
sandbox/win/src/named_pipe_dispatcher.cc:79: name->replace(0, 4, L"\\\\\?\\");
we should not be fixing names. If we don't like it, we don't help the renderer.