557598 - Support strict-transport-security (STS) in private browsing mode

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Sid Stamm [:geekboy or :sstamm]]

STS should not be disabled when in private mode, but any new STS information received from sites while in private mode should be purged when private mode is exited.

This is a bit different than how Chrome has implemented private mode:
http://code.google.com/p/chromium/issues/detail?id=33445

I think we should continue to honor STS when private mode is entered, but purge any site-triggered changes to the STS datastore when private mode is exited.  Any manually-changed STS data should be retained.

This probably involves creating a memory-only cache for STS while in private mode, and is why this has been separated into a new bug.

Comment 1

[Sid Stamm [:geekboy or :sstamm]]

In bug 495115 comment 50 kaie said:

> I understand the intent to not leave any traces, and not permanently remember
> the STS server.
> 
> I propose to improve our logic in the following way (feel free to forward this
> proposal to a separate bug):
> - if we are in private browsing mode, and we see a new host,
>   don't use EXPIRE_TIME, but use EXPIRE_SESSION
>   (a quick glance at nsPermissionManager shows that it won't
>    call "UpdateDB", which proably means, it will remembered in memory only)
> 
> - have nsStrictTransportSecurityService remember the list of
>   temporary overrides added during privacy mode.
> 
> - when private browsing mode ends,
>   remove all session-only STS hosts knowledge acquired during PBM

This is relevant to this bug.

Comment 2

[(no longer active)]

Comment 1 makes a lot of sense to me.

Comment 3

[Sid Stamm [:geekboy or :sstamm]]

dwitte suggested I make changes in the permission manager, since STS is the only consumer of EXPIRE_SESSION and EXPIRE_TIME permissions right now.  He suggested: 

1.  Purge EXPIRE_SESSION permissions when entering PBM
2.  Convert EXPIRE_NEVER permissions to EXPIRE_SESSION while in PBM
3.  Always skip UpdateDB when in PBM
4.  Purge EXPIRE_SESSION permissions when exiting PBM

The one problem I see with this approach surrounds EXPIRE_TIME permissions... they may or may not expire before leaving PBM.  Perhaps this calls for keeping a list of permissions set in PBM and deleting them after.  What do you think dwitte?

Comment 4

[[e-mail]]

