Make sure that base::MessagePumpForUI from different modules are isolated from each other and add p…

Make sure that base::MessagePumpForUI from different modules are isolated from each other and add protection from shatter attacks by placing |this| pointer to the used data associated with the message-only window (instead of blindly trusting the value of WPARAM).

BUG=124091
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=133134

[alexeypa (please no reviews), 2012-04-19 19:57:08]

Please take a look.

Apparently there are other instances of improper use of RegisterClass() and
GetModuleHandleEx() in the code base. I'm going to address them in a separate CL
and fix only the specifi problem affecting the Chromoting Host plugin.

[brettw, 2012-04-19 23:46:52]

lgtm

https://chromiumcodereview.appspot.com/10134001/diff/1/base/message_pump_win.cc
File base/message_pump_win.cc (right):

https://chromiumcodereview.appspot.com/10134001/diff/1/base/message_pump_win....
base/message_pump_win.cc:257: instance_ =
base::GetModuleFromAddress(window_procedure);
Can you add a comment here about why we need this rather than
GetModuleHandle(NULL)?

https://chromiumcodereview.appspot.com/10134001/diff/1/base/message_pump_win....
base/message_pump_win.cc:264: atom_ = RegisterClassExW(&wc);
I'd remove the W suffixes. We don't do this elsewhere and we explicitly never
compile in ANSI mode.

[alexeypa (please no reviews), 2012-04-20 00:21:53]

https://chromiumcodereview.appspot.com/10134001/diff/1/base/message_pump_win.cc
File base/message_pump_win.cc (right):

https://chromiumcodereview.appspot.com/10134001/diff/1/base/message_pump_win....
base/message_pump_win.cc:257: instance_ =
base::GetModuleFromAddress(window_procedure);
On 2012/04/19 23:46:52, brettw wrote:
> Can you add a comment here about why we need this rather than
> GetModuleHandle(NULL)?

Done.

https://chromiumcodereview.appspot.com/10134001/diff/1/base/message_pump_win....
base/message_pump_win.cc:264: atom_ = RegisterClassExW(&wc);
On 2012/04/19 23:46:52, brettw wrote:
> I'd remove the W suffixes. We don't do this elsewhere and we explicitly never
> compile in ANSI mode.

Done.

