Do not allow values to be migrated without MACs if the destination already has a MAC.

Do not allow values to be migrated without MACs if the destination already has a MAC.

BUG=414554
TEST=Added regression test PrefHashBrowserTestUntrustedAdditionToPrefs which fails without this CL and passes with it.

Committed: https://crrev.com/b004a56cd18f7519d1cceafb534a5865f93d988d
Cr-Commit-Position: refs/heads/master@{#295048}

[gab, 2014-09-16 03:35:43]

gab@chromium.org changed reviewers:
+ bauerb@chromium.org, erikwright@chromium.org

[gab, 2014-09-16 03:35:43]

Bernhard/Erik PTAL. A one-line logic change + a regression integration test.

I want to merge this up to stable AFAP so please both take a careful look at it
and try to think of any undesired side-effects this change may have.

Thanks!
Gab

[Bernhard Bauer, 2014-09-16 08:10:54]

lgtm

https://codereview.chromium.org/550343004/diff/1/chrome/browser/prefs/tracked...
File chrome/browser/prefs/tracked/pref_hash_browsertest.cc (right):

https://codereview.chromium.org/550343004/diff/1/chrome/browser/prefs/tracked...
chrome/browser/prefs/tracked/pref_hash_browsertest.cc:793: virtual void
AttackPreferencesOnDisk(
Does this need an OVERRIDE?

[gab, 2014-09-16 12:14:22]

Thanks!

CQing for the sake of expediency, Erik PTAL as soon as you get in.

https://codereview.chromium.org/550343004/diff/1/chrome/browser/prefs/tracked...
File chrome/browser/prefs/tracked/pref_hash_browsertest.cc (right):

https://codereview.chromium.org/550343004/diff/1/chrome/browser/prefs/tracked...
chrome/browser/prefs/tracked/pref_hash_browsertest.cc:793: virtual void
AttackPreferencesOnDisk(
On 2014/09/16 08:10:54, Bernhard Bauer wrote:
> Does this need an OVERRIDE?

Yep, and it already has it, see two lines below after params ;-)!

[gab, 2014-09-16 12:14:26]

The CQ bit was checked by gab@chromium.org

[commit-bot: I haz the power, 2014-09-16 12:14:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/550343004/1

[commit-bot: I haz the power, 2014-09-16 12:46:08]

Committed patchset #1 (id:1) as 740abaa7b981f73306ff5ec9d8c744fa0bd871b3

[commit-bot: I haz the power, 2014-09-16 12:47:22]

Patchset 1 (id:??) landed as
https://crrev.com/b004a56cd18f7519d1cceafb534a5865f93d988d
Cr-Commit-Position: refs/heads/master@{#295048}

[erikwright (departed), 2014-09-16 14:16:04]

LGTM. Good catch. I don't understand why we wrote it the other way.

[gab, 2014-09-16 14:22:14]

On 2014/09/16 14:16:04, erikwright wrote:
> LGTM. Good catch. 

> I don't understand why we wrote it the other way.

Probably simply because it made intuitive sense that mac_of_migrated_value =>
mac_in_secure_pref implies that if mac_of_migrated_value is NULL,
mac_in_secure_pref should also become NULL... (and this would also have been
fine had ClearHash() not been re-validating the super MAC (which it needs to do
to support clearing MACs of deprecated values being cleaned away)...

Can you guys think of other such use cases we should have integration tests for?

[gab, 2014-09-16 14:30:14]

On 2014/09/16 14:22:14, gab wrote:
> On 2014/09/16 14:16:04, erikwright wrote:
> > LGTM. Good catch. 
> 
> > I don't understand why we wrote it the other way.

This appears to have been the case throughout
https://codereview.chromium.org/324493002; we even changed it after patch set 12
to use ClearHash() instead of supporting ImportHash(NULL) when
|!desitnation_hash_missing|:
https://codereview.chromium.org/324493002/diff/250001/chrome/browser/prefs/tr...

My bad for not spotting this, I definitely remember being puzzled by this edge
case, but hadn't realized it wasn't so much of an edge case when tampering comes
into play...

> 
> Probably simply because it made intuitive sense that mac_of_migrated_value =>
> mac_in_secure_pref implies that if mac_of_migrated_value is NULL,
> mac_in_secure_pref should also become NULL... (and this would also have been
> fine had ClearHash() not been re-validating the super MAC (which it needs to
do
> to support clearing MACs of deprecated values being cleaned away)...
> 
> Can you guys think of other such use cases we should have integration tests
for?