Make default media device ID salts random by default

This results in different hashed device IDs on each session on embedders that don't have a specialized implementation of device ID salts such as WebView, Blimp and Content Shell. The new default helps prevent user fingerprinting on these embedders.

Since the new default logic is basically the same as for Chrome incognito mode, Chrome's implementation of salts has been updated to defer to the new default on incognito mode.

BUG=315022
Committed: https://crrev.com/4db1329e005388540eb07429ac97827ca9bc422b
Cr-Commit-Position: refs/heads/master@{#399883}

[Guido Urdaneta, 2016-05-17 11:31:01]

Description was changed from

==========
Add random media device ID salts to blimp, android webview and content shell.

BUG=315022
==========

to

==========
Add random media device ID salts to blimp, android webview and content shell.

BUG=315022
==========

[Guido Urdaneta, 2016-05-17 11:31:01]

guidou@chromium.org changed reviewers:
+ tommi@chromium.org

[Guido Urdaneta, 2016-05-17 11:54:58]

tommi: this is approach 5, as discussed in crbug.com/315022

[tommi (sloooow) - chröme, 2016-05-17 12:27:20]

lgtm.  would like the strings to be const whenever possible.

https://codereview.chromium.org/1987643002/diff/1/android_webview/browser/aw_...
File android_webview/browser/aw_resource_context.h (right):

https://codereview.chromium.org/1987643002/diff/1/android_webview/browser/aw_...
android_webview/browser/aw_resource_context.h:39: std::string
media_device_id_salt_;
can this be const?

https://codereview.chromium.org/1987643002/diff/1/blimp/engine/common/blimp_b...
File blimp/engine/common/blimp_browser_context.cc (right):

https://codereview.chromium.org/1987643002/diff/1/blimp/engine/common/blimp_b...
blimp/engine/common/blimp_browser_context.cc:60: std::string
media_device_id_salt_;
const?

[Guido Urdaneta, 2016-05-18 09:40:19]

https://codereview.chromium.org/1987643002/diff/1/android_webview/browser/aw_...
File android_webview/browser/aw_resource_context.h (right):

https://codereview.chromium.org/1987643002/diff/1/android_webview/browser/aw_...
android_webview/browser/aw_resource_context.h:39: std::string
media_device_id_salt_;
On 2016/05/17 12:27:19, tommi-chrömium wrote:
> can this be const?

Done.

https://codereview.chromium.org/1987643002/diff/1/blimp/engine/common/blimp_b...
File blimp/engine/common/blimp_browser_context.cc (right):

https://codereview.chromium.org/1987643002/diff/1/blimp/engine/common/blimp_b...
blimp/engine/common/blimp_browser_context.cc:60: std::string
media_device_id_salt_;
On 2016/05/17 12:27:20, tommi-chrömium wrote:
> const?

Done.

[Guido Urdaneta, 2016-05-18 09:50:03]

guidou@chromium.org changed reviewers:
+ avi@chromium.org, dtrainor@chromium.org, torne@chromium.org

[Guido Urdaneta, 2016-05-18 09:50:04]

dtrainor@chromium.org: Please review changes in blimp/

torne@chromium.org: Please review changes in android_webview/

avi@chromium.org: Please review changes in content/

[Guido Urdaneta, 2016-05-18 09:55:28]

guidou@chromium.org changed reviewers:
- dtrainor@chromium.org

[Guido Urdaneta, 2016-05-18 09:57:07]

guidou@chromium.org changed reviewers:
+ haibinlu@chromium.org

[Guido Urdaneta, 2016-05-18 09:57:07]

haibinlu@chromium.org: Please review changes in blimp/

[Guido Urdaneta, 2016-05-18 09:57:47]

Description was changed from

==========
Add random media device ID salts to blimp, android webview and content shell.

BUG=315022
==========

to

==========
Add random media device ID salts to blimp, android webview and content shell.

This results in different random IDs on each sessions, which helps prevent user
fingerprinting.

BUG=315022
==========

[Guido Urdaneta, 2016-05-18 09:58:28]

Description was changed from

==========
Add random media device ID salts to blimp, android webview and content shell.

This results in different random IDs on each sessions, which helps prevent user
fingerprinting.

BUG=315022
==========

to

==========
Add random media device ID salts to blimp, android webview and content shell.

This results in different hashed device IDs on each session, which helps prevent
user fingerprinting.

BUG=315022
==========

[Guido Urdaneta, 2016-05-18 09:58:53]

haibinlu@chromium.org: Please review changes in blimp/

[Torne, 2016-05-18 14:01:10]

android_webview LGTM - thanks for the simpler approach here! :)

[Avi (use Gerrit), 2016-05-18 15:15:16]

avi@chromium.org changed reviewers:
+ jam@chromium.org

[Avi (use Gerrit), 2016-05-18 15:15:16]

John, do you know about ResourceContext?

It's not clear to me why this complicated dance is going on. The original code
says that we need to save this salt to the user profile (see resource_context.h
line 59), but this CL talks about changing it to be per-session, which seems to
contradict that comment.

Are we making this per-session? I've read the five approaches listed in the bug
and it's still not clear to me what's going on.

