Open Bug 1299611 Opened 4 years ago Updated 2 years ago

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


(Core :: Security: Process Sandboxing, defect)

Not set




(Reporter: handyman, Unassigned)


(Whiteboard: sb+)


(2 files, 1 obsolete file)

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 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.
Attached patch catchoverrun.patch (obsolete) — Splinter Review
Assignee: nobody → davidp99
Note that PolicyBuffer is one size_t plus a list of PolicyOpcodes (this is the reason for the size_t added to the accounting.)
"Perfect" byte accounting to leave no unnecessary gaps in the buffer.
Attachment #8786921 - Attachment is obsolete: true
Attachment #8786941 - Flags: review?(aklotz)
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.
Attachment #8786941 - Flags: review?(aklotz) → review?(bobowen.code)
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.
Attachment #8786941 - Flags: review?(bobowen.code)
Component: Plug-ins → Security: Process Sandboxing
Whiteboard: sb+
Assignee: davidp99 → nobody
You need to log in before you can comment on or make changes to this bug.