Chrome browser process DLL blacklist.

Chrome browser process DLL blacklist.

This patch allows for blocking of module loading in the browser process.
It does not actually prevent any modules from loading.

BUG=329023
TEST=chrome_elf_unittests

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=241548

[robertshield, 2013-12-16 23:02:46]

PTAL, thanks!

[grt (UTC plus 2), 2013-12-16 23:33:49]

https://codereview.chromium.org/107663008/diff/20001/chrome_elf/blacklist.gypi
File chrome_elf/blacklist.gypi (right):

https://codereview.chromium.org/107663008/diff/20001/chrome_elf/blacklist.gyp...
chrome_elf/blacklist.gypi:12: 'blacklist/blacklist_interceptions.cc',
this appears to depend on stuff in base. please add an explicit dependency on
base to this target.

https://codereview.chromium.org/107663008/diff/20001/chrome_elf/blacklist.gyp...
chrome_elf/blacklist.gypi:33: 'type': 'shared_library',
it looks like this dll and the next are only loaded dynamically. should they not
be loadable_module targets like the third?

https://codereview.chromium.org/107663008/diff/20001/chrome_elf/blacklist/tes...
File chrome_elf/blacklist/test/blacklist_test.cc (right):

https://codereview.chromium.org/107663008/diff/20001/chrome_elf/blacklist/tes...
chrome_elf/blacklist/test/blacklist_test.cc:116: // Remove the DLL from the
blacklist. Ensure that it loads and that it's
it's -> its

https://codereview.chromium.org/107663008/diff/20001/chrome_elf/version_assem...
File chrome_elf/version_assembly_manifest_action.gypi (right):

https://codereview.chromium.org/107663008/diff/20001/chrome_elf/version_assem...
chrome_elf/version_assembly_manifest_action.gypi:37: }
what is the change here?

[robertshield, 2013-12-17 03:43:03]

Thanks, PTAL

https://codereview.chromium.org/107663008/diff/20001/chrome_elf/blacklist.gypi
File chrome_elf/blacklist.gypi (right):

https://codereview.chromium.org/107663008/diff/20001/chrome_elf/blacklist.gyp...
chrome_elf/blacklist.gypi:12: 'blacklist/blacklist_interceptions.cc',
On 2013/12/16 23:33:49, grt wrote:
> this appears to depend on stuff in base. please add an explicit dependency on
> base to this target.

This was explicitly not done to ensure no dependencies on base that would
involve pulling in compilation units that themselves pull in undesirable
dependencies. The three things being depended on are:

1) basictypes.h
2) string16.h
3) pe_image.h/pe_image.cc

1 and 2 are only header files on windows, 3 is defined in base_static which is
now depended on.

If 1) or 2) are ever refactored in such a way that this changes, chrome_elf
*should* break. Added comments here and in the .cc file to this effect.

https://codereview.chromium.org/107663008/diff/20001/chrome_elf/blacklist.gyp...
chrome_elf/blacklist.gypi:33: 'type': 'shared_library',
On 2013/12/16 23:33:49, grt wrote:
> it looks like this dll and the next are only loaded dynamically. should they
not
> be loadable_module targets like the third?

Done.

https://codereview.chromium.org/107663008/diff/20001/chrome_elf/blacklist/tes...
File chrome_elf/blacklist/test/blacklist_test.cc (right):

https://codereview.chromium.org/107663008/diff/20001/chrome_elf/blacklist/tes...
chrome_elf/blacklist/test/blacklist_test.cc:116: // Remove the DLL from the
blacklist. Ensure that it loads and that it's
On 2013/12/16 23:33:49, grt wrote:
> it's -> its

Done.

https://codereview.chromium.org/107663008/diff/20001/chrome_elf/version_assem...
File chrome_elf/version_assembly_manifest_action.gypi (right):

https://codereview.chromium.org/107663008/diff/20001/chrome_elf/version_assem...
chrome_elf/version_assembly_manifest_action.gypi:37: }
On 2013/12/16 23:33:49, grt wrote:
> what is the change here?

The addition of a newline that inexplicably doesn't show up in rietveld.

