Sbox IPC fix

Sbox IPC fix

BUG=52682
TEST=included



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

[rvargas (doing something else), 2010-08-19 20:55:52]

http://codereview.chromium.org/3142022/diff/5001/6002
File sandbox/src/crosscall_server.cc (right):

http://codereview.chromium.org/3142022/diff/5001/6002#newcode84
sandbox/src/crosscall_server.cc:84: // inside this function.
Could you add a comment here re-stating that everything inside buffer_base
should be untrusted?

http://codereview.chromium.org/3142022/diff/5001/6002#newcode124
sandbox/src/crosscall_server.cc:124: (declared_size < min_declared_size)) {
Does it make sense to add 
|| declared_size < sizeof(CrossCallParams)

here, and remove the previous if statement?

http://codereview.chromium.org/3142022/diff/5001/6002#newcode161
sandbox/src/crosscall_server.cc:161: return NULL;
Do we care about *output_size?. It seems like you do.

http://codereview.chromium.org/3142022/diff/5001/6003
File sandbox/src/ipc_unittest.cc (right):

http://codereview.chromium.org/3142022/diff/5001/6003#newcode351
sandbox/src/ipc_unittest.cc:351: EXPECT_FALSE(NULL == ccp);
nit: Sometimes you use EXPECT_FALSE(==) and sometimes EXPECT_TRUE(!=). Wouldn't
it be simpler to just use EXPECT_XX(cpp) everywhere? (that's a more common
pattern).

http://codereview.chromium.org/3142022/diff/5001/6003#newcode352
sandbox/src/ipc_unittest.cc:352: }
Do we have tests that attempt CreateFromBuffer with an arbitrary buffer?. I'm a
little concerned that by starting with a properly formatted buffer (with small
changes) we may be missing something else.

For instance, now we should be rejecting a buffer with the arguments not stored
sequentially (although that is a by product of the implementation), and there is
no test for that.

[cpu_(ooo_6.6-7.5), 2010-08-19 22:11:21]

code updated please look again.

http://codereview.chromium.org/3142022/diff/5001/6002
File sandbox/src/crosscall_server.cc (right):

http://codereview.chromium.org/3142022/diff/5001/6002#newcode124
sandbox/src/crosscall_server.cc:124: (declared_size < min_declared_size)) {
On 2010/08/19 20:55:52, rvargas wrote:
> Does it make sense to add 
> || declared_size < sizeof(CrossCallParams)
> 
> here, and remove the previous if statement?

I am open to it if you feel strongly. My intent was to separate the tests
against declared_size versus the others.

http://codereview.chromium.org/3142022/diff/5001/6002#newcode161
sandbox/src/crosscall_server.cc:161: return NULL;
On 2010/08/19 20:55:52, rvargas wrote:
> Do we care about *output_size?. It seems like you do.

Actually, I recant. The code should (an is) check the return value. Thus I
removed the *output_size = 0 of line 138.

http://codereview.chromium.org/3142022/diff/5001/6003
File sandbox/src/ipc_unittest.cc (right):

http://codereview.chromium.org/3142022/diff/5001/6003#newcode351
sandbox/src/ipc_unittest.cc:351: EXPECT_FALSE(NULL == ccp);
I changed it to be more consistent. When we wrote the original code gtest had an
issue with void* being interpreted as bool (or at least the testing doc
discouraged it) so all the tests have explicit check vs NULL. I just kept the
style of the file.

On 2010/08/19 20:55:52, rvargas wrote:
> nit: Sometimes you use EXPECT_FALSE(==) and sometimes EXPECT_TRUE(!=).
Wouldn't
> it be simpler to just use EXPECT_XX(cpp) everywhere? (that's a more common
> pattern).

http://codereview.chromium.org/3142022/diff/5001/6003#newcode352
sandbox/src/ipc_unittest.cc:352: }
On 2010/08/19 20:55:52, rvargas wrote:
> Do we have tests that attempt CreateFromBuffer with an arbitrary buffer?.

No, we don't have those. It would not be difficult to fill the buffer with
1,2,3,4 or some other simple pattern like 0xff. I added 3 cases but I rather not
take the combinatorial problem in this CL. If you have an interesting case I am
all for it.

 I'm a
> little concerned that by starting with a properly formatted buffer (with small
> changes) we may be missing something else.
> 
> For instance, now we should be rejecting a buffer with the arguments not
stored
> sequentially (although that is a by product of the implementation), and there
is
> no test for that.

We don't check for this sequential condition. This CL just makes the bounds much
more tight. I thought about that case but I failed to see the security impact.

[rvargas (doing something else), 2010-08-20 00:22:47]

LGTM

http://codereview.chromium.org/3142022/diff/5001/6002
File sandbox/src/crosscall_server.cc (right):

