Closed Bug 524904 Opened 15 years ago Closed 15 years ago

Add support for generic DLL blocklist

Categories

(Firefox :: General, defect, P1)

x86
Windows NT
defect

Tracking

()

RESOLVED FIXED
Firefox 3.7a1
Tracking Status
status1.9.2 --- beta2-fixed

People

(Reporter: vlad, Assigned: vlad)

References

Details

(Keywords: verified1.9.2, Whiteboard: [crashkill])

Attachments

(2 files, 3 obsolete files)

(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.
Flags: blocking-firefox3.6?
Attached patch the patch (obsolete) — Splinter Review
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.
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.
We could stick the code here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsWindowsWMain.cpp

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.
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.
Who's going to review this?
Flags: blocking-firefox3.6? → blocking-firefox3.6+
Priority: -- → P1
Whiteboard: [crashkill]
I can review the XPCOM stuff, actually.
See also bug 523350, "Warn users about malware if they crash with known-bad modules".
Depends on: 525103
Attached patch updated patch (obsolete) — Splinter Review
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.
Attachment #408799 - Attachment is obsolete: true
There are builds with this patch at https://build.mozilla.org/tryserver-builds/vladimir@mozilla.com-try-440024942f9c/ -- 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.
Attachment #408975 - Flags: review?(robert.bugzilla)
Attachment #408975 - Flags: review?(dvander)
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?

thanks!
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.
>+      } 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.
Attached patch updated (obsolete) — Splinter 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.
Attachment #408975 - Attachment is obsolete: true
Attachment #410054 - Flags: review?(shaver)
Attachment #410054 - Flags: review?(dvander)
Attachment #408975 - Flags: review?(shaver)
Attachment #408975 - Flags: review?(robert.bugzilla)
Attachment #408975 - Flags: review?(dvander)
Comment on attachment 410054 [details] [diff] [review]
updated

>+  // 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.)
Attachment #410054 - Flags: review?(shaver) → review+
Attached patch updated againSplinter Review
Yeah, ok, now with less string pointer goop.
Attachment #410054 - Attachment is obsolete: true
Attachment #410061 - Flags: review?(shaver)
Attachment #410054 - Flags: review?(dvander)
Comment on attachment 410061 [details] [diff] [review]
updated again

r=shaver -- follow-up bug for tests, or do them here?
Attachment #410061 - Flags: review?(shaver) → review+
Will just do them here -- I'll check in with a pre-blocked dll name and we'll go from there, maybe with jsctypes?
http://hg.mozilla.org/mozilla-central/rev/41c421f97869

I reserved "mozdllblockingtest.dll" and "mozdllblockingtest_versioned.dll" for testing.
(In reply to comment #18)
> http://hg.mozilla.org/mozilla-central/rev/41c421f97869
> 
> 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.
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.
I backed this out temporarily to see if it was the cause.
WINNT 5.2 mozilla-central test everythingelse went green prior to the back out, so possibly this was not the cause on there.
strncpy/wcsncpy don't null terminate, duhh.
Comment on attachment 410356 [details] [diff] [review]
updated again, v3

r=shaver. fricking strncpy.
Attachment #410356 - Flags: review+
With this patch I keep on getting this error:

c:\users\natch\documents\mozilla_trunk\mozilla-central\toolkit\xre\nsWindowsDllB
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
rowser/app'
make[4]: *** [libs] Error 2
make[4]: Leaving directory `/c/Users/Natch/Documents/mozilla_trunk/m-c_release/b
rowser'
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.
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 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());
Pushed to m-c and 1.9.2.  Needs tests.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
Vlad, do you have changeset id's for us? Would be nice. Thanks.
Target Milestone: --- → Firefox 3.7a1
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.
Target Milestone: --- → Firefox 3.7a1
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.
Accidentally I have build the first tryserver build with non-lowercase naming. Given by the tinderbox we get a crash on Linux:

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1257770520.1257771759.17707.gz&fulltext=1

No idea if that is related but I have to test it on Linux. Builds are located here:https://build.mozilla.org/tryserver-builds/hskupin@mozilla.com-bug524904-testwithGDS/
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)
Severity: normal → major
Keywords: verified1.9.2
For the Linux crash, I believe it's unrelated -- none of this code is executed on linux at all.
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?
That's a 64-bit integer containing all four version components as 16-bit integers, so you'd want:
0x000407D601FD04DCULL
(that's (4 << 48) | (2006 << 32) | (509 << 16) | 1244 )
That sounds like something that deserves a LIBRARY_VERSION(p1, p2, p3, p4) sort of macro, though Windows likely already provides one?
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?
Not as easily I don't think, and the need isn't nearly as great as on windows.
You need to log in before you can comment on or make changes to this bug.