[grt (UTC plus 2), 2013-12-17 04:28:01]

lgtm

[Sigurður Ásgeirsson, 2013-12-17 14:31:00]

lgtm with a couple of nits - it'd be great to have test coverage for import
dependency regressions.

https://codereview.chromium.org/107663008/diff/40001/chrome/chrome_exe.gypi
File chrome/chrome_exe.gypi (right):

https://codereview.chromium.org/107663008/diff/40001/chrome/chrome_exe.gypi#n...
chrome/chrome_exe.gypi:469: '../chrome_elf/chrome_elf.gyp:chrome_elf',
The DLL dependency list and order here could do with a test to protect against
regressions.

https://codereview.chromium.org/107663008/diff/40001/chrome_elf/chrome_elf.gyp
File chrome_elf/chrome_elf.gyp (right):

https://codereview.chromium.org/107663008/diff/40001/chrome_elf/chrome_elf.gy...
chrome_elf/chrome_elf.gyp:35: 'IgnoreAllDefaultLibraries': 'true',
I'd suggest adding a unittest to verify the user32 non-dependency in elf. It
shouldn't be a problem for tests to depend on chrome_elf itself?

[robertshield, 2013-12-17 14:34:10]

https://codereview.chromium.org/107663008/diff/40001/chrome/chrome_exe.gypi
File chrome/chrome_exe.gypi (right):

https://codereview.chromium.org/107663008/diff/40001/chrome/chrome_exe.gypi#n...
chrome/chrome_exe.gypi:469: '../chrome_elf/chrome_elf.gyp:chrome_elf',
On 2013/12/17 14:31:01, Sigurður Ásgeirsson wrote:
> The DLL dependency list and order here could do with a test to protect against
> regressions.

Agreed! Tests for this and the other are in a follow up CL here:
https://codereview.chromium.org/109483003/

https://codereview.chromium.org/107663008/diff/40001/chrome_elf/chrome_elf.gyp
File chrome_elf/chrome_elf.gyp (right):

https://codereview.chromium.org/107663008/diff/40001/chrome_elf/chrome_elf.gy...
chrome_elf/chrome_elf.gyp:35: 'IgnoreAllDefaultLibraries': 'true',
On 2013/12/17 14:31:01, Sigurður Ásgeirsson wrote:
> I'd suggest adding a unittest to verify the user32 non-dependency in elf. It
> shouldn't be a problem for tests to depend on chrome_elf itself?

Agreed! Tests for this and the other are in a follow up CL here:
https://codereview.chromium.org/109483003/

[cpu_(ooo_6.6-7.5), 2013-12-17 20:18:22]

lgtm

[commit-bot: I haz the power, 2013-12-17 20:43:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/robertshield@chromium.org/107663008/40001

[rvargas (doing something else), 2013-12-17 22:09:27]

Sorry for the delay.

https://codereview.chromium.org/107663008/diff/40001/chrome_elf/blacklist/bla...
File chrome_elf/blacklist/blacklist.cc (right):

https://codereview.chromium.org/107663008/diff/40001/chrome_elf/blacklist/bla...
chrome_elf/blacklist/blacklist.cc:42: VERSION_WIN8_1,      // Code named Windows
Blue
nit: the code name seems pointless at this point

https://codereview.chromium.org/107663008/diff/40001/chrome_elf/blacklist/bla...
chrome_elf/blacklist/blacklist.cc:144: HKEY beacon_key = 0;
nit: NULL

https://codereview.chromium.org/107663008/diff/40001/chrome_elf/blacklist/bla...
chrome_elf/blacklist/blacklist.cc:157: ::RegCloseKey(beacon_key);
Only close the key on ERROR_SUCCESS

https://codereview.chromium.org/107663008/diff/40001/chrome_elf/blacklist/bla...
chrome_elf/blacklist/blacklist.cc:166: // Adds the given dll name to the
blacklist.
drop the comment

https://codereview.chromium.org/107663008/diff/40001/chrome_elf/blacklist/bla...
chrome_elf/blacklist/blacklist.cc:184: // Removes the given dll name from the
blacklist.
ditto

https://codereview.chromium.org/107663008/diff/40001/chrome_elf/blacklist/bla...
chrome_elf/blacklist/blacklist.cc:223: // Tells the ServiceResolverThunks to
patch already patched functions.
tiny nit: tell the what? :). I think "resolver" is the critical word, I'm not
even sure why ResolverThunk is called that way instead of Resolver.