http://codereview.chromium.org/3142022/diff/5001/6002#newcode124
sandbox/src/crosscall_server.cc:124: (declared_size < min_declared_size)) {
On 2010/08/19 22:11:21, cpu wrote:
> On 2010/08/19 20:55:52, rvargas wrote:
> > Does it make sense to add 
> > || declared_size < sizeof(CrossCallParams)
> > 
> > here, and remove the previous if statement?
> 
> I am open to it if you feel strongly. My intent was to separate the tests
> against declared_size versus the others. 

My point was that I see the logic as more complicated than needed
(min_declared_size) so I find it a little harder to follow. If you really prefer
the way it reads this way I won't object.

http://codereview.chromium.org/3142022/diff/5001/6003
File sandbox/src/ipc_unittest.cc (right):

http://codereview.chromium.org/3142022/diff/5001/6003#newcode352
sandbox/src/ipc_unittest.cc:352: }
Well... I don't really see any value for the new test cases so if that's all we
can do we might as well remove them.

I think that my concern is that we are relying too much in our renderer-side
code to make the test easy to write, instead of performing a validation of
arbitrary data. Hopefully this is the last time that we are fixing this code so
that's kind of a moot point.

[Chris Evans, 2010-08-20 01:10:41]

Some comments.

http://codereview.chromium.org/3142022/diff/11001/12002
File sandbox/src/crosscall_server.cc (right):

http://codereview.chromium.org/3142022/diff/11001/12002#newcode108
sandbox/src/crosscall_server.cc:108: __try {
Can we get rid of this try and move to a model whereby we simply don't access
out-of-bounds memory?

http://codereview.chromium.org/3142022/diff/11001/12002#newcode116
sandbox/src/crosscall_server.cc:116: sizeof(CrossCallParamsEx) + (param_count *
sizeof(ParamInfo));
1) Should this be sizeof(CrossCallParams) ?

2) Should it be param_count + 1 ? (Looking at ActualCallParams, a 1-parameter
structure will have 2 ParamInfos: ParamInfo param_info_[NUMBER_PARAMS + 1];)

http://codereview.chromium.org/3142022/diff/11001/12002#newcode120
sandbox/src/crosscall_server.cc:120: // Integer overflow or computed size bigger
than untrusted buffer.
I think this integer overflow check is trying to be too clever. I also think
it's incorrect and that I could hit the NOTREACHED() at line 61.
Can we just check for param_count > 9 and reject at that point? No way that 9 *
sizeof(ParamInfo) is going to cause overflow.

http://codereview.chromium.org/3142022/diff/11001/12002#newcode139
sandbox/src/crosscall_server.cc:139: // untrusted buffer so we bail out as is.
Another reason I don't like this try block is that it's not clear if the memory
allocated just above could ever be leaked.

http://codereview.chromium.org/3142022/diff/11001/12002#newcode144
sandbox/src/crosscall_server.cc:144: const char* first_byte =
&backing_mem[min_declared_size];
The above check at line 126 would seem to permit "declared_size ==
min_declared_size", which would lead to last_byte < first_byte here? That seems
dangerous. Is there some missing check?

http://codereview.chromium.org/3142022/diff/11001/12002#newcode172
sandbox/src/crosscall_server.cc:172: if (index > GetParamsCount()) {
If index is 0-based, should this check be >= ?

[cpu_(ooo_6.6-7.5), 2010-08-20 03:57:28]

This CL got reverted because broke some tests. Please read my reply here but the
changes will appear in a new CL.

http://codereview.chromium.org/3142022/diff/11001/12002
File sandbox/src/crosscall_server.cc (right):

http://codereview.chromium.org/3142022/diff/11001/12002#newcode108
sandbox/src/crosscall_server.cc:108: __try {
On 2010/08/20 01:10:41, Chris Evans wrote:
> Can we get rid of this try and move to a model whereby we simply don't access
> out-of-bounds memory?

I think we no longer have that risk. I am looking and I don't see the danger. I
must be a remain of the old code.

I'll talk to Rick to see if he sees value on it.

http://codereview.chromium.org/3142022/diff/11001/12002#newcode116
sandbox/src/crosscall_server.cc:116: sizeof(CrossCallParamsEx) + (param_count *
sizeof(ParamInfo));
On 2010/08/20 01:10:41, Chris Evans wrote:
> 1) Should this be sizeof(CrossCallParams) ?
> 
> 2) Should it be param_count + 1 ? (Looking at ActualCallParams, a 1-parameter
> structure will have 2 ParamInfos: ParamInfo param_info_[NUMBER_PARAMS + 1];)

If I appy your two suggestions together then it is equivalent to the current
code since the difference between CrossCallParams and CrossCallParamsEx is
exactly one ParamInfo.

