Example of how to interpose yourself in a message stream.

IPC outgoing message filters interpose yourself in a message stream.  Minimally invasive baseline for building IPC tests to abuse browser along the lines of a compromised renderer.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=84076

[Tom Sepez, 2011-04-21 21:24:14]

John,

  This isn't something to be Commited yet, but I'm trying to reduce the impact
on the baseline code when I insert the message fuzzer.

  See the change to ipc_sync_channel.cc. This version tries to load it as an
external .so.  Its currently linux only.

[jam, 2011-04-26 19:11:35]

Not sure if you wanted me to look now?

I'd prefer that instead of adding that code to ipc_sync_channel, it can just
expose a setter for someone to filter all messages.  Then a higher module like
chrome, or browser_tests, can load the library.

[tsepez (do not use), 2011-04-26 20:28:09]

Ok.  Seems reasonable.  The follow-up question is from where to best invoke
this.  Got a pointer to a place in the renderer right after the IPC channel
is created?

On Tue, Apr 26, 2011 at 12:11 PM, <jam@chromium.org> wrote:

> Not sure if you wanted me to look now?
>
> I'd prefer that instead of adding that code to ipc_sync_channel, it can
> just
> expose a setter for someone to filter all messages.  Then a higher module
> like
> chrome, or browser_tests, can load the library.
>
>
> http://codereview.chromium.org/6711024/
>

[jam, 2011-04-27 21:17:17]

Are you trying to do this in browser_tests or in chrome?

If browser_tests, then I'd look at the main function in
out_of_proc_test_runner.cc.  If in Chrome, then BrowserMain()

On 2011/04/26 20:28:09, tsepez_google.com wrote:
> Ok.  Seems reasonable.  The follow-up question is from where to best invoke
> this.  Got a pointer to a place in the renderer right after the IPC channel
> is created?
> 
> On Tue, Apr 26, 2011 at 12:11 PM, <mailto:jam@chromium.org> wrote:
> 
> > Not sure if you wanted me to look now?
> >
> > I'd prefer that instead of adding that code to ipc_sync_channel, it can
> > just
> > expose a setter for someone to filter all messages.  Then a higher module
> > like
> > chrome, or browser_tests, can load the library.
> >
> >
> > http://codereview.chromium.org/6711024/
> >

[Tom Sepez, 2011-04-27 21:48:25]

Actually, I think I want to start with just the renderer, since that's the place
where bad guys can have code execution.  John, llease take a look at this,
excepting the ipcfuzz.cc which is still kinda fake.

[jam, 2011-04-27 21:58:07]

http://codereview.chromium.org/6711024/diff/33001/chrome/chrome.gyp
File chrome/chrome.gyp (right):

http://codereview.chromium.org/6711024/diff/33001/chrome/chrome.gyp#newcode877
chrome/chrome.gyp:877: 'chrome',
are all these needed?  i.e. chrome_resources/chrome_strings etc

http://codereview.chromium.org/6711024/diff/33001/content/common/child_thread.cc
File content/common/child_thread.cc (right):

http://codereview.chromium.org/6711024/diff/33001/content/common/child_thread...
content/common/child_thread.cc:72: SetIPCFuzzing(channel_.get());
the code that you added in content should be in chrome layer instead I think,
since content is the bare minimum for rendering a page using a multi-process
browser.  and I think you'll want this in chrome layer anyways in case you want
to do something special with chrome IPCs that the content layer doesn't know
about.

http://codereview.chromium.org/6711024/diff/33001/ipc/ipc_sync_channel.h
File ipc/ipc_sync_channel.h (right):

http://codereview.chromium.org/6711024/diff/33001/ipc/ipc_sync_channel.h#newc...
ipc/ipc_sync_channel.h:186: 
nit

[Tom Sepez, 2011-04-28 19:13:56]

the code that you added in content should be in chrome layer instead I think,
> since content is the bare minimum for rendering a page using a multi-process
> browser.  and I think you'll want this in chrome layer anyways in case you
want
> to do something special with chrome IPCs that the content layer doesn't know
> about.
> 

Not sure that there is a place in chrome to do this for all.  I didn't see any
interaction with the renderer's IPC channel outside of the content layer, for
example.  Is there a spot in chrome where we can get access to this?

[jam, 2011-04-28 21:30:33]

In the renderer, you can have RenderThread take a OutgoingMessageFilter*.

also, since you're doing this to messages being sent from the renderer, you can
also do this in the browser process when the message is being received

[Tom Sepez, 2011-04-28 23:01:29]

Ok, please review again. 

It seems more precise to fuzz the message before it's sent -- this mimics more
closely what an attacker could do -- providing a better end-to-end test (i.e.
any lurking bugs in the channel get exercised).

[jam, 2011-04-28 23:43:46]

http://codereview.chromium.org/6711024/diff/32021/chrome/chrome.gyp
File chrome/chrome.gyp (right):

