Chrome Settings Hardening

Chrome Settings Hardening

Introduces various settings enforcement levels when untrusted settings are detected.

Uses value-parametrized tests to test all enforcement levels:
https://code.google.com/p/googletest/wiki/AdvancedGuide#How_to_Write_Value-Parameterized_Tests

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=242967

[gab, 2013-12-31 01:24:51]

Erik, Berhard, and Alexei. PTAL.

Thanks!
Gab

[Bernhard Bauer, 2014-01-02 09:17:16]

LGTM!

[gab, 2014-01-02 13:33:13]

Mark, can you PTAL @ histograms, it appears Alexei is OOO.

Thanks!
Gab

[Mark P, 2014-01-02 18:26:25]

histograms.xml lgtm

[gab, 2014-01-02 18:40:22]

@mpearson: see question below.

Thanks!
Gab

https://codereview.chromium.org/122653005/diff/50001/chrome/browser/prefs/chr...
File chrome/browser/prefs/chrome_pref_service_factory.cc (right):

https://codereview.chromium.org/122653005/diff/50001/chrome/browser/prefs/chr...
chrome/browser/prefs/chrome_pref_service_factory.cc:73: const size_t
kTrackedPrefsReportingIDsCount = 14;
@erikwright pointed out that this might not be necessary.