[Guido Urdaneta, 2016-05-18 15:34:39]

On 2016/05/18 15:15:16, Avi wrote:
> John, do you know about ResourceContext?
> 
> It's not clear to me why this complicated dance is going on. The original code
> says that we need to save this salt to the user profile (see
resource_context.h
> line 59), but this CL talks about changing it to be per-session, which seems
to
> contradict that comment.
> 
> Are we making this per-session? I've read the five approaches listed in the
bug
> and it's still not clear to me what's going on.

This salt is used to hash media device IDs when they are reported to the
renderer process.
The purpose of this hashing is to prevent sites from fingerprinting users by
looking at the devices that users have available locally.
Hashed device IDs in chrome are the result of a hash of the real ID, the
security origin, and a random salt. The salt is stored  in the preferences of
the profile, so the device IDs are the same in future sessions with the same
profile and origin.

Other embedders like Blimp, WebView and Content Shell use hash(real_id,
security_origin, empty string), which makes fingerprinting easy.
WebView and Blimp have preferences, so they could follow the same approach as
Chrome. https://codereview.chromium.org/1981913002/ (not landed) does that with
WebView.

However, WebView and Blimp do not persist their preferences, so the salt would
be forgotten if stored in preferences. For these embedders, store the salt in a
field is simpler and gives the same results.
The disadvantage is that hashed device IDs will change in every session, but
that is preferable to having them easier to fingerprint.

Note however, I'll be glad to follow your advice on how to proceed in this
matter.

[jam, 2016-05-18 16:19:22]

On 2016/05/18 15:34:39, Guido Urdaneta wrote:
> On 2016/05/18 15:15:16, Avi wrote:
> > John, do you know about ResourceContext?
> > 
> > It's not clear to me why this complicated dance is going on. The original
code
> > says that we need to save this salt to the user profile (see
> resource_context.h
> > line 59), but this CL talks about changing it to be per-session, which seems
> to
> > contradict that comment.
> > 
> > Are we making this per-session? I've read the five approaches listed in the
> bug
> > and it's still not clear to me what's going on.
> 
> This salt is used to hash media device IDs when they are reported to the
> renderer process.
> The purpose of this hashing is to prevent sites from fingerprinting users by
> looking at the devices that users have available locally.
> Hashed device IDs in chrome are the result of a hash of the real ID, the
> security origin, and a random salt. The salt is stored  in the preferences of
> the profile, so the device IDs are the same in future sessions with the same
> profile and origin.
> 
> Other embedders like Blimp, WebView and Content Shell use hash(real_id,
> security_origin, empty string), which makes fingerprinting easy.
> WebView and Blimp have preferences, so they could follow the same approach as
> Chrome. https://codereview.chromium.org/1981913002/ (not landed) does that
with
> WebView.
> 
> However, WebView and Blimp do not persist their preferences, so the salt would
> be forgotten if stored in preferences. For these embedders, store the salt in
a
> field is simpler and gives the same results.
> The disadvantage is that hashed device IDs will change in every session, but
> that is preferable to having them easier to fingerprint.
> 
> Note however, I'll be glad to follow your advice on how to proceed in this
> matter.

Thanks for the explanations. Few comments:
-why is ResourceContext::GetMediaDeviceIDSalt asynchronous? it looks like the
chrome implementation can return the salt synchronously.
-a general pattern that we use for stuff like this is that the default
implementation of GetMediaDeviceIDSalt can have the code to return the random
string. then blimp/webview/content_shell don't have to override it and you don't
need to expose new methods in resource_context.h. any reason why that wouldn't
work here?
-chrome's ResourceContext's implementation can still call the parent class'
method to get a default string, which it can use if the pref wasn't set or if
it's in incognito.

[Kevin M, 2016-05-18 16:42:20]

kmarshall@chromium.org changed reviewers:
+ kmarshall@chromium.org

[Kevin M, 2016-05-18 16:42:21]

blimp lgtm

[Guido Urdaneta, 2016-05-18 18:26:33]

Description was changed from

==========
Add random media device ID salts to blimp, android webview and content shell.

This results in different hashed device IDs on each session, which helps prevent
user fingerprinting.

BUG=315022
==========

to

==========
This results in different hashed device IDs on each session, which helps prevent
user fingerprinting.

Also remove incognito logic from chrome's implementation as it can now rely on
the default implementation for incognito mode.

BUG=315022
==========

[Guido Urdaneta, 2016-05-18 18:32:43]