[commit-bot: I haz the power, 2012-04-20 00:22:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeypa@chromium.org/10134001/5001

[commit-bot: I haz the power, 2012-04-20 02:52:30]

Change committed as 133134

[jar (doing other things), 2012-05-15 21:39:42]

Folks are seeing some mysterious cases where threads are not responding on
Windows.... so I looked back to see who has been changing this code recently...
and spotted this fairly suspicious CL.

Can you motivate this better, or perchance explain what I'm missing?

If you can argue that the cases I cited never happen... that is all the more
reason to not have code to handle the cases.

http://codereview.chromium.org/10134001/diff/5001/base/message_pump_win.cc
File base/message_pump_win.cc (right):

http://codereview.chromium.org/10134001/diff/5001/base/message_pump_win.cc#ne...
base/message_pump_win.cc:183: if (self != NULL) {
Perchance we should not check the value of |self|, and should instead crash on
line 186 or 190 if we don't have a value to service the cases?

http://codereview.chromium.org/10134001/diff/5001/base/message_pump_win.cc#ne...
base/message_pump_win.cc:279: return;
These return on error condition appear to mask a significant fault, as we won't
have set the |this| pointer for retrieval, and hence all handling of tasks and
timers in nested message loops will silently be ignored.... simulating a hang
:-(

Shouldn't we just CHECK() and crash here, and on line 269?

[jar (doing other things), 2012-05-15 21:52:06]

One other alternative.....

You could leave the passing of |this| as a fallback, and use it when we can't
get a value out of the window.  That way you would not have to crash if/when you
can't set the value.

[darin (slow to review), 2012-05-15 22:09:25]

Not sure I understand why setting UserData is better than passing WPARAM values.
 It seems like an attacker could just as easily change the UserData before
calling PostMessage.  Perhaps I don't understand what "shatter attack" means.

Also, as Jim notes the use of DCHECK is contrary to the style guide:
http://dev.chromium.org/developers/coding-style#TOC-CHECK-vs-DCHECK-and-NOTRE...

[alexeypa (please no reviews), 2012-05-15 22:09:50]

On 2012/05/15 21:39:42, jar wrote:
> Folks are seeing some mysterious cases where threads are not responding on
> Windows.... so I looked back to see who has been changing this code
recently...
> and spotted this fairly suspicious CL.

Another change in this CL that can be important: the window class name for the
message pump is different now, so if the name was hardcoded elsewhere it will be
a problem.  

> You could leave the passing of |this| as a fallback, and use it when we can't
> get a value out of the window.  That way you would not have to crash if/when
you
> can't set the value.

Passing |this| via message parameters is dangerous. It becomes easier to
interpret garbage as a valid execution address. Setting a value via
SetWindowLongPtr should be as reliable as writing a member of a structure as
long as a window has been created successfully. It is pretty reliable and common
technique.

[alexeypa (please no reviews), 2012-05-15 22:10:27]

http://codereview.chromium.org/10134001/diff/5001/base/message_pump_win.cc
File base/message_pump_win.cc (right):

http://codereview.chromium.org/10134001/diff/5001/base/message_pump_win.cc#ne...
base/message_pump_win.cc:183: if (self != NULL) {
On 2012/05/15 21:39:42, jar wrote:
> Perchance we should not check the value of |self|, and should instead crash on
> line 186 or 190 if we don't have a value to service the cases?

|self| should never be NULL except for WM_CREATE message. So yes, we should
probably crash instead of silently ignoring the error.

http://codereview.chromium.org/10134001/diff/5001/base/message_pump_win.cc#ne...
base/message_pump_win.cc:269: DCHECK(atom_);
We should also CHECK here.

http://codereview.chromium.org/10134001/diff/5001/base/message_pump_win.cc#ne...
base/message_pump_win.cc:279: return;
On 2012/05/15 21:39:42, jar wrote:
> These return on error condition appear to mask a significant fault, as we
won't
> have set the |this| pointer for retrieval, and hence all handling of tasks and
> timers in nested message loops will silently be ignored.... simulating a hang
> :-(
> 
> Shouldn't we just CHECK() and crash here, and on line 269?

Those DCHECKs migrated from the previous version of the code to this CL. I think
CHECK is a better way to handle errors returned by RegisterClassEx and
CreateWindow. 

Now, ignoring the errors returned by RegisterClass(Ex) (and, maybe,
CreateWindow) seems to be a more common pattern in Chromium code than we would
want.

http://codereview.chromium.org/10134001/diff/5001/base/message_pump_win.cc#ne...
base/message_pump_win.cc:283: SetWindowLongPtr(message_hwnd_,
The return value of SetWindowLongPtr should be checked too.

[alexeypa (please no reviews), 2012-05-15 22:22:18]

On 2012/05/15 22:09:25, darin wrote:
> Not sure I understand why setting UserData is better than passing WPARAM
values.
>  It seems like an attacker could just as easily change the UserData before
> calling PostMessage.  Perhaps I don't understand what "shatter attack" means.

A quote from MSDN: 

Windows XP/2000: The SetWindowLongPtr function fails if the window specified by
the hWnd parameter does not belong to the same process as the calling thread.

Vista and above: The SetWindowLongPtr function fails if the process that owns
the window specified by the hWnd parameter is at a higher process privilege in
the UIPI hierarchy than the process the calling thread resides in.

Basically on XP/2000 it is possible to send WM_USER+1 with garbage WPARAM to
crash the receiving process but a different process should not be able to modify
GWLP_USERDATA. 

On Vista UIPI protects from WM_USER+1 with garbage WPARAM sent from processes
running at lower integrity levels. But it does not protect from anything else.

> Also, as Jim notes the use of DCHECK is contrary to the style guide:
>
http://dev.chromium.org/developers/coding-style#TOC-CHECK-vs-DCHECK-and-NOTRE...

I'll put together a CL adding those checks.

[alexeypa (please no reviews), 2012-05-15 22:24:11]

On 2012/05/15 21:39:42, jar wrote:
> Folks are seeing some mysterious cases where threads are not responding on
> Windows.... 

One more thing. I might be able to help you figuring out what is going on. At
least I can try. Please feel free to ping me if you have something to look at.