Added impersonation of the anonymous token around CloseClipboard

Added impersonation of the anonymous token around CloseClipboard

This patch adds impersonation of the anonymous token around calls
to the CloseClipboard system call. On Windows 8+ the win32k driver
captures the access token of the caller and makes it available to
other users on the desktop through the system call
GetClipboardAccessToken. This introduces a risk of privilege
escalation in sandboxed processes. By performing the impersonation
then whenever Chrome writes data to the clipboard only the anonymous
token is available.

BUG=440693
Committed: https://crrev.com/56332e21e9930fba480556fbc96c748994b4a7b7
Cr-Commit-Position: refs/heads/master@{#308372}

[forshaw, 2014-12-11 17:09:03]

forshaw@chromium.org changed reviewers:
+ dcheng@chromium.org, jschuh@chromium.org

[forshaw, 2014-12-11 17:09:04]

Please can you review this change. This to to harden token handling on Win8+
although it shouldn't affect earlier versions of Windows.

[jschuh, 2014-12-11 17:53:06]

Looks good with a nit...

https://codereview.chromium.org/792413003/diff/1/remoting/host/clipboard_win.cc
File remoting/host/clipboard_win.cc (right):

https://codereview.chromium.org/792413003/diff/1/remoting/host/clipboard_win....
remoting/host/clipboard_win.cc:35:
::ImpersonateAnonymousToken(GetCurrentThread());
I know the impersonate *shouldn't* fail, but it's best to catch it regardless.
So, something like:

  BOOL shouldRevert = ::ImpersonateAnonymousToken(GetCurrentThread());
  DCHECK(shouldRevert);
  ::CloseClipboard();
  if (shouldRevert)
    ::RevertToSelf();

[dcheng, 2014-12-11 18:18:23]

https://codereview.chromium.org/792413003/diff/1/remoting/host/clipboard_win.cc
File remoting/host/clipboard_win.cc (right):

https://codereview.chromium.org/792413003/diff/1/remoting/host/clipboard_win....
remoting/host/clipboard_win.cc:35:
::ImpersonateAnonymousToken(GetCurrentThread());
On 2014/12/11 at 17:53:06, jschuh wrote:
> I know the impersonate *shouldn't* fail, but it's best to catch it regardless.
So, something like:
> 
>   BOOL shouldRevert = ::ImpersonateAnonymousToken(GetCurrentThread());
>   DCHECK(shouldRevert);
>   ::CloseClipboard();
>   if (shouldRevert)
>     ::RevertToSelf();

Can we use a scoper to wrap this logic?
... also, unrelated but... what is this API even used for?

[jschuh, 2014-12-11 18:33:51]

On 2014/12/11 18:18:23, dcheng wrote:
>
https://codereview.chromium.org/792413003/diff/1/remoting/host/clipboard_win.cc
> File remoting/host/clipboard_win.cc (right):
> 
>
https://codereview.chromium.org/792413003/diff/1/remoting/host/clipboard_win....
> remoting/host/clipboard_win.cc:35:
> ::ImpersonateAnonymousToken(GetCurrentThread());
> On 2014/12/11 at 17:53:06, jschuh wrote:
> > I know the impersonate *shouldn't* fail, but it's best to catch it
regardless.
> So, something like:
> > 
> >   BOOL shouldRevert = ::ImpersonateAnonymousToken(GetCurrentThread());
> >   DCHECK(shouldRevert);
> >   ::CloseClipboard();
> >   if (shouldRevert)
> >     ::RevertToSelf();
> 
> Can we use a scoper to wrap this logic?

Pardon my naivete, but can you recommend a good one to use? (I skimmed and
didn't see an obvious one.)


> ... also, unrelated but... what is this API even used for?

Oh, duh. This definitely needs a comment explaining what's going on. The issue
is that on Win8+ opens a really odd hole for metro mode that lets you grab the
token of the last process to write to the clipboard. So, we want to prevent the
renderer from being able to force the browser's token to be written there, and
grabbed later by e.g. the GPU process.

[dcheng, 2014-12-11 18:39:04]

On 2014/12/11 at 18:33:51, jschuh wrote:
> On 2014/12/11 18:18:23, dcheng wrote:
> >
https://codereview.chromium.org/792413003/diff/1/remoting/host/clipboard_win.cc
> > File remoting/host/clipboard_win.cc (right):
> > 
> >
https://codereview.chromium.org/792413003/diff/1/remoting/host/clipboard_win....
> > remoting/host/clipboard_win.cc:35:
> > ::ImpersonateAnonymousToken(GetCurrentThread());
> > On 2014/12/11 at 17:53:06, jschuh wrote:
> > > I know the impersonate *shouldn't* fail, but it's best to catch it
regardless.
> > So, something like:
> > > 
> > >   BOOL shouldRevert = ::ImpersonateAnonymousToken(GetCurrentThread());
> > >   DCHECK(shouldRevert);
> > >   ::CloseClipboard();
> > >   if (shouldRevert)
> > >     ::RevertToSelf();
> > 
> > Can we use a scoper to wrap this logic?
> 
> Pardon my naivete, but can you recommend a good one to use? (I skimmed and
didn't see an obvious one.)
> 
> 

Sorry, I guess this wasn't clear. I don't think we have one, so it would be nice
to add one for this. But that being said, it's only used in one spot so... maybe
it's fine. I'll rubberstamp whatever's fine with you.

> > ... also, unrelated but... what is this API even used for?
> 
> Oh, duh. This definitely needs a comment explaining what's going on. The issue
is that on Win8+ opens a really odd hole for metro mode that lets you grab the
token of the last process to write to the clipboard. So, we want to prevent the
renderer from being able to force the browser's token to be written there, and
grabbed later by e.g. the GPU process.

[Sergey Ulanov, 2014-12-11 19:47:17]

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

[Sergey Ulanov, 2014-12-11 19:47:17]

I'm not sure you need the change in remoting host. Can you please CC me on the
bug so I can access it?

[Sergey Ulanov, 2014-12-11 20:03:59]

On 2014/12/11 19:47:17, Sergey Ulanov wrote:
> I'm not sure you need the change in remoting host. Can you please CC me on the
> bug so I can access it?

After looking at the but still can't tell if we need the change in
remoting/host. FWIW that code doesn't run in sandbox and is not shipped with
chrome. Are the any possible side effects?

[forshaw, 2014-12-11 22:19:26]

> After looking at the but still can't tell if we need the change in
> remoting/host. FWIW that code doesn't run in sandbox and is not shipped with
> chrome. Are the any possible side effects?

Well if it isn't shipped in the broker then I don't see it being a problem to
remove, makes my life easier. I'll revert unless Justin has any objections. That
said it _shouldn't_ have side effects..

[Wez, 2014-12-12 17:49:38]

The CL description makes it sound like any app can choose to impersonate
the user who put data on the clipboard, which seems an incredibly serious
vulnerability in the platform. e.g. In the case of remoting, we're setting
the clipboard from a LocalSystem process...

Hoping that I have got the wrong end of the stick...

On 11 December 2014 at 12:03, <sergeyu@chromium.org> wrote:
>
> On 2014/12/11 19:47:17, Sergey Ulanov wrote:
>
>> I'm not sure you need the change in remoting host. Can you please CC me
>> on the
>> bug so I can access it?
>>
>
> After looking at the but still can't tell if we need the change in
> remoting/host. FWIW that code doesn't run in sandbox and is not shipped
> with
> chrome. Are the any possible side effects?
>
> https://codereview.chromium.org/792413003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jschuh, 2014-12-12 17:53:08]

No, your read is correct. So, I think you very much want this change in
Chromoting.

[Wez, 2014-12-12 18:29:58]

If my reading is correct then surely the patch to Chrome has little or no
effect on the sanctity of Chrome's sandbox, since it can be broken out of
the moment any other app puts anything on the clipboard?  Basically we need
to block the GetClipboardAccessToken() API in the sandbox (I assume we
already do this for similar APIs like WTSGetUserToken()?).

On 12 December 2014 at 09:53, <jschuh@chromium.org> wrote:
>
> No, your read is correct. So, I think you very much want this change in
> Chromoting.
>
> https://codereview.chromium.org/792413003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Wez, 2014-12-12 19:31:58]

wez@chromium.org changed reviewers:
- wez@chromium.org

[forshaw, 2014-12-12 20:47:30]

forshaw@chromium.org changed reviewers:
- sergeyu@chromium.org

[forshaw, 2014-12-12 20:47:30]

Updated the CL with scoper, could you please review.

[jschuh, 2014-12-12 21:20:23]

On 2014/12/12 18:29:58, Wez wrote:
> If my reading is correct then surely the patch to Chrome has little or no
> effect on the sanctity of Chrome's sandbox, since it can be broken out of
> the moment any other app puts anything on the clipboard?  Basically we need
> to block the GetClipboardAccessToken() API in the sandbox (I assume we
> already do this for similar APIs like WTSGetUserToken()?).

There's a few different pieces to required in putting together an exploit. What
we're trying to prevent with just this patch is web content being able to make a
known, higher-privilege token available via the clipboard. However, we're not
aware of any path where the renderer itself can get at that token.

I can explain more offline if you want.

[jschuh, 2014-12-12 21:40:22]

lgtm

[dcheng, 2014-12-13 01:16:01]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2014-12-13 01:16:01]

rs lgtm

[dcheng, 2014-12-13 01:16:29]

The CQ bit was unchecked by dcheng@chromium.org

[dcheng, 2014-12-13 01:16:51]

one minor nit, but otherwise, lgtm

https://codereview.chromium.org/792413003/diff/40001/ui/base/clipboard/clipbo...
File ui/base/clipboard/clipboard_win.cc (right):

https://codereview.chromium.org/792413003/diff/40001/ui/base/clipboard/clipbo...
ui/base/clipboard/clipboard_win.cc:51: BOOL must_revert_;
Actually, you probably want DISALLOW_COPY_AND_ASSIGN here, just in case.

[forshaw, 2014-12-15 13:50:14]

forshaw@chromium.org changed reviewers:
- jschuh@chromium.org

[forshaw, 2014-12-15 13:50:15]

Updated the CL, please can you just double check.

Thanks.

[dcheng, 2014-12-15 16:58:19]

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

[dcheng, 2014-12-15 16:58:20]

still lgtm

https://codereview.chromium.org/792413003/diff/60001/ui/base/clipboard/clipbo...
File ui/base/clipboard/clipboard_win.cc (right):

https://codereview.chromium.org/792413003/diff/60001/ui/base/clipboard/clipbo...
ui/base/clipboard/clipboard_win.cc:52:
DISALLOW_COPY_AND_ASSIGN(AnonymousImpersonator);
Note: I usually see a newline between DISALLOW_COPY_AND_ASSIGN and the rest of
the class definition.

[forshaw, 2014-12-15 16:59:39]

The CQ bit was checked by forshaw@chromium.org

[commit-bot: I haz the power, 2014-12-15 17:00:26]

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

[Wez, 2014-12-15 17:07:11]

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

[Wez, 2014-12-15 17:07:12]

https://codereview.chromium.org/792413003/diff/60001/ui/base/clipboard/clipbo...
File ui/base/clipboard/clipboard_win.cc (right):

https://codereview.chromium.org/792413003/diff/60001/ui/base/clipboard/clipbo...
ui/base/clipboard/clipboard_win.cc:42: must_revert_ =
::ImpersonateAnonymousToken(::GetCurrentThread());
Under what circumstances can ImpersonateAnonymousToken fail?

Should we instead be CHECK()ing this, since if it fails, we're not impersonating
the anonymous token?

https://codereview.chromium.org/792413003/diff/60001/ui/base/clipboard/clipbo...
ui/base/clipboard/clipboard_win.cc:107: // a risk of EoP
nit: punctuation

Impersonating the anonymous token presumably impacts the behaviour of e.g.
putting files onto the clipboard - any application which expects to be able to
use GetClipboardAuthToken() to ensure that it has access to the files is going
to fail. We should document that likely impact, at least.

[commit-bot: I haz the power, 2014-12-15 18:10:20]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-12-15 18:10:59]

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

[forshaw, 2014-12-16 09:07:30]

forshaw@chromium.org changed reviewers:
- dcheng@chromium.org, jschuh@chromium.org, wez@chromium.org

[forshaw, 2014-12-16 09:07:31]

https://codereview.chromium.org/792413003/diff/60001/ui/base/clipboard/clipbo...
File ui/base/clipboard/clipboard_win.cc (right):

https://codereview.chromium.org/792413003/diff/60001/ui/base/clipboard/clipbo...
ui/base/clipboard/clipboard_win.cc:42: must_revert_ =
::ImpersonateAnonymousToken(::GetCurrentThread());
On 2014/12/15 17:07:11, Wez wrote:
> Under what circumstances can ImpersonateAnonymousToken fail?
> 
> Should we instead be CHECK()ing this, since if it fails, we're not
impersonating
> the anonymous token? 

It can fail if we're running under a restricted token, other than that it
shouldn't fail. However if we're in a restricted token it isn't as big an issue
if the token is captured. I don't think it needs a CHECK though as we don't want
to crash if it fails as it only makes a difference in a very limited set of
scenarios.

https://codereview.chromium.org/792413003/diff/60001/ui/base/clipboard/clipbo...
ui/base/clipboard/clipboard_win.cc:52:
DISALLOW_COPY_AND_ASSIGN(AnonymousImpersonator);
On 2014/12/15 16:58:20, dcheng wrote:
> Note: I usually see a newline between DISALLOW_COPY_AND_ASSIGN and the rest of
> the class definition.

Acknowledged.

https://codereview.chromium.org/792413003/diff/60001/ui/base/clipboard/clipbo...
ui/base/clipboard/clipboard_win.cc:107: // a risk of EoP
On 2014/12/15 17:07:11, Wez wrote:
> nit: punctuation
> 
> Impersonating the anonymous token presumably impacts the behaviour of e.g.
> putting files onto the clipboard - any application which expects to be able to
> use GetClipboardAuthToken() to ensure that it has access to the files is going
> to fail. We should document that likely impact, at least.

The only thing I think this should impact is anyone calling the
GetClipboardAccessToken system call which as far as I can tell is limited to a
few internal DLLs. It isn't documented from what I can determine or clear what
it's actual purpose is. It shouldn't affect the processing of files transferred
over the clipboard but it's always a possibility that something won't work
correctly afterwards.