Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(50)

Issue 1841573002: [Chrome ELF] New NT registry API. (Closed)

Created:
4 years, 8 months ago by penny
Modified:
4 years, 5 months ago
CC:
caitkp+watch_chromium.org, chromium-reviews, rickyz+watch_chromium.org, wfh+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

[Chrome ELF] New NT registry API. This CL is part of a chain of CLs: 1) "MITIGATION_EXTENSION_POINT_DISABLE support for children" (https://codereview.chromium.org/1835003003) 2) "MITIGATION_EXTENSION_POINT_DISABLE emergency off finch" (https://codereview.chromium.org/1836523004/) -> THIS 4) "Early browser security support" (https://codereview.chromium.org/1656453002) 5) "Turn on MITIGATION_EXTENSION_POINT_DISABLE" (https://codereview.chromium.org/1854323002) Added new chrome_elf_reg utility for a registry API that doesn't touch advapi32 (useful from DllMain). Direct calls to ntdll. Updated Chrome ELF to always use this new registry API. Adjusted the existing DLL blacklist to use a REG_MULTI_SZ comma-separated list instead of lots of individual reg values. Small changes to organize file structure and functional components under chrome_elf. Old common code now sits under "hook_util", "nt_registry", "breakpad", "dll_hash", and "blacklist". Fairly big changes to the chrome_elf tests (blacklist_test.cc, blacklist_test_main_dll.cc and chrome_elf_util_unittest.cc) were needed. Since ntdll bypasses any registry redirection (that tests use to keep the hive safe and isolated), I added in a way for the tests to access the redirection path (and pass that information on to the test DLL). This way the NT reg API can work with redirection during tests. Tests: 1) chrome_elf_unittests, chrome_elf_util_unittest.cc: ChromeElfUtilTest.NTRegistry is new (...but run all tests to exercise the new API being used by blacklist and utils). 2) unit_tests, chrome_elf_init_unittest_win.cc: ChromeBlacklistTrialTest* BUG=557798 CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win10_chromium_x64_rel_ng Committed: https://crrev.com/84fd669f74832ccab02fcbb8b96dc4ed58d11b43 Cr-Commit-Position: refs/heads/master@{#405307}

Patch Set 1 #

Patch Set 2 : Update #

Patch Set 3 : Clean up. #

Patch Set 4 : Fixed build. New util function to get the current user sid as a string (since we have it already). #

Patch Set 5 : Clean up OverrideRegistry function. #

Total comments: 49

Patch Set 6 : Code review fixes, part 1. #

Patch Set 7 : Updating chrome/install_static (chrome_elf_util). Pulled in latest trunk. #

Patch Set 8 : Fixing GYP #

Patch Set 9 : Fix my merge and 'cl format'. #

Patch Set 10 : Missed new unit tests for install_static. #

Total comments: 10

Patch Set 11 : Nit fixes & CreateRegKey now recursive. #

Total comments: 8

Patch Set 12 : Change recursion. #

Total comments: 11

Patch Set 13 : Branch update, and CreateRegKey adjustment. #

Total comments: 4

Patch Set 14 : Final nits. #

Patch Set 15 : PRESUBMIT to allow chrome_elf directory files to use wstring. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+3246 lines, -2530 lines) Patch
M PRESUBMIT.py View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +3 lines, -1 line 0 comments Download
M base/test/test_reg_util_win.h View 1 2 3 4 1 chunk +2 lines, -0 lines 0 comments Download
M base/test/test_reg_util_win.cc View 1 2 3 4 1 chunk +7 lines, -0 lines 0 comments Download
M chrome/browser/win/chrome_elf_init.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +16 lines, -10 lines 0 comments Download
M chrome/browser/win/chrome_elf_init_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +14 lines, -11 lines 0 comments Download
M chrome/chrome_installer.gypi View 1 2 3 4 5 6 7 8 1 chunk +1189 lines, -1189 lines 0 comments Download
M chrome/chrome_installer_static.gypi View 1 2 3 4 5 6 7 8 4 chunks +4 lines, -7 lines 0 comments Download
M chrome/install_static/BUILD.gn View 1 2 3 4 5 6 7 8 1 chunk +7 lines, -10 lines 0 comments Download
M chrome/install_static/DEPS View 1 2 3 4 5 6 1 chunk +3 lines, -4 lines 0 comments Download
M chrome/install_static/install_util.h View 1 2 3 4 5 6 7 8 9 10 9 chunks +21 lines, -24 lines 0 comments Download
M chrome/install_static/install_util.cc View 1 2 3 4 5 6 7 8 9 10 33 chunks +141 lines, -230 lines 0 comments Download
M chrome/install_static/install_util_unittest.cc View 1 2 3 4 5 6 7 8 9 10 2 chunks +2 lines, -2 lines 0 comments Download
M chrome_elf/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 8 chunks +54 lines, -34 lines 0 comments Download
M chrome_elf/OWNERS View 1 2 3 4 5 6 1 chunk +2 lines, -0 lines 0 comments Download
M chrome_elf/blacklist.gypi View 1 2 3 4 5 6 7 2 chunks +5 lines, -0 lines 0 comments Download
M chrome_elf/blacklist/blacklist.cc View 1 2 3 4 5 6 9 chunks +129 lines, -215 lines 0 comments Download
M chrome_elf/blacklist/blacklist_interceptions.cc View 1 2 3 4 5 6 7 8 9 10 11 12 8 chunks +14 lines, -15 lines 0 comments Download
M chrome_elf/blacklist/test/blacklist_test.cc View 1 2 3 4 5 6 7 8 9 10 11 12 7 chunks +107 lines, -23 lines 0 comments Download
D chrome_elf/blacklist/test/blacklist_test_main.cc View 1 2 1 chunk +0 lines, -17 lines 0 comments Download
M chrome_elf/blacklist/test/blacklist_test_main_dll.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +35 lines, -2 lines 0 comments Download
M chrome_elf/blacklist/test/blacklist_test_main_dll.def View 1 2 1 chunk +0 lines, -1 line 0 comments Download
D chrome_elf/breakpad.h View 1 2 3 4 5 1 chunk +0 lines, -34 lines 0 comments Download
D chrome_elf/breakpad.cc View 1 2 3 4 5 6 1 chunk +0 lines, -196 lines 0 comments Download
A + chrome_elf/breakpad/breakpad.h View 1 2 3 4 5 2 chunks +3 lines, -3 lines 0 comments Download
A + chrome_elf/breakpad/breakpad.cc View 1 2 3 4 5 6 7 chunks +23 lines, -54 lines 0 comments Download
M chrome_elf/chrome_elf.gyp View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +201 lines, -187 lines 0 comments Download
M chrome_elf/chrome_elf_constants.h View 2 chunks +17 lines, -0 lines 0 comments Download
M chrome_elf/chrome_elf_constants.cc View 2 chunks +12 lines, -0 lines 0 comments Download
M chrome_elf/chrome_elf_main.cc View 1 2 3 4 5 6 1 chunk +1 line, -2 lines 0 comments Download
M chrome_elf/chrome_elf_util_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 7 chunks +188 lines, -66 lines 0 comments Download
A + chrome_elf/hook_util/thunk_getter.h View 1 2 3 4 5 2 chunks +3 lines, -3 lines 0 comments Download
A + chrome_elf/hook_util/thunk_getter.cc View 1 2 3 4 5 2 chunks +5 lines, -2 lines 0 comments Download
A chrome_elf/nt_registry/BUILD.gn View 1 2 3 4 5 6 7 1 chunk +20 lines, -0 lines 0 comments Download
A chrome_elf/nt_registry/DEPS View 1 2 3 4 5 6 7 1 chunk +8 lines, -0 lines 0 comments Download
A chrome_elf/nt_registry/nt_registry.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +204 lines, -0 lines 0 comments Download
A chrome_elf/nt_registry/nt_registry.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +685 lines, -0 lines 0 comments Download
A chrome_elf/nt_registry/nt_registry.gyp View 1 2 3 4 5 6 7 8 9 10 1 chunk +51 lines, -0 lines 0 comments Download
D chrome_elf/thunk_getter.h View 1 2 3 4 5 1 chunk +0 lines, -16 lines 0 comments Download
D chrome_elf/thunk_getter.cc View 1 2 3 4 5 1 chunk +0 lines, -149 lines 0 comments Download
M sandbox/win/src/nt_internals.h View 1 2 3 4 5 3 chunks +70 lines, -23 lines 0 comments Download

