1286306 - Create test case to ensure Windows DLL blocklist does not break

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, defect)

Description

[Tracy Walker [:tracy]]

+++ 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.

Comment 1

[(No longer employed by Mozilla) Aaron Klotz]

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.

Comment 2

[(No longer employed by Mozilla) Aaron Klotz]

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.

Comment 3

[(No longer employed by Mozilla) Aaron Klotz]

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.

Comment 4

[Jim Mathies [:jimm]]

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.

Comment 6

[Mason Chang [Inactive] [:mchang]]

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.

Comment 7

[Jim Mathies [:jimm]]

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.

Comment 8

[Jim Mathies [:jimm]]

Attachment: wip

Comment 9

[Jim Mathies [:jimm]]

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

Comment 10

[Jim Mathies [:jimm]]

Attachment: test

Comment 11

[Benjamin Smedberg]

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.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
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

Comment 13

[Jim Mathies [:jimm]]

I forgot to remove the assert with this, will post a patch.

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/cf78eed5c0e2

Comment 15

[Jim Mathies [:jimm]]

Attachment: remove assert

Comment 16

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8780100 [details] [diff] [review]
remove assert

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

Thanks!

Comment 17

[Oriol Brufau [:Oriol]]

(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...

Comment 18

[Jim Mathies [:jimm]]

(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.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/03da0450bbfd
Remove temporary assertion now that we have a test. r=aklotz

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/03da0450bbfd