Remove unnecessary attempts at memory cleaning

Remove unnecessary attempts at memory cleaning

On some platforms, and inconsistently, we'd attempt to zap buffers by
writing all zeroes before freeing the memory. Besides our approach being
possible to be optimized out, it's inconsistent with the threat models
and the rest of the code.

BUG=none

Review-Url: https://codereview.chromium.org/2926333002
Cr-Commit-Position: refs/heads/master@{#478397}
Committed: https://chromium.googlesource.com/chromium/src/+/809e133889f27f882d9f4ad284a1916bcdf01d2d

[Ryan Sleevi, 2017-06-08 18:44:46]

rsleevi@chromium.org changed reviewers:
+ asanka@chromium.org, jschuh@chromium.org

[Ryan Sleevi, 2017-06-08 18:44:46]

Justin for review because I'm going to say he told me to do it if anyone asks
(... they totally did not), and to sanity check on first principles :)

Asanka for owners

[asanka, 2017-06-08 20:30:02]

lgtm.

This looks like it was meant to emulate SecureZeroMemory, of which one expected
side-effect was to reduce the incidence rate of passwords getting slurped into
crash reports.

No objections to removing Zap() and friends, esp since we don't extend the same
courtesy to web forms which are far more common.

https://blogs.msdn.microsoft.com/oldnewthing/20130529-00/?p=4223

[jschuh, 2017-06-09 16:15:35]

I took some time to ponder it a bit (nice manufacturing of blame, btw 😜). The
two things to consider here are 1) an attacker running code at equal or greater
privilege to Chrome and 2) any sensitive state the OS might leave behind due to
paging, crash logs, etc.

Case #1 is clearly outside our threat model. If the attacker is executing code
at equal or greater privilege, there's literally nothing we can do. So, there's
no point in burning cycles and adding complexity for theater.

Case #2 is a bit more nuanced. We do our best to ensure that long-lived
credentials (e.g. passwords and cookies) are protected at rest. However, we
can't do the same for credentials in memory, particularly once we hand them over
to the site's control. And even if we could reliably identify and taint-track
every credential everywhere, we can't make guarantees about what the OS might do
on our behalf, such as paging to disk or dumping crash files.

Most importantly, any guarantees about the confidentiality and integrity of
process memory really need to be provided by implicit OS mechanisms (e.g.
full-disk-encryption, ephemeral page file encryption, etc.). Since, every modern
OS can provide such mechanisms, I'm fine with dropping unreliable half measures
like this.

lgtm

[Ryan Sleevi, 2017-06-09 16:20:03]

The CQ bit was checked by rsleevi@chromium.org

[commit-bot: I haz the power, 2017-06-09 16:20:17]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-09 17:00:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-09 17:00:16]

Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ryan Sleevi, 2017-06-09 18:43:41]

The CQ bit was checked by rsleevi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-09 18:44:07]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-09 20:23:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-09 20:23:25]

Dry run: This issue passed the CQ dry run.

[Ryan Sleevi, 2017-06-09 20:23:51]

The CQ bit was checked by rsleevi@chromium.org

[Ryan Sleevi, 2017-06-09 20:23:52]

The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org,
jschuh@chromium.org
Link to the patchset: https://codereview.chromium.org/2926333002/#ps10001
(title: "Fix zeroization")

[commit-bot: I haz the power, 2017-06-09 20:24:22]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-09 20:31:25]

CQ is committing da patch.

Bot data: {"patchset_id": 10001, "attempt_start_ts": 1497039831847330,
"parent_rev": "15d1b34174529968cac05a8902c96368687a8fd4", "commit_rev":
"809e133889f27f882d9f4ad284a1916bcdf01d2d"}

[commit-bot: I haz the power, 2017-06-09 20:31:38]

Description was changed from

==========
Remove unnecessary attempts at memory cleaning

On some platforms, and inconsistently, we'd attempt to zap buffers by
writing all zeroes before freeing the memory. Besides our approach being
possible to be optimized out, it's inconsistent with the threat models
and the rest of the code.

BUG=none
==========

to

==========
Remove unnecessary attempts at memory cleaning

On some platforms, and inconsistently, we'd attempt to zap buffers by
writing all zeroes before freeing the memory. Besides our approach being
possible to be optimized out, it's inconsistent with the threat models
and the rest of the code.

BUG=none

Review-Url: https://codereview.chromium.org/2926333002
Cr-Commit-Position: refs/heads/master@{#478397}
Committed:
https://chromium.googlesource.com/chromium/src/+/809e133889f27f882d9f4ad284a1...
==========

[commit-bot: I haz the power, 2017-06-09 20:31:39]

Committed patchset #2 (id:10001) as
https://chromium.googlesource.com/chromium/src/+/809e133889f27f882d9f4ad284a1...