|
|
Created:
10 years, 4 months ago by cpu_(ooo_6.6-7.5) Modified:
9 years, 7 months ago CC:
chromium-reviews, Paweł Hajdan Jr. Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionSbox IPC fix
BUG=52682
TEST=included
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=56796
Patch Set 1 #Patch Set 2 : '' #
Total comments: 11
Patch Set 3 : '' #
Total comments: 11
Patch Set 4 : '' #
Messages
Total messages: 7 (0 generated)
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.
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.
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.
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 >= ?
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
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 > |