Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2871863002:
adds the security cookie check hook transform. (Closed)
Descriptionadds the security cookie check hook transform.
This transform redirects the __report_gsfailure function to a code-block
that crashes the process with an exception an EH can catch
('mov [deadbeef], 0'). This is particularly useful for fuzzers using
an exception handler in the target to monitor for crashes
(__report_gsfailure triggers an exception that can't be caught with an
EH).
BUG=
R=chrisha@chromium.org
Review-Url: https://codereview.chromium.org/2871863002
Committed: https://github.com/google/syzygy/commit/8a26461de5bc099059fc2078db9c083c0882f7d1
Patch Set 1 #
Total comments: 36
Patch Set 2 : address Chris' feedback #Patch Set 3 : forgot two periods #
Total comments: 10
Patch Set 4 : address another round of comments #
Total comments: 2
Patch Set 5 : removed the DCHECK_EQ #
Messages
Total messages: 22 (13 generated)
The CQ bit was checked by axelscht@gmail.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-syzygy-committers". Note that this has nothing to do with OWNERS files.
The CQ bit was unchecked by axelscht@gmail.com
Overall the code is very clean. I mostly have comments about coding style. Note that you can run "git cl format" from within a working branch and almost all nits (indents, alignment, spacing, & and * placement, etc) will be fixed automatically for you. Good habit is to "git cl format" right before you "git commit" anything. https://codereview.chromium.org/2871863002/diff/1/syzygy/instrument/instrumen... File syzygy/instrument/instrument.gyp (right): https://codereview.chromium.org/2871863002/diff/1/syzygy/instrument/instrumen... syzygy/instrument/instrument.gyp:75: 'transforms/security_cookie_check_hook_transform.h', Move these lines to before thunk_import_references_transform.* (alphabetical order) https://codereview.chromium.org/2871863002/diff/1/syzygy/instrument/instrumen... syzygy/instrument/instrument.gyp:141: 'transforms/security_cookie_check_hook_transform_unittest.cc', Ditto. https://codereview.chromium.org/2871863002/diff/1/syzygy/instrument/transform... File syzygy/instrument/transforms/security_cookie_check_hook_transform.cc (right): https://codereview.chromium.org/2871863002/diff/1/syzygy/instrument/transform... syzygy/instrument/transforms/security_cookie_check_hook_transform.cc:32: BlockGraph::Block* header_block Indent parameters another +2 (4 from start of line) https://codereview.chromium.org/2871863002/diff/1/syzygy/instrument/transform... syzygy/instrument/transforms/security_cookie_check_hook_transform.cc:33: ) { Move to the previous line. https://codereview.chromium.org/2871863002/diff/1/syzygy/instrument/transform... syzygy/instrument/transforms/security_cookie_check_hook_transform.cc:34: BlockGraph::Block *__report_gsfailure = nullptr; Move * to left (BlockGraph::Block* __report_gsfailure) Also, double underscore symbols are typically reserved names. May as well just call this 'report_gsfailure'. https://codereview.chromium.org/2871863002/diff/1/syzygy/instrument/transform... syzygy/instrument/transforms/security_cookie_check_hook_transform.cc:35: const BlockGraph::BlockMap &blocks = block_graph->blocks(); Move & to left. https://codereview.chromium.org/2871863002/diff/1/syzygy/instrument/transform... syzygy/instrument/transforms/security_cookie_check_hook_transform.cc:36: for (const auto &block : blocks) { Move & to left (auto& block) https://codereview.chromium.org/2871863002/diff/1/syzygy/instrument/transform... syzygy/instrument/transforms/security_cookie_check_hook_transform.cc:38: if (name == "__report_gsfailure") { Make an anonymous namespaced static global at the top of this file with this string value rather than repeating it everywhere: constexpr char kReportGsFailure[] = "__report_gsfailure"; https://codereview.chromium.org/2871863002/diff/1/syzygy/instrument/transform... syzygy/instrument/transforms/security_cookie_check_hook_transform.cc:39: __report_gsfailure = block_graph->GetBlockById(block.first); You've already got the block, no need to go through the block_graph to ask for it again. report_gsfailure = &block.second; https://codereview.chromium.org/2871863002/diff/1/syzygy/instrument/transform... syzygy/instrument/transforms/security_cookie_check_hook_transform.cc:49: LOG(INFO) << "Found a __report_gsfailure implementation, hooking it now.."; Remove extra period. https://codereview.chromium.org/2871863002/diff/1/syzygy/instrument/transform... syzygy/instrument/transforms/security_cookie_check_hook_transform.cc:52: pe::kCodeCharacteristics Both of these parameters fit on one line, indented +4 from the previous line (so +6 overall). https://codereview.chromium.org/2871863002/diff/1/syzygy/instrument/transform... syzygy/instrument/transforms/security_cookie_check_hook_transform.cc:53: ); Move to end of previous line. https://codereview.chromium.org/2871863002/diff/1/syzygy/instrument/transform... syzygy/instrument/transforms/security_cookie_check_hook_transform.cc:55: // All of the below is needed to build the instrumentation via the assembler Missing period on comment. https://codereview.chromium.org/2871863002/diff/1/syzygy/instrument/transform... syzygy/instrument/transforms/security_cookie_check_hook_transform.cc:63: 0 These likely all fit on one line. https://codereview.chromium.org/2871863002/diff/1/syzygy/instrument/transform... syzygy/instrument/transforms/security_cookie_check_hook_transform.cc:64: ); Move to end of previous line. https://codereview.chromium.org/2871863002/diff/1/syzygy/instrument/transform... syzygy/instrument/transforms/security_cookie_check_hook_transform.cc:66: BasicCodeBlock* bb = bbsg.AddBasicCodeBlock("__my_report_gsfailure"); Prefix with __syzygy_ maybe? Slightly more consistent with other transforms. https://codereview.chromium.org/2871863002/diff/1/syzygy/instrument/transform... syzygy/instrument/transforms/security_cookie_check_hook_transform.cc:71: assm::eax Indent another 2 spaces. https://codereview.chromium.org/2871863002/diff/1/syzygy/instrument/transform... syzygy/instrument/transforms/security_cookie_check_hook_transform.cc:72: ); Move to previous line. https://codereview.chromium.org/2871863002/diff/1/syzygy/instrument/transform... syzygy/instrument/transforms/security_cookie_check_hook_transform.cc:74: // Condense into a block Missing period on comment. https://codereview.chromium.org/2871863002/diff/1/syzygy/instrument/transform... syzygy/instrument/transforms/security_cookie_check_hook_transform.cc:77: LOG(ERROR) << "Failed to build __my_report_gsfailure block."; Create another string constant with this name and use it here as well? https://codereview.chromium.org/2871863002/diff/1/syzygy/instrument/transform... syzygy/instrument/transforms/security_cookie_check_hook_transform.cc:81: // Exactly one new block should have been created This should be a DCHECK (kind of like an assertion) rather than a runtime validation. No need for the comment as its self-documenting: DCHECK_EQ(1u, block_builder.new_blocks().size()); https://codereview.chromium.org/2871863002/diff/1/syzygy/instrument/transform... syzygy/instrument/transforms/security_cookie_check_hook_transform.cc:87: if (__report_gsfailure->references().size() != 1) { Ditto with this as a DCHECK assertion. https://codereview.chromium.org/2871863002/diff/1/syzygy/instrument/transform... syzygy/instrument/transforms/security_cookie_check_hook_transform.cc:96: BlockGraph::Block::kTransferInternalReferences Indent all of these another 2 spaces, try to fit as many parameters on a line as possible. https://codereview.chromium.org/2871863002/diff/1/syzygy/instrument/transform... syzygy/instrument/transforms/security_cookie_check_hook_transform.cc:97: ); Move to end of previous line. https://codereview.chromium.org/2871863002/diff/1/syzygy/instrument/transform... File syzygy/instrument/transforms/security_cookie_check_hook_transform.h (right): https://codereview.chromium.org/2871863002/diff/1/syzygy/instrument/transform... syzygy/instrument/transforms/security_cookie_check_hook_transform.h:39: typedef block_graph::BlockBuilder BlockBuilder; nit: We have a tendency to keep such typedefs in alphabetical order. https://codereview.chromium.org/2871863002/diff/1/syzygy/instrument/transform... syzygy/instrument/transforms/security_cookie_check_hook_transform.h:42: public block_graph::transforms::NamedBlockGraphTransformImpl< Bring the : to the next line and indent +4: class SecurityCookieCheckHookTransform : public block_graph::transforms::... https://codereview.chromium.org/2871863002/diff/1/syzygy/instrument/transform... syzygy/instrument/transforms/security_cookie_check_hook_transform.h:45: public: Indent +1 https://codereview.chromium.org/2871863002/diff/1/syzygy/instrument/transform... syzygy/instrument/transforms/security_cookie_check_hook_transform.h:46: SecurityCookieCheckHookTransform() { } No spaces in curly braces: {} https://codereview.chromium.org/2871863002/diff/1/syzygy/instrument/transform... syzygy/instrument/transforms/security_cookie_check_hook_transform.h:50: // BlockGraphTransformInterface Implementation ubernit: s/Implementation/implementation./ https://codereview.chromium.org/2871863002/diff/1/syzygy/instrument/transform... syzygy/instrument/transforms/security_cookie_check_hook_transform.h:52: BlockGraph* block_graph, Align these two lines with 'const'. https://codereview.chromium.org/2871863002/diff/1/syzygy/instrument/transform... File syzygy/instrument/transforms/security_cookie_check_hook_transform_unittest.cc (right): https://codereview.chromium.org/2871863002/diff/1/syzygy/instrument/transform... syzygy/instrument/transforms/security_cookie_check_hook_transform_unittest.cc:41: : public SecurityCookieCheckHookTransform { You've no need to expose anything via this inheritance, so simply test the original class directly. https://codereview.chromium.org/2871863002/diff/1/syzygy/instrument/transform... syzygy/instrument/transforms/security_cookie_check_hook_transform_unittest.cc:62: if (block.name() != "__my_report_gsfailure") Expose the string as a class static member in the transform, and refer to that instead of duplicating it here. https://codereview.chromium.org/2871863002/diff/1/syzygy/instrument/transform... syzygy/instrument/transforms/security_cookie_check_hook_transform_unittest.cc:79: // Check if the stub is a 'mov [deadbeef], eax' instruction Missing period on comment. https://codereview.chromium.org/2871863002/diff/1/syzygy/instrument/transform... syzygy/instrument/transforms/security_cookie_check_hook_transform_unittest.cc:94: } Instead of using the disassembler and everything, it would be totally fine to simply check the bytes match the expected assembly directly. https://codereview.chromium.org/2871863002/diff/1/syzygy/instrument/transform... syzygy/instrument/transforms/security_cookie_check_hook_transform_unittest.cc:107: &block_graph_, header_block_ Indent these lines another 2 spaces. https://codereview.chromium.org/2871863002/diff/1/syzygy/instrument/transform... syzygy/instrument/transforms/security_cookie_check_hook_transform_unittest.cc:108: )); Move to end of previous line.
Just a minor comment regarding communication on code reviews: We address each point one by one directly in the web tool, responding with a short "Done." for things that have been fixed, and otherwise explaining decisions made where appropriate. A handy record of the full review process. Please respond to further reviews directly here instead of in an offline email.
Another round of comments... nearly there :) https://codereview.chromium.org/2871863002/diff/40001/syzygy/instrument/instr... File syzygy/instrument/instrument.gyp (right): https://codereview.chromium.org/2871863002/diff/40001/syzygy/instrument/instr... syzygy/instrument/instrument.gyp:75: 'transforms/thunk_import_references_transform.cc', .cc before .h (alphabetical) https://codereview.chromium.org/2871863002/diff/40001/syzygy/instrument/trans... File syzygy/instrument/transforms/security_cookie_check_hook_transform.cc (right): https://codereview.chromium.org/2871863002/diff/40001/syzygy/instrument/trans... syzygy/instrument/transforms/security_cookie_check_hook_transform.cc:68: assm.mov(Operand(Displacement(0xdeadbeef)), assm::eax); Useful to expose this as another class constant, given that you then test against this value in the unittest? https://codereview.chromium.org/2871863002/diff/40001/syzygy/instrument/trans... syzygy/instrument/transforms/security_cookie_check_hook_transform.cc:80: if (report_gsfailure->referrers().size() != 1) { You could move this check up earlier, before building the new gsfailure replacement? https://codereview.chromium.org/2871863002/diff/40001/syzygy/instrument/trans... syzygy/instrument/transforms/security_cookie_check_hook_transform.cc:85: LOG(ERROR) << "Only a single referrer is expected."; Better: "Only a single referrer to __report_gsfailure is expected." https://codereview.chromium.org/2871863002/diff/40001/syzygy/instrument/trans... File syzygy/instrument/transforms/security_cookie_check_hook_transform_unittest.cc (right): https://codereview.chromium.org/2871863002/diff/40001/syzygy/instrument/trans... syzygy/instrument/transforms/security_cookie_check_hook_transform_unittest.cc:88: // mov [deadbeef], eax Check that the operands of the mov are what you expect as well?
Thanks Chris for the great feedback. I've addressed your comments in PatchSet 4. Cheers https://codereview.chromium.org/2871863002/diff/40001/syzygy/instrument/instr... File syzygy/instrument/instrument.gyp (right): https://codereview.chromium.org/2871863002/diff/40001/syzygy/instrument/instr... syzygy/instrument/instrument.gyp:75: 'transforms/thunk_import_references_transform.cc', On 2017/05/10 15:12:08, chrisha wrote: > .cc before .h (alphabetical) Done. https://codereview.chromium.org/2871863002/diff/40001/syzygy/instrument/trans... File syzygy/instrument/transforms/security_cookie_check_hook_transform.cc (right): https://codereview.chromium.org/2871863002/diff/40001/syzygy/instrument/trans... syzygy/instrument/transforms/security_cookie_check_hook_transform.cc:68: assm.mov(Operand(Displacement(0xdeadbeef)), assm::eax); On 2017/05/10 15:12:08, chrisha wrote: > Useful to expose this as another class constant, given that you then test > against this value in the unittest? Done. https://codereview.chromium.org/2871863002/diff/40001/syzygy/instrument/trans... syzygy/instrument/transforms/security_cookie_check_hook_transform.cc:80: if (report_gsfailure->referrers().size() != 1) { On 2017/05/10 15:12:08, chrisha wrote: > You could move this check up earlier, before building the new gsfailure > replacement? Done. https://codereview.chromium.org/2871863002/diff/40001/syzygy/instrument/trans... syzygy/instrument/transforms/security_cookie_check_hook_transform.cc:85: LOG(ERROR) << "Only a single referrer is expected."; On 2017/05/10 15:12:08, chrisha wrote: > Better: "Only a single referrer to __report_gsfailure is expected." Done. https://codereview.chromium.org/2871863002/diff/40001/syzygy/instrument/trans... File syzygy/instrument/transforms/security_cookie_check_hook_transform_unittest.cc (right): https://codereview.chromium.org/2871863002/diff/40001/syzygy/instrument/trans... syzygy/instrument/transforms/security_cookie_check_hook_transform_unittest.cc:88: // mov [deadbeef], eax On 2017/05/10 15:12:08, chrisha wrote: > Check that the operands of the mov are what you expect as well? Done.
lgtm modulo one last nit. Feel free to fix, upload and flip the CQ bit after that. https://codereview.chromium.org/2871863002/diff/60001/syzygy/instrument/trans... File syzygy/instrument/transforms/security_cookie_check_hook_transform.cc (right): https://codereview.chromium.org/2871863002/diff/60001/syzygy/instrument/trans... syzygy/instrument/transforms/security_cookie_check_hook_transform.cc:59: if (report_gsfailure->referrers().size() != 1) { (Sorry, didn't catch this before.) No need for both a DCHECK and a LOG(ERROR). Since this is a condition that can occur due to external data provided to the program, the LOG(ERROR) is appropriate. A DCHECK is appropriate for assertions about the behaviour of our own code that would indicate a logical error and/or programmer misuse of an API. So please remove the DCHECK.
Addressed the last comment :). Thanks again Chris! https://codereview.chromium.org/2871863002/diff/60001/syzygy/instrument/trans... File syzygy/instrument/transforms/security_cookie_check_hook_transform.cc (right): https://codereview.chromium.org/2871863002/diff/60001/syzygy/instrument/trans... syzygy/instrument/transforms/security_cookie_check_hook_transform.cc:59: if (report_gsfailure->referrers().size() != 1) { On 2017/05/10 20:18:42, chrisha wrote: > (Sorry, didn't catch this before.) > > No need for both a DCHECK and a LOG(ERROR). Since this is a condition that can > occur due to external data provided to the program, the LOG(ERROR) is > appropriate. > > A DCHECK is appropriate for assertions about the behaviour of our own code that > would indicate a logical error and/or programmer misuse of an API. > > So please remove the DCHECK. No worries - I was a bit confused about it too :). Done!
The CQ bit was checked by axelscht@gmail.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by axelscht@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from chrisha@chromium.org Link to the patchset: https://codereview.chromium.org/2871863002/#ps80001 (title: "removed the DCHECK_EQ")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1494452173642040,
"parent_rev": "dbb218b6d05ff1c17a4d86252b10880c2d8ebe08", "commit_rev":
"8a26461de5bc099059fc2078db9c083c0882f7d1"}
Description was changed from
==========
adds the security cookie check hook transform.
This transform redirects the __report_gsfailure function to a code-block
that crashes the process with an exception an EH can catch
('mov [deadbeef], 0'). This is particularly useful for fuzzers using
an exception handler in the target to monitor for crashes
(__report_gsfailure triggers an exception that can't be caught with an
EH).
BUG=
R=chrisha@chromium.org
==========
to
==========
adds the security cookie check hook transform.
This transform redirects the __report_gsfailure function to a code-block
that crashes the process with an exception an EH can catch
('mov [deadbeef], 0'). This is particularly useful for fuzzers using
an exception handler in the target to monitor for crashes
(__report_gsfailure triggers an exception that can't be caught with an
EH).
BUG=
R=chrisha@chromium.org
Review-Url: https://codereview.chromium.org/2871863002
Committed:
https://github.com/google/syzygy/commit/8a26461de5bc099059fc2078db9c083c0882f7d1
==========
Committed patchset #5 (id:80001) as https://github.com/google/syzygy/commit/8a26461de5bc099059fc2078db9c083c0882f7d1
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://github.com/google/syzygy/commit/8a26461de5bc099059fc2078db9c083c0882f7d1 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
