Sbox IPC fix

Second take, I had off-by-one bad check in line 164

for more info see review 3142022

BUG=52682
TEST=included


Review URL: http://codereview.chromium.org/3130037

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@56938 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 79 insertions, 40 deletions

diff --git a/sandbox/src/crosscall_params.h b/sandbox/src/crosscall_params.h
index 939925c..4c1e4fe 100644
--- a/sandbox/src/crosscall_params.h
+++ b/sandbox/src/crosscall_params.h
@@ -169,30 +169,33 @@ class CrossCallParams {
 // blob to be complex.
 //
 // As is, this class assumes that the layout of the blob is as follows. Assume
-// that NUMBER_PARAMS = 2:
+// that NUMBER_PARAMS = 2 and a 32-bit build:
 //
-// [ tag              4 bytes]
-// [ IsOnOut          4 bytes]
-// [ call return     52 bytes]
-// [ params count     4 bytes]
-// [ parameter 0 type 4 bytes]
-// [ parameter 0 /\   4 bytes] ---delta to ---\
-// [ parameter 0 size 4 bytes]                |
-// [ parameter 1 type 4 bytes]                |
-// [ parameter 1 /\   4 bytes]                |
-// [ parameter 1 size 4 bytes]                |
-// [ parameter 2 type 4 bytes]                |
-// [ parameter 2 /\   4 bytes] ----------------------\
-// [ parameter 2 size 4 bytes]                |      |
-// |-------------------------|                |      |
-// |                         | <--------------/      |
-// |                         |                       |
-// |                         | <---------------------/
-// |-------------------------|
+// [ tag                4 bytes]
+// [ IsOnOut            4 bytes]
+// [ call return       52 bytes]
+// [ params count       4 bytes]
+// [ parameter 0 type   4 bytes]
+// [ parameter 0 offset 4 bytes] ---delta to ---\
+// [ parameter 0 size   4 bytes]                |
+// [ parameter 1 type   4 bytes]                |
+// [ parameter 1 offset 4 bytes] ---------------|--\
+// [ parameter 1 size   4 bytes]                |  |
+// [ parameter 2 type   4 bytes]                |  |
+// [ parameter 2 offset 4 bytes] ----------------------\
+// [ parameter 2 size   4 bytes]                |  |   |
+// |---------------------------|                |  |   |
+// | value 0     (x bytes)     | <--------------/  |   |
+// | value 1     (y bytes)     | <-----------------/   |
+// |                           |                       |
+// | end of buffer             | <---------------------/
+// |---------------------------|
 //
 // Note that the actual number of params is NUMBER_PARAMS + 1
 // so that the size of each actual param can be computed from the difference
-// between one parameter and the next down
+// between one parameter and the next down. The offset of the last param
+// points to the end of the buffer and the type and size are undefined.
+//
 template <size_t NUMBER_PARAMS, size_t BLOCK_SIZE>
 class ActualCallParams : public CrossCallParams {
  public:
@@ -209,6 +212,14 @@ class ActualCallParams : public CrossCallParams {
     param_info_[0].offset_ = parameters_ - reinterpret_cast<char*>(this);
   }
 
+  // Testing-only method. Allows setting the apparent size to a wrong value.
+  // returns the previous size.
+  size_t OverrideSize(size_t new_size) {
+    size_t previous_size = param_info_[NUMBER_PARAMS].offset_;
+    param_info_[NUMBER_PARAMS].offset_ = new_size;
+    return previous_size;
+  }
+
   // Copies each paramter into the internal buffer. For each you must supply:
   // index: 0 for the first param, 1 for the next an so on
   bool CopyParamIn(size_t index, const void* parameter_address, size_t size,
diff --git a/sandbox/src/crosscall_server.cc b/sandbox/src/crosscall_server.cc
index da43c69..3ed99c8 100644
--- a/sandbox/src/crosscall_server.cc
+++ b/sandbox/src/crosscall_server.cc
@@ -85,6 +85,8 @@ void CrossCallParamsEx::operator delete(void* raw_memory) throw() {
 CrossCallParamsEx* CrossCallParamsEx::CreateFromBuffer(void* buffer_base,
                                                        size_t buffer_size,
                                                        size_t* output_size) {
+  // IMPORTANT: Everything inside buffer_base and derived from it such
+  // as param_count and declared_size is untrusted.
   if (NULL == buffer_base) {
     return NULL;
   }
@@ -94,10 +96,12 @@ CrossCallParamsEx* CrossCallParamsEx::CreateFromBuffer(void* buffer_base,
   if (buffer_size > kMaxBufferSize) {
     return NULL;
   }
+
   char* backing_mem = NULL;
   size_t param_count = 0;
+  size_t declared_size;
+  size_t min_declared_size;
   CrossCallParamsEx* copied_params = NULL;
-  size_t actual_size;
 
   // Touching the untrusted buffer is done under a SEH try block. This
   // will catch memory access violations so we don't crash.
@@ -107,38 +111,43 @@ CrossCallParamsEx* CrossCallParamsEx::CreateFromBuffer(void* buffer_base,
     // Check against the minimum size given the number of stated params
     // if too small we bail out.
     param_count = call_params->GetParamsCount();
-    if ((buffer_size - sizeof(CrossCallParams)) <
-        (sizeof(ptrdiff_t) * (param_count + 1))) {
-      // This test is subject to integer overflow but the next is not.
+
+    min_declared_size =
+        sizeof(CrossCallParamsEx) + (param_count * sizeof(ParamInfo));
+
+    if ((buffer_size < min_declared_size) ||
+        (sizeof(CrossCallParamsEx) > min_declared_size)) {
+      // Minimal computed size bigger than existing buffer or param_count
+      // integer overflow.
       return NULL;
     }
 
-    actual_size = GetActualBufferSize(param_count, buffer_base);
-    if ((actual_size > buffer_size) || (0 == actual_size)) {
-      // It is too big or too many declared parameters.
+    // Retrieve the declared size which if it fails returns 0.
+    declared_size = GetActualBufferSize(param_count, buffer_base);
+
+    if ((declared_size > buffer_size) ||
+        (declared_size < min_declared_size)) {
+      // Declared size is bigger than buffer or smaller than computed size
+      // or param_count 0 or bigger than 9.
       return NULL;
     }
 
     // Now we copy the actual amount of the message.
-    actual_size += sizeof(ParamInfo);  // To get the last offset.
-    *output_size = actual_size;
-    backing_mem = new char[actual_size];
-    memset(backing_mem, 0, actual_size);
-    // Note that this is a placement new.
-#pragma warning(push)
-#pragma warning(disable: 4291)  // No matching operator delete.
-    // TODO(cpu): Remove this warning.
-    copied_params = new(backing_mem)CrossCallParamsEx();
-#pragma warning(pop)
-    memcpy(backing_mem, call_params, actual_size);
+    *output_size = declared_size;
+    backing_mem = new char[declared_size];
+    copied_params = reinterpret_cast<CrossCallParamsEx*>(backing_mem);
+    memcpy(backing_mem, call_params, declared_size);
 
   } __except(EXCEPTION_EXECUTE_HANDLER) {
     // In case of a windows exception we know it occurred while touching the
     // untrusted buffer so we bail out as is.
+    delete [] backing_mem;
     return NULL;
   }
 
-  char* last_byte = &backing_mem[actual_size - 1];
+  const char* last_byte = &backing_mem[declared_size];
+  const char* first_byte = &backing_mem[min_declared_size];
+
   // Verify here that all and each parameters make sense. This is done in the
   // local copy.
   for (size_t ix =0; ix != param_count; ++ix) {
@@ -149,6 +158,7 @@ CrossCallParamsEx* CrossCallParamsEx::CreateFromBuffer(void* buffer_base,
     if ((NULL == address) ||               // No null params.
         (INVALID_TYPE >= type) || (LAST_TYPE <= type) ||  // Unknown type.
         (address < backing_mem) ||         // Start cannot point before buffer.
+        (address < first_byte) ||          // Start cannot point too low.
         (address > last_byte) ||           // Start cannot point past buffer.
         ((address + size) < address) ||    // Invalid size.
         ((address + size) > last_byte)) {  // End cannot point past buffer.
@@ -164,7 +174,7 @@ CrossCallParamsEx* CrossCallParamsEx::CreateFromBuffer(void* buffer_base,
 // Accessors to the parameters in the raw buffer.
 void* CrossCallParamsEx::GetRawParameter(size_t index, size_t* size,
                                          ArgType* type) {
-  if (index > GetParamsCount()) {
+  if (index >= GetParamsCount()) {
     return NULL;
   }
   // The size is always computed from the parameter minus the next
diff --git a/sandbox/src/ipc_unittest.cc b/sandbox/src/ipc_unittest.cc
index 909f144..5bf3c54 100644
--- a/sandbox/src/ipc_unittest.cc
+++ b/sandbox/src/ipc_unittest.cc
@@ -332,6 +332,24 @@ TEST(IPCTest, CrossCallValidation) {
     EXPECT_TRUE(NULL == ccp);
   }
 #endif  // defined(NDEBUG)
+
+  ActualCallParams<1, 256> params_3(kTag, 1);
+  params_3.CopyParamIn(0, &value, sizeof(value), false, ULONG_TYPE);
+  buffer = const_cast<void*>(params_3.GetBuffer());
+  EXPECT_TRUE(NULL != buffer);
+
+  size_t correct_size = params_3.OverrideSize(1);
+  ccp = CrossCallParamsEx::CreateFromBuffer(buffer, 256, &out_size);
+  EXPECT_TRUE(NULL == ccp);
+
+  // The correct_size is 8 bytes aligned.
+  params_3.OverrideSize(correct_size - 7);
+  ccp = CrossCallParamsEx::CreateFromBuffer(buffer, 256, &out_size);
+  EXPECT_TRUE(NULL == ccp);
+
+  params_3.OverrideSize(correct_size);
+  ccp = CrossCallParamsEx::CreateFromBuffer(buffer, 256, &out_size);
+  EXPECT_TRUE(NULL != ccp);
 }
 
 // This structure is passed to the mock server threads to simulate