Last Comment Bug 524904 - Add support for generic DLL blocklist
: Add support for generic DLL blocklist
: verified1.9.2
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: unspecified
: x86 Windows NT
P1 major (vote)
: Firefox 3.7a1
Assigned To: Vladimir Vukicevic [:vlad] [:vladv]
Depends on: 525103
  Show dependency treegraph
Reported: 2009-10-28 00:09 PDT by Vladimir Vukicevic [:vlad] [:vladv]
Modified: 2010-12-17 06:56 PST (History)
29 users (show)
dsicore: blocking‑firefox3.6+
ted: in‑testsuite?
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

the patch (8.40 KB, patch)
2009-10-28 00:10 PDT, Vladimir Vukicevic [:vlad] [:vladv]
no flags Details | Diff | Splinter Review
updated patch (17.22 KB, patch)
2009-10-28 16:39 PDT, Vladimir Vukicevic [:vlad] [:vladv]
no flags Details | Diff | Splinter Review
updated (17.75 KB, patch)
2009-11-03 14:39 PST, Vladimir Vukicevic [:vlad] [:vladv]
shaver: review+
Details | Diff | Splinter Review
updated again (17.37 KB, patch)
2009-11-03 15:11 PST, Vladimir Vukicevic [:vlad] [:vladv]
shaver: review+
Details | Diff | Splinter Review
updated again, v3 (17.77 KB, patch)
2009-11-04 15:02 PST, Vladimir Vukicevic [:vlad] [:vladv]
shaver: review+
Details | Diff | Splinter Review

