Adding blacklisted dlls to safe browsing incident reports.

Adding blacklisted dlls to safe browsing incident reports.

Also added a unit test to environment_data_collection_win_unittest.cc
Edited chrome_elf_init_win.cc and blacklist.cc so that the registry is
only cleared immediately before being repopulated (and safe browsing is not left
with an empty registry to read from).


BUG=

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

[krstnmnlsn, 2014-06-19 18:46:55]

grt - please see the safe browsing changes
mattm - could you owners review safe browsing
csharp - please check out the chrome elf changes

thanks all!

[grt (UTC plus 2), 2014-06-19 20:06:59]

looks pretty good. feedback below.

https://codereview.chromium.org/346763003/diff/110001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/environment_data_collection_win.cc (right):

https://codereview.chromium.org/346763003/diff/110001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/environment_data_collection_win.cc:95:
base::win::RegKey finch_blacklist_reigstry_key(
i think this function can be simplified to:
{
  PathSanitizer path_sanitizer;
  base::win::RegistryValueIterator iter(HKEY_CURRENT_USER,
                                        blacklist::kRegistryFinchListPath);
  for (; iter.Valid(); ++iter) {
    base::FilePath dll_name(iter.Value());
    path_sanitizer.StripHomeDirectory(&dll_name);
    process->add_blacklisted_dll(dll_name.AsUTF8Unsafe());
  }
}

https://codereview.chromium.org/346763003/diff/110001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/environment_data_collection_win.cc:120: 
nit: remove newline

https://codereview.chromium.org/346763003/diff/110001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/environment_data_collection_win_unittest.cc
(right):

https://codereview.chromium.org/346763003/diff/110001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/environment_data_collection_win_unittest.cc:142:
std::wstring process_report_dll =
use base::string16 rather than std::wstring to avoid presubmit errors

https://codereview.chromium.org/346763003/diff/110001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/environment_data_collection_win_unittest.cc:145: }
please add a test for the case where the registry holds a full path to a dll in
the user's home directory

https://codereview.chromium.org/346763003/diff/110001/chrome_elf/blacklist/bl...
File chrome_elf/blacklist/blacklist.cc (right):

https://codereview.chromium.org/346763003/diff/110001/chrome_elf/blacklist/bl...
chrome_elf/blacklist/blacklist.cc:397: return result == ERROR_SUCCESS;
i don't understand the return value from this function. rather than returning
true/false based on the success or failure of adding items to the blacklist, it
returns true/false based on whether or not the key could be closed. since the
return value is ignored anyway, how about making it a void function?

[krstnmnlsn, 2014-06-19 22:13:15]

https://codereview.chromium.org/346763003/diff/110001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/environment_data_collection_win.cc (right):

https://codereview.chromium.org/346763003/diff/110001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/environment_data_collection_win.cc:95:
base::win::RegKey finch_blacklist_reigstry_key(
On 2014/06/19 20:06:59, grt wrote:
> i think this function can be simplified to:
> {
>   PathSanitizer path_sanitizer;
>   base::win::RegistryValueIterator iter(HKEY_CURRENT_USER,
>                                         blacklist::kRegistryFinchListPath);
>   for (; iter.Valid(); ++iter) {
>     base::FilePath dll_name(iter.Value());
>     path_sanitizer.StripHomeDirectory(&dll_name);
>     process->add_blacklisted_dll(dll_name.AsUTF8Unsafe());
>   }
> }

wow, I didn't know that existed. that's much nicer.

https://codereview.chromium.org/346763003/diff/110001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/environment_data_collection_win.cc:120: 
On 2014/06/19 20:06:59, grt wrote:
> nit: remove newline

Done.

https://codereview.chromium.org/346763003/diff/110001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/environment_data_collection_win_unittest.cc
(right):

https://codereview.chromium.org/346763003/diff/110001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/environment_data_collection_win_unittest.cc:142:
std::wstring process_report_dll =
On 2014/06/19 20:06:59, grt wrote:
> use base::string16 rather than std::wstring to avoid presubmit errors

Done.

https://codereview.chromium.org/346763003/diff/110001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/environment_data_collection_win_unittest.cc:145: }
On 2014/06/19 20:06:59, grt wrote:
> please add a test for the case where the registry holds a full path to a dll
in
> the user's home directory

Done.

https://codereview.chromium.org/346763003/diff/110001/chrome_elf/blacklist/bl...
File chrome_elf/blacklist/blacklist.cc (right):

https://codereview.chromium.org/346763003/diff/110001/chrome_elf/blacklist/bl...
chrome_elf/blacklist/blacklist.cc:397: return result == ERROR_SUCCESS;
On 2014/06/19 20:06:59, grt wrote:
> i don't understand the return value from this function. rather than returning
> true/false based on the success or failure of adding items to the blacklist,
it
> returns true/false based on whether or not the key could be closed. since the
> return value is ignored anyway, how about making it a void function?

The return value is used by blacklist_test.cc. The way I've got the loop
condition I can't check for registry errors in that part (I think I saw this
format somewhere and copied it without thinking too much).  I could change the
condition to "result != ERROR_NO_MORE_ITEMS" and add a return false on the other
types failures inside?  Then at least it returns false on any failure   with the
registry.  I think for the test's purposes we'd still like to know if the key
failed to close right?  or no?

[mattm, 2014-06-19 22:46:31]

https://codereview.chromium.org/346763003/diff/150001/chrome/browser/chrome_e...
File chrome/browser/chrome_elf_init_win.cc (right):

https://codereview.chromium.org/346763003/diff/150001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_win.cc:119: HKEY_CURRENT_USER,
blacklist::kRegistryFinchListPath, KEY_SET_VALUE);
what happens with this registry entry when multiple instances of chrome are
running at once?

