1299611 - Adding policy rules to the Windows sandbox can cause a buffer overrun

Status: NEW

(Core :: Security: Process Sandboxing, defect)

Description

[David Parks [:handyman]]

Attachment: bigrules.patch: adds dummy sandbox file rules to cause the overrun

Quick summary: 
Right now, this is not an issue in "normal" builds but, if enough additional low-level policy rules are added to the sandbox, the "mullet" structure ("Opcodes in the front, Data in the back") will silently overwrite itself, causing mysterious crashes down the line.

The deal is that policy_low_level.cc attempts to pack the policies in a contiguous buffer.  The policies are broken into "OpCodes" and "Data".  Right now, everything fits (despite slightly incorrect accounting of the data size, which is a related bug) so there is no problem.  However, I had added a bunch of additional rules to PluginProcessParent ("AddSandboxAllowedFile") rules.  This swamped the structure, which fills itself by adding opcodes to the top of the memblock and adding data to the bottom.  When there is too much to write, they overwrite each other in the middle of the memory block.

The attachment bigrules.patch adds dummy sandbox file rules to cause the overrun.  It is just to demonstrate the problem.  The attachment catchoverrun.patch fixes the issue by adding ASSERTions to catch the failure.  It also fixes the byte accounting, which, as I said above, was slightly off.

Comment 1

[David Parks [:handyman]]

Attachment: catchoverrun.patch

Fix.

Comment 2

[David Parks [:handyman]]

Note that PolicyBuffer is one size_t plus a list of PolicyOpcodes (this is the reason for the size_t added to the accounting.)

Comment 3

[David Parks [:handyman]]

Attachment: Fix buffer allocation that caused security descriptors to be trashed

"Perfect" byte accounting to leave no unnecessary gaps in the buffer.

Comment 4

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8786941 [details] [diff] [review]
Fix buffer allocation that caused security descriptors to be trashed

I think this patch is going to cause problems because this code comes straight from upstream. I'm going to redirect to Bob in this case since he is in a better position to determine whether to upstream this patch, or alternatively determine how make the change in a way that is merge-friendly.

Comment 5

[Bob Owen (:bobowen) PTO until 13th Aug]

Comment on attachment 8786941 [details] [diff] [review]
Fix buffer allocation that caused security descriptors to be trashed

Nice find over the opcode count, that does look like a bug.

However, unless we're actually going to hit this with our rules, I think this should be reported and fixed upstream.

Comment 6

[David Parks [:handyman]]

Cool.  The Chromium engineers are on it.  See:

https://bugs.chromium.org/p/chromium/issues/detail?id=643293&can=2&q=low%20level&colspec=ID%20Pri%20M%20Stars%20ReleaseBlock%20Component%20Status%20Owner%20Summary%20OS%20Modified#

I don't think we need to track this.