https://codereview.chromium.org/107663008/diff/40001/chrome_elf/blacklist/bla...
File chrome_elf/blacklist/blacklist.h (right):

https://codereview.chromium.org/107663008/diff/40001/chrome_elf/blacklist/bla...
chrome_elf/blacklist/blacklist.h:22: // Attempt to create a beacon in the
current user's registry hive.
nit: Attempts

https://codereview.chromium.org/107663008/diff/40001/chrome_elf/blacklist/bla...
chrome_elf/blacklist/blacklist.h:24: // beacon, return false. Otherwise return
true.
nit: returns

https://codereview.chromium.org/107663008/diff/40001/chrome_elf/blacklist/bla...
chrome_elf/blacklist/blacklist.h:44: // Initialize the DLL blacklist in the
current process. This should be called
nit: initializes

https://codereview.chromium.org/107663008/diff/40001/chrome_elf/blacklist/bla...
File chrome_elf/blacklist/blacklist_interceptions.cc (right):

https://codereview.chromium.org/107663008/diff/40001/chrome_elf/blacklist/bla...
chrome_elf/blacklist/blacklist_interceptions.cc:35:
GetNtDllExportByName("NtQuerySection"));
Isn't it better to have an explicit initialization function and avoid patching
if any of the needed functions cannot be found?

That would also follow the style guide... and having static constructors with
DllMain is kind of yucky.