Messages

Total messages: 62 (31 generated)
penny
Hello all! See summary - this is one of five CLs. This one in particular ...
4 years, 8 months ago (2016-04-12 17:23:29 UTC) #4
Paweł Hajdan Jr.
https://codereview.chromium.org/1841573002/diff/120001/base/test/test_reg_util_win.h File base/test/test_reg_util_win.h (right): https://codereview.chromium.org/1841573002/diff/120001/base/test/test_reg_util_win.h#newcode43 base/test/test_reg_util_win.h:43: void OverrideRegistry(HKEY override, base::string16* override_path); Could you convert all ...
4 years, 8 months ago (2016-04-12 20:41:25 UTC) #5
penny
On 2016/04/12 20:41:25, Paweł Hajdan Jr. wrote: > https://codereview.chromium.org/1841573002/diff/120001/base/test/test_reg_util_win.h > File base/test/test_reg_util_win.h (right): > > ...
4 years, 8 months ago (2016-04-15 17:48:10 UTC) #6
Paweł Hajdan Jr.
base/test LGTM
4 years, 8 months ago (2016-04-18 13:42:00 UTC) #7
robertshield
Sorry for the delay on this, I'm half-way through and it's getting late - looks ...
4 years, 8 months ago (2016-04-20 05:16:09 UTC) #8
robertshield
Some more thoughts, getting closer to done :-) https://codereview.chromium.org/1841573002/diff/140001/chrome_elf/blacklist/test/blacklist_test.cc File chrome_elf/blacklist/test/blacklist_test.cc (right): https://codereview.chromium.org/1841573002/diff/140001/chrome_elf/blacklist/test/blacklist_test.cc#newcode142 chrome_elf/blacklist/test/blacklist_test.cc:142: ::wsprintf(path, ...
4 years, 8 months ago (2016-04-20 15:45:47 UTC) #9
robertshield
A few more quick notes, just the registry implementation left to go through. https://codereview.chromium.org/1841573002/diff/140001/chrome_elf/chrome_elf_reg.cc File ...
4 years, 8 months ago (2016-04-20 16:55:53 UTC) #10
penny
On 2016/04/20 16:55:53, robertshield wrote: > A few more quick notes, just the registry implementation ...
4 years, 7 months ago (2016-05-11 21:55:37 UTC) #12
penny
Hi Robert, All the fixes for your comments are done. Instead of using a temp ...
4 years, 6 months ago (2016-05-28 01:34:23 UTC) #22
robertshield
lgtm, minor nits https://codereview.chromium.org/1841573002/diff/360001/chrome_elf/chrome_elf.gyp File chrome_elf/chrome_elf.gyp (right): https://codereview.chromium.org/1841573002/diff/360001/chrome_elf/chrome_elf.gyp#newcode15 chrome_elf/chrome_elf.gyp:15: ##------------------------------------------------------------------------------ nit: wrap https://codereview.chromium.org/1841573002/diff/360001/chrome_elf/chrome_elf.gyp#newcode72 chrome_elf/chrome_elf.gyp:72: ##------------------------------------------------------------------------------ ...
4 years, 6 months ago (2016-06-17 13:40:03 UTC) #23
penny
Thanks very much Robert. I've uploaded nit fixes, and also made CreateRegKey behave recursively (so ...
4 years, 6 months ago (2016-06-22 23:13:15 UTC) #24
robertshield
This looks good, but it really feels like this CL is way too big. Is ...
4 years, 6 months ago (2016-06-23 02:29:36 UTC) #25
penny
Thanks for your time Robert. I've changed the recursion - and updated again with latest ...
4 years, 5 months ago (2016-06-25 21:13:44 UTC) #27
robertshield
Couple more nits coming, overall looks great, some quick comments below: On 2016/06/25 21:13:44, penny ...
4 years, 5 months ago (2016-06-26 02:10:00 UTC) #28
robertshield
Looks great, few quick nits on the createregkey implementation https://codereview.chromium.org/1841573002/diff/400001/chrome_elf/nt_registry/nt_registry.cc File chrome_elf/nt_registry/nt_registry.cc (right): https://codereview.chromium.org/1841573002/diff/400001/chrome_elf/nt_registry/nt_registry.cc#newcode24 chrome_elf/nt_registry/nt_registry.cc:24: ...
4 years, 5 months ago (2016-06-26 02:11:04 UTC) #29
Will Harris
sandbox/win rs lgtm. -- looks like this needs a rebase: chrome/browser/chrome_elf* moved in crrev.com/23c8fb7 and ...
4 years, 5 months ago (2016-06-29 15:46:43 UTC) #30
penny
Hi Robert, I've fixed the latest nits. Static/global variables are now PoD arrays (makes things ...
4 years, 5 months ago (2016-07-11 19:59:04 UTC) #33
robertshield
still, lg. One question re. ParseFullRegPath since it's a bit late and I'm a confused: ...
4 years, 5 months ago (2016-07-12 04:26:24 UTC) #34
penny
Thanks very much indeed for your careful reviews Robert. https://codereview.chromium.org/1841573002/diff/460001/chrome_elf/nt_registry/nt_registry.cc File chrome_elf/nt_registry/nt_registry.cc (right): https://codereview.chromium.org/1841573002/diff/460001/chrome_elf/nt_registry/nt_registry.cc#newcode142 chrome_elf/nt_registry/nt_registry.cc:142: ...
4 years, 5 months ago (2016-07-13 00:27:55 UTC) #35
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1841573002/480001
4 years, 5 months ago (2016-07-13 16:59:58 UTC) #38
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/217322)
4 years, 5 months ago (2016-07-13 17:10:38 UTC) #40
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1841573002/480001
4 years, 5 months ago (2016-07-13 17:56:10 UTC) #43
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/217390)
4 years, 5 months ago (2016-07-13 18:06:01 UTC) #45
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1841573002/480001
4 years, 5 months ago (2016-07-13 18:13:07 UTC) #48
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/217417)
4 years, 5 months ago (2016-07-13 18:24:05 UTC) #51
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1841573002/500001
4 years, 5 months ago (2016-07-13 21:11:11 UTC) #55
commit-bot: I haz the power
Your CL relies on deprecated CQ feature(s): * Specifying master names in CQ_INCLUDE_TRYBOTS part of ...
4 years, 5 months ago (2016-07-13 21:11:14 UTC) #56
commit-bot: I haz the power
Committed patchset #15 (id:500001)
4 years, 5 months ago (2016-07-13 22:35:52 UTC) #58
commit-bot: I haz the power
CQ bit was unchecked.
4 years, 5 months ago (2016-07-13 22:36:31 UTC) #59
commit-bot: I haz the power
Patchset 15 (id:??) landed as https://crrev.com/84fd669f74832ccab02fcbb8b96dc4ed58d11b43 Cr-Commit-Position: refs/heads/master@{#405307}
4 years, 5 months ago (2016-07-13 22:37:13 UTC) #61
Will Harris
4 years, 5 months ago (2016-07-13 23:01:16 UTC) #62
Message was sent while issue was closed.
On 2016/07/13 22:37:13, commit-bot: I haz the power wrote:
> Patchset 15 (id:??) landed as
> https://crrev.com/84fd669f74832ccab02fcbb8b96dc4ed58d11b43
> Cr-Commit-Position: refs/heads/master@{#405307}

🎆🎉!

Powered by Google App Engine
This is Rietveld 408576698