On 2016/05/18 16:19:22, jam OOO wrote:
> On 2016/05/18 15:34:39, Guido Urdaneta wrote:
> > On 2016/05/18 15:15:16, Avi wrote:
> > > John, do you know about ResourceContext?
> > > 
> > > It's not clear to me why this complicated dance is going on. The original
> code
> > > says that we need to save this salt to the user profile (see
> > resource_context.h
> > > line 59), but this CL talks about changing it to be per-session, which
seems
> > to
> > > contradict that comment.
> > > 
> > > Are we making this per-session? I've read the five approaches listed in
the
> > bug
> > > and it's still not clear to me what's going on.
> > 
> > This salt is used to hash media device IDs when they are reported to the
> > renderer process.
> > The purpose of this hashing is to prevent sites from fingerprinting users by
> > looking at the devices that users have available locally.
> > Hashed device IDs in chrome are the result of a hash of the real ID, the
> > security origin, and a random salt. The salt is stored  in the preferences
of
> > the profile, so the device IDs are the same in future sessions with the same
> > profile and origin.
> > 
> > Other embedders like Blimp, WebView and Content Shell use hash(real_id,
> > security_origin, empty string), which makes fingerprinting easy.
> > WebView and Blimp have preferences, so they could follow the same approach
as
> > Chrome. https://codereview.chromium.org/1981913002/ (not landed) does that
> with
> > WebView.
> > 
> > However, WebView and Blimp do not persist their preferences, so the salt
would
> > be forgotten if stored in preferences. For these embedders, store the salt
in
> a
> > field is simpler and gives the same results.
> > The disadvantage is that hashed device IDs will change in every session, but
> > that is preferable to having them easier to fingerprint.
> > 
> > Note however, I'll be glad to follow your advice on how to proceed in this
> > matter.
> 
> Thanks for the explanations. Few comments:
> -why is ResourceContext::GetMediaDeviceIDSalt asynchronous? it looks like the
> chrome implementation can return the salt synchronously.

I guess the reason the salt is returned as a callback is to give embedders more
flexibility.


> -a general pattern that we use for stuff like this is that the default
> implementation of GetMediaDeviceIDSalt can have the code to return the random
> string. then blimp/webview/content_shell don't have to override it and you
don't
> need to expose new methods in resource_context.h. any reason why that wouldn't
> work here?
> -chrome's ResourceContext's implementation can still call the parent class'
> method to get a default string, which it can use if the pref wasn't set or if
> it's in incognito.

My main concern about doing it this is adding a field to a class that was
originally intended to have only pure virtual methods.
Despite that, I think it's the simplest approach.
I just implemented it that way. Please let me know what you think.

[haibinlu, 2016-05-18 20:18:27]

lgtm

[Guido Urdaneta, 2016-06-01 15:17:37]

guidou@chromium.org changed reviewers:
+ jochen@chromium.org
- avi@chromium.org, haibinlu@chromium.org, kmarshall@chromium.org,
tommi@chromium.org, torne@chromium.org

[Guido Urdaneta, 2016-06-01 15:18:05]

+jochen: can you take a look?

[Guido Urdaneta, 2016-06-03 10:11:59]

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

[commit-bot: I haz the power, 2016-06-03 10:12:13]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1987643002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1987643002/80001

[commit-bot: I haz the power, 2016-06-03 10:25:08]

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

[commit-bot: I haz the power, 2016-06-03 10:25:10]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[Guido Urdaneta, 2016-06-03 11:17:41]

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

[commit-bot: I haz the power, 2016-06-03 11:17:53]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1987643002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1987643002/100001

[commit-bot: I haz the power, 2016-06-03 11:52:29]

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

[commit-bot: I haz the power, 2016-06-03 11:52:31]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Guido Urdaneta, 2016-06-03 12:05:48]

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

[commit-bot: I haz the power, 2016-06-03 12:06:00]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1987643002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1987643002/120001

[Guido Urdaneta, 2016-06-03 12:11:44]

Patchset #7 (id:120001) has been deleted

[jochen (gone - plz use gerrit), 2016-06-03 12:50:24]

jochen@chromium.org changed reviewers:
+ haibinlu@chromium.org, kmarshall@chromium.org, tommi@chromium.org,
torne@chromium.org

[jochen (gone - plz use gerrit), 2016-06-03 12:50:25]

https://codereview.chromium.org/1987643002/diff/140001/content/public/browser...
File content/public/browser/resource_context.h (right):

https://codereview.chromium.org/1987643002/diff/140001/content/public/browser...
content/public/browser/resource_context.h:79: static std::string
CreateRandomMediaDeviceIDSalt();
as jam@ pointed out, you don't need to expose this method, embedders can just
call ResourceContext::GetMediaDeviceIDSalt()

[Guido Urdaneta, 2016-06-03 13:09:44]

https://codereview.chromium.org/1987643002/diff/140001/content/public/browser...
File content/public/browser/resource_context.h (right):

https://codereview.chromium.org/1987643002/diff/140001/content/public/browser...
content/public/browser/resource_context.h:79: static std::string
CreateRandomMediaDeviceIDSalt();
On 2016/06/03 12:50:25, jochen wrote:
> as jam@ pointed out, you don't need to expose this method, embedders can just
> call ResourceContext::GetMediaDeviceIDSalt()

The MediaDeviceIDSalt class uses that method to generate a random salt and store
it in preferences. This class does not know about ResourceContext, but it's used
by the profile implementation of ResourceContext.

If we don't expose it we need either to duplicate the method in
MediaDeviceIDSalt or do some less trivial refactoring in order to reuse the
ResourceContext method.
Since the utility method is almost a one-liner it's not such a big deal to
duplicate it. Are you OK with this?

[jochen (gone - plz use gerrit), 2016-06-06 14:46:15]

