Closed Bug 1286306 Opened 3 years ago Closed 3 years ago

Create test case to ensure Windows DLL blocklist does not break

Categories

(Core :: Security: Process Sandboxing, defect)

Unspecified
Windows
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox50 --- affected
firefox51 --- fixed

People

(Reporter: tracy, Assigned: jimm, Mentored)

References

Details

(Whiteboard: sbwc2)

Attachments

(2 files, 1 obsolete file)

+++ This bug was initially created as a clone of Bug #1285356 +++

Jim suggests that I write a test case to ensure that the DDL blocklist works as expected.  He will write the code to expose the blocklist and mentor me on figuring out the rest.
Note that the DLL blocklist already contains sBlocklistInitFailed and sUser32BeforeBlocklist variables, so we just need to export an API from mozglue that evaluates those conditions and returns a single bool. From there it should be pretty easy to add to nsSystemInfo or whatever.
Summary: Create test case to ensure Windows DLL blocklist does not break due to sandboxing → Create test case to ensure Windows DLL blocklist does not break
Bug 1285356 is now on inbound. As part of this patch, I'd suggest that we remove the assertion that I added in that bug in favor of the tests added in this one.
Whiteboard: sbwc2
In https://bugzilla.mozilla.org/show_bug.cgi?id=1285356#c13 we already have a user reporting that the assertion is causing their debug build to fail when run locally. We're probably going to need this bug fixed sooner rather than later.
I have a patch to expose this but unfortunately my win7 dev workstation always fails the sUser32BeforeBlocklist test. Going to try testing in a vm.
Duplicate of this bug: 1289222
Just another data point, this only seems to happen for me when running via conemu, running the mozilla-build script. If I run via the mozilla-build environment directly, I don't get this error.
I'm having a heck of a time finding a system that doesn't fail the sUser32BeforeBlocklist test. I'vetested on my win7 workstation, a win8.1 touch tablet, and a windows 10 vmware image.

I'm going to go ahead and work up the simple test case as well so I can push it to try, see what happens.
Assignee: twalker → jmathies
Attached patch wip (obsolete) — Splinter Review
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0221edd9626f8e50491108559d265c91312c251a

20:13:01     INFO -  508 INFO TEST-START | toolkit/xre/test/browser_checkdllblockliststate.js
20:13:01     INFO -  MEMORY STAT | vsize 683MB | vsizeMaxContiguous 540MB | residentFast 266MB | heapAllocated 93MB
20:13:01     INFO -  509 INFO TEST-OK | toolkit/xre/test/browser_checkdllblockliststate.js | took 178ms
Attached patch testSplinter Review
Attachment #8775301 - Attachment is obsolete: true
Attachment #8776030 - Flags: review?(benjamin)
Comment on attachment 8776030 [details] [diff] [review]
test

Now that we no longer support this embedding API, we can make the linkage and flags a lot simpler, but this is good enough for now.
Attachment #8776030 - Flags: review?(benjamin) → review+
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf78eed5c0e2
Add an app info property exposing the state of the Windows dll blocklist, and test the value during browser test runs. r=bsmedberg
Keywords: checkin-needed
I forgot to remove the assert with this, will post a patch.
Flags: needinfo?(jmathies)
https://hg.mozilla.org/mozilla-central/rev/cf78eed5c0e2
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Attached patch remove assertSplinter Review
Flags: needinfo?(jmathies)
Attachment #8780100 - Flags: review?(aklotz)
Comment on attachment 8780100 [details] [diff] [review]
remove assert

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

Thanks!
Attachment #8780100 - Flags: review?(aklotz) → review+
(In reply to Jim Mathies [:jimm] from comment #15)
> Created attachment 8780100 [details] [diff] [review]
> remove assert

Is there any reason to not land this? That assert is so annoying...
(In reply to Oriol from comment #17)
> (In reply to Jim Mathies [:jimm] from comment #15)
> > Created attachment 8780100 [details] [diff] [review]
> > remove assert
> 
> Is there any reason to not land this? That assert is so annoying...

my bad, will land.
Pushed by jmathies@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/03da0450bbfd
Remove temporary assertion now that we have a test. r=aklotz
You need to log in before you can comment on or make changes to this bug.