Make incognito windows not inherit HSTS state from the main profile.

Make incognito windows not inherit HSTS state from the main profile.

The dynamic HSTS state is loaded by the TransportSecurityPersister. By moving it up into profile_impl (which isn't shared by Incognito profiles), the Incognito profiles don't ever load dynamic state.

BUG=104935
TEST=Inject an HSTS entry for a non-HTTPS site in the main profile using about:net-internals. Open an incognito window and verify that you can still access the HTTP site.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=116685

[agl, 2011-12-19 18:22:01]

(This is at the request of the Tor folks.)

[agl, 2011-12-20 19:43:06]

poke :)

[palmer, 2011-12-20 20:08:05]

I am ambivalent about the semantics of the change. I understand the problem it
solves, but it effectively takes away the benefits of HSTS across sessions. I
think it is related to this bug
https://code.google.com/p/chromium/issues/detail?id=104935 where there is not
consensus about what to do. (You should change BUG=none to BUG=104935.)

I see you allow Incognito windows to get HSTS state from the command line, but
that doesn't help Chrome OS users (and CrOS is a fine candidate for privacy- and
local-stored-state sensitive users), and in any case it is hard to use. The last
thing Tor users need is another difficult hoop to jump through, and they
especially need to get good transport security since they are exiting through
routers run by not just ISPs but anyone (see the various spying-on-exit-nodes
attacks).

I guess I'm leaning toward keeping the security of HSTS and giving ground on the
privacy aspect, especially because this change hardly solves the cross-site
tracking problem (and I bet this vector is the least-used by cross-site
trackers, currently). Tor users can get the "never store HSTS metadata" behavior
by using Incognito mode all the time, never using Normal mode. (I know at least
one person who does this, for this reason.)

CCing some other people to stir up discussion — I think we need more before we
make this change. If we do decide to do it, this CL is obviously good. I'd add
cevans but he is on vacation.

[wtc, 2011-12-20 22:14:42]

http://codereview.chromium.org/8997012/diff/2001/chrome/browser/profiles/off_...
File chrome/browser/profiles/off_the_record_profile_io_data.h (right):

http://codereview.chromium.org/8997012/diff/2001/chrome/browser/profiles/off_...
chrome/browser/profiles/off_the_record_profile_io_data.h:113: mutable
scoped_ptr<net::TransportSecurityState> transport_security_state_;

Can we just use the transport_security_state_ member of the
base class?

[agl, 2012-01-03 20:29:50]

I've addressed wtc's comment, but there is a design decision here: should
incognito use HSTS state learned from a normal profile?

On the "no" side: it prevent's HSTS-cookies from linking normal and incognito
profiles. We have a fresh HttpCache for incognito for example, not a read-only
version of the standard one.

On the "yes" side (the current behaviour): it protects users because we might
use HSTS for some sites that we wouldn't have otherwise.

Since we have the preloaded list and since incognito is so close to being usable
by Tor, my vote is for "no", which is the effect of this CL.

http://codereview.chromium.org/8997012/diff/2001/chrome/browser/profiles/off_...
File chrome/browser/profiles/off_the_record_profile_io_data.h (right):

http://codereview.chromium.org/8997012/diff/2001/chrome/browser/profiles/off_...
chrome/browser/profiles/off_the_record_profile_io_data.h:113: mutable
scoped_ptr<net::TransportSecurityState> transport_security_state_;
On 2011/12/20 22:14:42, wtc wrote:
> 
> Can we just use the transport_security_state_ member of the
> base class?

Ah, yes, we can. Done. (In fact, I've moved the persister out of the base class
since the incognito class doesn't use it any longer.)

[palmer, 2012-01-03 22:24:52]

> On the "no" side: it prevent's HSTS-cookies from linking normal and incognito
> profiles. We have a fresh HttpCache for incognito for example, not a read-only
> version of the standard one.

There's been an on-going thread/argument/rumination about whether or not
Incognito means, has ever meant, or should mean "defense against cross-site
identity linking". The original ambit of Incognito was apparently purely about
not having any local storage, but people (users and developers) have (naturally,
I might argue) started assuming that it also meant defense against cross-site
identity linking. Perhaps that's where the fresh HttpCache came from, or perhaps
that was just an easy-at-the-time way to implement saved vs. non-saved
HttpCache.

I'm neutral, but complicatedly.

1. HSTS can work strongly. By contrast, I believe there can be no strong defense
against cross-site identity linking. Although this fact is incidental, I believe
cross-site-linking attackers use better means than HSTS cookies and probably
always will. JavaScript- and Flash- based linking techniques are more robust and
less noisy/latent on the network, and we can never fix many of their techniques.

2. And yet, I really really want cross-site identity linking defense.

3. I feel dirty for saying this, but we could sort of punt by choosing some
default (either one) and adding an about:flags flag for people who want the
other choice.

Finally, I still think you should add BUG=104935 in the description, because if
we adopt this patch, it resolves that bug.

[wtc, 2012-01-04 23:51:28]

Patch Set 3 LGTM.  I am fine with the design decision made
by this CL.

I propose a simpler way to accomplish this below.  Please
let me know if it works.

http://codereview.chromium.org/8997012/diff/9001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_io_data.cc (left):

http://codereview.chromium.org/8997012/diff/9001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_io_data.cc:447:
command_line.GetSwitchValueASCII(switches::kHstsHosts)));