[grt (UTC plus 2), 2014-06-20 00:06:28]

https://codereview.chromium.org/346763003/diff/150001/chrome/browser/chrome_e...
File chrome/browser/chrome_elf_init_win.cc (right):

https://codereview.chromium.org/346763003/diff/150001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_win.cc:119: HKEY_CURRENT_USER,
blacklist::kRegistryFinchListPath, KEY_SET_VALUE);
On 2014/06/19 22:46:31, mattm wrote:
> what happens with this registry entry when multiple instances of chrome are
> running at once?

this is a good point. in time, we may have canary, stable, beta, and dev running
concurrently. at the moment, canary and one non-canary may be running.

[grt (UTC plus 2), 2014-06-20 01:09:21]

On 2014/06/20 00:06:28, grt wrote:
>
https://codereview.chromium.org/346763003/diff/150001/chrome/browser/chrome_e...
> File chrome/browser/chrome_elf_init_win.cc (right):
> 
>
https://codereview.chromium.org/346763003/diff/150001/chrome/browser/chrome_e...
> chrome/browser/chrome_elf_init_win.cc:119: HKEY_CURRENT_USER,
> blacklist::kRegistryFinchListPath, KEY_SET_VALUE);
> On 2014/06/19 22:46:31, mattm wrote:
> > what happens with this registry entry when multiple instances of chrome are
> > running at once?
> 
> this is a good point. in time, we may have canary, stable, beta, and dev
running
> concurrently. at the moment, canary and one non-canary may be running.

can the blacklist be stored in the ClientState key? this would handle these
variations properly. also, it looks like chromium and google chrome collide at
the moment. is this intended?

[grt (UTC plus 2), 2014-06-20 14:40:59]

https://codereview.chromium.org/346763003/diff/110001/chrome_elf/blacklist/bl...
File chrome_elf/blacklist/blacklist.cc (right):