On 2016/06/03 at 13:09:44, guidou wrote:
>
https://codereview.chromium.org/1987643002/diff/140001/content/public/browser...
> File content/public/browser/resource_context.h (right):
> 
>
https://codereview.chromium.org/1987643002/diff/140001/content/public/browser...
> content/public/browser/resource_context.h:79: static std::string
CreateRandomMediaDeviceIDSalt();
> On 2016/06/03 12:50:25, jochen wrote:
> > as jam@ pointed out, you don't need to expose this method, embedders can
just
> > call ResourceContext::GetMediaDeviceIDSalt()
> 
> The MediaDeviceIDSalt class uses that method to generate a random salt and
store it in preferences. This class does not know about ResourceContext, but
it's used by the profile implementation of ResourceContext.
> 
> If we don't expose it we need either to duplicate the method in
MediaDeviceIDSalt or do some less trivial refactoring in order to reuse the
ResourceContext method.
> Since the utility method is almost a one-liner it's not such a big deal to
duplicate it. Are you OK with this?
what about instantiating the media device id salt also for incognito mode?
shouldn't the incognito pref service just not persist the pref?

[jochen (gone - plz use gerrit), 2016-06-06 14:46:15]

On 2016/06/03 at 13:09:44, guidou wrote:
>
https://codereview.chromium.org/1987643002/diff/140001/content/public/browser...
> File content/public/browser/resource_context.h (right):
> 
>
https://codereview.chromium.org/1987643002/diff/140001/content/public/browser...
> content/public/browser/resource_context.h:79: static std::string
CreateRandomMediaDeviceIDSalt();
> On 2016/06/03 12:50:25, jochen wrote:
> > as jam@ pointed out, you don't need to expose this method, embedders can
just
> > call ResourceContext::GetMediaDeviceIDSalt()
> 
> The MediaDeviceIDSalt class uses that method to generate a random salt and
store it in preferences. This class does not know about ResourceContext, but
it's used by the profile implementation of ResourceContext.
> 
> If we don't expose it we need either to duplicate the method in
MediaDeviceIDSalt or do some less trivial refactoring in order to reuse the
ResourceContext method.
> Since the utility method is almost a one-liner it's not such a big deal to
duplicate it. Are you OK with this?
what about instantiating the media device id salt also for incognito mode?
shouldn't the incognito pref service just not persist the pref?

[Guido Urdaneta, 2016-06-08 09:52:03]

> what about instantiating the media device id salt also for incognito mode?
> shouldn't the incognito pref service just not persist the pref?

The problem with that is that, AFAIK, there is no standardized way to get the
PrefService in content.
All uses of a PrefService are specific to each embedder.
Another option could be to move MediaDeviceIDSalt to components, and expose the
static method to produce a salt string there.

[jochen (gone - plz use gerrit), 2016-06-08 15:04:16]

On 2016/06/08 at 09:52:03, guidou wrote:
> > what about instantiating the media device id salt also for incognito mode?
> > shouldn't the incognito pref service just not persist the pref?
> 
> The problem with that is that, AFAIK, there is no standardized way to get the
PrefService in content.
> All uses of a PrefService are specific to each embedder.
> Another option could be to move MediaDeviceIDSalt to components, and expose
the static method to produce a salt string there.

