Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2683823004:
Protect heap metadata in Oilpan. (Closed)
DescriptionProtect heap metadata in Oilpan.
Rather than checking a static 16-bit magic value to check a HeapObjectHeader's
integrity when DCHECK_IS_ON, use a random 32-bit canary value in all builds.
Place it before the HeapObjectHeader value, rather than after, so that linear
overwrites (e.g. from OOB writes starting from lower addresses) are more likely
to corrupt it.
This is not a complete fix for 633030; more work on other heap metadata is
coming next. This is the simplest possible start. In particular, in future work
we may xor the |m_encoded| field with the canary as well, to obfuscate its
meaning to attackers using an infoleak to learn about the Oilpan heap.
BUG=633030
Review-Url: https://codereview.chromium.org/2683823004
Cr-Commit-Position: refs/heads/master@{#450598}
Committed: https://chromium.googlesource.com/chromium/src/+/40cd3c13c54f9aa73fc0b47e59a7d962037e2f94
Patch Set 1 #Patch Set 2 : remove unnecessary 4-byte padding. #
Total comments: 3
Patch Set 3 : Move the implementation of |getMagic| down. #Patch Set 4 : Unconditionally assert 8-byte HOH size. #Messages
Total messages: 24 (14 generated)
The CQ bit was checked by palmer@chromium.org 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: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
Description was changed from
==========
Use checkHeader in production builds.
BUG=633030
==========
to
==========
Protect heap metadata in Oilpan.
Rather than checking a static 16-bit magic value to check a HeapObjectHeader's
integrity when DCHECK_IS_ON, use a random 32-bit canary value in all builds.
Place it before the HeapObjectHeader value, rather than after, so that linear
overwrites (e.g. from OOB writes starting from lower addresses) are more likely
to corrupt it.
This is not a complete fix for 633030; more work on other heap metadata is
coming next. This is the simplest possible start. In particular, in future work
we may xor the |m_encoded| field with the canary as well, to obfuscate its
meaning to attackers using an infoleak to learn about the Oilpan heap.
BUG=633030
==========
The CQ bit was checked by palmer@chromium.org 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.
palmer@chromium.org changed reviewers: + haraken@chromium.org, jschuh@chromium.org
Here's the simplest possible version of a metadata canary CL. (I closed the previous one, and abandoned some others, although ultimately I think we'll need to do more here, as I note in a TODO and in the CL description.) haraken, jschuh advises me that we need to push ahead with some form of Oilpan metadata protection, because the status quo is too severe a security regression. There is also doubt that the synthetic benchmarks (like Dromaeo) are good or relevant for real-world performance estimation. I'm trying to get the perf impact as low as possible, but it's going to be non-zero — but we have to do something. Oilpan metadata is too soft a target right now. That said, I do still think it's important to get numbers so we know where we are, and I am running some now. I think I *finally* have a basic grip on Blink benchmarking. Fingers crossed...
The change looks good. I want to see performance results for blink_perf.* and dromaeo.dom* on 64-bit and 32-bit though.
> I want to see performance results for blink_perf.* and dromaeo.dom* on 64-bit and 32-bit though. I sent you and jschuh a link to 64-bit Linux results on internal email. I'll try 32-bit Android on Monday.
sigbjornf@opera.com changed reviewers: + sigbjornf@opera.com
https://codereview.chromium.org/2683823004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/HeapPage.h (right): https://codereview.chromium.org/2683823004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/HeapPage.h:184: #if CPU(64BIT) To the extent that it is still useful, this static_assert() can now be unconditionally used. https://codereview.chromium.org/2683823004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/HeapPage.h:243: // Returns a random value. Just a syntactic issue -- could you declare getMagic() here and define it via an 'inline' below, like we do for other non-trivial methods of this class?
https://codereview.chromium.org/2683823004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/HeapPage.h (right): https://codereview.chromium.org/2683823004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/HeapPage.h:243: // Returns a random value. > Just a syntactic issue -- could you declare getMagic() here and define it via an 'inline' below, like we do for other non-trivial methods of this class? Done.
> third_party/WebKit/Source/platform/heap/HeapPage.h:184: #if CPU(64BIT) > To the extent that it is still useful, this static_assert() can now be unconditionally used. Done.
(The perf results are in Patchset 2; Patchsets 3 and 4 should have no perf impact.)
On 2017/02/14 23:03:42, palmer wrote: > (The perf results are in Patchset 2; Patchsets 3 and 4 should have no perf > impact.) Great, I don't observe any significant performance regression. LGTM.
The CQ bit was checked by palmer@chromium.org
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": 60001, "attempt_start_ts": 1487116500380240,
"parent_rev": "3ccd655f41b75f4ade6c2e760b25006fbbb0a472", "commit_rev":
"40cd3c13c54f9aa73fc0b47e59a7d962037e2f94"}
Message was sent while issue was closed.
Description was changed from
==========
Protect heap metadata in Oilpan.
Rather than checking a static 16-bit magic value to check a HeapObjectHeader's
integrity when DCHECK_IS_ON, use a random 32-bit canary value in all builds.
Place it before the HeapObjectHeader value, rather than after, so that linear
overwrites (e.g. from OOB writes starting from lower addresses) are more likely
to corrupt it.
This is not a complete fix for 633030; more work on other heap metadata is
coming next. This is the simplest possible start. In particular, in future work
we may xor the |m_encoded| field with the canary as well, to obfuscate its
meaning to attackers using an infoleak to learn about the Oilpan heap.
BUG=633030
==========
to
==========
Protect heap metadata in Oilpan.
Rather than checking a static 16-bit magic value to check a HeapObjectHeader's
integrity when DCHECK_IS_ON, use a random 32-bit canary value in all builds.
Place it before the HeapObjectHeader value, rather than after, so that linear
overwrites (e.g. from OOB writes starting from lower addresses) are more likely
to corrupt it.
This is not a complete fix for 633030; more work on other heap metadata is
coming next. This is the simplest possible start. In particular, in future work
we may xor the |m_encoded| field with the canary as well, to obfuscate its
meaning to attackers using an infoleak to learn about the Oilpan heap.
BUG=633030
Review-Url: https://codereview.chromium.org/2683823004
Cr-Commit-Position: refs/heads/master@{#450598}
Committed:
https://chromium.googlesource.com/chromium/src/+/40cd3c13c54f9aa73fc0b47e59a7...
==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/40cd3c13c54f9aa73fc0b47e59a7... |
