Corrects opcode counting when serializing LowLevelPolicy.

This fixes a bug where opcode count was incremented (+=)
rather than set when opcodes were serialized into backing memory.

In turn this allow todoing the use of a uint32_t for low level policy
|options_| internal storage.

policy_engine_opcodes.h:243
// TODO(cpu): Making |options_| a uint32_t would avoid casting, but causes
// test failures.  Somewhere code is relying on the size of this struct.
// http://crbug.com/420296

It turns out that the test failure occurred not because of a structure
being the wrong size. Instead when policy_low_level.cc
LowLevelPolicy::Done() was called twice (because a rule was
changed or added) the memory to which rules were stored was not
zeroed before filling the memory.
This meant while most memory was rewritten with
new values the count of opcodes was instead incremented.

It's not entirely clear to me why this did not break before as
this resulted in an incorrect opcode count for any case where
::Done() was called twice but my hypothesis is:

a) ::Done() was not called twice in production nor in most tests.
b) The memory allocated for policies is much larger than required
so no sanitizers would locate a OOB read or write.
c) The alignment of the structures with a uint16_t for |options_|
caused the rule engine, when attempting to read twice as many
rules as required, to interpret the 'next' rule as a
OP_ALWAYS_FALSE and so return something sensible in these
cases. This changed when the size of the value, and packing,
stretched the size of the PolicyOpcode structure, so that when
reading past the end of the rules an invalid opcode type of
0x10 would be read (the next entry's count) which resulted
in the test failing.

Bug: 420296
Change-Id: I806a70d66fba9d4cd831df64658ded2c9545be9a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1867476
Commit-Queue: Alex Gough <ajgo@chromium.org>
Reviewed-by: Will Harris <wfh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#707525}

sandbox/win/src/policy_engine_opcodes.h

@@ -193,9 +193,7 @@ class PolicyOpcode {
   uint32_t GetOptions() const { return options_; }
 
   // Sets the stored options such as kPolNegateEval.
-  void SetOptions(uint32_t options) {
-    options_ = base::checked_cast<uint16_t>(options);
-  }
+  void SetOptions(uint32_t options) { options_ = options; }
 
  private:
   static const size_t kArgumentCount = 4;  // The number of supported argument.
@@ -214,10 +212,7 @@ class PolicyOpcode {
                             MatchContext* match);
   OpcodeID opcode_id_;
   int16_t parameter_;
-  // TODO(cpu): Making |options_| a uint32_t would avoid casting, but causes
-  // test failures.  Somewhere code is relying on the size of this struct.
-  // http://crbug.com/420296
-  uint16_t options_;
+  uint32_t options_;
   OpcodeArgument arguments_[PolicyOpcode::kArgumentCount];
 };
 

sandbox/win/src/policy_low_level.cc

@@ -113,10 +113,10 @@ bool LowLevelPolicy::Done() {
       svc_opcode_count += op_count;
     }
 
-    current_buffer->opcode_count += svc_opcode_count;
-    size_t policy_byte_count =
+    current_buffer->opcode_count = svc_opcode_count;
+    size_t policy_buffers_occupied =
         (svc_opcode_count * sizeof(PolicyOpcode)) / sizeof(current_buffer[0]);
-    current_buffer = &current_buffer[policy_byte_count + 1];
+    current_buffer = &current_buffer[policy_buffers_occupied + 1];
   }
 
   return true;

sandbox/win/src/policy_low_level_unittest.cc

@@ -618,4 +618,50 @@ TEST(PolicyEngineTest, PolicyRuleCopyConstructorTwoStrings) {
   EXPECT_EQ(POLICY_MATCH, result);
   EXPECT_EQ(ASK_BROKER, pol_ev_copy.GetAction());
 }
+
+TEST(PolicyEngineTest, PolicyGenDoneCalledTwice) {
+  SetupNtdllImports();
+  // The specific rules here are not important.
+  PolicyRule pr_orig(ASK_BROKER);
+  EXPECT_TRUE(pr_orig.AddStringMatch(IF, 0, L"hello.*", CASE_SENSITIVE));
+
+  PolicyRule pr_copy(pr_orig);
+  EXPECT_TRUE(pr_orig.AddStringMatch(IF_NOT, 0, L"*.txt", CASE_SENSITIVE));
+  EXPECT_TRUE(pr_copy.AddStringMatch(IF_NOT, 0, L"*.txt", CASE_SENSITIVE));
+
+  PolicyGlobal* policy = MakePolicyMemory();
+  LowLevelPolicy policyGen(policy);
+  EXPECT_TRUE(policyGen.AddRule(1, &pr_orig));
+  EXPECT_TRUE(policyGen.AddRule(2, &pr_copy));
+  EXPECT_TRUE(policyGen.Done());
+
+  // Obtain opcode counts.
+  size_t tc1 = policy->entry[1]->opcode_count;
+  size_t tc2 = policy->entry[2]->opcode_count;
+
+  // Call Done() again.
+  EXPECT_TRUE(policyGen.Done());
+
+  // Expect same opcode counts.
+  EXPECT_EQ(tc1, policy->entry[1]->opcode_count);
+  EXPECT_EQ(tc2, policy->entry[2]->opcode_count);
+
+  // Confirm the rules work as before.
+  const wchar_t* name = nullptr;
+  POLPARAMS_BEGIN(eval_params)
+    POLPARAM(name)
+  POLPARAMS_END;
+
+  PolicyResult result;
+  PolicyProcessor pol_ev_orig(policy->entry[1]);
+  name = L"domo.txt";
+  result = pol_ev_orig.Evaluate(kShortEval, eval_params, _countof(eval_params));
+  EXPECT_EQ(NO_POLICY_MATCH, result);
+
+  name = L"hello.bmp";
+  result = pol_ev_orig.Evaluate(kShortEval, eval_params, _countof(eval_params));
+  EXPECT_EQ(POLICY_MATCH, result);
+  EXPECT_EQ(ASK_BROKER, pol_ev_orig.GetAction());
+}
+
 }  // namespace sandbox