Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
406043003:
Adding the VerifyModule function (and helpers) to safe browsing. (Closed)
|
Created:
6 years, 5 months ago by krstnmnlsn Modified:
6 years, 4 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionAdding the VerifyModule function (and helpers) to safe browsing.
This function allows us to compare a module in memory with
its original on disk, and identify any differences (after
accounting for relocations). The state of the module
(MODULE_UNKNOWN, MODULE_UNMODIFIED or MODULE_MODIFIED) is
returned along with a list of the possibly modified exported
functions of the module.
BUG=401854
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288319
Patch Set 1 : #
Total comments: 26
Patch Set 2 : #
Total comments: 14
Patch Set 3 : Responding to csharp's comments. See patch set 2 for additional files that have been changed, but not in patch 3. #Patch Set 4 : fixed branch issues! #
Total comments: 26
Patch Set 5 : greg's fixes #
Total comments: 18
Patch Set 6 : chris' fixes #
Total comments: 4
Patch Set 7 : #
Total comments: 40
Patch Set 8 : Copying over changes so that modified exported functions are returned by VerifyModule #
Total comments: 40
Patch Set 9 : #
Total comments: 8
Patch Set 10 : #Patch Set 11 : rebasing #Messages
Total messages: 36 (0 generated)
Hey all, Here is the first piece of code to start verifying modules loaded into chrome. @robert: so.. there are pointers everywhere. But I don't own any of them, so I don't think scoped_ptrs would be appropriate here? @grt: mattm seems to be OOO until the 28th? Do you know who else I could bother for safe_browsing owners review? @anyone: EnumRelocsCallback recomputes the memory and disk code section addresses on each iteration, would it be better to store those 'globally' in the anonymous namespace?
https://codereview.chromium.org/406043003/diff/40001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/verifier.h (right): https://codereview.chromium.org/406043003/diff/40001/chrome/browser/safe_brow... chrome/browser/safe_browsing/verifier.h:12: MODULE_STATE_UNKNOWN = 0, nit: no need for " = 0" here. https://codereview.chromium.org/406043003/diff/40001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/406043003/diff/40001/chrome/chrome_browser.gy... chrome/chrome_browser.gypi:2690: 'browser/safe_browsing/verifier.cc', "verifier" is very generic. how about module_integrity_verifier?
some comments. let's chat about using PeImageReader instead of PEImageAsData. https://codereview.chromium.org/406043003/diff/40001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/verifier.cc (right): https://codereview.chromium.org/406043003/diff/40001/chrome/browser/safe_brow... chrome/browser/safe_browsing/verifier.cc:30: bool GetCodeAddrsAndSize(const base::win::PEImage mem_peimage, const base::win::PEImage& https://codereview.chromium.org/406043003/diff/40001/chrome/browser/safe_brow... chrome/browser/safe_browsing/verifier.cc:32: BYTE*& mem_code_addr, chromium passes out params by pointer rather than by non-const ref https://codereview.chromium.org/406043003/diff/40001/chrome/browser/safe_brow... chrome/browser/safe_browsing/verifier.cc:129: ModuleState VerifyModule(wchar_t* module_name) { const wchar_t* https://codereview.chromium.org/406043003/diff/40001/chrome/browser/safe_brow... chrome/browser/safe_browsing/verifier.cc:130: HMODULE module_handle = GetModuleHandle(module_name); the module could be unloaded immediately after this call, leading to a crash or other undesired behavior below. do something like this instead: HMODULE module_handle = NULL; if (!GetModuleHandleEx(0, module_name, &module_handle)) return MODULE_STATE_UNKNOWN; base::ScopedNativeLibrary native_library(module_handle); so that a reference is held on the module for the lifetime of this function. https://codereview.chromium.org/406043003/diff/40001/chrome/browser/safe_brow... chrome/browser/safe_browsing/verifier.cc:134: WCHAR module_path[MAX_PATH] = {0}; nit: {0} -> {} https://codereview.chromium.org/406043003/diff/40001/chrome/browser/safe_brow... chrome/browser/safe_browsing/verifier.cc:135: if (GetModuleFileNameEx( msdn says: "To retrieve the name of a module in the current process, use the GetModuleFileName function. This is more efficient and more reliable than calling GetModuleFileNameEx with a handle to the current process." https://codereview.chromium.org/406043003/diff/40001/chrome/browser/safe_brow... chrome/browser/safe_browsing/verifier.cc:135: if (GetModuleFileNameEx( check the return value. if the buffer is too small, arraysize(module_path) is returned and the string may not be terminated, leading to an assortment of bad stuff below. depending on how much you care about handling long paths, you could either return MODULE_STATE_UNKNOWN if arraysize(module_path) is returned, or switch from using a stack-based array to a heap-based string. for example: base::string16 module_path; DWORD buffer_length = MAX_PATH; do { DWORD length = GetModuleFileName(module_handle, WriteInto(&module_path, buffer_length), buffer_length); if (length < buffer_length) { module_path.resize(length); break; } if (buffer_length & 0x80000000) return MODULE_STATE_UNKNOWN; buffer_length *= 2; } while(true); https://codereview.chromium.org/406043003/diff/40001/chrome/browser/safe_brow... chrome/browser/safe_browsing/verifier.cc:136: GetCurrentProcess(), module_handle, module_path, MAX_PATH) == 0) MAX_PATH -> arraysize(module_path) https://codereview.chromium.org/406043003/diff/40001/chrome/browser/safe_brow... chrome/browser/safe_browsing/verifier.cc:141: base::win::PEImageAsData disk_peimage( it would be safer to use safe_browsing::PeImageReader rather than PEImageAsData. i think the former is more careful when it comes to checking offsets. PeImageReader may not currently have all of the features you need, but it shouldn't be too difficult to extend it. https://codereview.chromium.org/406043003/diff/40001/chrome/browser/safe_brow... chrome/browser/safe_browsing/verifier.cc:163: if (num_bytes_different - bytes_corrected_by_reloc == 0) ?num_bytes_different == bytes_corrected_by_reloc? https://codereview.chromium.org/406043003/diff/40001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/verifier.h (right): https://codereview.chromium.org/406043003/diff/40001/chrome/browser/safe_brow... chrome/browser/safe_browsing/verifier.h:19: ModuleState VerifyModule(); ModuleState VerifyModule(const wchar_t* module);
Feedback much appreciated! In thanks, I have doubled the code length (unittests, sorry). The unittests are currently a rough draft, I had a couple of questions: 1) In order to build my test dll I added a gyp file (chrome/browser/safe_browsing/verifier_test/verifier_unittest.gyp) and made it a dependency of the unit_tests. Is that a reasonable way of doing things? Should I instead add this to some other gyp somewhere? 2) Everything I'm doing is windows specific. Do we append _win to the name in that case, or only when there is also a non-win version? Also, I have added my things in the gyp file adjacent to other windows specific stuff I found in safebrowsing in the hope that it'll be compiled at the right time (and on the right bots). I'd appreciate a check by anyone who actually knows what they're doing there. 3) Tests do not have very much coverage. I could expose a few more helpers and test them (such as AddrIsInCodeSection and CountBytesDiffInMemory) but I don't see any convenient way to test the EnumRelocsCallback or GetCodeAddrsAndSize separately from their caller VerifyModule. 4) This is not a question but I'm still working on VerifyModuleModified. Please ignore it. https://codereview.chromium.org/406043003/diff/40001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/verifier.cc (right): https://codereview.chromium.org/406043003/diff/40001/chrome/browser/safe_brow... chrome/browser/safe_browsing/verifier.cc:30: bool GetCodeAddrsAndSize(const base::win::PEImage mem_peimage, On 2014/07/22 17:39:49, grt wrote: > const base::win::PEImage& Done. https://codereview.chromium.org/406043003/diff/40001/chrome/browser/safe_brow... chrome/browser/safe_browsing/verifier.cc:32: BYTE*& mem_code_addr, On 2014/07/22 17:39:50, grt wrote: > chromium passes out params by pointer rather than by non-const ref done. The page on smartpointers is saying to use a raw pointers or WeakPtr when you don't own the obj/value. Is there a preference? https://codereview.chromium.org/406043003/diff/40001/chrome/browser/safe_brow... chrome/browser/safe_browsing/verifier.cc:129: ModuleState VerifyModule(wchar_t* module_name) { On 2014/07/22 17:39:49, grt wrote: > const wchar_t* Done. https://codereview.chromium.org/406043003/diff/40001/chrome/browser/safe_brow... chrome/browser/safe_browsing/verifier.cc:130: HMODULE module_handle = GetModuleHandle(module_name); On 2014/07/22 17:39:49, grt wrote: > the module could be unloaded immediately after this call, leading to a crash or > other undesired behavior below. do something like this instead: > > HMODULE module_handle = NULL; > if (!GetModuleHandleEx(0, module_name, &module_handle)) > return MODULE_STATE_UNKNOWN; > base::ScopedNativeLibrary native_library(module_handle); > > so that a reference is held on the module for the lifetime of this function. Cool! I was wondering about this, and thought (/was hoping) maybe the modules we are interacting with would be important enough (chrome, ntdll.dll) that we'd be in trouble if they unloaded anyways. But that might not always be the case. https://codereview.chromium.org/406043003/diff/40001/chrome/browser/safe_brow... chrome/browser/safe_browsing/verifier.cc:134: WCHAR module_path[MAX_PATH] = {0}; On 2014/07/22 17:39:50, grt wrote: > nit: {0} -> {} Done. https://codereview.chromium.org/406043003/diff/40001/chrome/browser/safe_brow... chrome/browser/safe_browsing/verifier.cc:135: if (GetModuleFileNameEx( On 2014/07/22 17:39:49, grt wrote: > check the return value. if the buffer is too small, arraysize(module_path) is > returned and the string may not be terminated, leading to an assortment of bad > stuff below. depending on how much you care about handling long paths, you could > either return MODULE_STATE_UNKNOWN if arraysize(module_path) is returned, or > switch from using a stack-based array to a heap-based string. for example: > > base::string16 module_path; > DWORD buffer_length = MAX_PATH; > do { > DWORD length = GetModuleFileName(module_handle, > WriteInto(&module_path, buffer_length), > buffer_length); > if (length < buffer_length) { > module_path.resize(length); > break; > } > if (buffer_length & 0x80000000) > return MODULE_STATE_UNKNOWN; > buffer_length *= 2; > } while(true); Resolution: trusting in all reasonable cases MAX_PATH will be sufficient, giving up and returning MODULE_STATE_UNKNOWN on failure (ie. if arraysize(module_path) is returned). https://codereview.chromium.org/406043003/diff/40001/chrome/browser/safe_brow... chrome/browser/safe_browsing/verifier.cc:135: if (GetModuleFileNameEx( On 2014/07/22 17:39:50, grt wrote: > msdn says: "To retrieve the name of a module in the current process, use the > GetModuleFileName function. This is more efficient and more reliable than > calling GetModuleFileNameEx with a handle to the current process." Done. https://codereview.chromium.org/406043003/diff/40001/chrome/browser/safe_brow... chrome/browser/safe_browsing/verifier.cc:136: GetCurrentProcess(), module_handle, module_path, MAX_PATH) == 0) On 2014/07/22 17:39:49, grt wrote: > MAX_PATH -> arraysize(module_path) right, thanks. https://codereview.chromium.org/406043003/diff/40001/chrome/browser/safe_brow... chrome/browser/safe_browsing/verifier.cc:141: base::win::PEImageAsData disk_peimage( On 2014/07/22 17:39:49, grt wrote: > it would be safer to use safe_browsing::PeImageReader rather than PEImageAsData. > i think the former is more careful when it comes to checking offsets. > PeImageReader may not currently have all of the features you need, but it > shouldn't be too difficult to extend it. Continuing to use PEImageAsData for now, I could add a comment about the wish to add relocs to PeImageReader/merge the two peimage parsers? https://codereview.chromium.org/406043003/diff/40001/chrome/browser/safe_brow... chrome/browser/safe_browsing/verifier.cc:163: if (num_bytes_different - bytes_corrected_by_reloc == 0) On 2014/07/22 17:39:49, grt wrote: > ?num_bytes_different == bytes_corrected_by_reloc? Done. :P https://codereview.chromium.org/406043003/diff/40001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/verifier.h (right): https://codereview.chromium.org/406043003/diff/40001/chrome/browser/safe_brow... chrome/browser/safe_browsing/verifier.h:12: MODULE_STATE_UNKNOWN = 0, On 2014/07/21 20:41:59, grt wrote: > nit: no need for " = 0" here. Yea, I wasn't sure what the convention was (Chromium code/the style guide seem to use both). No zero it is. https://codereview.chromium.org/406043003/diff/40001/chrome/browser/safe_brow... chrome/browser/safe_browsing/verifier.h:19: ModuleState VerifyModule(); On 2014/07/22 17:39:50, grt wrote: > ModuleState VerifyModule(const wchar_t* module); Yikes/thanks! (As you can tell, the version I was actually running/testing with was not identical to the uploaded stuff, sorry for the resulting silliness...) https://codereview.chromium.org/406043003/diff/40001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/406043003/diff/40001/chrome/chrome_browser.gy... chrome/chrome_browser.gypi:2690: 'browser/safe_browsing/verifier.cc', On 2014/07/21 20:41:59, grt wrote: > "verifier" is very generic. how about module_integrity_verifier? Okay, switched. My tests still use plain verifier because otherwise the paths to them (in chrome_tests_unit) is ~160 chars. If you can think of a more informative short name I'd be happy to switch those too.
VerifyModuleModified fixed (lets pretend this never happened). See previous message for a few questions I had.
https://codereview.chromium.org/406043003/diff/180001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/module_integrity_verifier.cc (right): https://codereview.chromium.org/406043003/diff/180001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.cc:21: int bytes_corrected_by_reloc = 0; This seems to be only used in VerifyModule, so it should probably be a local variable there. https://codereview.chromium.org/406043003/diff/180001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.cc:25: bool unknown_reloc_type = false; This probably should also be a local variable in VerifyModule that is passed to EnumRelocsCallbacks https://codereview.chromium.org/406043003/diff/180001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/verifier_test/verifier_unittest.cc (right): https://codereview.chromium.org/406043003/diff/180001/chrome/browser/safe_bro... chrome/browser/safe_browsing/verifier_test/verifier_unittest.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. This file should probably live a level higher and be called module_integrity_verifier_unittest.cc to make it easier for people to find when looking at the source file (I'd still keep the rest of the files here though) https://codereview.chromium.org/406043003/diff/180001/chrome/browser/safe_bro... chrome/browser/safe_browsing/verifier_test/verifier_unittest.cc:27: base::ScopedNativeLibrary mem_dll_handle; All class data member names need to end with a trailing _ https://codereview.chromium.org/406043003/diff/180001/chrome/browser/safe_bro... chrome/browser/safe_browsing/verifier_test/verifier_unittest.cc:41: ASSERT_TRUE(disk_peimage_ptr->VerifyMagic()); Move up a line to check right after creation https://codereview.chromium.org/406043003/diff/180001/chrome/browser/safe_bro... chrome/browser/safe_browsing/verifier_test/verifier_unittest.cc:78: // Construct test pointer and try with CountBytesDiffInPtr. nit: pointer -> pointers https://codereview.chromium.org/406043003/diff/180001/chrome/browser/safe_bro... chrome/browser/safe_browsing/verifier_test/verifier_unittest.cc:98: EXPECT_EQ(2, CountBytesDiffInPtr(num_a, num_b)); also check "EXPECT_EQ(2, CountBytesDiffInPtr(num_b, num_a));" (just as a basic sanity check)
@mattm, could you owners review the changes to safe browsing? https://codereview.chromium.org/406043003/diff/180001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/module_integrity_verifier.cc (right): https://codereview.chromium.org/406043003/diff/180001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.cc:21: int bytes_corrected_by_reloc = 0; On 2014/07/29 14:28:15, csharp wrote: > This seems to be only used in VerifyModule, so it should probably be a local > variable there. It's used in the EnumRelocsCallback (hidden in some collapsed lines below). I actually debated squishing it into the cookie with the disk_peimage, but that seemed excessively ugly. https://codereview.chromium.org/406043003/diff/180001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.cc:25: bool unknown_reloc_type = false; On 2014/07/29 14:28:15, csharp wrote: > This probably should also be a local variable in VerifyModule that is passed to > EnumRelocsCallbacks Do you think it would be better to make some sort of struct and hand all these things through the cookie (the cookie is the only way to pass inputs to EnumRelocsCallbacks)? https://codereview.chromium.org/406043003/diff/180001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/verifier_test/verifier_unittest.cc (right): https://codereview.chromium.org/406043003/diff/180001/chrome/browser/safe_bro... chrome/browser/safe_browsing/verifier_test/verifier_unittest.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2014/07/29 14:28:15, csharp wrote: > This file should probably live a level higher and be called > module_integrity_verifier_unittest.cc to make it easier for people to find when > looking at the source file (I'd still keep the rest of the files here though) Done. https://codereview.chromium.org/406043003/diff/180001/chrome/browser/safe_bro... chrome/browser/safe_browsing/verifier_test/verifier_unittest.cc:27: base::ScopedNativeLibrary mem_dll_handle; On 2014/07/29 14:28:15, csharp wrote: > All class data member names need to end with a trailing _ Right! https://codereview.chromium.org/406043003/diff/180001/chrome/browser/safe_bro... chrome/browser/safe_browsing/verifier_test/verifier_unittest.cc:41: ASSERT_TRUE(disk_peimage_ptr->VerifyMagic()); On 2014/07/29 14:28:15, csharp wrote: > Move up a line to check right after creation Done. https://codereview.chromium.org/406043003/diff/180001/chrome/browser/safe_bro... chrome/browser/safe_browsing/verifier_test/verifier_unittest.cc:78: // Construct test pointer and try with CountBytesDiffInPtr. On 2014/07/29 14:28:15, csharp wrote: > nit: pointer -> pointers Done. https://codereview.chromium.org/406043003/diff/180001/chrome/browser/safe_bro... chrome/browser/safe_browsing/verifier_test/verifier_unittest.cc:98: EXPECT_EQ(2, CountBytesDiffInPtr(num_a, num_b)); On 2014/07/29 14:28:15, csharp wrote: > also check "EXPECT_EQ(2, CountBytesDiffInPtr(num_b, num_a));" (just as a basic > sanity check) Good idea.
I think something strange may have happened with your git branches. The latest patchset doesn't seem correct. Ping me if you'd like help sorting things out.
now with all files in the latest patch.
a few suggestions. more to come. https://codereview.chromium.org/406043003/diff/220001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/module_integrity_verifier.cc (right): https://codereview.chromium.org/406043003/diff/220001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.cc:21: int bytes_corrected_by_reloc = 0; instead of using global state (which makes concurrent verifications impossible), please define a struct for this data and create it on the stack in VerifyModule and pass a pointer to it via |cookie|. for example: struct RelocEnumerationState { explicit RelocEnumerationState(HMODULE hModule); ~RelocEnumerationState(); base::win::PEImageAsData disk_peimage; int bytes_corrected_by_reloc; bool unknown_reloc_type; DISALLOW_COPY_AND_ASSIGN(RelocEnumerationState); }; RelocEnumerationState::RelocEnumerationState(HMODULE hModule) : disk_peimage(hModule), bytes_corrected_by_reloc(), unknown_reloc_type() { } RelocEnumerationState::~RelocEnumerationState() { } and then instantiate this on line 147 and pass a pointer to it through the call to EnumRelocs. https://codereview.chromium.org/406043003/diff/220001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.cc:109: // Section size will be the same as in the loaded module. is this necessarily true? VirtualSize (size of the section when mapped into memory) may be greater than or less than SizeOfRawData. in the former case (VirtualSize is bigger), the section is zero padded when mapped. in the latter case (VirtualSize is smaller), SizeOfRawData has been rounded up to the nearest OptionalHeader.FileAlignment boundary. https://codereview.chromium.org/406043003/diff/220001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.cc:142: if (length == arraysize(module_path)) this function returns 0 on failure, so: if (!length || length == arraysize(module_path)) https://codereview.chromium.org/406043003/diff/220001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.cc:146: mapped_module.Initialize(base::FilePath(module_path)); returns false if the file cannot be mapped https://codereview.chromium.org/406043003/diff/220001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/module_integrity_verifier.h (right): https://codereview.chromium.org/406043003/diff/220001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.h:8: #include <windows.h> both of these are no longer needed if you switch away from using MS types (see suggestions below) https://codereview.chromium.org/406043003/diff/220001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.h:11: #include "base/files/file_path.h" remove https://codereview.chromium.org/406043003/diff/220001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.h:12: #include "base/files/memory_mapped_file.h" remove https://codereview.chromium.org/406043003/diff/220001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.h:13: #include "base/scoped_native_library.h" remove https://codereview.chromium.org/406043003/diff/220001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.h:14: #include "base/win/pe_image.h" forward-declare PEImage and PEImageAsData rather than including the header here https://codereview.chromium.org/406043003/diff/220001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.h:30: BYTE** mem_code_addr, is there any reason not to use const uint8_t* in place of BYTE* everywhere? https://codereview.chromium.org/406043003/diff/220001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.h:32: SIZE_T* code_size); uint32_t (VirtualSize is always a 32-bit unsigned value) https://codereview.chromium.org/406043003/diff/220001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.h:35: int CountBytesDiffInPtr(intptr_t num_a, intptr_t num_b); uintptr_t
Some fixes, thanks! https://codereview.chromium.org/406043003/diff/220001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/module_integrity_verifier.cc (right): https://codereview.chromium.org/406043003/diff/220001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.cc:21: int bytes_corrected_by_reloc = 0; On 2014/07/29 19:32:22, grt wrote: > instead of using global state (which makes concurrent verifications impossible), > please define a struct for this data and create it on the stack in VerifyModule > and pass a pointer to it via |cookie|. for example: > > struct RelocEnumerationState { > explicit RelocEnumerationState(HMODULE hModule); > ~RelocEnumerationState(); > > base::win::PEImageAsData disk_peimage; > int bytes_corrected_by_reloc; > bool unknown_reloc_type; > > DISALLOW_COPY_AND_ASSIGN(RelocEnumerationState); > }; > > RelocEnumerationState::RelocEnumerationState(HMODULE hModule) > : disk_peimage(hModule), > bytes_corrected_by_reloc(), > unknown_reloc_type() { > } > > RelocEnumerationState::~RelocEnumerationState() { > } > > and then instantiate this on line 147 and pass a pointer to it through the call > to EnumRelocs. Okay yes, I was asking earlier if I should do this. Done. https://codereview.chromium.org/406043003/diff/220001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.cc:109: // Section size will be the same as in the loaded module. On 2014/07/29 19:32:22, grt wrote: > is this necessarily true? VirtualSize (size of the section when mapped into > memory) may be greater than or less than SizeOfRawData. in the former case > (VirtualSize is bigger), the section is zero padded when mapped. in the latter > case (VirtualSize is smaller), SizeOfRawData has been rounded up to the nearest > OptionalHeader.FileAlignment boundary. Ah, I misunderstood that. It should be safe/sufficient to take the minimum of the two then yes? https://codereview.chromium.org/406043003/diff/220001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.cc:142: if (length == arraysize(module_path)) On 2014/07/29 19:32:22, grt wrote: > this function returns 0 on failure, so: > if (!length || length == arraysize(module_path)) Done. https://codereview.chromium.org/406043003/diff/220001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.cc:146: mapped_module.Initialize(base::FilePath(module_path)); On 2014/07/29 19:32:22, grt wrote: > returns false if the file cannot be mapped Done. https://codereview.chromium.org/406043003/diff/220001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/module_integrity_verifier.h (right): https://codereview.chromium.org/406043003/diff/220001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.h:8: #include <windows.h> On 2014/07/29 19:32:23, grt wrote: > both of these are no longer needed if you switch away from using MS types (see > suggestions below) Great! https://codereview.chromium.org/406043003/diff/220001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.h:11: #include "base/files/file_path.h" On 2014/07/29 19:32:23, grt wrote: > remove Done. https://codereview.chromium.org/406043003/diff/220001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.h:12: #include "base/files/memory_mapped_file.h" On 2014/07/29 19:32:23, grt wrote: > remove Done. https://codereview.chromium.org/406043003/diff/220001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.h:13: #include "base/scoped_native_library.h" On 2014/07/29 19:32:23, grt wrote: > remove Had meant to remove these! https://codereview.chromium.org/406043003/diff/220001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.h:14: #include "base/win/pe_image.h" On 2014/07/29 19:32:23, grt wrote: > forward-declare PEImage and PEImageAsData rather than including the header here Done. https://codereview.chromium.org/406043003/diff/220001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.h:30: BYTE** mem_code_addr, On 2014/07/29 19:32:22, grt wrote: > is there any reason not to use const uint8_t* in place of BYTE* everywhere? Nope, done. https://codereview.chromium.org/406043003/diff/220001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.h:32: SIZE_T* code_size); On 2014/07/29 19:32:23, grt wrote: > uint32_t (VirtualSize is always a 32-bit unsigned value) Done. https://codereview.chromium.org/406043003/diff/220001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.h:35: int CountBytesDiffInPtr(intptr_t num_a, intptr_t num_b); On 2014/07/29 19:32:23, grt wrote: > uintptr_t Done.
https://codereview.chromium.org/406043003/diff/240001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/module_integrity_verifier.cc (right): https://codereview.chromium.org/406043003/diff/240001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.cc:21: // The number of bytes made to agree (between memory and disk) when the nit: I'd add a blank line above this comment https://codereview.chromium.org/406043003/diff/240001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.cc:24: // Set true if the relocation table contains a reloc of type that we don't nit: I'd add a blank line above this comment https://codereview.chromium.org/406043003/diff/240001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.cc:63: base::win::PEImageAsData disk_peimage = reloc_enum_state->disk_peimage; Probably worth making this a constant pointer (const base::win::PEImageAsData*) to avoid always making a copy here (unless disk_peimage is suppose to be a copy) https://codereview.chromium.org/406043003/diff/240001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.cc:84: uint8_t* new_value = (*reinterpret_cast<uint8_t**>(address)) + delta; new_value->new_address (maybe?) https://codereview.chromium.org/406043003/diff/240001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.cc:89: // Check that the adding delta did correct the value to agree with disk. nit: "that the adding delta did correct" -> "that adding the delta corrected" https://codereview.chromium.org/406043003/diff/240001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.cc:98: break; Could you add a brief comment explaining why we can just break in this case? https://codereview.chromium.org/406043003/diff/240001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.cc:182: int num_bytes_different = Can this be moved to line 187? (I don't need we need it until then) https://codereview.chromium.org/406043003/diff/240001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/module_integrity_verifier.h (right): https://codereview.chromium.org/406043003/diff/240001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.h:10: namespace base { namespace win { Each namespace needs to go on its own line
https://codereview.chromium.org/406043003/diff/240001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/module_integrity_verifier.cc (right): https://codereview.chromium.org/406043003/diff/240001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.cc:21: // The number of bytes made to agree (between memory and disk) when the On 2014/07/30 15:22:08, csharp wrote: > nit: I'd add a blank line above this comment Done. https://codereview.chromium.org/406043003/diff/240001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.cc:24: // Set true if the relocation table contains a reloc of type that we don't On 2014/07/30 15:22:08, csharp wrote: > nit: I'd add a blank line above this comment Done. https://codereview.chromium.org/406043003/diff/240001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.cc:63: base::win::PEImageAsData disk_peimage = reloc_enum_state->disk_peimage; On 2014/07/30 15:22:08, csharp wrote: > Probably worth making this a constant pointer (const base::win::PEImageAsData*) > to avoid always making a copy here (unless disk_peimage is suppose to be a copy) Done. https://codereview.chromium.org/406043003/diff/240001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.cc:84: uint8_t* new_value = (*reinterpret_cast<uint8_t**>(address)) + delta; On 2014/07/30 15:22:08, csharp wrote: > new_value->new_address (maybe?) Yeah so I was debating this. I worried it would be confusing to call it something like new_address, because it's not actually replacing address (we're modifying the value address points to, which technically happens to be another address...). https://codereview.chromium.org/406043003/diff/240001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.cc:89: // Check that the adding delta did correct the value to agree with disk. On 2014/07/30 15:22:08, csharp wrote: > nit: "that the adding delta did correct" -> "that adding the delta corrected" Done. https://codereview.chromium.org/406043003/diff/240001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.cc:98: break; On 2014/07/30 15:22:08, csharp wrote: > Could you add a brief comment explaining why we can just break in this case? I meant to, thanks! https://codereview.chromium.org/406043003/diff/240001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.cc:182: int num_bytes_different = On 2014/07/30 15:22:08, csharp wrote: > Can this be moved to line 187? (I don't need we need it until then) Sure. https://codereview.chromium.org/406043003/diff/240001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/module_integrity_verifier.h (right): https://codereview.chromium.org/406043003/diff/240001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.h:10: namespace base { namespace win { On 2014/07/30 15:22:08, csharp wrote: > Each namespace needs to go on its own line Okay I'll switch. I was following a couple of examples I found in chromium since I didn't see anything specific in the style guide.
lgtm with minor comments. Greg is a lot more familiar with Windows code than me, so please wait for his LGTM as well. https://codereview.chromium.org/406043003/diff/240001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/module_integrity_verifier.h (right): https://codereview.chromium.org/406043003/diff/240001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.h:10: namespace base { namespace win { On 2014/07/30 18:36:07, krstnmnlsn wrote: > On 2014/07/30 15:22:08, csharp wrote: > > Each namespace needs to go on its own line > > Okay I'll switch. I was following a couple of examples I found in chromium since > I didn't see anything specific in the style guide. It is in the Google C++ guide (http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Namespace_Form...) https://codereview.chromium.org/406043003/diff/260001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/module_integrity_verifier.cc (right): https://codereview.chromium.org/406043003/diff/260001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.cc:22: // The number of bytes made to agree (between memory and disk) when the This comment confused me a bit, maybe just say "The number of bytes made equivalent between memory and disk due to relocations." https://codereview.chromium.org/406043003/diff/260001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.cc:81: return true; Does it make sense to pull this check out above the switch? (would the other relocs also only care if the change was in code)
https://codereview.chromium.org/406043003/diff/240001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/module_integrity_verifier.h (right): https://codereview.chromium.org/406043003/diff/240001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.h:10: namespace base { namespace win { On 2014/07/30 19:29:42, csharp wrote: > On 2014/07/30 18:36:07, krstnmnlsn wrote: > > On 2014/07/30 15:22:08, csharp wrote: > > > Each namespace needs to go on its own line > > > > Okay I'll switch. I was following a couple of examples I found in chromium > since > > I didn't see anything specific in the style guide. > > It is in the Google C++ guide > (http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Namespace_Form...) Yeah I saw that, I just meant specific to namespaces+forwarding. It seemed like a plausible special case at the time... https://codereview.chromium.org/406043003/diff/260001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/module_integrity_verifier.cc (right): https://codereview.chromium.org/406043003/diff/260001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.cc:22: // The number of bytes made to agree (between memory and disk) when the On 2014/07/30 19:29:42, csharp wrote: > This comment confused me a bit, maybe just say "The number of bytes made > equivalent between memory and disk due to relocations." Done. https://codereview.chromium.org/406043003/diff/260001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.cc:81: return true; On 2014/07/30 19:29:42, csharp wrote: > Does it make sense to pull this check out above the switch? (would the other > relocs also only care if the change was in code) They would care yes. Thanks!
moar https://codereview.chromium.org/406043003/diff/220001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/module_integrity_verifier.cc (right): https://codereview.chromium.org/406043003/diff/220001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.cc:109: // Section size will be the same as in the loaded module. On 2014/07/30 14:34:35, krstnmnlsn wrote: > On 2014/07/29 19:32:22, grt wrote: > > is this necessarily true? VirtualSize (size of the section when mapped into > > memory) may be greater than or less than SizeOfRawData. in the former case > > (VirtualSize is bigger), the section is zero padded when mapped. in the latter > > case (VirtualSize is smaller), SizeOfRawData has been rounded up to the > nearest > > OptionalHeader.FileAlignment boundary. > > Ah, I misunderstood that. It should be safe/sufficient to take the minimum of > the two then yes? I believe so, yes. https://codereview.chromium.org/406043003/diff/280001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/module_integrity_verifier.cc (right): https://codereview.chromium.org/406043003/diff/280001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.cc:29: private: https://codereview.chromium.org/406043003/diff/280001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.cc:46: for (int i = 0; i < static_cast<int>(code_size); ++i) { unsigned -> signed is implementation defined if the original value is too big to fit in the target type, so this cast is dangerous. how about: for (uint8_t* end = disk_code_start + code_size; disk_code_start != end; ) { if (*disk_code_start++ != *memory_code_start++) ++counter; } https://codereview.chromium.org/406043003/diff/280001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.cc:74: &code_size)) i thought chromium style called for braces around this since the "if" spans multiple lines, but if "git cl format" allows it, then i guess i'm wrong. okay. https://codereview.chromium.org/406043003/diff/280001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.cc:86: reinterpret_cast<uint8_t*>(mem_peimage.module()); why cast to uint8_t* and then assign to uintptr_t? https://codereview.chromium.org/406043003/diff/280001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.cc:95: if (new_value == *disk_address) { nit: remove braces https://codereview.chromium.org/406043003/diff/280001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.cc:104: default: { nit: remove braces https://codereview.chromium.org/406043003/diff/280001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.cc:107: reloc_enum_state->unknown_reloc_type = true; consider putting: UMA_HISTOGRAM_ENUMERATION("SafeBrowsing.ModuleBaseRelocation", type, IMAGE_REL_BASED_DIR64+1); up around line 80 so that you can see if these unhandled cases are happening in the field. you'll need a corresponding change to src/tools/metrics/histograms/histograms.xml to add the histogram and its enum type. https://codereview.chromium.org/406043003/diff/280001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.cc:145: for (int i = 0; i < sizeof(num_a); ++i) { is this equivalent? for (uintptr_t delta = num_a ^ num_b; delta; delta >>= 8) { if (delta & 0xFF) ++num_bytes; } if i'm not mistaken, this will terminate once low-order byte differences have been found (and not loop at all when the pointers are identical). https://codereview.chromium.org/406043003/diff/280001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/module_integrity_verifier_unittest.cc (right): https://codereview.chromium.org/406043003/diff/280001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier_unittest.cc:17: const wchar_t kTestDllName[] = L"verifier_test_dll.dll"; put in unnamed namespace https://codereview.chromium.org/406043003/diff/280001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier_unittest.cc:21: SafeBrowsingModuleVerifierTest() {} may as well remove these https://codereview.chromium.org/406043003/diff/280001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier_unittest.cc:44: ASSERT_TRUE(PathService::Get(base::DIR_EXE, ¤t_dir)); is this needed? doesn't the loader check the exe's directory first by default? https://codereview.chromium.org/406043003/diff/280001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier_unittest.cc:52: ASSERT_TRUE(NULL != *mem_handle); as clumsy as it is: ASSERT_NE(static_cast<HMODULE>(NULL), *mem_handle); https://codereview.chromium.org/406043003/diff/280001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier_unittest.cc:58: ASSERT_TRUE(NULL != module_handle); ASSERT_NE(static_cast<HMODULE>(NULL), module_handle); https://codereview.chromium.org/406043003/diff/280001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier_unittest.cc:63: ASSERT_NE(length, arraysize(module_path)); swap args. reason: the expected value comes first in ASSERT_EQ, so i think it's a good idea to do the same for ASSERT_NE. https://codereview.chromium.org/406043003/diff/280001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier_unittest.cc:70: private: not needed for tests https://codereview.chromium.org/406043003/diff/280001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/406043003/diff/280001/chrome/chrome_browser.g... chrome/chrome_browser.gypi:2668: 'browser/safe_browsing/module_integrity_verifier.cc', _win.cc and _win.h https://codereview.chromium.org/406043003/diff/280001/chrome/chrome_tests_uni... File chrome/chrome_tests_unit.gypi (right): https://codereview.chromium.org/406043003/diff/280001/chrome/chrome_tests_uni... chrome/chrome_tests_unit.gypi:1268: 'browser/safe_browsing/module_integrity_verifier_unittest.cc', _win_unittest.cc
Greg pointed out it would make more sense to add the modified-export-handling code here (https://codereview.chromium.org/434163002/). I have tried to incorporate the suggestions from both CLs. https://codereview.chromium.org/406043003/diff/220001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/module_integrity_verifier.cc (right): https://codereview.chromium.org/406043003/diff/220001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.cc:109: // Section size will be the same as in the loaded module. On 2014/07/31 17:03:33, grt (no reviews Aug 9 - 27) wrote: > On 2014/07/30 14:34:35, krstnmnlsn wrote: > > On 2014/07/29 19:32:22, grt wrote: > > > is this necessarily true? VirtualSize (size of the section when mapped into > > > memory) may be greater than or less than SizeOfRawData. in the former case > > > (VirtualSize is bigger), the section is zero padded when mapped. in the > latter > > > case (VirtualSize is smaller), SizeOfRawData has been rounded up to the > > nearest > > > OptionalHeader.FileAlignment boundary. > > > > Ah, I misunderstood that. It should be safe/sufficient to take the minimum of > > the two then yes? > > I believe so, yes. Done. https://codereview.chromium.org/406043003/diff/280001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/module_integrity_verifier.cc (right): https://codereview.chromium.org/406043003/diff/280001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.cc:29: On 2014/07/31 17:03:33, grt (no reviews Aug 9 - 27) wrote: > private: Done. https://codereview.chromium.org/406043003/diff/280001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.cc:46: for (int i = 0; i < static_cast<int>(code_size); ++i) { On 2014/07/31 17:03:33, grt (no reviews Aug 9 - 27) wrote: > unsigned -> signed is implementation defined if the original value is too big to > fit in the target type, so this cast is dangerous. how about: > > for (uint8_t* end = disk_code_start + code_size; disk_code_start != end; ) { > if (*disk_code_start++ != *memory_code_start++) > ++counter; > } Right, had tried to fix this in the other branch with uint32_t, using an actual pointer seems more semantically correct though. Done. https://codereview.chromium.org/406043003/diff/280001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.cc:74: &code_size)) On 2014/07/31 17:03:34, grt (no reviews Aug 9 - 27) wrote: > i thought chromium style called for braces around this since the "if" spans > multiple lines, but if "git cl format" allows it, then i guess i'm wrong. okay. Don't see anything. They do say we can add them if we want them however. Would you like some braces? https://codereview.chromium.org/406043003/diff/280001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.cc:86: reinterpret_cast<uint8_t*>(mem_peimage.module()); On 2014/07/31 17:03:33, grt (no reviews Aug 9 - 27) wrote: > why cast to uint8_t* and then assign to uintptr_t? The original plan was to store pointers as pointers, but delta is not technically an address so I thought storing it as an actual pointer would be bad? Compromised on uintptr_t, which is still misleading... I could change everything to uintptr_t? https://codereview.chromium.org/406043003/diff/280001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.cc:95: if (new_value == *disk_address) { On 2014/07/31 17:03:34, grt (no reviews Aug 9 - 27) wrote: > nit: remove braces Done. https://codereview.chromium.org/406043003/diff/280001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.cc:104: default: { On 2014/07/31 17:03:33, grt (no reviews Aug 9 - 27) wrote: > nit: remove braces Done. https://codereview.chromium.org/406043003/diff/280001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.cc:107: reloc_enum_state->unknown_reloc_type = true; On 2014/07/31 17:03:33, grt (no reviews Aug 9 - 27) wrote: > consider putting: > UMA_HISTOGRAM_ENUMERATION("SafeBrowsing.ModuleBaseRelocation", > type, IMAGE_REL_BASED_DIR64+1); > up around line 80 so that you can see if these unhandled cases are happening in > the field. you'll need a corresponding change to > src/tools/metrics/histograms/histograms.xml to add the histogram and its enum > type. I like this idea! Added a histogram, I made the value-name pairs of the enum agree with the winnt.h based relocation types. Should we try to handle machine specific differences instead? https://codereview.chromium.org/406043003/diff/280001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.cc:145: for (int i = 0; i < sizeof(num_a); ++i) { On 2014/07/31 17:03:33, grt (no reviews Aug 9 - 27) wrote: > is this equivalent? > for (uintptr_t delta = num_a ^ num_b; delta; delta >>= 8) { > if (delta & 0xFF) > ++num_bytes; > } > > if i'm not mistaken, this will terminate once low-order byte differences have > been found (and not loop at all when the pointers are identical). right, why not take the difference right away. thanks! https://codereview.chromium.org/406043003/diff/280001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/module_integrity_verifier_unittest.cc (right): https://codereview.chromium.org/406043003/diff/280001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier_unittest.cc:17: const wchar_t kTestDllName[] = L"verifier_test_dll.dll"; On 2014/07/31 17:03:34, grt (no reviews Aug 9 - 27) wrote: > put in unnamed namespace Right. https://codereview.chromium.org/406043003/diff/280001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier_unittest.cc:21: SafeBrowsingModuleVerifierTest() {} On 2014/07/31 17:03:34, grt (no reviews Aug 9 - 27) wrote: > may as well remove these Done. https://codereview.chromium.org/406043003/diff/280001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier_unittest.cc:44: ASSERT_TRUE(PathService::Get(base::DIR_EXE, ¤t_dir)); On 2014/07/31 17:03:34, grt (no reviews Aug 9 - 27) wrote: > is this needed? doesn't the loader check the exe's directory first by default? Yep, seems like I don't need it. https://codereview.chromium.org/406043003/diff/280001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier_unittest.cc:52: ASSERT_TRUE(NULL != *mem_handle); On 2014/07/31 17:03:34, grt (no reviews Aug 9 - 27) wrote: > as clumsy as it is: > ASSERT_NE(static_cast<HMODULE>(NULL), *mem_handle); Done. https://codereview.chromium.org/406043003/diff/280001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier_unittest.cc:58: ASSERT_TRUE(NULL != module_handle); On 2014/07/31 17:03:34, grt (no reviews Aug 9 - 27) wrote: > ASSERT_NE(static_cast<HMODULE>(NULL), module_handle); Done. https://codereview.chromium.org/406043003/diff/280001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier_unittest.cc:63: ASSERT_NE(length, arraysize(module_path)); On 2014/07/31 17:03:34, grt (no reviews Aug 9 - 27) wrote: > swap args. reason: the expected value comes first in ASSERT_EQ, so i think it's > a good idea to do the same for ASSERT_NE. Done. https://codereview.chromium.org/406043003/diff/280001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier_unittest.cc:70: private: On 2014/07/31 17:03:34, grt (no reviews Aug 9 - 27) wrote: > not needed for tests Done. https://codereview.chromium.org/406043003/diff/280001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/406043003/diff/280001/chrome/chrome_browser.g... chrome/chrome_browser.gypi:2668: 'browser/safe_browsing/module_integrity_verifier.cc', On 2014/07/31 17:03:34, grt (no reviews Aug 9 - 27) wrote: > _win.cc and _win.h Done. https://codereview.chromium.org/406043003/diff/280001/chrome/chrome_tests_uni... File chrome/chrome_tests_unit.gypi (right): https://codereview.chromium.org/406043003/diff/280001/chrome/chrome_tests_uni... chrome/chrome_tests_unit.gypi:1268: 'browser/safe_browsing/module_integrity_verifier_unittest.cc', On 2014/07/31 17:03:34, grt (no reviews Aug 9 - 27) wrote: > _win_unittest.cc Done.
Greg pointed out it would make more sense to add the modified-export-handling code here (https://codereview.chromium.org/434163002/). I have tried to incorporate the suggestions from both CLs. https://codereview.chromium.org/406043003/diff/220001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/module_integrity_verifier.cc (right): https://codereview.chromium.org/406043003/diff/220001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.cc:109: // Section size will be the same as in the loaded module. On 2014/07/31 17:03:33, grt (no reviews Aug 9 - 27) wrote: > On 2014/07/30 14:34:35, krstnmnlsn wrote: > > On 2014/07/29 19:32:22, grt wrote: > > > is this necessarily true? VirtualSize (size of the section when mapped into > > > memory) may be greater than or less than SizeOfRawData. in the former case > > > (VirtualSize is bigger), the section is zero padded when mapped. in the > latter > > > case (VirtualSize is smaller), SizeOfRawData has been rounded up to the > > nearest > > > OptionalHeader.FileAlignment boundary. > > > > Ah, I misunderstood that. It should be safe/sufficient to take the minimum of > > the two then yes? > > I believe so, yes. Done. https://codereview.chromium.org/406043003/diff/280001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/module_integrity_verifier.cc (right): https://codereview.chromium.org/406043003/diff/280001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.cc:29: On 2014/07/31 17:03:33, grt (no reviews Aug 9 - 27) wrote: > private: Done. https://codereview.chromium.org/406043003/diff/280001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.cc:46: for (int i = 0; i < static_cast<int>(code_size); ++i) { On 2014/07/31 17:03:33, grt (no reviews Aug 9 - 27) wrote: > unsigned -> signed is implementation defined if the original value is too big to > fit in the target type, so this cast is dangerous. how about: > > for (uint8_t* end = disk_code_start + code_size; disk_code_start != end; ) { > if (*disk_code_start++ != *memory_code_start++) > ++counter; > } Right, had tried to fix this in the other branch with uint32_t, using an actual pointer seems more semantically correct though. Done. https://codereview.chromium.org/406043003/diff/280001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.cc:74: &code_size)) On 2014/07/31 17:03:34, grt (no reviews Aug 9 - 27) wrote: > i thought chromium style called for braces around this since the "if" spans > multiple lines, but if "git cl format" allows it, then i guess i'm wrong. okay. Don't see anything. They do say we can add them if we want them however. Would you like some braces? https://codereview.chromium.org/406043003/diff/280001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.cc:86: reinterpret_cast<uint8_t*>(mem_peimage.module()); On 2014/07/31 17:03:33, grt (no reviews Aug 9 - 27) wrote: > why cast to uint8_t* and then assign to uintptr_t? The original plan was to store pointers as pointers, but delta is not technically an address so I thought storing it as an actual pointer would be bad? Compromised on uintptr_t, which is still misleading... I could change everything to uintptr_t? https://codereview.chromium.org/406043003/diff/280001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.cc:95: if (new_value == *disk_address) { On 2014/07/31 17:03:34, grt (no reviews Aug 9 - 27) wrote: > nit: remove braces Done. https://codereview.chromium.org/406043003/diff/280001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.cc:104: default: { On 2014/07/31 17:03:33, grt (no reviews Aug 9 - 27) wrote: > nit: remove braces Done. https://codereview.chromium.org/406043003/diff/280001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.cc:107: reloc_enum_state->unknown_reloc_type = true; On 2014/07/31 17:03:33, grt (no reviews Aug 9 - 27) wrote: > consider putting: > UMA_HISTOGRAM_ENUMERATION("SafeBrowsing.ModuleBaseRelocation", > type, IMAGE_REL_BASED_DIR64+1); > up around line 80 so that you can see if these unhandled cases are happening in > the field. you'll need a corresponding change to > src/tools/metrics/histograms/histograms.xml to add the histogram and its enum > type. I like this idea! Added a histogram, I made the value-name pairs of the enum agree with the winnt.h based relocation types. Should we try to handle machine specific differences instead? https://codereview.chromium.org/406043003/diff/280001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.cc:145: for (int i = 0; i < sizeof(num_a); ++i) { On 2014/07/31 17:03:33, grt (no reviews Aug 9 - 27) wrote: > is this equivalent? > for (uintptr_t delta = num_a ^ num_b; delta; delta >>= 8) { > if (delta & 0xFF) > ++num_bytes; > } > > if i'm not mistaken, this will terminate once low-order byte differences have > been found (and not loop at all when the pointers are identical). right, why not take the difference right away. thanks! https://codereview.chromium.org/406043003/diff/280001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/module_integrity_verifier_unittest.cc (right): https://codereview.chromium.org/406043003/diff/280001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier_unittest.cc:17: const wchar_t kTestDllName[] = L"verifier_test_dll.dll"; On 2014/07/31 17:03:34, grt (no reviews Aug 9 - 27) wrote: > put in unnamed namespace Right. https://codereview.chromium.org/406043003/diff/280001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier_unittest.cc:21: SafeBrowsingModuleVerifierTest() {} On 2014/07/31 17:03:34, grt (no reviews Aug 9 - 27) wrote: > may as well remove these Done. https://codereview.chromium.org/406043003/diff/280001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier_unittest.cc:44: ASSERT_TRUE(PathService::Get(base::DIR_EXE, ¤t_dir)); On 2014/07/31 17:03:34, grt (no reviews Aug 9 - 27) wrote: > is this needed? doesn't the loader check the exe's directory first by default? Yep, seems like I don't need it. https://codereview.chromium.org/406043003/diff/280001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier_unittest.cc:52: ASSERT_TRUE(NULL != *mem_handle); On 2014/07/31 17:03:34, grt (no reviews Aug 9 - 27) wrote: > as clumsy as it is: > ASSERT_NE(static_cast<HMODULE>(NULL), *mem_handle); Done. https://codereview.chromium.org/406043003/diff/280001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier_unittest.cc:58: ASSERT_TRUE(NULL != module_handle); On 2014/07/31 17:03:34, grt (no reviews Aug 9 - 27) wrote: > ASSERT_NE(static_cast<HMODULE>(NULL), module_handle); Done. https://codereview.chromium.org/406043003/diff/280001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier_unittest.cc:63: ASSERT_NE(length, arraysize(module_path)); On 2014/07/31 17:03:34, grt (no reviews Aug 9 - 27) wrote: > swap args. reason: the expected value comes first in ASSERT_EQ, so i think it's > a good idea to do the same for ASSERT_NE. Done. https://codereview.chromium.org/406043003/diff/280001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier_unittest.cc:70: private: On 2014/07/31 17:03:34, grt (no reviews Aug 9 - 27) wrote: > not needed for tests Done. https://codereview.chromium.org/406043003/diff/280001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/406043003/diff/280001/chrome/chrome_browser.g... chrome/chrome_browser.gypi:2668: 'browser/safe_browsing/module_integrity_verifier.cc', On 2014/07/31 17:03:34, grt (no reviews Aug 9 - 27) wrote: > _win.cc and _win.h Done. https://codereview.chromium.org/406043003/diff/280001/chrome/chrome_tests_uni... File chrome/chrome_tests_unit.gypi (right): https://codereview.chromium.org/406043003/diff/280001/chrome/chrome_tests_uni... chrome/chrome_tests_unit.gypi:1268: 'browser/safe_browsing/module_integrity_verifier_unittest.cc', On 2014/07/31 17:03:34, grt (no reviews Aug 9 - 27) wrote: > _win_unittest.cc Done.
asvitkine@chromium.org: Please review histograms.xml Thanks again!
https://codereview.chromium.org/406043003/diff/380001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/module_integrity_verifier_win.cc (right): https://codereview.chromium.org/406043003/diff/380001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier_win.cc:175: "SafeBrowsing.ModuleBaseRelocation", type, IMAGE_REL_BASED_DIR64 + 1); How do you know |type| will always be in range? Perhaps in this case, it's better to use a sparse histogram (see UMA_HISTOGRAM_SPARSE_SLOWLY), which doesn't require specifying a max. That is, if you're OK with the overhead of it using a map (i.e. which may involve an allocation). https://codereview.chromium.org/406043003/diff/380001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/406043003/diff/380001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:25029: + A windows only historgram. Records when an unknown base relocation type is Nit: capitalize Windows. Also, you can move the first sentence to the end - or alternative incorporate it into the next sentence - e.g. "(Windows only) Records when an unknown base..." That way, the first sentence actually describes what this does, rather than "A windows only histogram."
I'm uncomfortably excited. https://codereview.chromium.org/406043003/diff/280001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/module_integrity_verifier.cc (right): https://codereview.chromium.org/406043003/diff/280001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.cc:74: &code_size)) On 2014/08/04 15:18:15, krstnmnlsn wrote: > On 2014/07/31 17:03:34, grt (no reviews Aug 9 - 27) wrote: > > i thought chromium style called for braces around this since the "if" spans > > multiple lines, but if "git cl format" allows it, then i guess i'm wrong. > okay. > > Don't see anything. They do say we can add them if we want them however. Would > you like some braces? Naw, if cl-format is happy, I'm happy. https://codereview.chromium.org/406043003/diff/280001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.cc:86: reinterpret_cast<uint8_t*>(mem_peimage.module()); On 2014/08/04 15:18:15, krstnmnlsn wrote: > On 2014/07/31 17:03:33, grt (no reviews Aug 9 - 27) wrote: > > why cast to uint8_t* and then assign to uintptr_t? > > The original plan was to store pointers as pointers, but delta is not > technically an address so I thought storing it as an actual pointer would be > bad? Compromised on uintptr_t, which is still misleading... > I could change everything to uintptr_t? Ah, I misread this before. Deltas are often signed (e.g., ptrdiff_t is signed while size_t is unsigned) since they ordinarily can be positive or negative. If that's the case here, then intptr_t is probably the right choice. Does this sound safe to you? https://codereview.chromium.org/406043003/diff/280001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.cc:107: reloc_enum_state->unknown_reloc_type = true; On 2014/08/04 15:18:14, krstnmnlsn wrote: > On 2014/07/31 17:03:33, grt (no reviews Aug 9 - 27) wrote: > > consider putting: > > UMA_HISTOGRAM_ENUMERATION("SafeBrowsing.ModuleBaseRelocation", > > type, IMAGE_REL_BASED_DIR64+1); > > up around line 80 so that you can see if these unhandled cases are happening > in > > the field. you'll need a corresponding change to > > src/tools/metrics/histograms/histograms.xml to add the histogram and its enum > > type. > > I like this idea! Added a histogram, I made the value-name pairs of the enum > agree with the winnt.h based relocation types. Should we try to handle machine > specific differences instead? Can you clarify the question? What are the machine-specific differences? https://codereview.chromium.org/406043003/diff/380001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/module_integrity_verifier_win.cc (right): https://codereview.chromium.org/406043003/diff/380001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier_win.cc:7: #include <set> #include "base/containers/hash_tables.h" https://codereview.chromium.org/406043003/diff/380001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier_win.cc:56: Export(void* addr, std::string name); const std::string& https://codereview.chromium.org/406043003/diff/380001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier_win.cc:81: // Checks each byte in the module's code section again the corresponding byte on please add to the doc comment that |exports| must be sorted https://codereview.chromium.org/406043003/diff/380001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier_win.cc:86: std::vector<Export> exports, const std::vector<Export>& https://codereview.chromium.org/406043003/diff/380001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier_win.cc:90: std::vector<Export>::iterator export_it = exports.begin(); const_iterator https://codereview.chromium.org/406043003/diff/380001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier_win.cc:99: Export addr = Export addr(reinterpret_cast<void*>(mem_code_start), std::string()); https://codereview.chromium.org/406043003/diff/380001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier_win.cc:117: void AddBytesCorrectedByReloc(ModuleVerificationState* state, swap args since |state| is modified https://codereview.chromium.org/406043003/diff/380001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier_win.cc:128: if (fixed & 0xFF) { nit: no braces https://codereview.chromium.org/406043003/diff/380001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier_win.cc:129: state->reloc_addr.insert(address + i); this depends on the endianness of the machine, doesn't it? do you care? https://codereview.chromium.org/406043003/diff/380001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier_win.cc:131: ++i; put this in the for loop (fixed >>= 8, ++i)? i think it'll just fit without wrapping: for (uintptr_t fixed = diff_before & shared_after; fixed; fixed >>= 8, ++i) { if (fixed & 0xFF) state->reloc_addr.insert(address + i); } https://codereview.chromium.org/406043003/diff/380001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier_win.cc:174: UMA_HISTOGRAM_ENUMERATION( if this is done on line 161, then you'll be able to see how often these unhandled types occur relative to the handled ones. is that valuable? https://codereview.chromium.org/406043003/diff/380001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier_win.cc:175: "SafeBrowsing.ModuleBaseRelocation", type, IMAGE_REL_BASED_DIR64 + 1); On 2014/08/05 14:51:21, Alexei Svitkine wrote: > How do you know |type| will always be in range? If it's out of range, the PE image is malformed in some way. The NT loader may not even know what to do with it. > Perhaps in this case, it's better to use a sparse histogram (see > UMA_HISTOGRAM_SPARSE_SLOWLY), which doesn't require specifying a max. That is, > if you're OK with the overhead of it using a map (i.e. which may involve an > allocation). Do values that exceed the max end up in an overflow bucket? Does UMA_HISTOGRAM_ENUMERATION keep an overflow bucket? Can it be used with one? It might be enough to know that unknown values are seen (and how often) without knowing their exact values. https://codereview.chromium.org/406043003/diff/380001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier_win.cc:191: if (name) { nit: no braces https://codereview.chromium.org/406043003/diff/380001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier_win.cc:193: } do you care about handling exports-by-ordinal? https://codereview.chromium.org/406043003/diff/380001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier_win.cc:291: if (num_bytes_different == 0) return num_bytes_different ? MODULE_STATE_MODIFIED : MODULE_STATE_UNMODIFIED; https://codereview.chromium.org/406043003/diff/380001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/module_integrity_verifier_win.h (right): https://codereview.chromium.org/406043003/diff/380001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier_win.h:10: #include <set> #include <string> https://codereview.chromium.org/406043003/diff/380001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/module_integrity_verifier_win_unittest.cc (right): https://codereview.chromium.org/406043003/diff/380001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier_win_unittest.cc:26: base::ScopedNativeLibrary mem_dll_handle_; data members come below methods (http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order)
Thanks for the feedback guys! Perhaps you should take a short lie down greg... https://codereview.chromium.org/406043003/diff/280001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/module_integrity_verifier.cc (right): https://codereview.chromium.org/406043003/diff/280001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.cc:86: reinterpret_cast<uint8_t*>(mem_peimage.module()); On 2014/08/05 15:51:55, grt (no reviews Aug 9 - 27) wrote: > On 2014/08/04 15:18:15, krstnmnlsn wrote: > > On 2014/07/31 17:03:33, grt (no reviews Aug 9 - 27) wrote: > > > why cast to uint8_t* and then assign to uintptr_t? > > > > The original plan was to store pointers as pointers, but delta is not > > technically an address so I thought storing it as an actual pointer would be > > bad? Compromised on uintptr_t, which is still misleading... > > I could change everything to uintptr_t? > > Ah, I misread this before. Deltas are often signed (e.g., ptrdiff_t is signed > while size_t is unsigned) since they ordinarily can be positive or negative. If > that's the case here, then intptr_t is probably the right choice. Does this > sound safe to you? Right, that sounds perfect to me. This code has been heavily reshuffled so I think this change now amounts to making image_base_delta and code_section_delta intptr_t's in ModuleVerificationState. https://codereview.chromium.org/406043003/diff/280001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.cc:107: reloc_enum_state->unknown_reloc_type = true; On 2014/08/05 15:51:55, grt (no reviews Aug 9 - 27) wrote: > On 2014/08/04 15:18:14, krstnmnlsn wrote: > > On 2014/07/31 17:03:33, grt (no reviews Aug 9 - 27) wrote: > > > consider putting: > > > UMA_HISTOGRAM_ENUMERATION("SafeBrowsing.ModuleBaseRelocation", > > > type, IMAGE_REL_BASED_DIR64+1); > > > up around line 80 so that you can see if these unhandled cases are happening > > in > > > the field. you'll need a corresponding change to > > > src/tools/metrics/histograms/histograms.xml to add the histogram and its > enum > > > type. > > > > I like this idea! Added a histogram, I made the value-name pairs of the enum > > agree with the winnt.h based relocation types. Should we try to handle > machine > > specific differences instead? > > Can you clarify the question? What are the machine-specific differences? So in my enum I have things like "<int value="5" label="IMAGE_REL_BASED_MACHINE_SPECIFIC_5"/>" Because winnt.h defines IMAGE_REL_BASED_MACHINE_SPECIFIC_5 as 5. However they also define IMAGE_REL_BASED_MIPS_JMPADDR and IMAGE_REL_BASED_MIPS_JMPADDR16 as 5. So receiving a 5 could mean either of those two (depending on the machine). Do you think it's worth differentiating right now? https://codereview.chromium.org/406043003/diff/380001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/module_integrity_verifier_win.cc (right): https://codereview.chromium.org/406043003/diff/380001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier_win.cc:7: #include <set> On 2014/08/05 15:51:55, grt (no reviews Aug 9 - 27) wrote: > #include "base/containers/hash_tables.h" Done. https://codereview.chromium.org/406043003/diff/380001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier_win.cc:56: Export(void* addr, std::string name); On 2014/08/05 15:51:56, grt (no reviews Aug 9 - 27) wrote: > const std::string& Done. https://codereview.chromium.org/406043003/diff/380001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier_win.cc:81: // Checks each byte in the module's code section again the corresponding byte on On 2014/08/05 15:51:55, grt (no reviews Aug 9 - 27) wrote: > please add to the doc comment that |exports| must be sorted Done. https://codereview.chromium.org/406043003/diff/380001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier_win.cc:86: std::vector<Export> exports, On 2014/08/05 15:51:55, grt (no reviews Aug 9 - 27) wrote: > const std::vector<Export>& Done. https://codereview.chromium.org/406043003/diff/380001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier_win.cc:90: std::vector<Export>::iterator export_it = exports.begin(); On 2014/08/05 15:51:55, grt (no reviews Aug 9 - 27) wrote: > const_iterator Done. https://codereview.chromium.org/406043003/diff/380001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier_win.cc:99: Export addr = On 2014/08/05 15:51:56, grt (no reviews Aug 9 - 27) wrote: > Export addr(reinterpret_cast<void*>(mem_code_start), std::string()); Done. https://codereview.chromium.org/406043003/diff/380001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier_win.cc:117: void AddBytesCorrectedByReloc(ModuleVerificationState* state, On 2014/08/05 15:51:55, grt (no reviews Aug 9 - 27) wrote: > swap args since |state| is modified Done. https://codereview.chromium.org/406043003/diff/380001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier_win.cc:128: if (fixed & 0xFF) { On 2014/08/05 15:51:56, grt (no reviews Aug 9 - 27) wrote: > nit: no braces Done. https://codereview.chromium.org/406043003/diff/380001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier_win.cc:129: state->reloc_addr.insert(address + i); On 2014/08/05 15:51:55, grt (no reviews Aug 9 - 27) wrote: > this depends on the endianness of the machine, doesn't it? do you care? Oh hmm. I do care if windows ever stores big endian addresses. What is the normal way of handling that in chrome? I found some macros people seem to be using (see above). https://codereview.chromium.org/406043003/diff/380001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier_win.cc:131: ++i; On 2014/08/05 15:51:55, grt (no reviews Aug 9 - 27) wrote: > put this in the for loop (fixed >>= 8, ++i)? i think it'll just fit without > wrapping: > > for (uintptr_t fixed = diff_before & shared_after; fixed; fixed >>= 8, ++i) { > if (fixed & 0xFF) > state->reloc_addr.insert(address + i); > } With (one) space to spare! https://codereview.chromium.org/406043003/diff/380001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier_win.cc:174: UMA_HISTOGRAM_ENUMERATION( On 2014/08/05 15:51:56, grt (no reviews Aug 9 - 27) wrote: > if this is done on line 161, then you'll be able to see how often these > unhandled types occur relative to the handled ones. is that valuable? That would probably give a better picture yes. https://codereview.chromium.org/406043003/diff/380001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier_win.cc:175: "SafeBrowsing.ModuleBaseRelocation", type, IMAGE_REL_BASED_DIR64 + 1); On 2014/08/05 15:51:55, grt (no reviews Aug 9 - 27) wrote: > On 2014/08/05 14:51:21, Alexei Svitkine wrote: > > How do you know |type| will always be in range? > > If it's out of range, the PE image is malformed in some way. The NT loader may > not even know what to do with it. > > > Perhaps in this case, it's better to use a sparse histogram (see > > UMA_HISTOGRAM_SPARSE_SLOWLY), which doesn't require specifying a max. That is, > > if you're OK with the overhead of it using a map (i.e. which may involve an > > allocation). > > Do values that exceed the max end up in an overflow bucket? Does > UMA_HISTOGRAM_ENUMERATION keep an overflow bucket? Can it be used with one? It > might be enough to know that unknown values are seen (and how often) without > knowing their exact values. If it doesn't have an overflow bucket I could add one manually? I agree that larger values shouldn't in theory appear... https://codereview.chromium.org/406043003/diff/380001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier_win.cc:191: if (name) { On 2014/08/05 15:51:56, grt (no reviews Aug 9 - 27) wrote: > nit: no braces Done. https://codereview.chromium.org/406043003/diff/380001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier_win.cc:193: } On 2014/08/05 15:51:55, grt (no reviews Aug 9 - 27) wrote: > do you care about handling exports-by-ordinal? I'm not sure what you mean? Currently, the code is only interested in determining which export a modified byte in memory could belong to. I didn't think the ordinals could help with that, since (I thought) ordinal's aren't guaranteed to be ordered as in memory? https://codereview.chromium.org/406043003/diff/380001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier_win.cc:291: if (num_bytes_different == 0) On 2014/08/05 15:51:55, grt (no reviews Aug 9 - 27) wrote: > return num_bytes_different ? MODULE_STATE_MODIFIED : MODULE_STATE_UNMODIFIED; Done. https://codereview.chromium.org/406043003/diff/380001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/module_integrity_verifier_win.h (right): https://codereview.chromium.org/406043003/diff/380001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier_win.h:10: #include <set> On 2014/08/05 15:51:56, grt (no reviews Aug 9 - 27) wrote: > #include <string> Done. https://codereview.chromium.org/406043003/diff/380001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/module_integrity_verifier_win_unittest.cc (right): https://codereview.chromium.org/406043003/diff/380001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier_win_unittest.cc:26: base::ScopedNativeLibrary mem_dll_handle_; On 2014/08/05 15:51:56, grt (no reviews Aug 9 - 27) wrote: > data members come below methods > (http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order) Done. https://codereview.chromium.org/406043003/diff/380001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/406043003/diff/380001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:25029: + A windows only historgram. Records when an unknown base relocation type is On 2014/08/05 14:51:21, Alexei Svitkine wrote: > Nit: capitalize Windows. Also, you can move the first sentence to the end - or > alternative incorporate it into the next sentence - e.g. "(Windows only) Records > when an unknown base..." That way, the first sentence actually describes what > this does, rather than "A windows only histogram." Done.
https://codereview.chromium.org/406043003/diff/380001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/module_integrity_verifier_win.cc (right): https://codereview.chromium.org/406043003/diff/380001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier_win.cc:175: "SafeBrowsing.ModuleBaseRelocation", type, IMAGE_REL_BASED_DIR64 + 1); On 2014/08/05 19:17:48, krstnmnlsn wrote: > On 2014/08/05 15:51:55, grt (no reviews Aug 9 - 27) wrote: > > On 2014/08/05 14:51:21, Alexei Svitkine wrote: > > > How do you know |type| will always be in range? > > > > If it's out of range, the PE image is malformed in some way. The NT loader may > > not even know what to do with it. > > > > > Perhaps in this case, it's better to use a sparse histogram (see > > > UMA_HISTOGRAM_SPARSE_SLOWLY), which doesn't require specifying a max. That > is, > > > if you're OK with the overhead of it using a map (i.e. which may involve an > > > allocation). > > > > Do values that exceed the max end up in an overflow bucket? Does > > UMA_HISTOGRAM_ENUMERATION keep an overflow bucket? Can it be used with one? It > > might be enough to know that unknown values are seen (and how often) without > > knowing their exact values. > > If it doesn't have an overflow bucket I could add one manually? I agree that > larger values shouldn't in theory appear... Yes, there will be an overflow bucket by default. My point is that it's not clear to me what ensures |type| is in range. What stops a newer version of Windows and MS dev tools from introducing a new reloc type or for there to exist some undocumented reloc types that Microsoft uses internally? If you're OK keeping as is, please add a comment explaining why IMAGE_REL_BASED_DIR64 + 1 is the correct value to use here (e.g. by pointing at a msdn URL or something).
On 2014/08/05 19:27:07, Alexei Svitkine wrote: > https://codereview.chromium.org/406043003/diff/380001/chrome/browser/safe_bro... > File chrome/browser/safe_browsing/module_integrity_verifier_win.cc (right): > > https://codereview.chromium.org/406043003/diff/380001/chrome/browser/safe_bro... > chrome/browser/safe_browsing/module_integrity_verifier_win.cc:175: > "SafeBrowsing.ModuleBaseRelocation", type, IMAGE_REL_BASED_DIR64 + 1); > On 2014/08/05 19:17:48, krstnmnlsn wrote: > > On 2014/08/05 15:51:55, grt (no reviews Aug 9 - 27) wrote: > > > On 2014/08/05 14:51:21, Alexei Svitkine wrote: > > > > How do you know |type| will always be in range? > > > > > > If it's out of range, the PE image is malformed in some way. The NT loader > may > > > not even know what to do with it. > > > > > > > Perhaps in this case, it's better to use a sparse histogram (see > > > > UMA_HISTOGRAM_SPARSE_SLOWLY), which doesn't require specifying a max. That > > is, > > > > if you're OK with the overhead of it using a map (i.e. which may involve > an > > > > allocation). > > > > > > Do values that exceed the max end up in an overflow bucket? Does > > > UMA_HISTOGRAM_ENUMERATION keep an overflow bucket? Can it be used with one? > It > > > might be enough to know that unknown values are seen (and how often) without > > > knowing their exact values. > > > > If it doesn't have an overflow bucket I could add one manually? I agree that > > larger values shouldn't in theory appear... > > Yes, there will be an overflow bucket by default. My point is that it's not > clear to me what ensures |type| is in range. What stops a newer version of > Windows and MS dev tools from introducing a new reloc type or for there to exist > some undocumented reloc types that Microsoft uses internally? > > If you're OK keeping as is, please add a comment explaining why > IMAGE_REL_BASED_DIR64 + 1 is the correct value to use here (e.g. by pointing at > a msdn URL or something). The Microsoft PE and COFF Specification (http://msdn.microsoft.com/en-us/gg463119.aspx) says that valid relocation types go up to that value. I'm not sure that this histogram needs to be the way we find out about new values. That said, if the overhead of SPARSE_SLOWLY isn't that big of a deal, it's probably fine to use it here. What are the conditions for an allocation, each time a new value is added to the histogram?
responding to comments. will have another look at the code a bit later. https://codereview.chromium.org/406043003/diff/280001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/module_integrity_verifier.cc (right): https://codereview.chromium.org/406043003/diff/280001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier.cc:107: reloc_enum_state->unknown_reloc_type = true; On 2014/08/05 19:17:47, krstnmnlsn wrote: > On 2014/08/05 15:51:55, grt (no reviews Aug 9 - 27) wrote: > > On 2014/08/04 15:18:14, krstnmnlsn wrote: > > > On 2014/07/31 17:03:33, grt (no reviews Aug 9 - 27) wrote: > > > > consider putting: > > > > UMA_HISTOGRAM_ENUMERATION("SafeBrowsing.ModuleBaseRelocation", > > > > type, IMAGE_REL_BASED_DIR64+1); > > > > up around line 80 so that you can see if these unhandled cases are > happening > > > in > > > > the field. you'll need a corresponding change to > > > > src/tools/metrics/histograms/histograms.xml to add the histogram and its > > enum > > > > type. > > > > > > I like this idea! Added a histogram, I made the value-name pairs of the > enum > > > agree with the winnt.h based relocation types. Should we try to handle > > machine > > > specific differences instead? > > > > Can you clarify the question? What are the machine-specific differences? > > So in my enum I have things like > "<int value="5" label="IMAGE_REL_BASED_MACHINE_SPECIFIC_5"/>" > Because winnt.h defines IMAGE_REL_BASED_MACHINE_SPECIFIC_5 as 5. However they > also define IMAGE_REL_BASED_MIPS_JMPADDR and IMAGE_REL_BASED_MIPS_JMPADDR16 as > 5. So receiving a 5 could mean either of those two (depending on the machine). > Do you think it's worth differentiating right now? No, just storing the values is fine. We have CPU info elsewhere in the protobuf, so we can figure out which symbolic name it is if needed. https://codereview.chromium.org/406043003/diff/380001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/module_integrity_verifier_win.cc (right): https://codereview.chromium.org/406043003/diff/380001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier_win.cc:129: state->reloc_addr.insert(address + i); On 2014/08/05 19:17:49, krstnmnlsn wrote: > On 2014/08/05 15:51:55, grt (no reviews Aug 9 - 27) wrote: > > this depends on the endianness of the machine, doesn't it? do you care? > > Oh hmm. I do care if windows ever stores big endian addresses. What is the > normal way of handling that in chrome? I found some macros people seem to be > using (see above). maybe something like: #if defined(ARCH_CPU_LITTLE_ENDIAN) ... #else #error Big-endian architectures are unsupported. #endif somewhere in this function https://codereview.chromium.org/406043003/diff/380001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier_win.cc:193: } On 2014/08/05 19:17:48, krstnmnlsn wrote: > On 2014/08/05 15:51:55, grt (no reviews Aug 9 - 27) wrote: > > do you care about handling exports-by-ordinal? > > I'm not sure what you mean? Currently, the code is only interested in > determining which export a modified byte in memory could belong to. I didn't > think the ordinals could help with that, since (I thought) ordinal's aren't > guaranteed to be ordered as in memory? Functions can be exported/imported by ordinal rather than by name; see http://msdn.microsoft.com/library/e7tsx612.aspx. By name is useful as-is, so please put a TODO in the code to add support for ordinals. The Microsoft Portable Executable and Common Object File Format Specification has gory details.
looks great. just some final nits. https://codereview.chromium.org/406043003/diff/420001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/module_integrity_verifier_win.cc (right): https://codereview.chromium.org/406043003/diff/420001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier_win.cc:81: // disk. The list |exports| must be sorted. Returns a list of comment nitpick: consider something like "Adds the names of functions exported by name to |modified_exports|." since it isn't a list and it isn't returned from the function. https://codereview.chromium.org/406043003/diff/420001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier_win.cc:81: // disk. The list |exports| must be sorted. Returns a list of similarly, "the list |exports|" could be simply "|exports| must be sorted" since it isn't a list. https://codereview.chromium.org/406043003/diff/420001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier_win.cc:81: // disk. The list |exports| must be sorted. Returns a list of "...on disk, returning the number of bytes differing between the two." https://codereview.chromium.org/406043003/diff/420001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier_win.cc:87: ModuleVerificationState* state, const ModuleVerificationState& state here and in ByteAccountedForByReloc. https://codereview.chromium.org/406043003/diff/420001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier_win.cc:118: #if __BYTE_ORDER == __LITTLE_ENDIAN awesome. i think it's better to use the chromium macro for this, though: #include "base/build_config.h" #if defined(ARCH_CPU_LITTLE_ENDIAN) ... #else ... #endif
On Tue, Aug 5, 2014 at 9:37 PM, <grt@chromium.org> wrote: > > The Microsoft PE and COFF Specification > (http://msdn.microsoft.com/en-us/gg463119.aspx) says that valid > relocation types > go up to that value. I'm not sure that this histogram needs to be the way > we > find out about new values. That said, if the overhead of SPARSE_SLOWLY > isn't > that big of a deal, it's probably fine to use it here. What are the > conditions > for an allocation, each time a new value is added to the histogram? It basically uses a lock and a map. So when a new value is inserted, it will add an entry to the map, which may result in an allocation. It uses a lock on the Histogram object to ensure that it's thread safe (since using a map from multiple threads isn't). (Non-sparse histograms simply pre-allocate an array to hold the values for all buckets.) That's the overhead of a sparse slowly. I'm okay either way: either using a sparse histogram or keeping as-is but adding a comment explaining the max value with a pointer to the docs. Up to you. On Wed, Aug 6, 2014 at 11:07 AM, <grt@chromium.org> wrote: > looks great. just some final nits. > > > https://codereview.chromium.org/406043003/diff/420001/ > chrome/browser/safe_browsing/module_integrity_verifier_win.cc > File chrome/browser/safe_browsing/module_integrity_verifier_win.cc > (right): > > https://codereview.chromium.org/406043003/diff/420001/ > chrome/browser/safe_browsing/module_integrity_verifier_win.cc#newcode81 > chrome/browser/safe_browsing/module_integrity_verifier_win.cc:81: // > disk. The list |exports| must be sorted. Returns a list of > comment nitpick: consider something like "Adds the names of functions > exported by name to |modified_exports|." since it isn't a list and it > isn't returned from the function. > > https://codereview.chromium.org/406043003/diff/420001/ > chrome/browser/safe_browsing/module_integrity_verifier_win.cc#newcode81 > chrome/browser/safe_browsing/module_integrity_verifier_win.cc:81: // > disk. The list |exports| must be sorted. Returns a list of > similarly, "the list |exports|" could be simply "|exports| must be > sorted" since it isn't a list. > > https://codereview.chromium.org/406043003/diff/420001/ > chrome/browser/safe_browsing/module_integrity_verifier_win.cc#newcode81 > chrome/browser/safe_browsing/module_integrity_verifier_win.cc:81: // > disk. The list |exports| must be sorted. Returns a list of > "...on disk, returning the number of bytes differing between the two." > > https://codereview.chromium.org/406043003/diff/420001/ > chrome/browser/safe_browsing/module_integrity_verifier_win.cc#newcode87 > chrome/browser/safe_browsing/module_integrity_verifier_win.cc:87: > ModuleVerificationState* state, > const ModuleVerificationState& state here and in > ByteAccountedForByReloc. > > https://codereview.chromium.org/406043003/diff/420001/ > chrome/browser/safe_browsing/module_integrity_verifier_win.cc#newcode118 > chrome/browser/safe_browsing/module_integrity_verifier_win.cc:118: #if > __BYTE_ORDER == __LITTLE_ENDIAN > awesome. i think it's better to use the chromium macro for this, though: > #include "base/build_config.h" > > #if defined(ARCH_CPU_LITTLE_ENDIAN) > ... > #else > ... > #endif > > https://codereview.chromium.org/406043003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/08/06 15:15:12, Alexei Svitkine wrote: > On Tue, Aug 5, 2014 at 9:37 PM, <mailto:grt@chromium.org> wrote: > > > > The Microsoft PE and COFF Specification > > (http://msdn.microsoft.com/en-us/gg463119.aspx) says that valid > > relocation types > > go up to that value. I'm not sure that this histogram needs to be the way > > we > > find out about new values. That said, if the overhead of SPARSE_SLOWLY > > isn't > > that big of a deal, it's probably fine to use it here. What are the > > conditions > > for an allocation, each time a new value is added to the histogram? > > > It basically uses a lock and a map. So when a new value is inserted, it > will add an entry to the map, which may result in an allocation. It uses a > lock on the Histogram object to ensure that it's thread safe (since using a > map from multiple threads isn't). (Non-sparse histograms simply > pre-allocate an array to hold the values for all buckets.) That's the > overhead of a sparse slowly. > > I'm okay either way: either using a sparse histogram or keeping as-is but > adding a comment explaining the max value with a pointer to the docs. Up to > you. This isn't performance-critical code, so SPARSE_SLOWLY sounds good to me.
After much discussion now going with the SPARSE_SLOWLY histogram. Thanks all! https://codereview.chromium.org/406043003/diff/420001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/module_integrity_verifier_win.cc (right): https://codereview.chromium.org/406043003/diff/420001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier_win.cc:81: // disk. The list |exports| must be sorted. Returns a list of On 2014/08/06 15:07:08, grt (no reviews Aug 9 - 27) wrote: > "...on disk, returning the number of bytes differing between the two." Triple Done https://codereview.chromium.org/406043003/diff/420001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier_win.cc:87: ModuleVerificationState* state, On 2014/08/06 15:07:08, grt (no reviews Aug 9 - 27) wrote: > const ModuleVerificationState& state here and in ByteAccountedForByReloc. Done. https://codereview.chromium.org/406043003/diff/420001/chrome/browser/safe_bro... chrome/browser/safe_browsing/module_integrity_verifier_win.cc:118: #if __BYTE_ORDER == __LITTLE_ENDIAN On 2014/08/06 15:07:08, grt (no reviews Aug 9 - 27) wrote: > awesome. i think it's better to use the chromium macro for this, though: > #include "base/build_config.h" > > #if defined(ARCH_CPU_LITTLE_ENDIAN) > ... > #else > ... > #endif Done.
lgtm, thanks By the way, since this CL is fairly large, you probably want to file a crbug to track this work.
lgtm
lgtm. please follow asvitkine's advice about filing a bug with a summary along the lines of "Include loaded module integrity info in safe browsing incident reports." and give it the Cr-UI-Browser-SafeBrowsing label. then use that issue # as the BUG= line here and in https://codereview.chromium.org/440753002/. also, it looks like the CL description is a little out of date. please update it before committing. thanks.
The CQ bit was checked by krstnmnlsn@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/krstnmnlsn@chromium.org/406043003/460001
Message was sent while issue was closed.
Change committed as 288319
Message was sent while issue was closed.
this got reverted because the buildbots (and soon win trybots) were using isolate. you will want to list the dll (verifier_test_dll.dll?) in chrome\unit_tests.isolate |