https://codereview.chromium.org/107663008/diff/40001/chrome_elf/blacklist/bla...
chrome_elf/blacklist/blacklist_interceptions.cc:52: string16
GetBackingModuleFilePath(PVOID address) {
it would have been nice to use the functions from sb_nt_util instead of having
slightly different versions using stl. oh well.

https://codereview.chromium.org/107663008/diff/40001/chrome_elf/blacklist/bla...
chrome_elf/blacklist/blacklist_interceptions.cc:79: if (!NT_SUCCESS(ret)) {
nit: no {}

https://codereview.chromium.org/107663008/diff/40001/chrome_elf/blacklist/bla...
chrome_elf/blacklist/blacklist_interceptions.cc:140: EXCEPTION_EXECUTE_HANDLER :
EXCEPTION_CONTINUE_SEARCH) {
I'm curious about why letting the process crash here.

https://codereview.chromium.org/107663008/diff/40001/chrome_elf/blacklist/bla...
chrome_elf/blacklist/blacklist_interceptions.cc:149: base::win::PEImage
pe(module);
this whole thing has to be inside the try block, not just reading the name.

[commit-bot: I haz the power, 2013-12-17 22:25:21]

Retried try job too often on win_x64_rel for step(s) base_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...

[robertshield, 2013-12-18 02:34:43]

Thanks Ricardo, PTAL

https://codereview.chromium.org/107663008/diff/40001/chrome_elf/blacklist/bla...
File chrome_elf/blacklist/blacklist.cc (right):

https://codereview.chromium.org/107663008/diff/40001/chrome_elf/blacklist/bla...
chrome_elf/blacklist/blacklist.cc:42: VERSION_WIN8_1,      // Code named Windows
Blue
On 2013/12/17 22:09:27, rvargas wrote:
> nit: the code name seems pointless at this point

Done.

https://codereview.chromium.org/107663008/diff/40001/chrome_elf/blacklist/bla...
chrome_elf/blacklist/blacklist.cc:144: HKEY beacon_key = 0;
On 2013/12/17 22:09:27, rvargas wrote:
> nit: NULL

Done.

https://codereview.chromium.org/107663008/diff/40001/chrome_elf/blacklist/bla...
chrome_elf/blacklist/blacklist.cc:157: ::RegCloseKey(beacon_key);
On 2013/12/17 22:09:27, rvargas wrote:
> Only close the key on ERROR_SUCCESS

Done.

https://codereview.chromium.org/107663008/diff/40001/chrome_elf/blacklist/bla...
chrome_elf/blacklist/blacklist.cc:166: // Adds the given dll name to the
blacklist.
On 2013/12/17 22:09:27, rvargas wrote:
> drop the comment

Done.

https://codereview.chromium.org/107663008/diff/40001/chrome_elf/blacklist/bla...
chrome_elf/blacklist/blacklist.cc:184: // Removes the given dll name from the
blacklist.
On 2013/12/17 22:09:27, rvargas wrote:
> ditto

Done.

https://codereview.chromium.org/107663008/diff/40001/chrome_elf/blacklist/bla...
chrome_elf/blacklist/blacklist.cc:223: // Tells the ServiceResolverThunks to
patch already patched functions.
On 2013/12/17 22:09:27, rvargas wrote:
> tiny nit: tell the what? :). I think "resolver" is the critical word, I'm not
> even sure why ResolverThunk is called that way instead of Resolver.

Done.

https://codereview.chromium.org/107663008/diff/40001/chrome_elf/blacklist/bla...
File chrome_elf/blacklist/blacklist.h (right):

https://codereview.chromium.org/107663008/diff/40001/chrome_elf/blacklist/bla...
chrome_elf/blacklist/blacklist.h:22: // Attempt to create a beacon in the
current user's registry hive.
On 2013/12/17 22:09:27, rvargas wrote:
> nit: Attempts

Done.

https://codereview.chromium.org/107663008/diff/40001/chrome_elf/blacklist/bla...
chrome_elf/blacklist/blacklist.h:24: // beacon, return false. Otherwise return
true.
On 2013/12/17 22:09:27, rvargas wrote:
> nit: returns

Done.

https://codereview.chromium.org/107663008/diff/40001/chrome_elf/blacklist/bla...
chrome_elf/blacklist/blacklist.h:44: // Initialize the DLL blacklist in the
current process. This should be called
On 2013/12/17 22:09:27, rvargas wrote:
> nit: initializes

Done.

https://codereview.chromium.org/107663008/diff/40001/chrome_elf/blacklist/bla...
File chrome_elf/blacklist/blacklist_interceptions.cc (right):

https://codereview.chromium.org/107663008/diff/40001/chrome_elf/blacklist/bla...
chrome_elf/blacklist/blacklist_interceptions.cc:35:
GetNtDllExportByName("NtQuerySection"));
On 2013/12/17 22:09:27, rvargas wrote:
> Isn't it better to have an explicit initialization function and avoid patching
> if any of the needed functions cannot be found?
> 
> That would also follow the style guide... and having static constructors with
> DllMain is kind of yucky.

Done.