no, I meant for the profile_io_data, we currently use a static hash for OTR (or
after your change, don't create a MediaDeviceIDSalt at all). Why not just always
create one there?

[Guido Urdaneta, 2016-06-08 16:14:08]

On 2016/06/08 15:04:16, jochen wrote:
> On 2016/06/08 at 09:52:03, guidou wrote:
> > > what about instantiating the media device id salt also for incognito mode?
> > > shouldn't the incognito pref service just not persist the pref?
> > 
> > The problem with that is that, AFAIK, there is no standardized way to get
the
> PrefService in content.
> > All uses of a PrefService are specific to each embedder.
> > Another option could be to move MediaDeviceIDSalt to components, and expose
> the static method to produce a salt string there.
> 
> no, I meant for the profile_io_data, we currently use a static hash for OTR
(or
> after your change, don't create a MediaDeviceIDSalt at all). Why not just
always
> create one there?

I believe the reason is that the OTR pref service starts out with the same
values as the original profile.
I tried always creating the MediaDeviceIDSalt and in incognito mode the salt was
already present in the pref service, thus producing the same device IDs.
Using profile->GetOffTheRecordPrefs() in OTR mode makes no difference either.

[jam, 2016-06-09 15:52:29]

On 2016/05/18 18:32:43, Guido Urdaneta wrote:
> On 2016/05/18 16:19:22, jam OOO wrote:
> > On 2016/05/18 15:34:39, Guido Urdaneta wrote:
> > > On 2016/05/18 15:15:16, Avi wrote:
> > > > John, do you know about ResourceContext?
> > > > 
> > > > It's not clear to me why this complicated dance is going on. The
original
> > code
> > > > says that we need to save this salt to the user profile (see
> > > resource_context.h
> > > > line 59), but this CL talks about changing it to be per-session, which
> seems
> > > to
> > > > contradict that comment.
> > > > 
> > > > Are we making this per-session? I've read the five approaches listed in
> the
> > > bug
> > > > and it's still not clear to me what's going on.
> > > 
> > > This salt is used to hash media device IDs when they are reported to the
> > > renderer process.
> > > The purpose of this hashing is to prevent sites from fingerprinting users
by
> > > looking at the devices that users have available locally.
> > > Hashed device IDs in chrome are the result of a hash of the real ID, the
> > > security origin, and a random salt. The salt is stored  in the preferences
> of
> > > the profile, so the device IDs are the same in future sessions with the
same
> > > profile and origin.
> > > 
> > > Other embedders like Blimp, WebView and Content Shell use hash(real_id,
> > > security_origin, empty string), which makes fingerprinting easy.
> > > WebView and Blimp have preferences, so they could follow the same approach
> as
> > > Chrome. https://codereview.chromium.org/1981913002/ (not landed) does that
> > with
> > > WebView.
> > > 
> > > However, WebView and Blimp do not persist their preferences, so the salt
> would
> > > be forgotten if stored in preferences. For these embedders, store the salt
> in
> > a
> > > field is simpler and gives the same results.
> > > The disadvantage is that hashed device IDs will change in every session,
but
> > > that is preferable to having them easier to fingerprint.
> > > 
> > > Note however, I'll be glad to follow your advice on how to proceed in this
> > > matter.
> > 
> > Thanks for the explanations. Few comments:
> > -why is ResourceContext::GetMediaDeviceIDSalt asynchronous? it looks like
the
> > chrome implementation can return the salt synchronously.
> 
> I guess the reason the salt is returned as a callback is to give embedders
more
> flexibility.

Let's add flexibility when we need it. But until then, no need to complicate the
api further.

> 
> 
> > -a general pattern that we use for stuff like this is that the default
> > implementation of GetMediaDeviceIDSalt can have the code to return the
random
> > string. then blimp/webview/content_shell don't have to override it and you
> don't
> > need to expose new methods in resource_context.h. any reason why that
wouldn't
> > work here?
> > -chrome's ResourceContext's implementation can still call the parent class'
> > method to get a default string, which it can use if the pref wasn't set or
if
> > it's in incognito.
> 
> My main concern about doing it this is adding a field to a class that was
> originally intended to have only pure virtual methods.

field? i meant that ResourceContext's GetMediaDeviceIDSalt() would have a
default implementation. it's already not pure.

> Despite that, I think it's the simplest approach.
> I just implemented it that way. Please let me know what you think.

[Guido Urdaneta, 2016-06-09 16:07:13]

On 2016/06/09 15:52:29, jam wrote:
> On 2016/05/18 18:32:43, Guido Urdaneta wrote:
> > On 2016/05/18 16:19:22, jam OOO wrote:
> > > On 2016/05/18 15:34:39, Guido Urdaneta wrote:
> > > > On 2016/05/18 15:15:16, Avi wrote:
> > > > > John, do you know about ResourceContext?
> > > > > 
> > > > > It's not clear to me why this complicated dance is going on. The
> original
> > > code
> > > > > says that we need to save this salt to the user profile (see
> > > > resource_context.h
> > > > > line 59), but this CL talks about changing it to be per-session, which
> > seems
> > > > to
> > > > > contradict that comment.
> > > > > 
> > > > > Are we making this per-session? I've read the five approaches listed
in
> > the
> > > > bug
> > > > > and it's still not clear to me what's going on.
> > > > 
> > > > This salt is used to hash media device IDs when they are reported to the
> > > > renderer process.
> > > > The purpose of this hashing is to prevent sites from fingerprinting
users
> by
> > > > looking at the devices that users have available locally.
> > > > Hashed device IDs in chrome are the result of a hash of the real ID, the
> > > > security origin, and a random salt. The salt is stored  in the
preferences
> > of
> > > > the profile, so the device IDs are the same in future sessions with the
> same
> > > > profile and origin.
> > > > 
> > > > Other embedders like Blimp, WebView and Content Shell use hash(real_id,
> > > > security_origin, empty string), which makes fingerprinting easy.
> > > > WebView and Blimp have preferences, so they could follow the same
approach
> > as
> > > > Chrome. https://codereview.chromium.org/1981913002/ (not landed) does
that
> > > with
> > > > WebView.
> > > > 
> > > > However, WebView and Blimp do not persist their preferences, so the salt
> > would
> > > > be forgotten if stored in preferences. For these embedders, store the
salt
> > in
> > > a
> > > > field is simpler and gives the same results.
> > > > The disadvantage is that hashed device IDs will change in every session,
> but
> > > > that is preferable to having them easier to fingerprint.
> > > > 
> > > > Note however, I'll be glad to follow your advice on how to proceed in
this
> > > > matter.
> > > 
> > > Thanks for the explanations. Few comments:
> > > -why is ResourceContext::GetMediaDeviceIDSalt asynchronous? it looks like
> the
> > > chrome implementation can return the salt synchronously.
> > 
> > I guess the reason the salt is returned as a callback is to give embedders
> more
> > flexibility.
> 
> Let's add flexibility when we need it. But until then, no need to complicate
the
> api further.
> 

I agree. Should we change the API to return a string?
I would prefer do that in an upcoming CL since that would require changes in a
lot of places that use the callback.
In the meantime I can add a TODO. Would that be OK with you?



> > My main concern about doing it this is adding a field to a class that was
> > originally intended to have only pure virtual methods.
> 
> field? i meant that ResourceContext's GetMediaDeviceIDSalt() would have a
> default implementation. it's already not pure.
> 

The current default implementation returns the empty string (well, a callback
that returns an empty string).
We want to change it to return a random string, but it has to be the same random
string on every call to ResourceContext::GetMediaDeviceIDSalt.
The field is to store that random string. Do you have a different implementation
strategy in mind?

[jam, 2016-06-13 18:19:55]

(sorry for delay, I missed the earlier comment)

On 2016/06/09 16:07:13, Guido Urdaneta wrote:
> On 2016/06/09 15:52:29, jam wrote:
> > On 2016/05/18 18:32:43, Guido Urdaneta wrote:
> > > On 2016/05/18 16:19:22, jam OOO wrote:
> > > > On 2016/05/18 15:34:39, Guido Urdaneta wrote:
> > > > > On 2016/05/18 15:15:16, Avi wrote:
> > > > > > John, do you know about ResourceContext?
> > > > > > 
> > > > > > It's not clear to me why this complicated dance is going on. The
> > original
> > > > code
> > > > > > says that we need to save this salt to the user profile (see
> > > > > resource_context.h
> > > > > > line 59), but this CL talks about changing it to be per-session,
which
> > > seems
> > > > > to
> > > > > > contradict that comment.
> > > > > > 
> > > > > > Are we making this per-session? I've read the five approaches listed
> in
> > > the
> > > > > bug
> > > > > > and it's still not clear to me what's going on.
> > > > > 
> > > > > This salt is used to hash media device IDs when they are reported to
the
> > > > > renderer process.
> > > > > The purpose of this hashing is to prevent sites from fingerprinting
> users
> > by
> > > > > looking at the devices that users have available locally.
> > > > > Hashed device IDs in chrome are the result of a hash of the real ID,
the
> > > > > security origin, and a random salt. The salt is stored  in the
> preferences
> > > of
> > > > > the profile, so the device IDs are the same in future sessions with
the
> > same
> > > > > profile and origin.
> > > > > 
> > > > > Other embedders like Blimp, WebView and Content Shell use
hash(real_id,
> > > > > security_origin, empty string), which makes fingerprinting easy.
> > > > > WebView and Blimp have preferences, so they could follow the same
> approach
> > > as
> > > > > Chrome. https://codereview.chromium.org/1981913002/ (not landed) does
> that
> > > > with
> > > > > WebView.
> > > > > 
> > > > > However, WebView and Blimp do not persist their preferences, so the
salt
> > > would
> > > > > be forgotten if stored in preferences. For these embedders, store the
> salt
> > > in
> > > > a
> > > > > field is simpler and gives the same results.
> > > > > The disadvantage is that hashed device IDs will change in every
session,
> > but
> > > > > that is preferable to having them easier to fingerprint.
> > > > > 
> > > > > Note however, I'll be glad to follow your advice on how to proceed in
> this
> > > > > matter.
> > > > 
> > > > Thanks for the explanations. Few comments:
> > > > -why is ResourceContext::GetMediaDeviceIDSalt asynchronous? it looks
like
> > the
> > > > chrome implementation can return the salt synchronously.
> > > 
> > > I guess the reason the salt is returned as a callback is to give embedders
> > more
> > > flexibility.
> > 
> > Let's add flexibility when we need it. But until then, no need to complicate
> the
> > api further.
> > 
> 
> I agree. Should we change the API to return a string?
> I would prefer do that in an upcoming CL since that would require changes in a
> lot of places that use the callback.
> In the meantime I can add a TODO. Would that be OK with you?

Let's do it here. There aren't that many places (only 11 hits total for
GetMediaDeviceIDSalt)

> 
> 
> 
> > > My main concern about doing it this is adding a field to a class that was
> > > originally intended to have only pure virtual methods.
> > 
> > field? i meant that ResourceContext's GetMediaDeviceIDSalt() would have a
> > default implementation. it's already not pure.
> > 
> 
> The current default implementation returns the empty string (well, a callback
> that returns an empty string).
> We want to change it to return a random string, but it has to be the same
random
> string on every call to ResourceContext::GetMediaDeviceIDSalt.
> The field is to store that random string. Do you have a different
implementation
> strategy in mind?

I understand chrome's implementation does that, but the others (content_shell
etc) aren't. So I'm not seeing how that changes the default implementation
(which doesn't need to do this)?

[Guido Urdaneta, 2016-06-14 15:01:20]

guidou@chromium.org changed reviewers:
- haibinlu@chromium.org, jochen@chromium.org, kmarshall@chromium.org,
tommi@chromium.org, torne@chromium.org

[Guido Urdaneta, 2016-06-14 15:21:30]

> Let's do it here. There aren't that many places (only 11 hits total for
> GetMediaDeviceIDSalt)
> 

Done.

> > The current default implementation returns the empty string (well, a
callback
> > that returns an empty string).
> > We want to change it to return a random string, but it has to be the same
> random
> > string on every call to ResourceContext::GetMediaDeviceIDSalt.
> > The field is to store that random string. Do you have a different
> implementation
> > strategy in mind?
> 
> I understand chrome's implementation does that, but the others (content_shell
> etc) aren't. So I'm not seeing how that changes the default implementation
> (which doesn't need to do this)?

The goal of this CL to change all implementations of ResourceContext to return a
salt consisting of random string instead of the empty string.
Note that for a given ResourceContext, multiple calls to GetMediaDeviceIDSalt()
should always return the same (randomly generated) string.


In a previous comment you suggested the following:

> -a general pattern that we use for stuff like this is that the default
> implementation of GetMediaDeviceIDSalt can have the code to return the random
> string. then blimp/webview/content_shell don't have to override it and you
don't
> need to expose new methods in resource_context.h. any reason why that wouldn't
> work here?
> -chrome's ResourceContext's implementation can still call the parent class'
> method to get a default string, which it can use if the pref wasn't set or if
> it's in incognito.

The latest patchset has an implementation of this idea.
The only caveat is that it exposes a static CreateRandomMediaDeviceIDSalt()
utility method in ResourceContext (the old MediaDeviceIDSalt::CreateSalt()).
It's practically a one-liner so we can instead duplicate it in MediaDeviceIDSalt
if you think that's better.
Note that just calling content::ResourceContext::GetMediaDeviceIDSalt() to get a
random salt would not work for the MediaDeviceIDSalt::Reset() method, since it
must ensure a new (different) salt is stored in the preferences.

[jam, 2016-06-14 16:20:33]

https://codereview.chromium.org/1987643002/diff/180001/chrome/browser/media/m...
File chrome/browser/media/media_device_id_salt.cc (right):

https://codereview.chromium.org/1987643002/diff/180001/chrome/browser/media/m...
chrome/browser/media/media_device_id_salt.cc:20: if
(media_device_id_salt_.GetValue().empty()) {
i'm confused, the cl description says "This results in different hashed device
IDs on each session, which helps prevent user fingerprinting." which I
understood to mean that there is a different salt each time chrome starts. am i
understanding correctly? If so, then how come the salt is saved in prefs?

https://codereview.chromium.org/1987643002/diff/180001/content/browser/resour...
File content/browser/resource_context_impl.cc (right):

https://codereview.chromium.org/1987643002/diff/180001/content/browser/resour...
content/browser/resource_context_impl.cc:9: #include <string>
nit: no need to add, this is in the header

https://codereview.chromium.org/1987643002/diff/180001/content/public/browser...
File content/public/browser/resource_context.h (right):

https://codereview.chromium.org/1987643002/diff/180001/content/public/browser...
content/public/browser/resource_context.h:58: // The salt should be stored in
the current user profile and should be reset
nit: don't mention user profile, since that's a chrome concept

https://codereview.chromium.org/1987643002/diff/180001/content/public/browser...
content/public/browser/resource_context.h:62: // Utility function useful for
implementations
nit: make it explicit that this only needs to be called if 
1) the embedder needs to use a new salt 
2) the embedder saves its salt across restart

[jam, 2016-06-14 17:09:17]

lgtm

Guido clarified over IM that this is for blimp/content_shell/webview and will
update the description.

[Guido Urdaneta, 2016-06-14 17:13:36]

Description was changed from

==========
This results in different hashed device IDs on each session, which helps prevent
user fingerprinting.

Also remove incognito logic from chrome's implementation as it can now rely on
the default implementation for incognito mode.

BUG=315022
==========

to

==========
This results in different hashed device IDs on each session on embedders that
don't have a specialized implementation of device ID salts. This helps prevent
user fingerprinting on these embedders.

Since the new default logic is basically the same as for Chrome incognito mode,
Chrome's implementation of salts has been updated to defer to the new default on
incognito mode.

BUG=315022
==========

[Guido Urdaneta, 2016-06-14 17:14:43]

Description was changed from

==========
This results in different hashed device IDs on each session on embedders that
don't have a specialized implementation of device ID salts. This helps prevent
user fingerprinting on these embedders.

Since the new default logic is basically the same as for Chrome incognito mode,
Chrome's implementation of salts has been updated to defer to the new default on
incognito mode.

BUG=315022
==========

to

==========
This results in different hashed device IDs on each session on embedders that
don't have a specialized implementation of device ID salts such as WebView,
Blimp and Content Shell. The new default helps prevent user fingerprinting on
these embedders.

Since the new default logic is basically the same as for Chrome incognito mode,
Chrome's implementation of salts has been updated to defer to the new default on
incognito mode.

BUG=315022
==========

[Guido Urdaneta, 2016-06-14 17:30:57]

https://codereview.chromium.org/1987643002/diff/180001/chrome/browser/media/m...
File chrome/browser/media/media_device_id_salt.cc (right):

https://codereview.chromium.org/1987643002/diff/180001/chrome/browser/media/m...
chrome/browser/media/media_device_id_salt.cc:20: if
(media_device_id_salt_.GetValue().empty()) {
On 2016/06/14 16:20:33, jam wrote:
> i'm confused, the cl description says "This results in different hashed device
> IDs on each session, which helps prevent user fingerprinting." which I
> understood to mean that there is a different salt each time chrome starts. am
i
> understanding correctly? If so, then how come the salt is saved in prefs?

Clarified via IM and in the updated CL description.

https://codereview.chromium.org/1987643002/diff/180001/content/browser/resour...
File content/browser/resource_context_impl.cc (right):

https://codereview.chromium.org/1987643002/diff/180001/content/browser/resour...
content/browser/resource_context_impl.cc:9: #include <string>
On 2016/06/14 16:20:33, jam wrote:
> nit: no need to add, this is in the header

Done.

https://codereview.chromium.org/1987643002/diff/180001/content/public/browser...
File content/public/browser/resource_context.h (right):

https://codereview.chromium.org/1987643002/diff/180001/content/public/browser...
content/public/browser/resource_context.h:58: // The salt should be stored in
the current user profile and should be reset
On 2016/06/14 16:20:33, jam wrote:
> nit: don't mention user profile, since that's a chrome concept

Done.

https://codereview.chromium.org/1987643002/diff/180001/content/public/browser...
content/public/browser/resource_context.h:62: // Utility function useful for
implementations
On 2016/06/14 16:20:33, jam wrote:
> nit: make it explicit that this only needs to be called if 
> 1) the embedder needs to use a new salt 
> 2) the embedder saves its salt across restart

Done.

[Guido Urdaneta, 2016-06-14 17:31:08]

The CQ bit was checked by guidou@chromium.org

[Guido Urdaneta, 2016-06-14 17:31:09]

The patchset sent to the CQ was uploaded after l-g-t-m from tommi@chromium.org,
torne@chromium.org, kmarshall@chromium.org, haibinlu@chromium.org,
jam@chromium.org
Link to the patchset: https://codereview.chromium.org/1987643002/#ps200001
(title: "jam's comments")

[commit-bot: I haz the power, 2016-06-14 17:32:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1987643002/200001

[commit-bot: I haz the power, 2016-06-14 18:04:56]

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

[commit-bot: I haz the power, 2016-06-14 18:04:59]

Try jobs failed on following builders:
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[Guido Urdaneta, 2016-06-15 11:00:15]

The CQ bit was checked by guidou@chromium.org

[Guido Urdaneta, 2016-06-15 11:00:16]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org,
tommi@chromium.org, kmarshall@chromium.org, torne@chromium.org,
haibinlu@chromium.org
Link to the patchset: https://codereview.chromium.org/1987643002/#ps260001
(title: "change return type from const string to string as it makes no
difference")

[commit-bot: I haz the power, 2016-06-15 11:00:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1987643002/260001

[commit-bot: I haz the power, 2016-06-15 11:26:50]

Description was changed from

==========
This results in different hashed device IDs on each session on embedders that
don't have a specialized implementation of device ID salts such as WebView,
Blimp and Content Shell. The new default helps prevent user fingerprinting on
these embedders.

Since the new default logic is basically the same as for Chrome incognito mode,
Chrome's implementation of salts has been updated to defer to the new default on
incognito mode.

BUG=315022
==========

to

==========
This results in different hashed device IDs on each session on embedders that
don't have a specialized implementation of device ID salts such as WebView,
Blimp and Content Shell. The new default helps prevent user fingerprinting on
these embedders.

Since the new default logic is basically the same as for Chrome incognito mode,
Chrome's implementation of salts has been updated to defer to the new default on
incognito mode.

BUG=315022
==========

[commit-bot: I haz the power, 2016-06-15 11:26:52]

Committed patchset #13 (id:260001)

[commit-bot: I haz the power, 2016-06-15 11:26:58]

CQ bit was unchecked

[commit-bot: I haz the power, 2016-06-15 11:29:50]

Description was changed from

==========
This results in different hashed device IDs on each session on embedders that
don't have a specialized implementation of device ID salts such as WebView,
Blimp and Content Shell. The new default helps prevent user fingerprinting on
these embedders.

Since the new default logic is basically the same as for Chrome incognito mode,
Chrome's implementation of salts has been updated to defer to the new default on
incognito mode.

BUG=315022
==========

to

==========
This results in different hashed device IDs on each session on embedders that
don't have a specialized implementation of device ID salts such as WebView,
Blimp and Content Shell. The new default helps prevent user fingerprinting on
these embedders.

Since the new default logic is basically the same as for Chrome incognito mode,
Chrome's implementation of salts has been updated to defer to the new default on
incognito mode.

BUG=315022

Committed: https://crrev.com/4db1329e005388540eb07429ac97827ca9bc422b
Cr-Commit-Position: refs/heads/master@{#399883}
==========

[commit-bot: I haz the power, 2016-06-15 11:29:52]

Patchset 13 (id:??) landed as
https://crrev.com/4db1329e005388540eb07429ac97827ca9bc422b
Cr-Commit-Position: refs/heads/master@{#399883}

[Guido Urdaneta, 2016-06-15 12:54:57]

A revert of this CL (patchset #13 id:260001) has been created in
https://codereview.chromium.org/2065383003/ by guidou@chromium.org.

The reason for reverting is: Reverting because it is breaking the WebRTC
MacTester bot.
https://build.chromium.org/p/chromium.webrtc/builders/Mac%20Tester/builds/55969/.