http://codereview.chromium.org/3142022/diff/11001/12002#newcode120
sandbox/src/crosscall_server.cc:120: // Integer overflow or computed size bigger
than untrusted buffer.
On 2010/08/20 01:10:41, Chris Evans wrote:
> I think this integer overflow check is trying to be too clever. I also think
> it's incorrect and that I could hit the NOTREACHED() at line 61.
> Can we just check for param_count > 9 and reject at that point? No way that 9
*
> sizeof(ParamInfo) is going to cause overflow.

Maybe the way is tested is too obscure but that is what we do: note that for
params > 9 declared_size is 0 which fails the test in line 126 as long
min_declared_size is positive which is what is tested in line 118.

http://codereview.chromium.org/3142022/diff/11001/12002#newcode144
sandbox/src/crosscall_server.cc:144: const char* first_byte =
&backing_mem[min_declared_size];
On 2010/08/20 01:10:41, Chris Evans wrote:
> The above check at line 126 would seem to permit "declared_size ==
> min_declared_size", which would lead to last_byte < first_byte here? That
seems
> dangerous. Is there some missing check?

Yes, sort of. See my next CL.

http://codereview.chromium.org/3142022/diff/11001/12002#newcode172
sandbox/src/crosscall_server.cc:172: if (index > GetParamsCount()) {
On 2010/08/20 01:10:41, Chris Evans wrote:
> If index is 0-based, should this check be >= ?

yep. fixed

[cevans, 2010-08-20 05:30:47]

On Thu, Aug 19, 2010 at 8:57 PM, <cpu@chromium.org> wrote:

> This CL got reverted because broke some tests. Please read my reply here
> but the
> changes will appear in a new CL.


I LGTM'ed the new CL. Thanks for taking care of that.
Looks like a relatively safe change - can I interest you in merging it to
M6? :)


>
>
>
>
> http://codereview.chromium.org/3142022/diff/11001/12002
> File sandbox/src/crosscall_server.cc (right):
>
> http://codereview.chromium.org/3142022/diff/11001/12002#newcode108
> sandbox/src/crosscall_server.cc:108: __try {
> On 2010/08/20 01:10:41, Chris Evans wrote:
>
>> Can we get rid of this try and move to a model whereby we simply don't
>>
> access
>
>> out-of-bounds memory?
>>
>
> I think we no longer have that risk. I am looking and I don't see the
> danger. I must be a remain of the old code.
>
> I'll talk to Rick to see if he sees value on it.
>
>
> http://codereview.chromium.org/3142022/diff/11001/12002#newcode116
> sandbox/src/crosscall_server.cc:116: sizeof(CrossCallParamsEx) +
> (param_count * sizeof(ParamInfo));
> On 2010/08/20 01:10:41, Chris Evans wrote:
>
>> 1) Should this be sizeof(CrossCallParams) ?
>>
>
>  2) Should it be param_count + 1 ? (Looking at ActualCallParams, a
>>
> 1-parameter
>
>> structure will have 2 ParamInfos: ParamInfo param_info_[NUMBER_PARAMS
>>
> + 1];)
>
> If I appy your two suggestions together then it is equivalent to the
> current code since the difference between CrossCallParams and
> CrossCallParamsEx is exactly one ParamInfo.


Ah, I did wonder about that. Cool.

Cheers
Chris


>
> http://codereview.chromium.org/3142022/diff/11001/12002#newcode120
> sandbox/src/crosscall_server.cc:120: // Integer overflow or computed
> size bigger than untrusted buffer.
> On 2010/08/20 01:10:41, Chris Evans wrote:
>
>> I think this integer overflow check is trying to be too clever. I also
>>
> think
>
>> it's incorrect and that I could hit the NOTREACHED() at line 61.
>> Can we just check for param_count > 9 and reject at that point? No way
>>
> that 9 *
>
>> sizeof(ParamInfo) is going to cause overflow.
>>
>
> Maybe the way is tested is too obscure but that is what we do: note that
> for params > 9 declared_size is 0 which fails the test in line 126 as
> long min_declared_size is positive which is what is tested in line 118.
>
>
> http://codereview.chromium.org/3142022/diff/11001/12002#newcode144
> sandbox/src/crosscall_server.cc:144: const char* first_byte =
> &backing_mem[min_declared_size];
> On 2010/08/20 01:10:41, Chris Evans wrote:
>
>> The above check at line 126 would seem to permit "declared_size ==
>> min_declared_size", which would lead to last_byte < first_byte here?
>>
> That seems
>
>> dangerous. Is there some missing check?
>>
>
> Yes, sort of. See my next CL.
>
>
> http://codereview.chromium.org/3142022/diff/11001/12002#newcode172
> sandbox/src/crosscall_server.cc:172: if (index > GetParamsCount()) {
> On 2010/08/20 01:10:41, Chris Evans wrote:
>
>> If index is 0-based, should this check be >= ?
>>
>
> yep. fixed
>
>
> http://codereview.chromium.org/3142022/show
>