Closed Bug 1014002 Opened 10 years ago Closed 10 years ago

BinScope is currently failing for plugin-container.exe when doing a debug build with --enable-content-sandbox

Categories

(Core :: Security, defect)

x86_64
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: bobowen, Assigned: TimAbraldes)

References

Details

Attachments

(1 file)

I noticed this when trying to get tests running on tinderbox with the sandbox enabled.

Here's the relevant part of the BinScope log:

<item>
  <targetVersion>32.0a1 Debug</targetVersion>
  <targetPath>obj-i686-pc-mingw32/dist/bin/plugin-container.exe</targetPath>
  <issueClass>Binary</issueClass>
  <issueType>FunctionPointersCheck</issueType>
  <key>FunctionPointersCheck:obj-i686-pc-mingw32/dist/bin/plugin-container.exe</key>
  <testTarget>
    <category>Binary</category>
    <key>83A6BB88F0DE08CA34225F8285F4985B92D8FE22</key>
    <type>Target</type>
    <logTime>2014-05-20T15:43:44.8162531+01:00</logTime>
  </testTarget>
  <result>FAIL</result>
  <instance>1</instance>
  <FunctionPointer0>Function pointer initialize_proc_thread_attribute_list could be overrun by the following buffers:
hexdig
private_mem
</FunctionPointer0>
  <FunctionPointer1>Function pointer _imp___onexit could be overrun by the following buffers:
hexdig
private_mem
</FunctionPointer1>
  <FunctionPointer2>Function pointer update_proc_thread_attribute_list could be overrun by the following buffers:
hexdig
private_mem
</FunctionPointer2>
  <FunctionPointer3>Function pointer delete_proc_thread_attribute_list could be overrun by the following buffers:
hexdig
private_mem
</FunctionPointer3>
  <type>BinScopeCheck</type>
  <logTime>2014-05-20T15:43:44.8162531+01:00</logTime>
</item>
This is also happening with the patch from bug 985252 applied.

I've downloaded BinScope and run it on my local builds. I'm able to reproduce the error messages mentioned in comment 0.

The function pointers that BinScope is complaining about - `initialize_proc_thread_attribute_list`, `update_proc_thread_attribute_list`, and `delete_proc_thread_attribute_list` - are all in startup_information.cc. See [1], [2], and [3], respectively.

The `private_mem` and `hexdig` buffers are in dtoa.cc. See [4] and [5] respectively.

In the build output, the only difference I see in the command line that builds plugin_container.exe is that it includes "../../security/sandbox/sandbox_s.lib" in addition to other libs and object files. This is as expected.

Here is the command line that builds "sandbox_s.lib" (some .obj files omitted):
  c:/builds/moz2_slave/try-w32-d-00000000000000000000/build/obj-firefox/_virtualenv/Scripts/python.exe c:/builds/moz2_slave/try-w32-d-00000000000000000000/build/config/expandlibs_gen.py --depend .deps/sandbox_s.lib.desc.pp -o sandbox_s.lib.desc [...] dtoa.obj [...] startup_information.obj [...]

Both dtoa.obj and startup_information.obj are present (as expected).

At this point I'm rather perplexed because I would expect cl.exe (which is called somehow by our python wrappers) to arrange the buffers and the function pointers in such a way that BinScope wouldn't freak out. If cl.exe isn't doing that, I would have expected us to run into this issue (with other buffers and function pointers) long before including sandbox_s.lib in plugin-container.exe.

[1] http://hg.mozilla.org/mozilla-central/annotate/6a984e21c2ca/security/sandbox/chromium/base/win/startup_information.cc#l18
[2] http://hg.mozilla.org/mozilla-central/annotate/6a984e21c2ca/security/sandbox/chromium/base/win/startup_information.cc#l28
[3] http://hg.mozilla.org/mozilla-central/annotate/6a984e21c2ca/security/sandbox/chromium/base/win/startup_information.cc#l32
[4] http://hg.mozilla.org/mozilla-central/annotate/6a984e21c2ca/security/sandbox/chromium/base/third_party/dmg_fp/dtoa.cc#l229
[5] http://hg.mozilla.org/mozilla-central/annotate/6a984e21c2ca/security/sandbox/chromium/base/third_party/dmg_fp/dtoa.cc#l1490
I ran BinScope's "FunctionPointers" test (the one that is failing) against firefox.exe and against a plugin-container.exe that doesn't include sandbox_s.lib.

Here is the output I got:
  "No function pointers detected"

Apparently firefox.exe and plugin-container.exe pass the "FunctionPointers" test because they don't have any static/global function pointers (they can have stack-based function pointers which would be dealt with in a separate BinScope test and which would be made safe by the /GS flag to cl.exe). As soon as we try to complicate plugin-container.exe by including sandbox_s.lib, BinScope's "FunctionPointers" test starts failing. I ran the FunctionPointers check against xul.dll and I got a huge number of failures (the log generated was larger than 1MB).

My current suggestions would be that we do the following:
  1) Disable the FunctionPointers check. It is disabled by default and (I believe) generates too many warnings/failures to really be useful
  2) Run BinScope against our built DLLs in addition to our EXEs. It's kind of pointless to run this tool against firefox.exe without also running it against xul.dll (where the majority of our code lives). I've filed bug 1021214 for this.
Attached patch Patch v1Splinter Review
So... maybe something like this.

Try run with the change from this patch applied and the changes from the patch in bug 985252 applied:
  https://tbpl.mozilla.org/?tree=Try&rev=5264a3e88ec9

Ted - Of the Build Config peers you seem to have been the most involved in getting BinScope up and running. Can you take this review or maybe delegate it? Thanks!
Assignee: nobody → tabraldes
Status: NEW → ASSIGNED
Attachment #8437296 - Flags: review?(ted)
Ted - review ping
Comment on attachment 8437296 [details] [diff] [review]
Patch v1

Review of attachment 8437296 [details] [diff] [review]:
-----------------------------------------------------------------

Seems sensible. Sorry for the delay.
Attachment #8437296 - Flags: review?(ted) → review+
https://hg.mozilla.org/mozilla-central/rev/549e5f2e49d0
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: