Windows sbox: Warmup locales before sandbox lockdown (and tests)

Add sbox tests related to warming up of locales.
Warm up locales in LowerToken() after RevertToSelf() as existing warmup was not working on Win 8.1 x64.
Remove existing warmup which was done outside of LowerToken().

BUG=464430
Committed: https://crrev.com/1b10aabba1f519e1484f61c1d46caf4ac49a1f36
Cr-Commit-Position: refs/heads/master@{#347857}

[liamjm (20p), 2015-09-02 23:06:36]

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

[Will Harris, 2015-09-03 20:58:03]

https://codereview.chromium.org/1324523008/diff/1/sandbox/win/src/lpc_policy_...
File sandbox/win/src/lpc_policy_test.cc (right):

https://codereview.chromium.org/1324523008/diff/1/sandbox/win/src/lpc_policy_...
sandbox/win/src/lpc_policy_test.cc:24: ::GetUserDefaultLangID();
is there a way of verifying the return value here is valid? Or does this just
crash if the sandbox is not correctly warmed up?

Perhaps you could call GetUserDefaultLangID from the broker then pass this as a
parameter to the target and verify they are equal?

[liamjm (20p), 2015-09-04 17:46:20]

On 2015/09/03 20:58:03, Will Harris wrote:
>
https://codereview.chromium.org/1324523008/diff/1/sandbox/win/src/lpc_policy_...
> File sandbox/win/src/lpc_policy_test.cc (right):
> 
>
https://codereview.chromium.org/1324523008/diff/1/sandbox/win/src/lpc_policy_...
> sandbox/win/src/lpc_policy_test.cc:24: ::GetUserDefaultLangID();
> is there a way of verifying the return value here is valid? Or does this just
> crash if the sandbox is not correctly warmed up?
> 
> Perhaps you could call GetUserDefaultLangID from the broker then pass this as
a
> parameter to the target and verify they are equal?

Each call will crash if not warmed up suitably.

But good point re checking value. For each test, I've added code:
* in the broker to get the value
* pass the value as an arg to the test
* check the value in the sbox test.

[liamjm (20p), 2015-09-04 21:10:18]

liamjm@chromium.org changed reviewers:
+ isherman@chromium.org, jochen@chromium.org

[jschuh, 2015-09-04 22:26:25]

I am not here, but if I were I would note that the sandbox should warmup only
what it explicitly needs, and the onus is on the caller to perform any
additional warmup. So, I don't think this patch is a good idea unless there's
something in the sandbox code that's depending on the locale (which I don't
believe to be the case).

[Ilya Sherman, 2015-09-04 22:28:28]

histograms.xml lgtm

[Will Harris, 2015-09-04 22:43:28]

On 2015/09/04 22:26:25, jschuh (very slow) wrote:
> I am not here, but if I were I would note that the sandbox should warmup only
> what it explicitly needs, and the onus is on the caller to perform any
> additional warmup. So, I don't think this patch is a good idea unless there's
> something in the sandbox code that's depending on the locale (which I don't
> believe to be the case).

I agree but the issue here is that the warmup for the locale must go midway
through the call to LowerToken()

so either we split LowerToken() up into two parts and have the callers call both
in sequence, or we do what liamjm is doing here, I thought the latter seemed
more elegant.

[jschuh, 2015-09-04 22:52:59]

On 2015/09/04 22:43:28, Will Harris wrote:
> On 2015/09/04 22:26:25, jschuh (very slow) wrote:
> > I am not here, but if I were I would note that the sandbox should warmup
only
> > what it explicitly needs, and the onus is on the caller to perform any
> > additional warmup. So, I don't think this patch is a good idea unless
there's
> > something in the sandbox code that's depending on the locale (which I don't
> > believe to be the case).
> 
> I agree but the issue here is that the warmup for the locale must go midway
> through the call to LowerToken()
> 
> so either we split LowerToken() up into two parts and have the callers call
both
> in sequence, or we do what liamjm is doing here, I thought the latter seemed
> more elegant.

Why does the warmup need to be moved? I don't see anything earlier in LowerToken
that should require this. Unless warming up the locales is undoing the registry
cleanup that RegDisablePredefinedCache does, which would be bad because it means
we'd leave behind open handles to registry keys that should not be accessible
from inside the sandbox.

[liamjm (20p), 2015-09-04 23:13:58]