Description User image Vladimir Vukicevic [:vlad] [:vladv] 2009-10-28 00:09:58 PDT
(Three strikes and you're out with bugzilla's search; if there's an existing bug on this, please dup it forward.)

We need the ability to prevent arbitrary DLLs from loading into our process.  This approach seems to work -- the core NT dll loading function is LdrLoadDll inside ntdll.dll; we can patch this and and redirect to our function where we get a chance to make the load fail.  This is not foolproof; truly malicious software could still probably get a DLL loaded in other ways, but this gives us a generic way to block "normal" DLLs, likely including LSPs.

The sample stub function here just blocks uxtheme.dll from loading, which means we don't get xp/vista/win7 theme and get a win2k-looking browser.
Comment 1 User image Vladimir Vukicevic [:vlad] [:vladv] 2009-10-28 00:10:57 PDT
Created attachment 408799 [details] [diff] [review]
the patch

Should be stable on Win2K/NT/Vista/Win7, and if it discovers code it doesn't understand at LdrLoadDll, it can abort and not patch anything.
Comment 2 User image Matthias Versen [:Matti] 2009-10-28 00:33:22 PDT
could this live somewhere in toolkit as blocklist extension ?
We would be able to update the dll blocklist and Thunderbird and Seamonkey could use this.
Comment 3 User image Ted Mielczarek [:ted.mielczarek] 2009-10-28 03:36:45 PDT
We could stick the code here:

and allow each app to specify its own list, if we wanted. I'm pretty sure this couldn't work as an extension, it needs to do things too early in startup.
Comment 4 User image Vladimir Vukicevic [:vlad] [:vladv] 2009-10-28 11:27:02 PDT
Yeah, it would have to go in WindowsWMain if it wants to go into toolkit.  It needs to happen before xpcom startup at the very least, ideally before nspr startup and any network access.
Comment 5 User image Damon Sicore (:damons) 2009-10-28 15:13:23 PDT
Who's going to review this?
Comment 6 User image Mike Shaver (:shaver -- probably not reading bugmail closely) 2009-10-28 15:25:21 PDT
I can review the XPCOM stuff, actually.
Comment 7 User image Jesse Ruderman 2009-10-28 15:27:48 PDT
See also bug 523350, "Warn users about malware if they crash with known-bad modules".
Comment 8 User image Vladimir Vukicevic [:vlad] [:vladv] 2009-10-28 16:39:58 PDT
Created attachment 408975 [details] [diff] [review]
updated patch

Updated patch, now with an actual blocklist, with uxtheme.dll on it as an example.  Also added the ability to block dlls by version, blocking all with a version <= a given version.
Comment 9 User image Vladimir Vukicevic [:vlad] [:vladv] 2009-10-29 17:20:22 PDT
There are builds with this patch at -- uxtheme.dll is in the blocklist, so you should see a win2k-style browser.  Would be great to get some testing across XP/Vista/Win7.
Comment 10 User image Vladimir Vukicevic [:vlad] [:vladv] 2009-11-02 13:41:32 PST
Comment on attachment 408975 [details] [diff] [review]
updated patch

dvander, can you take a look at the x86 mucking/patching that's done here?

rob, can you do a quick pass on all the win32 usage?

Comment 11 User image David Anderson [:dvander] 2009-11-02 15:11:10 PST
Comment on attachment 408975 [details] [diff] [review]
updated patch

(origBytes[nBytes] >= 0x88 && origBytes[nBytes] <= 0x8B) {
>+	// various MOVs
>+        nBytes += 2;

There are also 0xB? MOVs that are symmetrical. Either one can be up to around 10 bytes. If the intent is to bail out if we don't understand the instruction, we should probably peek at the next byte here to make sure it is in a 2-byte format.

(origBytes[nBytes] == 0x55 ||
>+                 origBytes[nBytes] == 0x51)
>+      {
>+	// 1-byte PUSH
>+        nBytes++;

This could be >= 0x50 && <= 0x56, since the encoding is 0x50+r.
Comment 12 User image Matt Woodrow (:mattwoodrow) 2009-11-02 16:06:18 PST
>+      } else if (origBytes[nBytes] == 0x68 ||
>+		 origBytes[nBytes] == 0xE8 ||
>+		 origBytes[nBytes] == 0xE9)
>+      {
>+	// PUSH or CALL/JMP with 4-byte operand
>+        nBytes += 5;

Both 0xE8 (call) and 0xE9 (jump) use 32bit relative addressing for the operand so will fail if copied directly into the trampoline.
The offset is relative to the following instruction, so this needs to be adjusted for it's new location in the trampoline.
Comment 13 User image Vladimir Vukicevic [:vlad] [:vladv] 2009-11-03 14:39:07 PST
Created attachment 410054 [details] [diff] [review]

Updated, with feedback about insn generation.  We don't need to handle the complex call/jump from my inspection, so I took it out.
Comment 14 User image Mike Shaver (:shaver -- probably not reading bugmail closely) 2009-11-03 14:44:39 PST
Comment on attachment 410054 [details] [diff] [review]

>+  // Dirty secret about this UNICODE_STRING: it's not guaranteed to be
>+  // null-terminated, and Length is supposed to contain the number of
>+  // characters.  But in the UNICODE_STRING passed to this function,
>+  // that doesn't seem to be true -- Length is often much bigger than
>+  // the actual valid characters of the string, which seems to always
>+  // be null terminated.  So, we take the minimum of len or the length
>+  // to the first null byte, if any, but we still can't assume the null
>+  // termination.
>+  int len = moduleFileName->Length;
>+  wchar_t *fn_buf = moduleFileName->Buffer;
>+  int count = 0;
>+  while (count < len && fn_buf[count] != 0)
>+    count++;

Is this better than just copying the buffer to one that's known to be long enough, and then forcibly NUL-terminating?  Seems like that would be a lot cleaner, and require us to inspect fewer lines of pointer math and raw string manipulation code.

r=shaver; can land for beta without tests, but please add some once that dust has settled.  (I don't think it'll be too hard.)
Comment 15 User image Vladimir Vukicevic [:vlad] [:vladv] 2009-11-03 15:11:55 PST
Created attachment 410061 [details] [diff] [review]
updated again

Yeah, ok, now with less string pointer goop.
Comment 16 User image Mike Shaver (:shaver -- probably not reading bugmail closely) 2009-11-03 15:14:50 PST
Comment on attachment 410061 [details] [diff] [review]
updated again

r=shaver -- follow-up bug for tests, or do them here?
Comment 17 User image Vladimir Vukicevic [:vlad] [:vladv] 2009-11-03 22:34:14 PST
Will just do them here -- I'll check in with a pre-blocked dll name and we'll go from there, maybe with jsctypes?
Comment 18 User image Vladimir Vukicevic [:vlad] [:vladv] 2009-11-03 22:39:05 PST

I reserved "mozdllblockingtest.dll" and "mozdllblockingtest_versioned.dll" for testing.
Comment 19 User image Ted Mielczarek [:ted.mielczarek] 2009-11-04 06:36:14 PST
(In reply to comment #18)
> I reserved "mozdllblockingtest.dll" and "mozdllblockingtest_versioned.dll" for
> testing.

Did you mean to check that in? Or was that a note for a future version? The changeset you linked doesn't contain those names.
Comment 20 User image Jim Mathies [:jimm] 2009-11-04 08:58:59 PST
Potential crashes from this landing on 

WINNT 5.2 mozilla-central test everythingelse
WINNT 5.1 mozilla-central talos nochrome
WINNT 5.1 mozilla-central talos tp4

Although the stacks don't point to anything specific.
Comment 22 User image Jim Mathies [:jimm] 2009-11-04 09:05:17 PST
I backed this out temporarily to see if it was the cause.
Comment 23 User image Jim Mathies [:jimm] 2009-11-04 10:49:20 PST
WINNT 5.2 mozilla-central test everythingelse went green prior to the back out, so possibly this was not the cause on there.
Comment 24 User image Vladimir Vukicevic [:vlad] [:vladv] 2009-11-04 15:02:18 PST
Created attachment 410356 [details] [diff] [review]
updated again, v3

strncpy/wcsncpy don't null terminate, duhh.
Comment 25 User image Mike Shaver (:shaver -- probably not reading bugmail closely) 2009-11-04 15:07:11 PST
Comment on attachment 410356 [details] [diff] [review]
updated again, v3

r=shaver. fricking strncpy.
Comment 26 User image Nochum Sossonko [:Natch] 2009-11-04 21:37:47 PST
With this patch I keep on getting this error:

locklist.cpp(134) : error C2065: 'STATUS_DLL_NOT_FOUND' : undeclared identifier
make[5]: *** [nsBrowserApp.obj] Error 2
make[5]: Leaving directory `/c/Users/Natch/Documents/mozilla_trunk/m-c_release/b
make[4]: *** [libs] Error 2
make[4]: Leaving directory `/c/Users/Natch/Documents/mozilla_trunk/m-c_release/b
make[3]: *** [libs_tier_app] Error 2
make[3]: Leaving directory `/c/Users/Natch/Documents/mozilla_trunk/m-c_release'
make[2]: *** [tier_app] Error 2
make[2]: Leaving directory `/c/Users/Natch/Documents/mozilla_trunk/m-c_release'
make[1]: *** [default] Error 2
make[1]: Leaving directory `/c/Users/Natch/Documents/mozilla_trunk/m-c_release'
make: *** [build] Error 2

Is there any sdk requirements (or any others) to build with this blocklist stuff?

My mozconfig:

mk_add_options MOZ_CO_PROJECT=browser
ac_add_options --enable-application=browser
ac_add_options --disable-static
ac_add_options --disable-libxul
ac_add_options --with-windows-version=600
mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/../m-c_release

I'm building with MSVC8.
Comment 27 User image Nochum Sossonko [:Natch] 2009-11-04 22:02:27 PST
Ok, running |make| in /toolkit/xre fixed it for me (I #included <NTStatus.h> but no longer require that), so not really sure what the deal is there...
Comment 28 User image timeless 2009-11-05 01:28:44 PST
Comment on attachment 410356 [details] [diff] [review]
updated again, v3

please add a null check ..., unless we're simply saying "yes, let's crash":
>+  nsAutoArrayPtr<wchar_t> fname = new wchar_t[len+1];
>+      nsAutoArrayPtr<wchar_t> full_fname = new wchar_t[pathlen+1];
>+        nsAutoArrayPtr<unsigned char> infoData = new unsigned char[infoSize];

>+  fname[len] = 0; // *ncpy considered harmful


>+  if (!ok)
>+    PR_LogPrint ("LdrLoadDll hook failed, no dll blocklisting active");

why the space before ( ?

why the trailing blank lines:
>diff --git a/toolkit/xre/nsWindowsDllBlocklist.h b/toolkit/xre/nsWindowsDllBlocklist.h

I claim that these should be PR_LogPrint:

>+        //printf ("Unknown x86 instruction byte 0x%02x, aborting trampoline\n", origBytes[nBytes]);
>+      //printf ("Too big!");
>+      //printf ("VirtualProtectEx failed! %d\n", GetLastError());
Comment 29 User image Vladimir Vukicevic [:vlad] [:vladv] 2009-11-05 12:24:57 PST
Pushed to m-c and 1.9.2.  Needs tests.
Comment 30 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2009-11-05 14:48:03 PST
Vlad, do you have changeset id's for us? Would be nice. Thanks.
Comment 32 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2009-11-08 05:20:29 PST
Vladimir, when QA wants to verify this new feature it looks like that we have to build Firefox on our own with new entries added to nsWindowsDllBlocklist.h? I have a good example of a crashing module which I would like to test on 1.9.2.
Comment 33 User image Vladimir Vukicevic [:vlad] [:vladv] 2009-11-08 14:08:07 PST
Correct, or if you have a crashing module, if you can, rename it to the pre-blocked testing one, "mozdllblockingtest.dll".  If it's a component or something of that type, it should still continue to work/crash.
Comment 34 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2009-11-09 05:13:01 PST
Accidentally I have build the first tryserver build with non-lowercase naming. Given by the tinderbox we get a crash on Linux:

No idea if that is related but I have to test it on Linux. Builds are located here:
Comment 35 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2009-11-10 09:53:26 PST
Ok, I did a second test to verify that this implementation is working. I'm aware of a startup crash with Google Desktop Search 4. It's installing a couple of files inside the components folder including the GoogleDesktopMozilla.dll. Once you have started Firefox the crash will happen immediately. So I have sent a patch to the tryserver which includes the googledesktopmozilla.dll in the block list. It can be downloaded from the same location as given in comment 34.

While testing the patched version and unpatched versions in parallel the former one didn't ever crash. I used WinDBG to analyze the loaded modules and it doesn't list the DLL mentioned above.

Marking as verified on 1.9.2 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2b3pre) Gecko/20091109 Namoroka/3.6b3pre (.NET CLR 3.5.30729)
Comment 36 User image Vladimir Vukicevic [:vlad] [:vladv] 2009-11-10 14:24:27 PST
For the Linux crash, I believe it's unrelated -- none of this code is executed on linux at all.
Comment 37 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2009-11-13 06:17:41 PST
Vlad, I missed the test against a specific version of a module. I wanted to try that now but I don't know how the version info is build off:

> + { "mozdllblockingtest_versioned.dll", 0x0000000400000000ULL },

What would I have to enter for a file version number like 4.2006.509.1244?
Comment 38 User image Ted Mielczarek [:ted.mielczarek] 2009-11-13 06:42:24 PST
That's a 64-bit integer containing all four version components as 16-bit integers, so you'd want:
(that's (4 << 48) | (2006 << 32) | (509 << 16) | 1244 )
Comment 39 User image Mike Shaver (:shaver -- probably not reading bugmail closely) 2009-11-13 07:07:58 PST
That sounds like something that deserves a LIBRARY_VERSION(p1, p2, p3, p4) sort of macro, though Windows likely already provides one?
Comment 40 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2009-11-17 08:41:34 PST
Vlad, while testing the whitelisting feature on OS X and Linux I wonder if we could have a similar feature for those platforms so we could blocklist .dylib and .so libraries. Is that possible and shall I file two bugs so we can get it implemented? Or do we strictly limit it for Windows right now?
Comment 41 User image Vladimir Vukicevic [:vlad] [:vladv] 2009-11-17 11:08:32 PST
Not as easily I don't think, and the need isn't nearly as great as on windows.

Note You need to log in before you can comment on or make changes to this bug.