https://codereview.chromium.org/107663008/diff/40001/chrome_elf/blacklist/bla...
chrome_elf/blacklist/blacklist_interceptions.cc:52: string16
GetBackingModuleFilePath(PVOID address) {
On 2013/12/17 22:09:27, rvargas wrote:
> it would have been nice to use the functions from sb_nt_util instead of having
> slightly different versions using stl. oh well.

It could perhaps have been done, though iirc it might have needed some
additional refactoring of the sb_nt_util code. I'll leave a TODO and look into
it in a follow up CL.

https://codereview.chromium.org/107663008/diff/40001/chrome_elf/blacklist/bla...
chrome_elf/blacklist/blacklist_interceptions.cc:79: if (!NT_SUCCESS(ret)) {
On 2013/12/17 22:09:27, rvargas wrote:
> nit: no {}

Done.

https://codereview.chromium.org/107663008/diff/40001/chrome_elf/blacklist/bla...
chrome_elf/blacklist/blacklist_interceptions.cc:140: EXCEPTION_EXECUTE_HANDLER :
EXCEPTION_CONTINUE_SEARCH) {
On 2013/12/17 22:09:27, rvargas wrote:
> I'm curious about why letting the process crash here.

It's more about avoiding the general badness of not crashing if an exception
type that should not be handled occurs. 

The only exception type I'd expect from the wrapped code is an access violation
if I run out of the module into a non-readable location. If something else
occurs, I don't think I should be handling it here.

https://codereview.chromium.org/107663008/diff/40001/chrome_elf/blacklist/bla...
chrome_elf/blacklist/blacklist_interceptions.cc:149: base::win::PEImage
pe(module);
On 2013/12/17 22:09:27, rvargas wrote:
> this whole thing has to be inside the try block, not just reading the name.

Done.

[rvargas (doing something else), 2013-12-18 03:19:26]

lgtm

https://codereview.chromium.org/107663008/diff/60001/chrome_elf/blacklist/bla...
File chrome_elf/blacklist/blacklist_interceptions.cc (right):

https://codereview.chromium.org/107663008/diff/60001/chrome_elf/blacklist/bla...
chrome_elf/blacklist/blacklist_interceptions.cc:50: if
(!g_nt_query_virtual_memory_func)
should never happen

https://codereview.chromium.org/107663008/diff/60001/chrome_elf/blacklist/bla...
chrome_elf/blacklist/blacklist_interceptions.cc:95: if
(!g_nt_query_section_func)
should never happen

https://codereview.chromium.org/107663008/diff/60001/chrome_elf/blacklist/bla...
chrome_elf/blacklist/blacklist_interceptions.cc:220: if
(g_nt_unmap_view_of_section_func) {
should always be true now

[robertshield, 2013-12-18 04:51:16]

Thanks!

https://codereview.chromium.org/107663008/diff/60001/chrome_elf/blacklist/bla...
File chrome_elf/blacklist/blacklist_interceptions.cc (right):

https://codereview.chromium.org/107663008/diff/60001/chrome_elf/blacklist/bla...
chrome_elf/blacklist/blacklist_interceptions.cc:50: if
(!g_nt_query_virtual_memory_func)
On 2013/12/18 03:19:27, rvargas wrote:
> should never happen

Done.

https://codereview.chromium.org/107663008/diff/60001/chrome_elf/blacklist/bla...
chrome_elf/blacklist/blacklist_interceptions.cc:95: if
(!g_nt_query_section_func)
On 2013/12/18 03:19:27, rvargas wrote:
> should never happen

Done.

https://codereview.chromium.org/107663008/diff/60001/chrome_elf/blacklist/bla...
chrome_elf/blacklist/blacklist_interceptions.cc:220: if
(g_nt_unmap_view_of_section_func) {
On 2013/12/18 03:19:27, rvargas wrote:
> should always be true now

Done.

[commit-bot: I haz the power, 2013-12-18 04:58:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/robertshield@chromium.org/107663008/10...

[commit-bot: I haz the power, 2013-12-18 12:37:09]

Change committed as 241548

[Sigurður Ásgeirsson, 2013-12-18 14:16:00]

Post-hoc nit. Maybe const-cleanse when you touch this next?

https://codereview.chromium.org/107663008/diff/40001/chrome_elf/blacklist/bla...
File chrome_elf/blacklist/blacklist_interceptions.cc (right):

https://codereview.chromium.org/107663008/diff/40001/chrome_elf/blacklist/bla...
chrome_elf/blacklist/blacklist_interceptions.cc:134: char* image_name =
reinterpret_cast<char*>(pe.RVAToAddr(exports->Name));
nit: while you're in there, perhaps const char*

[robertshield, 2013-12-18 17:01:35]

Done.

https://codereview.chromium.org/107663008/diff/40001/chrome_elf/blacklist/bla...
File chrome_elf/blacklist/blacklist_interceptions.cc (right):

https://codereview.chromium.org/107663008/diff/40001/chrome_elf/blacklist/bla...
chrome_elf/blacklist/blacklist_interceptions.cc:134: char* image_name =
reinterpret_cast<char*>(pe.RVAToAddr(exports->Name));
On 2013/12/18 14:16:01, Sigurður Ásgeirsson wrote:
> nit: while you're in there, perhaps const char*

Done.