It may be a good idea to explain here or in the header file
that transport_security_state_ is initialized by a derived
class.

It seems that we can still initialize transport_security_state_
here in the base class, because the two derived classes
would be initializing it the same way (from the command
line option).  We just need to move transport_security_persister_
to ProfileImplIOData.  transport_security_state_ can remain
a private member of the base class.  ProfileImplIOData can
access it using the transport_security_state() getter.

Will that work?

[wtc, 2012-01-04 23:56:39]

On 2012/01/03 22:24:52, Chris P. wrote:
>
> There's been an on-going thread/argument/rumination about whether or not
> Incognito means, has ever meant, or should mean "defense against cross-site
> identity linking". The original ambit of Incognito was apparently purely about
> not having any local storage,

Yes.  This is shown by the original name of this feature, "off the
record", which is still used in the source code.

> but people (users and developers) have (naturally,
> I might argue) started assuming that it also meant defense against cross-site
> identity linking.

I suspect the new expectation is partially encouraged by the final
name of this feature, "incognito".

> Perhaps that's where the fresh HttpCache came from, or perhaps
> that was just an easy-at-the-time way to implement saved vs. non-saved
> HttpCache.

It's the latter.

[wtc, 2012-01-06 01:23:20]

agl: you may want to reword the CL's commit message.
It's not clear to me why removing the persister from the
incognito mode's profile io data causes incognito windows
to not inherit HSTS state from the main profile.  Would be
nice to explain that in the commit message.  Thanks!

[agl, 2012-01-06 16:14:09]

On 2012/01/06 01:23:20, wtc wrote:
> agl: you may want to reword the CL's commit message.

Done. Thanks!

[commit-bot: I haz the power, 2012-01-06 16:14:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/agl@chromium.org/8997012/22001

[commit-bot: I haz the power, 2012-01-06 16:14:19]

Presubmit check for 8997012-22001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
chrome/browser/profiles/profile_impl_io_data.cc,chrome/browser/profiles/profile_impl_io_data.h,chrome/browser/profiles/profile_io_data.cc,chrome/browser/profiles/profile_io_data.h

[agl, 2012-01-06 16:20:28]

(Throwing to Miranda for OWNERS approval.)

[Miranda Callahan, 2012-01-06 16:30:51]

On 2012/01/06 16:20:28, agl wrote:
> (Throwing to Miranda for OWNERS approval.)

LGTM for profiles/* changes.

[commit-bot: I haz the power, 2012-01-06 17:02:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/agl@chromium.org/8997012/22001

[commit-bot: I haz the power, 2012-01-06 18:21:12]

Change committed as 116685

[wtc, 2012-01-06 18:45:52]

I have one last question.  Thanks.

http://codereview.chromium.org/8997012/diff/9001/chrome/browser/profiles/off_...
File chrome/browser/profiles/off_the_record_profile_io_data.cc (right):

http://codereview.chromium.org/8997012/diff/9001/chrome/browser/profiles/off_...
chrome/browser/profiles/off_the_record_profile_io_data.cc:163:
main_context->set_transport_security_state(transport_security_state_.get());

One reason I am confused is this code: it seems to make the
main profile use the transport security state of the incognito
mode.  Is main_context not the URL request context of the main
profile?

[agl, 2012-01-06 19:08:47]

http://codereview.chromium.org/8997012/diff/9001/chrome/browser/profiles/off_...
File chrome/browser/profiles/off_the_record_profile_io_data.cc (right):

http://codereview.chromium.org/8997012/diff/9001/chrome/browser/profiles/off_...
chrome/browser/profiles/off_the_record_profile_io_data.cc:163:
main_context->set_transport_security_state(transport_security_state_.get());
On 2012/01/06 18:45:52, wtc wrote:
> Is main_context not the URL request context of the main
> profile?

I don't believe so. There's a main_context (a.k.a. main_request_context_) per
profile, which means that it's specific to the Incognito profile.

It's "main" simply in contrast to the extensions request context (which is also
profile specific.) It's not "main" as in "main profile".