https://codereview.chromium.org/346763003/diff/110001/chrome_elf/blacklist/bl...
chrome_elf/blacklist/blacklist.cc:397: return result == ERROR_SUCCESS;
On 2014/06/19 22:13:15, krstnmnlsn wrote:
> On 2014/06/19 20:06:59, grt wrote:
> > i don't understand the return value from this function. rather than
returning
> > true/false based on the success or failure of adding items to the blacklist,
> it
> > returns true/false based on whether or not the key could be closed. since
the
> > return value is ignored anyway, how about making it a void function?
> 
> The return value is used by blacklist_test.cc. The way I've got the loop
> condition I can't check for registry errors in that part (I think I saw this
> format somewhere and copied it without thinking too much).  I could change the
> condition to "result != ERROR_NO_MORE_ITEMS" and add a return false on the
other
> types failures inside?  Then at least it returns false on any failure   with
the
> registry.  I think for the test's purposes we'd still like to know if the key
> failed to close right?  or no?

since the return value isn't used by the production code, checking it in the
test is harmful since the test can potentially fail when the production code
would work fine (all values are read from the registry and used but false was
returned because the key couldn't be closed) or pass when the production code
would fail (all values couldn't be read but the key was closed).

if the call to CheckBlacklistedDllsNotLoaded in the test verifies that every
item in the registry is correctly blacklisted, then i think this can safely
become a void function and we'll have a more accurate test.

make sense?

[krstnmnlsn, 2014-06-20 18:02:58]

https://codereview.chromium.org/346763003/diff/110001/chrome_elf/blacklist/bl...
File chrome_elf/blacklist/blacklist.cc (right):

https://codereview.chromium.org/346763003/diff/110001/chrome_elf/blacklist/bl...
chrome_elf/blacklist/blacklist.cc:397: return result == ERROR_SUCCESS;
On 2014/06/20 14:40:59, grt wrote:
> On 2014/06/19 22:13:15, krstnmnlsn wrote:
> > On 2014/06/19 20:06:59, grt wrote:
> > > i don't understand the return value from this function. rather than
> returning
> > > true/false based on the success or failure of adding items to the
blacklist,
> > it
> > > returns true/false based on whether or not the key could be closed. since
> the
> > > return value is ignored anyway, how about making it a void function?
> > 
> > The return value is used by blacklist_test.cc. The way I've got the loop
> > condition I can't check for registry errors in that part (I think I saw this
> > format somewhere and copied it without thinking too much).  I could change
the
> > condition to "result != ERROR_NO_MORE_ITEMS" and add a return false on the
> other
> > types failures inside?  Then at least it returns false on any failure   with
> the
> > registry.  I think for the test's purposes we'd still like to know if the
key
> > failed to close right?  or no?
> 
> since the return value isn't used by the production code, checking it in the
> test is harmful since the test can potentially fail when the production code
> would work fine (all values are read from the registry and used but false was
> returned because the key couldn't be closed) or pass when the production code
> would fail (all values couldn't be read but the key was closed).
> 
> if the call to CheckBlacklistedDllsNotLoaded in the test verifies that every
> item in the registry is correctly blacklisted, then i think this can safely
> become a void function and we'll have a more accurate test.
> 
> make sense?

Okay I'll do that.

https://codereview.chromium.org/346763003/diff/150001/chrome/browser/chrome_e...
File chrome/browser/chrome_elf_init_win.cc (right):

https://codereview.chromium.org/346763003/diff/150001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_win.cc:119: HKEY_CURRENT_USER,
blacklist::kRegistryFinchListPath, KEY_SET_VALUE);
On 2014/06/20 00:06:27, grt wrote:
> On 2014/06/19 22:46:31, mattm wrote:
> > what happens with this registry entry when multiple instances of chrome are
> > running at once?
> 
> this is a good point. in time, we may have canary, stable, beta, and dev
running
> concurrently. at the moment, canary and one non-canary may be running.

To split up the registry values by channel running would require knowing the
channel very early in start up (in blacklist.cc) and that doesn't seem easy to
do (and may require code duplication).  So, for the moment at least, I will just
add a comment explaining the situation.  
I'll also make a separate CL fixing it so that chromium is not using the same
registry key.

[grt (UTC plus 2), 2014-06-20 18:19:50]

https://codereview.chromium.org/346763003/diff/180001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/environment_data_collection_win_unittest.cc
(right):

https://codereview.chromium.org/346763003/diff/180001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/environment_data_collection_win_unittest.cc:152:
safe_browsing::PathSanitizer path_sanitizer;
i think it's better to use the PathService directly to get the home directory so
that this test isn't coupled to the implementation of PathSanitizer.

https://codereview.chromium.org/346763003/diff/180001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/environment_data_collection_win_unittest.cc:155:
.AsUTF16Unsafe();
could use .value() here since this is a win-specific test

[krstnmnlsn, 2014-06-20 20:26:44]

https://codereview.chromium.org/346763003/diff/180001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/environment_data_collection_win_unittest.cc
(right):

https://codereview.chromium.org/346763003/diff/180001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/environment_data_collection_win_unittest.cc:152:
safe_browsing::PathSanitizer path_sanitizer;
On 2014/06/20 18:19:50, grt wrote:
> i think it's better to use the PathService directly to get the home directory
so
> that this test isn't coupled to the implementation of PathSanitizer.

Done.

https://codereview.chromium.org/346763003/diff/180001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/environment_data_collection_win_unittest.cc:155:
.AsUTF16Unsafe();
On 2014/06/20 18:19:50, grt wrote:
> could use .value() here since this is a win-specific test

Done.

[grt (UTC plus 2), 2014-06-20 20:43:58]

lgtm. please remove "patch from issue 323953002" from the CL description.

[mattm, 2014-06-20 21:05:54]

lgtm with nits

https://codereview.chromium.org/346763003/diff/200001/chrome/browser/chrome_e...
File chrome/browser/chrome_elf_init_win.cc (right):

https://codereview.chromium.org/346763003/diff/200001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_win.cc:114: // blacklist registry data in one
instance before the second has a change to
s/change/chance/

https://codereview.chromium.org/346763003/diff/200001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/environment_data_collection_win_unittest.cc
(right):

https://codereview.chromium.org/346763003/diff/200001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/environment_data_collection_win_unittest.cc:155:
PathService::Get(base::DIR_HOME, &path);
check return value

[krstnmnlsn, 2014-06-23 13:44:36]

https://codereview.chromium.org/346763003/diff/200001/chrome/browser/chrome_e...
File chrome/browser/chrome_elf_init_win.cc (right):

https://codereview.chromium.org/346763003/diff/200001/chrome/browser/chrome_e...
chrome/browser/chrome_elf_init_win.cc:114: // blacklist registry data in one
instance before the second has a change to
On 2014/06/20 21:05:54, mattm wrote:
> s/change/chance/

Done.

https://codereview.chromium.org/346763003/diff/200001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/environment_data_collection_win_unittest.cc
(right):

https://codereview.chromium.org/346763003/diff/200001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/environment_data_collection_win_unittest.cc:155:
PathService::Get(base::DIR_HOME, &path);
On 2014/06/20 21:05:54, mattm wrote:
> check return value

Done.

[csharp, 2014-06-23 17:14:34]

lgtm

https://codereview.chromium.org/346763003/diff/240001/chrome_elf/blacklist/bl...
File chrome_elf/blacklist/blacklist.cc (right):

https://codereview.chromium.org/346763003/diff/240001/chrome_elf/blacklist/bl...
chrome_elf/blacklist/blacklist.cc:407: name_len = name_len + 1;
Nit: add a blank line above

[krstnmnlsn, 2014-06-27 15:33:27]

Rebased onto Patrick's re-committed change.

https://codereview.chromium.org/346763003/diff/240001/chrome_elf/blacklist/bl...
File chrome_elf/blacklist/blacklist.cc (right):

https://codereview.chromium.org/346763003/diff/240001/chrome_elf/blacklist/bl...
chrome_elf/blacklist/blacklist.cc:407: name_len = name_len + 1;
On 2014/06/23 17:14:34, csharp wrote:
> Nit: add a blank line above

Done.

[krstnmnlsn, 2014-06-27 17:21:51]

The CQ bit was checked by krstnmnlsn@chromium.org

[commit-bot: I haz the power, 2014-06-27 17:24:11]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/krstnmnlsn@chromium.org/346763003/310001

[commit-bot: I haz the power, 2014-06-27 18:11:48]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-06-27 19:27:12]

Change committed as 280380