http://codereview.chromium.org/6711024/diff/32021/chrome/chrome.gyp#newcode852
chrome/chrome.gyp:852: 'test_support_common',
nit: spacing is off

http://codereview.chromium.org/6711024/diff/32021/content/common/child_thread.h
File content/common/child_thread.h (right):

http://codereview.chromium.org/6711024/diff/32021/content/common/child_thread...
content/common/child_thread.h:46: // Adds a filter to rewrite outgoing messages
(used mainly for testing).
how about just making channel() public, it seems that should have been done from
the start.  that way don't need to add methods to wrap the channel one.

http://codereview.chromium.org/6711024/diff/32021/ipc/ipc_sync_channel.h
File ipc/ipc_sync_channel.h (right):

http://codereview.chromium.org/6711024/diff/32021/ipc/ipc_sync_channel.h#newc...
ipc/ipc_sync_channel.h:197: OutgoingMessageFilter *outgoing_message_filter_;
nit: * then space

http://codereview.chromium.org/6711024/diff/32021/ipc/ipc_sync_channel.h#newc...
ipc/ipc_sync_channel.h:207: class OutgoingMessageFilter {
since this doesn't have to do with sync messages, i think it's better to put
this in IPC::ChannelProxy.  then this interface can be put right after
IPC::ChannelProxy::MessageFilter.

[jam, 2011-04-28 23:47:14]

On 2011/04/28 23:01:29, Tom Sepez wrote:
> Ok, please review again. 
> 
> It seems more precise to fuzz the message before it's sent -- this mimics more
> closely what an attacker could do -- providing a better end-to-end test (i.e.
> any lurking bugs in the channel get exercised).

do we have any error checking in IPC::Channel that currently would get triggered
in the renderer, but which an attacker would disable?

[Tom Sepez, 2011-05-02 18:38:07]

John, another version.  Thanks heaps.

> do we have any error checking in IPC::Channel that currently would get
triggered
> in the renderer, but which an attacker would disable?

I didn't think there was any error checking, since the pickle is opaque to the
channel (which also is the reason that fuzzing it is challenging).

[jam, 2011-05-02 20:06:10]

lgtm with the one change

http://codereview.chromium.org/6711024/diff/40002/ipc/ipc_sync_channel.cc
File ipc/ipc_sync_channel.cc (right):

http://codereview.chromium.org/6711024/diff/40002/ipc/ipc_sync_channel.cc#new...
ipc/ipc_sync_channel.cc:403: if (outgoing_message_filter())
this should be done in ChannelProxy::Send

[Tom Sepez, 2011-05-02 20:14:09]

Wondering if I can do that.  The behaviour of the message filter is actually to
either return the original message, or cobble together a new one and free the
original.  This preserves the read-only nature of the underlying pickle once it
has been initialized.  So I worry what happens when sync_msg is pushed ... and
the use after free possibility, but it looks like the original code would have
the same problem.  I didn't think the message was ref counted, so how does this
work?

[jam, 2011-05-02 20:20:12]

On Mon, May 2, 2011 at 1:14 PM, <tsepez@chromium.org> wrote:

> Wondering if I can do that.  The behaviour of the message filter is
> actually to
> either return the original message, or cobble together a new one and free
> the
> original.  This preserves the read-only nature of the underlying pickle
> once it
> has been initialized.  So I worry what happens when sync_msg is pushed


The message isn't stored, only information from it.  Is that what you were
referring to?


> ... and
> the use after free possibility, but it looks like the original code would
> have
> the same problem.  I didn't think the message was ref counted, so how does
> this
> work?
>
>
>
>
> http://codereview.chromium.org/6711024/
>

[Tom Sepez, 2011-05-02 20:31:42]

> The message isn't stored, only information from it.  Is that what you were
> referring to?
> 

Yes.  So knowing that, and looking at what push() actually does, it seems that
I'm good except for the reply deserializer, which becomes owned by the context
and set to null in the object.  ipcfuzz.cc wants a deserializer so that it can
clone the location of the output parameters into the new message.

[Tom Sepez, 2011-05-02 21:16:40]

Ah, but if its already preserved above the channel proxy, the duplicated message
can just have a dummy deserializer.

[Tom Sepez, 2011-05-02 23:55:38]

New version doesn't touch ipc_sync_message, but there is a cost -- changes in
ipcfuzz.cc.  There's an ugly hack to get the message ID of the cloned message to
match what was previously pushed above us.

[jam, 2011-05-03 06:39:19]

lgtm (I always prefer ugly hacks in testing code at the expense of shipping code
:) )

[jam, 2011-05-03 06:39:26]

http://codereview.chromium.org/6711024/diff/42013/chrome/tools/ipclist/ipcfuz...
File chrome/tools/ipclist/ipcfuzz.cc (right):

http://codereview.chromium.org/6711024/diff/42013/chrome/tools/ipclist/ipcfuz...
chrome/tools/ipclist/ipcfuzz.cc:214: // No-op fuzzer.  Rewrites each message
unchaged to check if the message
nit: unchanged