I added it so that the same count is reported on every platform (e.g., if the
last kTrackedPref is ifdef'ed out on some platforms).

Is this required that the count reported via the histogram macros be the same
across all platforms (or even across multiple calls to the same histogram -- see
pref_hash_filter.cc for the histograms themselves)?

[Mark P, 2014-01-02 19:32:46]

https://codereview.chromium.org/122653005/diff/50001/chrome/browser/prefs/chr...
File chrome/browser/prefs/chrome_pref_service_factory.cc (right):

https://codereview.chromium.org/122653005/diff/50001/chrome/browser/prefs/chr...
chrome/browser/prefs/chrome_pref_service_factory.cc:73: const size_t
kTrackedPrefsReportingIDsCount = 14;
On 2014/01/02 18:40:22, gab wrote:
> @erikwright pointed out that this might not be necessary.
> 
> I added it so that the same count is reported on every platform (e.g., if the
> last kTrackedPref is ifdef'ed out on some platforms).
> 
> Is this required that the count reported via the histogram macros be the same
> across all platforms (or even across multiple calls to the same histogram --
see
> pref_hash_filter.cc for the histograms themselves)?

erikwright@ is correct that this is not strictly necessary.  As long as your
emissions to your UMA enum histogram are in the valid range (i.e., the max
number you emit on a particular platform is less than the number of buckets you
pass to the UMA_HISTOGRAM_ENUMERATION macro), then you should be fine.  This
appears to be the case.

*However*, I'd prefer you keep the code as-is, using the same number of buckets
on every platform.  The dashboard code uses conflicting buckets for error
detection.  Most of your clients will emit to buckets such as [1, 2), [12, 13),
etc.  You'll never normally hit the overflow [14, infinity) bucket.  But suppose
one of your clients on a platform that uses a smaller maximum bucket has a bit
error or corrupted memory or something and emits to the overflow bucket.  Now
the dashboard will see a [12, infinity) count along with a set of normal [12,
13) counts.  I'm not sure how smart it'll be to handle this case, and even if it
does handle it well now, it might cause issues with our plans for better
removing bad data in the future.  In other words, I'd feel more comfortable if
the histogram used the same bucketing on all platforms.

[erikwright (departed), 2014-01-02 19:34:48]

On 2014/01/02 19:32:46, Mark P wrote:
>
https://codereview.chromium.org/122653005/diff/50001/chrome/browser/prefs/chr...
> File chrome/browser/prefs/chrome_pref_service_factory.cc (right):
> 
>
https://codereview.chromium.org/122653005/diff/50001/chrome/browser/prefs/chr...
> chrome/browser/prefs/chrome_pref_service_factory.cc:73: const size_t
> kTrackedPrefsReportingIDsCount = 14;
> On 2014/01/02 18:40:22, gab wrote:
> > @erikwright pointed out that this might not be necessary.
> > 
> > I added it so that the same count is reported on every platform (e.g., if
the
> > last kTrackedPref is ifdef'ed out on some platforms).
> > 
> > Is this required that the count reported via the histogram macros be the
same
> > across all platforms (or even across multiple calls to the same histogram --
> see
> > pref_hash_filter.cc for the histograms themselves)?
> 
> erikwright@ is correct that this is not strictly necessary.  As long as your
> emissions to your UMA enum histogram are in the valid range (i.e., the max
> number you emit on a particular platform is less than the number of buckets
you
> pass to the UMA_HISTOGRAM_ENUMERATION macro), then you should be fine.  This
> appears to be the case.
> 
> *However*, I'd prefer you keep the code as-is, using the same number of
buckets
> on every platform.  The dashboard code uses conflicting buckets for error
> detection.  Most of your clients will emit to buckets such as [1, 2), [12,
13),
> etc.  You'll never normally hit the overflow [14, infinity) bucket.  But
suppose
> one of your clients on a platform that uses a smaller maximum bucket has a bit
> error or corrupted memory or something and emits to the overflow bucket.  Now
> the dashboard will see a [12, infinity) count along with a set of normal [12,
> 13) counts.  I'm not sure how smart it'll be to handle this case, and even if
it
> does handle it well now, it might cause issues with our plans for better
> removing bad data in the future.  In other words, I'd feel more comfortable if
> the histogram used the same bucketing on all platforms.

But won't this situation necessarily occur when we add new values to the enum?
Which we will definitely do?

[Mark P, 2014-01-02 20:07:24]

On 2014/01/02 19:34:48, erikwright wrote:
> On 2014/01/02 19:32:46, Mark P wrote:
> >
>
https://codereview.chromium.org/122653005/diff/50001/chrome/browser/prefs/chr...
> > File chrome/browser/prefs/chrome_pref_service_factory.cc (right):
> > 
> >
>
https://codereview.chromium.org/122653005/diff/50001/chrome/browser/prefs/chr...
> > chrome/browser/prefs/chrome_pref_service_factory.cc:73: const size_t
> > kTrackedPrefsReportingIDsCount = 14;
> > On 2014/01/02 18:40:22, gab wrote:
> > > @erikwright pointed out that this might not be necessary.
> > > 
> > > I added it so that the same count is reported on every platform (e.g., if
> the
> > > last kTrackedPref is ifdef'ed out on some platforms).
> > > 
> > > Is this required that the count reported via the histogram macros be the
> same
> > > across all platforms (or even across multiple calls to the same histogram
--
> > see
> > > pref_hash_filter.cc for the histograms themselves)?
> > 
> > erikwright@ is correct that this is not strictly necessary.  As long as your
> > emissions to your UMA enum histogram are in the valid range (i.e., the max
> > number you emit on a particular platform is less than the number of buckets
> you
> > pass to the UMA_HISTOGRAM_ENUMERATION macro), then you should be fine.  This
> > appears to be the case.
> > 
> > *However*, I'd prefer you keep the code as-is, using the same number of
> buckets
> > on every platform.  The dashboard code uses conflicting buckets for error
> > detection.  Most of your clients will emit to buckets such as [1, 2), [12,
> 13),
> > etc.  You'll never normally hit the overflow [14, infinity) bucket.  But
> suppose
> > one of your clients on a platform that uses a smaller maximum bucket has a
bit
> > error or corrupted memory or something and emits to the overflow bucket. 
Now
> > the dashboard will see a [12, infinity) count along with a set of normal
[12,
> > 13) counts.  I'm not sure how smart it'll be to handle this case, and even
if
> it
> > does handle it well now, it might cause issues with our plans for better
> > removing bad data in the future.  In other words, I'd feel more comfortable
if
> > the histogram used the same bucketing on all platforms.
> 
> But won't this situation necessarily occur when we add new values to the enum?
> Which we will definitely do?

Yeah, you're right.  I guess my brain isn't back from vacation yet. :-|
But still in that case the buckets are consistent per Chrome version.
I'll still reiterate my earlier comment: I think the suggestion of dropping the
explicit max and letting it vary by platforms works, but it leaves a bad taste
in my mouth.  You might find other opinions from other UMA team members.

--mark

[erikwright (departed), 2014-01-02 20:16:09]

OK. It's my feeling that manually keeping this "max" up to date with the
platform variable enum and passing it around has its own downsides in terms
of code complexity/clarity and API. It places a data consistency burden on
the client of PrefHashFilter which we are either forced to validate inside
PrefHashFilter (rendering the passing of the value more or less irrelevant)
or not validate (opening ourselves up to buggy results if a client passes
invalid data).

I'm also concerned that it can lead to misunderstanding of the purpose of
the bucket sizes, ultimately perpetuating Gab's original misunderstanding
of the purpose/implications of the value.

If Mark's recommendation is the official UMA recommendation we should of
course take it. But if it's just personal opinion I would ask that we
address uma-team@ or another appropriate list for guidance.


On Thu, Jan 2, 2014 at 3:07 PM, <mpearson@chromium.org> wrote:

> On 2014/01/02 19:34:48, erikwright wrote:
>
>> On 2014/01/02 19:32:46, Mark P wrote:
>> >
>>
>
> https://codereview.chromium.org/122653005/diff/50001/
> chrome/browser/prefs/chrome_pref_service_factory.cc
>
>> > File chrome/browser/prefs/chrome_pref_service_factory.cc (right):
>> >
>> >
>>
>
> https://codereview.chromium.org/122653005/diff/50001/
> chrome/browser/prefs/chrome_pref_service_factory.cc#newcode73
>
>> > chrome/browser/prefs/chrome_pref_service_factory.cc:73: const size_t
>> > kTrackedPrefsReportingIDsCount = 14;
>> > On 2014/01/02 18:40:22, gab wrote:
>> > > @erikwright pointed out that this might not be necessary.
>> > >
>> > > I added it so that the same count is reported on every platform
>> (e.g., if
>> the
>> > > last kTrackedPref is ifdef'ed out on some platforms).
>> > >
>> > > Is this required that the count reported via the histogram macros be
>> the
>> same
>> > > across all platforms (or even across multiple calls to the same
>> histogram
>>
> --
>
>> > see
>> > > pref_hash_filter.cc for the histograms themselves)?
>> >
>> > erikwright@ is correct that this is not strictly necessary.  As long
>> as your
>> > emissions to your UMA enum histogram are in the valid range (i.e., the
>> max
>> > number you emit on a particular platform is less than the number of
>> buckets
>> you
>> > pass to the UMA_HISTOGRAM_ENUMERATION macro), then you should be fine.
>>  This
>> > appears to be the case.
>> >
>> > *However*, I'd prefer you keep the code as-is, using the same number of
>> buckets
>> > on every platform.  The dashboard code uses conflicting buckets for
>> error
>> > detection.  Most of your clients will emit to buckets such as [1, 2),
>> [12,
>> 13),
>> > etc.  You'll never normally hit the overflow [14, infinity) bucket.  But
>> suppose
>> > one of your clients on a platform that uses a smaller maximum bucket
>> has a
>>
> bit
>
>> > error or corrupted memory or something and emits to the overflow bucket.
>>
> Now
>
>> > the dashboard will see a [12, infinity) count along with a set of normal
>>
> [12,
>
>> > 13) counts.  I'm not sure how smart it'll be to handle this case, and
>> even
>>
> if
>
>> it
>> > does handle it well now, it might cause issues with our plans for better
>> > removing bad data in the future.  In other words, I'd feel more
>> comfortable
>>
> if
>
>> > the histogram used the same bucketing on all platforms.
>>
>
>  But won't this situation necessarily occur when we add new values to the
>> enum?
>> Which we will definitely do?
>>
>
> Yeah, you're right.  I guess my brain isn't back from vacation yet. :-|
> But still in that case the buckets are consistent per Chrome version.
> I'll still reiterate my earlier comment: I think the suggestion of
> dropping the
> explicit max and letting it vary by platforms works, but it leaves a bad
> taste
> in my mouth.  You might find other opinions from other UMA team members.
>
> --mark
>
>
> https://codereview.chromium.org/122653005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[erikwright (departed), 2014-01-03 19:08:13]

https://codereview.chromium.org/122653005/diff/50001/chrome/browser/prefs/chr...
File chrome/browser/prefs/chrome_pref_service_factory.cc (right):

https://codereview.chromium.org/122653005/diff/50001/chrome/browser/prefs/chr...
chrome/browser/prefs/chrome_pref_service_factory.cc:80:
PrefHashFilter::EnforcementLevel level;
nit: I would reverse this since, intuitively, a map maps from the first field to
the second (key-value, not "value-key").

Also, consider using an actual map like:

std::map<string, level> m;
m.insert("a", A);
m.insert("b", B);

if (trial) {
  map::iterator it = m.find(group_name);
  if (it != m.end())
    return *it;
}
return default;

I'm not sure if it will be more or less clear. Slightly less efficient (NlogN?)
but N is small and this is only done once per profile I think, so code
simplicity should dominate.

https://codereview.chromium.org/122653005/diff/300001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_hash_filter.cc (right):

https://codereview.chromium.org/122653005/diff/300001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_filter.cc:78: DCHECK(pref_store_contents);
move the DCHECK out of the for loop?

https://codereview.chromium.org/122653005/diff/300001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_filter.cc:118: // TODO(gab): Store the
|old_value| to provide an undo UI.
nit: not sure if this is clearly desired or not. Presumably we will neither
forget this without the TODO nor is the TODO likely to trigger the
implementation?

https://codereview.chromium.org/122653005/diff/300001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_hash_filter_unittest.cc (right):

https://codereview.chromium.org/122653005/diff/300001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_filter_unittest.cc:355: 
Can't you do away with this hack by using

testing::Values(PrefHashFilter::NO_ENFORCEMENT, ...,
PrefHashFilter::ENFORCE_ALL)

instead of

testing::Range(...)

?

See
https://code.google.com/p/googletest/wiki/AdvancedGuide#Value_Parameterized_T...

[gab, 2014-01-03 19:26:09]

Erik, PTAL.

Thanks!
Gab

https://codereview.chromium.org/122653005/diff/50001/chrome/browser/prefs/chr...
File chrome/browser/prefs/chrome_pref_service_factory.cc (right):

https://codereview.chromium.org/122653005/diff/50001/chrome/browser/prefs/chr...
chrome/browser/prefs/chrome_pref_service_factory.cc:80:
PrefHashFilter::EnforcementLevel level;
On 2014/01/03 19:08:13, erikwright wrote:
> nit: I would reverse this since, intuitively, a map maps from the first field
to
> the second (key-value, not "value-key").
> 
> Also, consider using an actual map like:
> 
> std::map<string, level> m;
> m.insert("a", A);
> m.insert("b", B);
> 
> if (trial) {
>   map::iterator it = m.find(group_name);
>   if (it != m.end())
>     return *it;
> }
> return default;
> 
> I'm not sure if it will be more or less clear. Slightly less efficient
(NlogN?)
> but N is small and this is only done once per profile I think, so code
> simplicity should dominate.

Reversed the mapping (i.e., name first).

I still prefer the current form though. Building a map in local scope just to
perform a single search in it seems counter-intuitive to me.

https://codereview.chromium.org/122653005/diff/300001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_hash_filter.cc (right):

https://codereview.chromium.org/122653005/diff/300001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_filter.cc:78: DCHECK(pref_store_contents);
On 2014/01/03 19:08:14, erikwright wrote:
> move the DCHECK out of the for loop?

Of course! Good catch!

https://codereview.chromium.org/122653005/diff/300001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_filter.cc:118: // TODO(gab): Store the
|old_value| to provide an undo UI.
On 2014/01/03 19:08:14, erikwright wrote:
> nit: not sure if this is clearly desired or not. Presumably we will neither
> forget this without the TODO nor is the TODO likely to trigger the
> implementation?

The TODO highlights where the implementation should hang off of IMO.

Otherwise the |old_value| variable is injustified, at which point I should pass
NULL to RemovePath, at which point it's obscure to the next developer where he
should plug his code I find.

https://codereview.chromium.org/122653005/diff/300001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_hash_filter_unittest.cc (right):

https://codereview.chromium.org/122653005/diff/300001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_filter_unittest.cc:355: 
On 2014/01/03 19:08:14, erikwright wrote:
> Can't you do away with this hack by using
> 
> testing::Values(PrefHashFilter::NO_ENFORCEMENT, ...,
> PrefHashFilter::ENFORCE_ALL)
> 
> instead of
> 
> testing::Range(...)
> 
> ?
> 
> See
>
https://code.google.com/p/googletest/wiki/AdvancedGuide#Value_Parameterized_T...

Yes, but I wanted to avoid explicitly listing all levels, as-is this test is
robust to more levels being added.

https://codereview.chromium.org/122653005/diff/400001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_hash_filter_unittest.cc (right):

https://codereview.chromium.org/122653005/diff/400001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_filter_unittest.cc:63: return &unseen_value;
I could also return a bogus void*; doesn't need to be a Value, WDYT?

[erikwright (departed), 2014-01-03 19:54:41]

LGTM. One alternative for the parameterization and I leave it up to you to
decide if it's better or not:

PrefHashFilter::EnforcementLevel kAllLevels[] = {X, Y, Z}
CompileTimeAssertion(arraysize(kAllLevels) == ENFORCE_MAX - ENFORCE_NONE);

INSTANTIATE_TEST_CASE_P(..., testing::ValuesIn(kAllLevels), ...);

https://codereview.chromium.org/122653005/diff/400001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_hash_filter_unittest.cc (right):

https://codereview.chromium.org/122653005/diff/400001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_filter_unittest.cc:63: return &unseen_value;
On 2014/01/03 19:26:10, gab wrote:
> I could also return a bogus void*; doesn't need to be a Value, WDYT?

Up to you. You could return |this| if you want.

Returning a literal like 0xdeadbeef might require some kind of cast to ensure
that you don't get type mismatch errors in 64-bit. Not sure...

[gab, 2014-01-03 21:29:03]

Thanks!

https://codereview.chromium.org/122653005/diff/400001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_hash_filter_unittest.cc (right):

https://codereview.chromium.org/122653005/diff/400001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_filter_unittest.cc:63: return &unseen_value;
On 2014/01/03 19:54:42, erikwright wrote:
> On 2014/01/03 19:26:10, gab wrote:
> > I could also return a bogus void*; doesn't need to be a Value, WDYT?
> 
> Up to you. You could return |this| if you want.
> 
> Returning a literal like 0xdeadbeef might require some kind of cast to ensure
> that you don't get type mismatch errors in 64-bit. Not sure...

Used void* cast of 0xBAD; makes it more obvious in failure logs (I checked and
it does show up as 0x00000BAD)

[gab, 2014-01-03 21:30:25]

On 2014/01/03 19:54:41, erikwright wrote:
> LGTM. One alternative for the parameterization and I leave it up to you to
> decide if it's better or not:
> 
> PrefHashFilter::EnforcementLevel kAllLevels[] = {X, Y, Z}
> CompileTimeAssertion(arraysize(kAllLevels) == ENFORCE_MAX - ENFORCE_NONE);
> 
> INSTANTIATE_TEST_CASE_P(..., testing::ValuesIn(kAllLevels), ...);

I still prefer the current form which no one will need to care about after this
is committed (and it's not a tough hack to understand should someone care).
Whereas the proposed form would require a manual update.

Cheers!
Gab

[commit-bot: I haz the power, 2014-01-03 21:31:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gab@chromium.org/122653005/540001

[commit-bot: I haz the power, 2014-01-03 23:59:58]

Change committed as 242967