On 2015/09/04 22:52:59, jschuh (very slow) wrote:
> On 2015/09/04 22:43:28, Will Harris wrote:
> > On 2015/09/04 22:26:25, jschuh (very slow) wrote:
> > > I am not here, but if I were I would note that the sandbox should warmup
> only
> > > what it explicitly needs, and the onus is on the caller to perform any
> > > additional warmup. So, I don't think this patch is a good idea unless
> there's
> > > something in the sandbox code that's depending on the locale (which I
don't
> > > believe to be the case).
> > 
> > I agree but the issue here is that the warmup for the locale must go midway
> > through the call to LowerToken()
> > 
> > so either we split LowerToken() up into two parts and have the callers call
> both
> > in sequence, or we do what liamjm is doing here, I thought the latter seemed
> > more elegant.
> 
> Why does the warmup need to be moved? I don't see anything earlier in
LowerToken
> that should require this. Unless warming up the locales is undoing the
registry
> cleanup that RegDisablePredefinedCache does, which would be bad because it
means
> we'd leave behind open handles to registry keys that should not be accessible
> from inside the sandbox.

In my testing, the current warmup does not work on Win 8.1 x84 (but does work on
x86). On x64, when the process is impersonating, the locale information is not
cached, therefore it can't be warmed up. After RevertToSelf() is called, the
cache is used and warmup works. Currently RevertToSelf() is near the start of
LowerToken(), before the call to close the ALPC handles. The warmup needs to
happen between these points. 
This is commented in the CL:
https://codereview.chromium.org/1324523008/diff/20001/sandbox/win/src/target_...
As wfh@ said, another approach is to split LowerToken() up, but that seem uglier
and more complex for callers.
Thoughts? I'm happy with other approaches, this is the best (by my judgement) so
far.

[jschuh, 2015-09-04 23:49:33]

Ah, serves me right for not reading the whole CL. Sounds like a reasonable
solution under the circumstances.

On 2015/09/04 23:13:58, liamjm wrote:
> On 2015/09/04 22:52:59, jschuh (very slow) wrote:
> > On 2015/09/04 22:43:28, Will Harris wrote:
> > > On 2015/09/04 22:26:25, jschuh (very slow) wrote:
> > > > I am not here, but if I were I would note that the sandbox should warmup
> > only
> > > > what it explicitly needs, and the onus is on the caller to perform any
> > > > additional warmup. So, I don't think this patch is a good idea unless
> > there's
> > > > something in the sandbox code that's depending on the locale (which I
> don't
> > > > believe to be the case).
> > > 
> > > I agree but the issue here is that the warmup for the locale must go
midway
> > > through the call to LowerToken()
> > > 
> > > so either we split LowerToken() up into two parts and have the callers
call
> > both
> > > in sequence, or we do what liamjm is doing here, I thought the latter
seemed
> > > more elegant.
> > 
> > Why does the warmup need to be moved? I don't see anything earlier in
> LowerToken
> > that should require this. Unless warming up the locales is undoing the
> registry
> > cleanup that RegDisablePredefinedCache does, which would be bad because it
> means
> > we'd leave behind open handles to registry keys that should not be
accessible
> > from inside the sandbox.
> 
> In my testing, the current warmup does not work on Win 8.1 x84 (but does work
on
> x86). On x64, when the process is impersonating, the locale information is not
> cached, therefore it can't be warmed up. After RevertToSelf() is called, the
> cache is used and warmup works. Currently RevertToSelf() is near the start of
> LowerToken(), before the call to close the ALPC handles. The warmup needs to
> happen between these points. 
> This is commented in the CL:
>
https://codereview.chromium.org/1324523008/diff/20001/sandbox/win/src/target_...
> As wfh@ said, another approach is to split LowerToken() up, but that seem
uglier
> and more complex for callers.
> Thoughts? I'm happy with other approaches, this is the best (by my judgement)
so
> far.

[jochen (gone - plz use gerrit), 2015-09-08 12:29:09]

components & content lgtm

[Will Harris, 2015-09-09 00:26:16]

lgtm

[liamjm (20p), 2015-09-09 00:33:23]

The CQ bit was checked by liamjm@chromium.org

[commit-bot: I haz the power, 2015-09-09 00:33:49]

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

[commit-bot: I haz the power, 2015-09-09 08:57:41]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-09-09 08:59:03]

Patchset 2 (id:??) landed as
https://crrev.com/1b10aabba1f519e1484f61c1d46caf4ac49a1f36
Cr-Commit-Position: refs/heads/master@{#347857}

[liamjm (20p), 2015-09-11 20:09:05]

Reverted in
https://chromium.googlesource.com/chromium/src/+/5bd897d39d2a36399944f4d3c87f...

Re-opening to make fixes.

[jochen (gone - plz use gerrit), 2015-09-14 09:48:13]

can you please create a new CL for the fixes?

just upload the original patch as first patchset.

[liamjm (20p), 2015-10-21 01:19:25]

On 2015/09/14 09:48:13, jochen wrote:
> can you please create a new CL for the fixes?
> 
> just upload the original patch as first patchset.

Sure.
Here is the new CL
https://codereview.chromium.org/1419483002/