(In reply to comment #3)
> 1.  Purge EXPIRE_SESSION permissions when entering PBM
> 2.  Convert EXPIRE_NEVER permissions to EXPIRE_SESSION while in PBM

That sounds wrong. PBM has never been about sessionifying user-initiated actions like adding permissions or bookmarks. If you want to do that for STS, we should come up with a different plan.

> 3.  Always skip UpdateDB when in PBM

Likewise, we still want to add permanent exceptions.

> The one problem I see with this approach surrounds EXPIRE_TIME permissions...
> they may or may not expire before leaving PBM.  Perhaps this calls for keeping
> a list of permissions set in PBM and deleting them after.  What do you think
> dwitte?

Why would we want to delete permanent exceptions at all?

One thing STS could do is detect PBM and change the lifetime it specifies. Is that what you're after?

Comment 5

[Sid Stamm [:geekboy or :sstamm]]

(In reply to comment #4)
> > 2.  Convert EXPIRE_NEVER permissions to EXPIRE_SESSION while in PBM
> That sounds wrong. PBM has never been about sessionifying user-initiated
> actions like adding permissions or bookmarks. If you want to do that for STS,
> we should come up with a different plan.

Hm, good point.  I guess EXPIRE_NEVER permissions for STS would be user-initiated and probably not removed when exiting PBM.

> > 3.  Always skip UpdateDB when in PBM
> Likewise, we still want to add permanent exceptions.

Yeah, true.  This should say "Skip UpdateDB for EXPIRE_SESSION and EXPIRE_TIME".

> One thing STS could do is detect PBM and change the lifetime it specifies. Is
> that what you're after?

Hm... not sure.  I think the biggest sticking point is what to do with those pesky permissions when (1) they overwrite permissions added before entering PBM and (2) how do we differentiate EXPIRE_TIME permissions added in PBM from those that pre-existed?

Maybe two lists are the way to go, but I'm worried a bit about performance there if we have to double the number of lookups during PBM.

Comment 6

[(no longer active)]

(In reply to comment #4)
> (In reply to comment #3)
> > 1.  Purge EXPIRE_SESSION permissions when entering PBM
> > 2.  Convert EXPIRE_NEVER permissions to EXPIRE_SESSION while in PBM
> 
> That sounds wrong. PBM has never been about sessionifying user-initiated
> actions like adding permissions or bookmarks. If you want to do that for STS,
> we should come up with a different plan.

Is there any UI involved where the user sets a permission explicitly?

Comment 7

[Sid Stamm [:geekboy or :sstamm]]

(In reply to comment #6)
> Is there any UI involved where the user sets a permission explicitly?

I think for all non-HSTS permissions, yes, there's a UI (part of page info).  

Specifically for HSTS, there's no UI yet at all and no ability to set these permissions manually until bug 572803 lands.  When the UI is built in, all UI-based HSTS permissions will be EXPIRE_NEVER, but the rest of the site-initiated permissions will be EXPIRE_TIME.

Comment 8

[(no longer active)]

Hmm, so users do not make an explicit choice whether to allow a website to set the STS permission, right?  Therefore, setting this permission cannot be considered as an explicit action.  But setting the permissions from the planned UI is definitely an explicit action on part of the user.

Comment 9

[Sid Stamm [:geekboy or :sstamm]]

Ehsan: exactly.  The permissions set by web sites will always have an expiration time.  Permissions that will be set by the UI (bug 572803) will be permanent like other permissions.  The goal is when exiting PBM, we will flush anything auto-set by sites during PBM, but anything set manually through the UI should stick.

Comment 10

[(no longer active)]

(In reply to comment #9)
> Ehsan: exactly.  The permissions set by web sites will always have an
> expiration time.  Permissions that will be set by the UI (bug 572803) will be
> permanent like other permissions.  The goal is when exiting PBM, we will flush
> anything auto-set by sites during PBM, but anything set manually through the UI
> should stick.

OK, this makes perfect sense then!

Comment 11

[Sid Stamm [:geekboy or :sstamm]]

Attachment: proposed patch

First whack at a patch.  I need to go through and clean it up a bit, but I welcome any feedback.

Comment 12

[(no longer active)]

Comment on attachment 477759 [details] [diff] [review]
proposed patch

Just to make sure that I wouldn't forget to take a look at this...

Comment 13

[(no longer active)]

Comment on attachment 477759 [details] [diff] [review]
proposed patch

>+////////////////////////////////////////////////////////////////////////////////
>+
>+nsSTSHostEntry::nsSTSHostEntry(const char* aHost)
>+{
>+  mHost = NS_strdup(aHost);
>+}

Nit: move the initialization to the initializer list?  Also, why is it OK to leave the rest of the members uninitialized?

>+   // figure out if we're starting in private browsing mode
>+   nsCOMPtr<nsIPrivateBrowsingService> pbs =
>+     do_GetService(NS_PRIVATE_BROWSING_SERVICE_CONTRACTID);
>+   if (pbs)
>+     pbs->GetPrivateBrowsingEnabled(&mInPrivateMode);
>+
>+   mObserverService = mozilla::services::GetObserverService();
>+   if (mObserverService)
>+     mObserverService->AddObserver(this, NS_PRIVATE_BROWSING_SWITCH_TOPIC, PR_TRUE);

Hmm, nsStrictTransportSecurityService doesn't implement nsISupportsWeakReference, right?  So you shouldn't be passing PR_TRUE here.

>+   if (!mPrivateModeHostTable.Init())
>+     return NS_ERROR_OUT_OF_MEMORY;

This is wasteful, as many users may never enter PB in a given session.  It's better to initialize this when needed (i.e., when we enter PB mode, or here if mInPrivateMode is true).

>+nsresult
>+nsStrictTransportSecurityService::AddPermission(nsIURI     *aURI,
>+                                                const char *aType,
>+                                                PRUint32   aPermission,
>+                                                PRUint32   aExpireType,
>+                                                PRInt64    aExpireTime)
>+{
>+    // Private mode doesn't address user-set (EXPIRE_NEVER) permissions.
>+    if (mInPrivateMode && aExpireType != nsIPermissionManager::EXPIRE_NEVER) {
>+      nsCAutoString host;
>+      nsresult rv = GetHost(aURI, host);
>+      if (NS_FAILED(rv)) return NS_OK;

Why is this safe?

>+      // TODO : EXPIRE_NEVER permissions trump anything that shows up in the
>+      // HTTP header, so if there's an EXPIRE_NEVER permission already don't
>+      // store anything new.
>+      // Currently there's no way to get the type of expiry out of the
>+      // permission manager.
>+
>+      // Update in mPrivateModeHostTable only
>+      nsSTSHostEntry* entry = mPrivateModeHostTable.PutEntry(host.get());
>+      if (strcmp(aType, STS_SUBDOMAIN_PERMISSION) == 0)
>+        entry->mIncludeSubdomains = PR_TRUE;
>+      else
>+        entry->mDeleted = PR_FALSE;
>+      entry->mExpireTime = aExpireTime;

I'm not sure if I understand the logic here, but then again I don't know a lot about STS internals...

>+nsresult
>+nsStrictTransportSecurityService::RemovePermission(const nsACString &aHost,
>+                                                   const char       *aType)
>+{
>+    nsresult rv;
>+    if (mInPrivateMode) {
>+      nsSTSHostEntry* entry = mPrivateModeHostTable.GetEntry(PromiseFlatCString(aHost).get());
>+
>+      if (!entry)
>+        return NS_OK;
>+
>+      PRUint32 permmgrValue;
>+      nsCOMPtr<nsIURI> uri;
>+      rv = NS_NewURI(getter_AddRefs(uri),
>+                     NS_LITERAL_CSTRING("http://") + aHost);
>+      NS_ENSURE_SUCCESS(rv, rv);
>+
>+      rv = mPermMgr->TestExactPermission(uri, aType, &permmgrValue);
>+      NS_ENSURE_SUCCESS(rv, rv);
>+
>+      // If Permission exists in DB, store a "deleted" mask for the permission
>+      // in mPrivateModeHostTable (either update mPrivateModeHostTable to have
>+      // the deleted mask, or add one).
>+      if (permmgrValue != nsIPermissionManager::UNKNOWN_ACTION) {
>+        entry->mDeleted = PR_TRUE;
>+        entry->mIncludeSubdomains = PR_FALSE;
>+        return NS_OK;
>+      }
>+
>+      // Otherwise, permission doesn't exist in the real permission manager, so
>+      // delete the locally cached copy.
>+      mPrivateModeHostTable.RawRemoveEntry(entry);

Again, I'm not claiming to understand a lot about the logic here, but I noticed something.  What if you try to remove an entry which has been added to the DB outside of the PB mode while you're inside PB mode?

>+nsresult
>+nsStrictTransportSecurityService::TestPermission(nsIURI     *aURI,
>+                                                 const char *aType,
>+                                                 PRUint32   *aPermission,
>+                                                 PRBool     testExact)

Again I'm no expert here, but...

>+{
>+    // set default
>+    *aPermission = nsIPermissionManager::UNKNOWN_ACTION;
>+
>+    nsCAutoString host;
>+    nsresult rv = GetHost(aURI, host);
>+    if (NS_FAILED(rv)) return NS_OK;
>+
>+    nsSTSHostEntry *entry;
>+    PRUint32 actualExactPermission;
>+    PRUint32 offset = 0;
>+    PRInt64 now = PR_Now() / 1000;

I'd move these variables to inside the if block below.

>+    // Used for testing permissions as we walk up the domain tree.
>+    nsCOMPtr<nsIURI> domainWalkURI;
>+
>+    if (mInPrivateMode) {
>+      // in parallel, loop over private mode cache and also the real permission
>+      // manager... ignoring any masked as "deleted" in the local cache.
>+      do {
>+        entry = mPrivateModeHostTable.GetEntry(host.get() + offset);
>+        if (entry && entry->mExpireTime < now) {
>+          entry->mDeleted = PR_TRUE;
>+          entry->mIncludeSubdomains = PR_FALSE;
>+        }
>+
>+        rv = NS_NewURI(getter_AddRefs(domainWalkURI),
>+                       NS_LITERAL_CSTRING("http://")
>+                        + Substring(host, offset));
>+        NS_ENSURE_SUCCESS(rv, rv);
>+
>+        rv = mPermMgr->TestExactPermission(domainWalkURI,
>+                                           aType,
>+                                           &actualExactPermission);
>+        NS_ENSURE_SUCCESS(rv, rv);
>+
>+        if (!entry) {
>+          if (actualExactPermission != nsIPermissionManager::UNKNOWN_ACTION) {
>+            // no cached data but a permission in the permission manager: use it.
>+            *aPermission = actualExactPermission;
>+            break;
>+          }
>+        }
>+        else if (entry->mDeleted || (strcmp(aType, STS_SUBDOMAIN_PERMISSION) == 0
>+                                    && !entry->mIncludeSubdomains)) {
>+          // don't examine permission manager data when it's masked by a
>+          // "deleted" flag.

Seems like the condition is checking for more than the comment says.

>+        }
>+        else {
>+          // use the cached permission!
>+          *aPermission = nsIPermissionManager::ALLOW_ACTION;

Are all cached permissions "allow"?

>+class nsSTSHostEntry : public PLDHashEntryHdr
>+{
>+  public:
>+    nsSTSHostEntry(const char* aHost);
>+    nsSTSHostEntry(const nsSTSHostEntry& toCopy);
>+    ~nsSTSHostEntry() {}

Nit: dtor is not needed (although it won't kill anyone if you include it...)

More nit: mark the first ctor explicit?

>+    const char* mHost;
>+    PRUint32    mExpireTime;
>+    PRBool      mDeleted;
>+    PRBool      mIncludeSubdomains;

PRPackedBool would save you some space there...


Final comments: this really needs the eyes of someone familiar with STS as well, and of course, it does need extensive automated tests.

Comment 14

[Johnny Stenback (:jst)]

Blocking, as this lets sites determine whether or not the user is in private browsing mode or not.

Comment 15

[Sid Stamm [:geekboy or :sstamm]]

Attachment: proposed patch

Now with mochitests.  The tests are pretty broad, and hopefully cover most of the edge cases.  The new tests check (1) behavior before entering PBM, (2) behavior within PBM after clearing any STS data and (3) behavior after exiting PBM but not resetting STS data.  Ultimately the first and third phases should look the same, and STS should work during PBM.

Comment 16

[(no longer active)]

Comment on attachment 492748 [details] [diff] [review]
proposed patch

Looks great to me!

Comment 17

[Daniel Veditz [:dveditz]]

Comment on attachment 492748 [details] [diff] [review]
proposed patch

>+nsSTSHostEntry::nsSTSHostEntry(const char* aHost)
>+  : mHost(NS_strdup(aHost))

>+nsSTSHostEntry::nsSTSHostEntry(const nsSTSHostEntry& toCopy)
>+  : mHost(toCopy.mHost)

The difference here worries me. How do you know when you need to free the host string and when it's a pointer to some other entry's host string--and if it is what if you free that other one first? Could mHost be a nsCString and manage its own memory (possibly adopt()ing the existing host buffer to avoid copies if that was your concern?).

> nsresult
> nsStrictTransportSecurityService::SetStsState(nsIURI* aSourceURI,
>                                               PRInt64 maxage,
>                                               PRBool includeSubdomains)
> {
>+  nsresult rv;
>+

You could wait to define this until you call AddPermission a little later. That way future reviewers don't have to check that it's not used/returned before its set or whether it needs to be initialized to a default value ;-)

>+nsStrictTransportSecurityService::AddPermission(nsIURI     *aURI,
>+                                                const char *aType,
>+                                                PRUint32   aPermission,
>+                                                PRUint32   aExpireType,
>+                                                PRInt64    aExpireTime)
>+{
>+    // Private mode doesn't address user-set (EXPIRE_NEVER) permissions: let
>+    // those be stored persistently.
>+    if (mInPrivateMode && aExpireType != nsIPermissionManager::EXPIRE_NEVER) {

Would be easier to grok if you reversed the test (!pb || is_permanent) and bailed out with the direct assignment up here rather than suspend that thought for 20 lines. Bonus: the main work of the function wouldn't be indented. (Don't feel that strongly about it if you disagree on style grounds.)

Same comment for RemovePermission and TestPermission.

>+      // AddPermission() will be called twice if the STS header encountered has
>+      // includeSubdomains (first for the main permission and second for the
>+      // subdomains permission). If AddPermission() gets called a second time
>+      // with the STS_SUBDOMAIN_PERMISSION, we just have to flip that bit in
>+      // the nsSTSHostEntry.
>+      if (strcmp(aType, STS_SUBDOMAIN_PERMISSION) == 0)
>+        entry->mIncludeSubdomains = PR_TRUE;
>+      else
>+        entry->mDeleted = PR_FALSE;

The else clause here seems a bit of a non sequitur. Isn't that initialized that way when the node is created? What does it have to do with it being not a subdomain permission?

>+      // Make changes in mPrivateModeHostTable only, so any changes will be
>+      // rolled back when exiting private mode.
>+      nsSTSHostEntry* entry = mPrivateModeHostTable.GetEntry(PromiseFlatCString(aHost).get());

You don't use PromiseFlatCString calling GetEntry() in AddPermission or TestPermission. Do you need it in Remove? If so those other places could be trouble. One difference is that Add and Test are called with URIs and Remove is called with a string. Why the inconsistency? You should already have a flat string in your nsCAutoString so it could just be you're using too generic a string reference when you defined this API. You can do const PromiseFlatCString &aHost I'm pretty sure.

>+      // if there's no entry in mPrivateModeHostTable, there's nothing to do.
>+      if (!entry) return NS_OK;

Is that true? What if the site stopped sending STS headers last week and it finally expired today, when we're in private mode? Wouldn't we just be "reading through" to the permission manager, and therefore have to create a "deleted" fake entry?

>+      // Check to see if there's STS data stored for this host in the
>+      // permission manager (probably set outside private mode).
>+      PRUint32 permmgrValue;
>+      rv = mPermMgr->TestExactPermission(uri, aType, &permmgrValue);

Yeah, here. Shouldn't you do that before bailing above?

Seems like the only time you ever clear STS is if you receive a new header with a max-age of 0. Seems like nsStrictTransportSecurityService::IsStsURI() ought to be checking the expiration of the settings (deleting entries as necessary) so you don't force TLS on a new connection to a host you haven't visited in a long while. The IsStsURI() check ought to be happening before we decide how to contact the host, long before we potentially get an STS header back.

r- primarily on the memory management issue. I guess I'll file a separate bug on that last bit.

Comment 18

[Sid Stamm [:geekboy or :sstamm]]

Thanks Dan.

The permission manager automatically expires permissions set as type EXPIRE_TIME, so that's not an issue.  The memory cache for STS does, however, need to do that for its local copy.

Comment 19

[Sid Stamm [:geekboy or :sstamm]]

(In reply to comment #17)
> Comment on attachment 492748 [details] [diff] [review]
> proposed patch
> 
> >+nsSTSHostEntry::nsSTSHostEntry(const char* aHost)
> >+  : mHost(NS_strdup(aHost))
> 
> >+nsSTSHostEntry::nsSTSHostEntry(const nsSTSHostEntry& toCopy)
> >+  : mHost(toCopy.mHost)
> 
> The difference here worries me. How do you know when you need to free the host
> string and when it's a pointer to some other entry's host string--and if it is
> what if you free that other one first? Could mHost be a nsCString and manage
> its own memory (possibly adopt()ing the existing host buffer to avoid copies if
> that was your concern?).

Would it be sufficient to change mHost to an nsCString and trash the NS_strDup()?  Looks like the nsCString class automatically calls .Assign() when the = operator is used.  I'm less worried about re-allocating a duplicate string than having memory issues or leaks, and my string-fu is a bit weak here.

> >+    if (mInPrivateMode && aExpireType != nsIPermissionManager::EXPIRE_NEVER) {
> 
> Would be easier to grok if you reversed the test (!pb || is_permanent) and
> bailed out with the direct assignment up here rather than suspend that thought
> for 20 lines. Bonus: the main work of the function wouldn't be indented. (Don't
> feel that strongly about it if you disagree on style grounds.)
> 
> Same comment for RemovePermission and TestPermission.

Great idea.  Flipped.

> >+      if (strcmp(aType, STS_SUBDOMAIN_PERMISSION) == 0)
> >+        entry->mIncludeSubdomains = PR_TRUE;
> >+      else
> >+        entry->mDeleted = PR_FALSE;
> 
> The else clause here seems a bit of a non sequitur. Isn't that initialized that
> way when the node is created? What does it have to do with it being not a
> subdomain permission?

Yeah, good point, the permission type is irrelevant here.  With respect to the mDeleted assigment: I have to re-set mDeleted in case mPrivateModeHostTable.PutEntry() returns an existing and deleted entry (instead of a new one), which it does when the slot is full.  I will remove the else statement (so all new entries are non-deleted) and put in a comment to explain this.

> >+      // Make changes in mPrivateModeHostTable only, so any changes will be
> >+      // rolled back when exiting private mode.
> >+      nsSTSHostEntry* entry = mPrivateModeHostTable.GetEntry(PromiseFlatCString(aHost).get());
> 
> You don't use PromiseFlatCString calling GetEntry() in AddPermission or
> TestPermission. Do you need it in Remove? If so those other places could be
> trouble. One difference is that Add and Test are called with URIs and Remove is
> called with a string. Why the inconsistency? 

This mirrors the nsIPermissionManager API.  My goal with this patch was to wrap calls to the nsIPermissionManager API that only change what happens during private browsing mode (a bit easier to understand).  The internal TestPermission/RemovePermission/AddPermission functions in nsStrictTransportSecurityService mimic those in nsIPermissionManager so the rest of the code looks the same as if it were using the permission manager.

> You should already have a flat
> string in your nsCAutoString so it could just be you're using too generic a
> string reference when you defined this API. You can do const PromiseFlatCString
> &aHost I'm pretty sure.

My ns-string-fu is pretty weak, I must admit.  I don't see "PromiseFlatCString" used as a parameter type anywhere else.  Can you point me to some code that does this?

> >+      // if there's no entry in mPrivateModeHostTable, there's nothing to do.
> >+      if (!entry) return NS_OK;
> 
> Is that true? What if the site stopped sending STS headers last week and it
> finally expired today, when we're in private mode? Wouldn't we just be "reading
> through" to the permission manager, and therefore have to create a "deleted"
> fake entry?

We could, but couldn't we just let it expire for real?  If I understand it correctly, private browsing is intended to prevent knowledge of where you've been, not to prevent knowledge that you used the browser at all.  Additionally, current code in the PM's TestPermission() function is going to delete an expired permission before we see it, so we'd have to hack up the permission manager to be aware of private browsing mode (which it currently isn't).

> >+      // Check to see if there's STS data stored for this host in the
> >+      // permission manager (probably set outside private mode).
> >+      PRUint32 permmgrValue;
> >+      rv = mPermMgr->TestExactPermission(uri, aType, &permmgrValue);
> 
> Yeah, here. Shouldn't you do that before bailing above?

Yeah, I should probably store the deleted mask _no matter what_ if there's something in the permission manager.  Good catch.

Comment 20

[Sid Stamm [:geekboy or :sstamm]]

Attachment: proposed patch

Dveditz's review comments taken into account.

Comment 21

[Daniel Veditz [:dveditz]]

Comment on attachment 496244 [details] [diff] [review]
proposed patch

>+    if (!mInPrivateMode || aExpireType == nsIPermissionManager::EXPIRE_NEVER) {
>+      // Not in private mode: store permissions persistently.
>+      return mPermMgr->Add(aURI, aType, aPermission, aExpireType, aExpireTime);
>+    }
>+
>+    // Private mode doesn't address user-set (EXPIRE_NEVER) permissions: let
>+    // those be stored persistently.

This last comment needs to be moved up with the other comment (or above the 'if'). At this point we're done with that decision.

Looks good, r=dveditz

Comment 22

[Sid Stamm [:geekboy or :sstamm]]

Attachment: patch

updated patch for bit rot.

Comment 23

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/57db6ae6d832