Closed Bug 760307 (preload-hsts) Opened 12 years ago Closed 12 years ago

implement support for preloaded strict-transport-security (HSTS) sites

Categories

(Core :: Networking, enhancement)

Product:
Core
Component:
Networking
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla17

People

(Reporter: geekboy, Assigned: keeler)

References

Details

(Keywords: sec-want, Whiteboard: [parity-Chrome])

Attachments

(1 file, 12 obsolete files)

patch v11
44.94 KB, patch
keeler
: review+
Details | Diff | Splinter Review 
We should implement a preloaded mechanism for HSTS sites to the first time they're accessed, they are "upgraded" to https.

Google maintains a list of domains that want HSTS (they ask to be added to the list), and offered to let us pull it down and use it.  I think we should take them up on it, scrub out irrelevant data, and ship it with Firefox.

Feature Page:
https://wiki.mozilla.org/Privacy/Features/HSTS_Preload_List

URL for chrome list:
https://src.chromium.org/viewvc/chrome/trunk/src/net/base/transport_security_state_static.json
Maybe wrong component, I think this is the right one.  The HSTS code is in netwerk and security/manager.
Component: Security: PSM → Networking
QA Contact: psm → networking
Whiteboard: [parity-Chrome]
This list would be continuously updated (important e.g. if a website on there decides to switch CA providers) and included with recent updates (in case of Firefox also in ESR versions) just like the public suffix list?
Is there already an outline for making this process quick and smooth (e.g. so that you are informed about changes to the list right away)?
Attached patch prototype (obsolete) — — Splinter Review 
Assignee: nobody → dkeeler
Status: NEW → ASSIGNED

        Attachment #633313 -
        Flags: feedback?(sstamm)

    
    

    
  
Comment on attachment 633313 [details] [diff] [review]
prototype

Review of attachment 633313 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for working on this!

::: security/manager/boot/src/nsSTSPreloadList.h
@@ +2,5 @@
> +#define __nsSTSPreloadList_h__
> +
> +#include <prtypes.h>
> +
> +class nsSTSPreload

Try not to put this class in a header if it can be avoided.

@@ +12,5 @@
> +
> +    nsSTSPreload(const char *aHost, bool aIncludeSubdomains) :
> +      mHost(aHost),
> +      mIncludeSubdomains(aIncludeSubdomains),
> +      mLength(strlen(aHost)) {}

Do not use strlen here since the inputs are all constant strings. Use the same/similar trickery that NS_MULTILINE_LITERAL_STRING uses.

Also, we cannot use a constructor here because we have a rule against running static initializers during startup.

@@ +15,5 @@
> +      mIncludeSubdomains(aIncludeSubdomains),
> +      mLength(strlen(aHost)) {}
> +};
> +
> +static const nsSTSPreload kSTSPreloadList[] = {

Do not put this list in a header file.

@@ +111,5 @@
> +  nsSTSPreload("www.gmail.com", false),
> +  nsSTSPreload("www.googlemail.com", false),
> +};
> +
> +static const PRUint32 kSTSPreloadListSize = sizeof(kSTSPreloadList) / sizeof(nsSTSPreload);

PR_ARRAY_SIZE() or ArrayLength from mozilla/Util.h (mfbt/Util.h)

::: security/manager/boot/src/nsStrictTransportSecurityService.cpp
@@ +303,5 @@
> +  for (PRUint32 i = 0; i < kSTSPreloadListSize; i++) {
> +    // This is similar to how nsPermissionManager handles exact host match
> +    // vs matching a subdomain. Try to match the full host name, and if
> +    // the entry we're on allows subdomains, march up the domain tree to see
> +    // if this host is a subdomain of the entry.

(Not a rhetorical question): Is it necessary to have to implementations of the includesSubdomains matching logic? It would be better to have one implementation of the includesSubdomains matching logic if that isn't too much work.

::: security/manager/ssl/tests/mochitest/stricttransportsecurity/test_sts_preloadlist.html
@@ +1,1 @@
> +<!DOCTYPE HTML>

Please make this test an xpcshell test instead of a mochitest if possible.

@@ +19,5 @@
> +  ok(stss, "Need strict transport security service for this test");
> +  ok(!stss.isStsHost("nonexistent.mozilla.com"), "nonexistent.mozilla.com should not be an STS host");
> +  // Note: the following were taken from the STS preload list
> +  // as of June 2012. If the list changes, this test will need to be modified.
> +  ok(stss.isStsHost("health.google.com"), "health.google.com should be an STS host");

Let's not include a case in the test for every single entry in the list.

Please make it clear where you are testing the includeSubdomain functionality.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch (obsolete)
        — 
          — Splinter Review
      

    


  
Regarding your question of having two implementations of the subdomain matching logic: one case is doing hash table lookups and the other is looping through a list, so there's not really a useful way to factor out the common logic.

        Attachment #633313 -
        Attachment is obsolete: true

        Attachment #633313 -
        Flags: feedback?(sstamm)

        Attachment #633617 -
        Flags: review?(bsmith)

        Attachment #633617 -
        Flags: review?(bsmith) → review?(honzab.moz)

    
    

    
  
Comment on attachment 633617 [details] [diff] [review]
patch

Review of attachment 633617 [details] [diff] [review]:
-----------------------------------------------------------------

Leaving open to discuss the comments for now.

If this is in a rush, then I can r+ after a second quick look at the current changes.

::: security/manager/boot/src/nsStrictTransportSecurityService.cpp
@@ +37,5 @@
> +    const PRUint32 mLength;
> +    const bool mIncludeSubdomains;
> +};
> +
> +#include "nsSTSPreloadList.h"

IMO, sorry Brian, it is quit strange to define this class here and use it in the header.  Maybe rename nsSTSPreloadList.h to nsSTSPreloadList.inc to make it more visible it is not a standard header file, but just something technically separated.

@@ +308,5 @@
>    return IsStsURI(uri, aResult);
>  }
>  
> +bool
> +nsStrictTransportSecurityService::IsHostInPreloadList(const nsACString &aHost) {

{ on a new line

@@ +309,5 @@
>  }
>  
> +bool
> +nsStrictTransportSecurityService::IsHostInPreloadList(const nsACString &aHost) {
> +  for (PRUint32 i = 0; i < kSTSPreloadListSize; i++) {

++i

@@ +316,5 @@
> +    // the entry we're on allows subdomains, march up the domain tree to see
> +    // if this host is a subdomain of the entry.
> +    PRUint32 offset = 0;
> +    do {
> +      const char *substr = aHost.Data() + offset;

s/Data()/BeginReading()/

@@ +328,5 @@
> +      }
> +      offset = aHost.FindChar('.', offset) + 1;
> +    } while (offset > 0);
> +  }
> +  return false;

This means MxN search patterns.  It is not very efficient IMO.

Isn't it better to convert the list to a hash table and look up aHost portions in it?  You can also add aHost to the hash table when it matches a sub-domain to "cache" the result.

Probably have two hash tables, one for non-subdomain-matching and one for subdomain-matching.

It would be more efficient, also since the list will grow in time.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch v2 (obsolete)
        — 
          — Splinter Review
      

    


  
Thanks for your review.
We're not in a rush, but as long as we have momentum on this, we may as well keep moving forward.
With respect to efficiency, I agree that a linear scan for every domain level is not the fastest. However, since there are only about 100 items on the list and no dns hostname will have more than 128 levels (and most will have no more than 3 or 4), maybe we're okay with this. I do think a hash table is too heavyweight for our needs right now (plus, it would need initialization, which takes time). As an in-between solution, I re-wrote the algorithm using binary search. We probably won't notice a speed difference right now, but it does mean we can handle a longer preload list.

        Attachment #633617 -
        Attachment is obsolete: true

        Attachment #633617 -
        Flags: review?(honzab.moz)

        Attachment #635031 -
        Flags: review?(honzab.moz)

    
    

    
  
Sweet!  I'll take a deeper look tomorrow.

    
    

    
  
Forigve me, I am confused. Why doesn't the patch just push the list into the permissions manager (i.e., what setstsstate does)? The issue of searching through the list and how to make that fast is a problem for all STS lookups, and not just the preload list, right?

    
    

    
  
That way involves detecting when the list needs reloading (new profile, clear site preferences, firefox update, etc.) which is hard to get right and takes time on startup.

    
    

    
  
Also, we have to be able to differentiate between items that the user wanted to remove (or add) and those that the preloaded list affects.

Consider when we push new preload lists to users.  Some users may have added x.com (by visting the site or using an add-on) to HSTS sites even though it was on the preload list.  Perhaps x.com asks to be removed from the list.  When the user gets an update to the preload list, we don't want to purge their "override" choice to add x.com to the list of HSTS sites.

Basically, this list should define defaults (when browser state is cleared, etc), but anything set by the user or the users' activities should be kept separate so we can override defaults with the users' customizations.

    
    

    
  
Isn't there a place where default settings are stored?  I am not sure why it would take time on startup.

Further, imagine you add site.com to the HSTS list, and now it is hardcoded in native code. Turns out someone somewhere messed up; and site.com doesn't work with HSTS. Now due to HSTS being turned on, I can't access the site anymore until the next firefox update (i.e., support can't tell me--'hey change the preference in about:permissions')?

    
    

    
  
@geekboy: ok. Although, I still think hardcoding a list in C++ code is not a great idea.

    
    

    
  
STS has a way of turning off STS--by setting the expiration time to zero in the HTTP header. At least initially, we should ensure that this mechanism works for the pre-loaded sites too.

Until we have some mechanism for updating the list independently of updating Firefox, it is OK to have it hard-coded in the C++ source code.

    
    

    
  
Comment on attachment 635031 [details] [diff] [review]
patch v2

Review of attachment 635031 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/manager/boot/src/nsStrictTransportSecurityService.cpp
@@ +36,5 @@
> +  public:
> +    const char *mHost;
> +    const PRUint32 mLength;
> +    const bool mIncludeSubdomains;
> +};

While you have renamed the include file, add this class def into it as well please.

@@ +313,5 @@
> +{
> +  const char *keyStr = (const char *)key;
> +  const nsSTSPreload *preloadEntry = (const nsSTSPreload *)entry;
> +  return memcmp(keyStr, preloadEntry->mHost, 
> +                NS_MAX((PRUint32)strlen(keyStr), preloadEntry->mLength));

Hmm.. shouldn't there be strcmp rather?  Since you are using NS_MAX, you can read past the memory of keyStr or mHost, can't you?

Maybe I'm wrong, but then when you would use NS_MIN and would have a host in the list like "www.foo.com.www.bar.com" then test for "www.foo.com" would pass, am I right?

So, please change this to strcmp.

        Attachment #635031 -
        Flags: review?(honzab.moz)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch v3 (obsolete)
        — 
          — Splinter Review
      

    


  
Again, thanks for the quick review.
I also took the string lengths out of the list because we don't need them.

        Attachment #635031 -
        Attachment is obsolete: true

        Attachment #635522 -
        Flags: review?(honzab.moz)

    
    

    
  
Comment on attachment 635522 [details] [diff] [review]
patch v3

Review of attachment 635522 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab

Interesting that the test didn't catch the memcmp issue.

        Attachment #635522 -
        Flags: review?(honzab.moz) → review+

    
    

    
  
Try run: https://tbpl.mozilla.org/?tree=Try&rev=08fcbfe74765

My understanding of memcmp is that it exits upon finding the first differing byte. When one string is shorter than the other, the terminating null will be different, so the behaviour would be correct. However, I agree that strcmp is the best function to use here.

    
    

    
  
Please add a test that a Strict-Transport-Security header with a 0 expiration time undoes HSTS for a preloaded site.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch v4 (obsolete)
        — 
          — Splinter Review
      

    


  
To make a header with max-age=0 override the preload list for more than just one run of firefox (which would be easy), I changed the semantics of the sts permissions slightly. UNKNOWN_ACTION means we've never seen any sts header relating to that host. ALLOW_ACTION means it is an sts host. DENY_ACTION means max-age=0 has been seen at some point, and it potentially overrides an entry in the preload list. Of course, if another header is seen, the max-age=0 one is overwritten.
Any feedback/comments would be nice because I'm not sure this is what we want to do.

        Attachment #635522 -
        Attachment is obsolete: true

        Attachment #635982 -
        Flags: feedback?(sstamm)

    
    

    
  

      
      
      
      

        Attached file
          
        getPreloadList.py (obsolete)
        — 
      

    


  
This generates the preload file. (It'll be included in the patch when it gets checked in, but I'm uploading it for now so people can look at it.)

    
    

    
  
Comment on attachment 635982 [details] [diff] [review]
patch v4

We talked about this in depth in person earlier before... it's a good direction and I believe the plan is to extend the current processing model to have an additional layer (with knockout records in the PB cache as before, but also in the permission manager).

This would all be a bit simpler if we had only one permission for HSTS instead of two, but that's bug 607124.

    
    

    
  
Comment on attachment 635982 [details] [diff] [review]
patch v4

We talked about this in depth in person earlier before... it's a good direction and I believe the plan is to extend the current processing model to have an additional layer (with knockout records in the PB cache as before, but also in the permission manager).

This would all be a bit simpler if we had only one permission for HSTS instead of two, but that's bug 607124.

        Attachment #635982 -
        Flags: feedback?(sstamm)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch v5 (obsolete)
        — 
          — Splinter Review
      

    


  
This implements what we discussed today.

        Attachment #635982 -
        Attachment is obsolete: true

        Attachment #638037 -
        Flags: feedback?(sstamm)

    
    

    
  
Comment on attachment 638037 [details] [diff] [review]
patch v5

Review of attachment 638037 [details] [diff] [review]:
-----------------------------------------------------------------

I think this is looking really good.  I provide a few comments below.
I still need to go over the tests and the .py script yet, so not removing the feedback request.

::: security/manager/boot/src/nsStrictTransportSecurityService.cpp
@@ +18,5 @@
>  
> +// A note about the preload list:
> +// Since knockout values must be kept, any encountered permission value of
> +// nsIPermissionManager::DENY_ACTION indicates a negative assertion of
> +// STS state. Such situations must be favored over the preload list.

The phrase "knockout" might be unclear to others reading the code... I know you and I are immersed in that terminology, but others may not be.  Perhaps you could put a little note here about why we need "knockouts" (to override the preload list in case a user or web site wants to remove itself from the list of HSTS hosts).

@@ +317,5 @@
> +// domain of the original host. So, if aHost is in the preload list, we only
> +// return true if that host includes subdomains or if aHost is the exact host.
> +bool
> +nsStrictTransportSecurityService::IsHostInPreloadList(const char *aHost,
> +                                                      bool aIsExactHost)

Not sure if it makes more readability sense, but what do you think about calling this function "hasPermissionSet" and the second parameter is called  aCheckSubdomainsPerm?  Might be a bit more readable... "oh hey, this function looks to see if this host has the STS permission, and if the boolean flag is set, looks at subdomains instead".

@@ +367,5 @@
>  
> +  // First check the exact host. This involves first checking for an entry in
> +  // the private browsing table. If that entry exists, we don't want to check
> +  // in either the permission manager or the preload list. We only want to use
> +  // the stored permission if it is not a knockout entry, however.

Why did you remove the TestPermission() abstraction in this class and put all that code here?   Was there a reason you couldn't just modify that function to include the new logic?

@@ +375,5 @@
> +    if (!entry->mExpired &&
> +        entry->mStsPermission == nsIPermissionManager::ALLOW_ACTION) {
> +      *aResult = true;
> +      return NS_OK;
> +    }

You probably want to check for "knockout" entries too, and return early if you find one, right?  That would probably look the same as the if condition, but use ::DENY_ACTION, iirc.

@@ +384,5 @@
> +    STSLOG(("Found permission manager entry for %s", host.get()));
> +    if (permMgrPermission == nsIPermissionManager::ALLOW_ACTION) {
> +      *aResult = true;
> +      return NS_OK;
> +    }

same here, regarding "knockouts".  Seems to me that this code wouldn't stop looking for values if it found a knockout.

@@ +419,5 @@
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    // Do the same thing as with the exact host, except now we're looking at
> +    // ancestor domains of the original host. So, we have to look at the
> +    // include subdomains permissions.

You might wanna put a comment in here to indicate that you have to look for the "sts/use" permission, and then read the "sts/subd" permission only if there is a "sts/use" permission since there's not always a "/subd" permission when there's a "/use" and you must stop looking when you find a "/use".  Took me a while to remember what we talked about and figure this code out.

@@ +564,5 @@
>        entry->mIncludeSubdomains = true;
>      }
> +    if (strcmp(aType, STS_PERMISSION) == 0) {
> +      entry->mStsPermission = aPermission;
> +    }

Just so I understand correctly, this is because you've changed it so mDeleted is no longer used and the entry just keeps a DENY_ACTION when it's deleted (and thus a knockout)?

    
    

    
  
Comment on attachment 638037 [details] [diff] [review]
patch v5

Review of attachment 638037 [details] [diff] [review]:
-----------------------------------------------------------------

Tests look good!

::: security/manager/ssl/tests/unit/test_sts_preloadlist.js
@@ +10,5 @@
> +
> +function Observer() {}
> +Observer.prototype = {
> +  observe: function(subject, topic, data) {
> +    do_execute_soon(gNextTest);

When all the tests have completed, you should probably clean up any changes you made to the permission manager to avoid polluting the other tests.  Example: http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/tests/mochitest/stricttransportsecurity/test_sts_privatebrowsing.html?force=1#163

@@ +21,5 @@
> +function run_test() {
> +  do_test_pending();
> +
> +  // check that a host not in the list is not identified as an sts host
> +  do_check_true(!gSTSService.isStsHost("nonexistent.mozilla.com"));

you can use do_check_false(foo) here, instead of do_check_true(!foo).

@@ +72,5 @@
> +  do_check_true(gSTSService.isStsHost("another.subdomain.cert.se"));
> +  gSTSService.processStsHeader(uri, "max-age=1000");
> +  do_check_true(gSTSService.isStsHost("subdomain.cert.se"));
> +  do_check_true(gSTSService.isStsHost("sibling.cert.se"));
> +  do_check_true(!gSTSService.isStsHost("another.subdomain.cert.se"));

I was just about to write a comment suggesting this test... great.  Could you draw an ASCII diagram of the domain tree and mark which ones should be HSTS hosts and which should not?  It would help readers grok this test.

        Attachment #638037 -
        Flags: feedback?(sstamm) → feedback+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch v6 (obsolete)
        — 
          — Splinter Review
      

    


  
I've addressed Sid's feedback in this revision.
A few comments:
 - The IsHostInPreloadList function: I couldn't find a good way to make this function make sense, so I turned it into a simple lookup and made the caller responsible for correctly interpreting the data.
 - I removed TestPermission() because originally it was called twice: once for STS_PERMISSION and again for STS_SUBDOMAIN_PERMISSION. The new logic didn't lend itself well to doing two lookups in this way, and I felt the abstraction was no longer necessary or helpful.
 - We actually can't quit early when we find a knockout entry, because an ancestor domain might have a non-knockout entry with include subdomains set.

        Attachment #638037 -
        Attachment is obsolete: true

        Attachment #640381 -
        Flags: review?(honzab.moz)

    
    

    
  
Comment on attachment 640381 [details] [diff] [review]
patch v6

Review of attachment 640381 [details] [diff] [review]:
-----------------------------------------------------------------

Maybe call getPreloadList.py more specifically like getPreloadHSTSList.py

I didn't check the tests yet.

::: security/manager/boot/src/nsStrictTransportSecurityService.cpp
@@ +342,5 @@
> +  nsCAutoString host;
> +  nsresult rv = GetHost(aURI, host);
> +  if (NS_FAILED(rv)) return NS_OK;
> +
> +  nsSTSHostEntry *entry = nsnull;

Better name might be pbEntry.

@@ +346,5 @@
> +  nsSTSHostEntry *entry = nsnull;
> +  const nsSTSPreload *preload = nsnull;
> +  PRUint32 permMgrPermission;
> +  PRUint32 offset = 0;
> +  PRInt64 now = PR_Now() / 1000;

Isn't there some "k" for the 1000 in NSPR?

BTW, nicer is to define right before use.

@@ +356,5 @@
> +  // flag as expired any entries encountered that have expired.  We only
> +  // flag the nsSTSHostEntry because there could be some data in the
> +  // permission manager that -- if not in private mode -- would have been
> +  // overwritten by newly encountered STS data.
> +  if (entry && (now > entry->mExpireTime) && (entry->mExpireTime != 0)) {

maybe reverse now > entry->mExpireTime and entry->mExpireTime != 0 for a bit more optimal condition.

@@ +401,5 @@
> +  // Used for testing permissions as we walk up the domain tree.
> +  nsCOMPtr<nsIURI> domainWalkURI;
> +
> +  STSLOG(("no non-knockout data for %s found, walking up domain", host.get()));
> +  for (offset = host.FindChar('.', offset) + 1; offset > 0;

offset > 0; on a new line too please.

@@ +413,5 @@
> +      entry->mExpired = true;
> +    }
> +
> +    rv = NS_NewURI(getter_AddRefs(domainWalkURI),
> +                    NS_LITERAL_CSTRING("http://") + Substring(host, offset));

Indention.

Cache Substring(host, offset) and use instead of host.get() + offset.  Or just cache host.get() + offset, at least.

@@ +427,5 @@
> +    // include subdomains permissions (although we still have to check for the
> +    // STS_PERMISSION first to check that this is an sts host and not a
> +    // knockout entry - and again, if it is a knockout entry, we stop looking
> +    // for data on it and skip to the next higher up ancestor domain).
> +    if (entry != nsnull &&

I think != nsnull shouldn't be used.  Just do if (entry).

@@ +462,5 @@
> +    STSLOG(("no non-knockout data for %s found, walking up domain",
> +            host.get() + offset));
> +  }
> +
> +  // Use whatever we ended up with, which defaults to false.

With the code here *I think* the following undesired scenario is possible:
- set STS_SUBDOMAIN_PERMISSION to ALLOW_ACTION for example.com
- set STS_PERMISSION to DENY_ACTION for sub.example.com
- navigate to http://sub.example.com
- we will redirect to https://sub.example.com since there is a subdomain allow permission for example.com

The same scenario is possible when in PB mode.

Leaving r? because of this.

@@ +580,4 @@
>  
>      // Also refresh the expiration time.
>      entry->mExpireTime = aExpireTime;
> +    entry->mExpired = false;

Maybe having setter and getter like:
- SetExpireTime(et) {mET = et; mExpired = false;} 
- IsExpired() {return mExpired ? true : (mExpired = now > mET); }

At least because you are duplicating the code for this.

@@ +607,5 @@
>      nsSTSHostEntry* entry = mPrivateModeHostTable.GetEntry(aHost.get());
>  
> +    if (!entry) {
> +      entry = mPrivateModeHostTable.PutEntry(aHost.get());
> +      STSLOG(("Created private mode deleted mask for for %s", aHost.get()));

for for

@@ +613,5 @@
>  
> +    if (strcmp(aType, STS_PERMISSION) == 0) {
> +      entry->mStsPermission = nsIPermissionManager::DENY_ACTION;
> +    }
> +    if (strcmp(aType, STS_SUBDOMAIN_PERMISSION) == 0) {

else if ?

::: security/manager/tools/getPreloadList.py
@@ +31,5 @@
> +def filterComments(stream):
> +  lines = []
> +  for line in stream:
> +    if line.find("//") == -1:
> +      lines.append(line)

Hmm.. is there a simple way to just cut all after // ?

    
    

    
  
Comment on attachment 640381 [details] [diff] [review]
patch v6

No reaction, dropping r flag to clean my r q.

        Attachment #640381 -
        Flags: review?(honzab.moz)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch v7 (obsolete)
        — 
          — Splinter Review
      

    


  
Thanks for the review. I wasn't sure if you were going to comment on the tests, so I was waiting for that.

Regarding the scenario you describe, my understanding is that's the correct behavior according to the draft. When we receive "max-age=0" for sub.example.com, we stop regarding it as an sts host, meaning we forget any state relating to it. However, if we have a preload entry for example.com with includeSubdomains set (which is the equivalent of receiving a header from example.com with includeSubdomains set), then sub.example.com is an sts host according to that information (as is any other subdomain).

        Attachment #640381 -
        Attachment is obsolete: true

        Attachment #641972 -
        Flags: review?(honzab.moz)

    
    

    
  
Comment on attachment 641972 [details] [diff] [review]
patch v7

Review of attachment 641972 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to David Keeler from comment #30)
> Created attachment 641972 [details] [diff] [review]
> patch v7
> 
> Thanks for the review. I wasn't sure if you were going to comment on the
> tests, so I was waiting for that.
> 
> Regarding the scenario you describe, my understanding is that's the correct
> behavior according to the draft. When we receive "max-age=0" for
> sub.example.com, we stop regarding it as an sts host, meaning we forget any
> state relating to it. However, if we have a preload entry for example.com
> with includeSubdomains set (which is the equivalent of receiving a header
> from example.com with includeSubdomains set), then sub.example.com is an sts
> host according to that information (as is any other subdomain).

Yes, you are right.  Thanks for the explanation.

r=honzab, just check on the comments please.

::: security/manager/boot/src/nsStrictTransportSecurityService.cpp
@@ +395,5 @@
> +  STSLOG(("no non-knockout data for %s found, walking up domain", host.get()));
> +  PRUint32 offset = 0;
> +  for (offset = host.FindChar('.', offset) + 1;
> +       offset > 0;
> +       offset = host.FindChar('.', offset) + 1) {

Sorry not to mention this the last time, but how is this code gonna behave for FQDN (with the trailing dot) ?

Could you please test?  I think the condition offset > 0 should be offset > 1 because of that, otherwise subdomain will be an empty string.  I don't think you want to execute the loop with it.

::: security/manager/boot/src/nsStrictTransportSecurityService.h
@@ +98,5 @@
> +    }
> +
> +    bool IsExpired()
> +    {
> +      if (mExpired || mExpireTime == 0) {

Why also mExpireTime == 0 ?  I think it is always set to some value since it is always set to NOW + MaxAge.  I wonder why you actually need mExpired flag when you can just drop mExpireTime to zero to remember the expiration.

::: security/manager/ssl/tests/unit/test_bug627234.js
@@ +24,5 @@
> +  // (we have to remove any state added to the sts service so as to not muck
> +  // with other tests).
> +  var uri = Services.io.newURI("http://localhost", null, null);
> +  gSTSService.removeStsState(uri);
> +}

Is this just an improved version of the test in bug 627234?  Shouldn't be this file in that patch rather?

::: security/manager/ssl/tests/unit/test_sts_preloadlist.js
@@ +59,5 @@
> +  do_check_false(gSTSService.isStsHost("subdomain.epoxate.com"));
> +  // check that an entry at the end of the list is an sts host
> +  do_check_true(gSTSService.isStsHost("www.googlemail.com"));
> +  // check that a subdomain is not an sts host (includeSubdomains is not set)
> +  do_check_false(gSTSService.isStsHost("a.subdomain.www.googlemail.com"));

Some blank lines would help :)

        Attachment #641972 -
        Flags: review?(honzab.moz) → review+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch v8 (obsolete)
        — 
          — Splinter Review
      

    


  
Thanks again for the review (carrying over r+ for minor changes).

I fixed the issue with potentially searching on an empty string, but the problem with FQDNs is larger than that. I'll file a follow-up bug to fix it.

I added a comment about mExpireTime. (TL;DR: mExpireTime == 0 means the entry does not expire.)

I took out the test for bug 627234. I had it in because if this patch lands first, it'll fix that bug. We'll just need to make sure the test gets in the code base one way or another.

        Attachment #641972 -
        Attachment is obsolete: true

        Attachment #643033 -
        Flags: review+

        Attachment #637600 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch v9 (obsolete)
        — 
          — Splinter Review
      

    


  
Had to re-base. Also, bsmith asked for more intuitive permission names.

        Attachment #643033 -
        Attachment is obsolete: true

        Attachment #643033 -
        Flags: review?(bsmith)

        Attachment #650682 -
        Flags: review?(bsmith)

    
    

    
  
Comment on attachment 650682 [details] [diff] [review]
patch v9

Review of attachment 650682 [details] [diff] [review]:
-----------------------------------------------------------------

when you make the change to the normalization of HTTP/HTTPS, and the change to the includesSubdomains/max-age stuff, and modified the test appropriately, have sstamm review that change. r+ assuming the nits and sstamm's r+ on those changes.

::: security/manager/boot/src/nsStrictTransportSecurityService.cpp
@@ +27,5 @@
> +// PERMISSION_KNOCKOUT_HSTS (nsIPermissionManager::DENY_ACTION).
> +#include "nsSTSPreloadList.inc"
> +
> +#define PERMISSION_IS_HSTS nsIPermissionManager::ALLOW_ACTION
> +#define PERMISSION_IS_NOT_HSTS nsIPermissionManager::UNKNOWN_ACTION

s/PERMISSION_/STS_/
s/IS_NOT_HSTS/STS_UNSET/

@@ +28,5 @@
> +#include "nsSTSPreloadList.inc"
> +
> +#define PERMISSION_IS_HSTS nsIPermissionManager::ALLOW_ACTION
> +#define PERMISSION_IS_NOT_HSTS nsIPermissionManager::UNKNOWN_ACTION
> +#define PERMISSION_KNOCKOUT_HSTS nsIPermissionManager::DENY_ACTION

Nit: use parentheses around the value of the macros.

@@ +348,5 @@
> +// data. May return null.
> +const nsSTSPreload *
> +nsStrictTransportSecurityService::GetPreloadListEntry(const char *aHost)
> +{
> +  return (const nsSTSPreload *)bsearch(aHost,

Nit: space before bsearch.

@@ +350,5 @@
> +nsStrictTransportSecurityService::GetPreloadListEntry(const char *aHost)
> +{
> +  return (const nsSTSPreload *)bsearch(aHost,
> +                                       kSTSPreloadList,
> +                                       kSTSPreloadListSize,

just use PR_ARRAY_SIZE(kSTSPreloadList)

@@ +359,2 @@
>  NS_IMETHODIMP
>  nsStrictTransportSecurityService::IsStsURI(nsIURI* aURI, bool* aResult)

Make sure we are processing the data consistently with https:// or http:// URIs as the key into the permission manager.

@@ +367,5 @@
> +  *aResult = false;
> +
> +  nsCAutoString host;
> +  nsresult rv = GetHost(aURI, host);
> +  if (NS_FAILED(rv)) return NS_OK;

Nit: GetHost should always succeed, so NS_ENSURE_SUCCESS(rv, rv) is better.

@@ +423,5 @@
> +  nsCOMPtr<nsIURI> domainWalkURI;
> +  nsCOMPtr<nsIPrincipal> domainWalkPrincipal;
> +  const char *subdomain;
> +
> +  STSLOG(("no non-knockout data for %s found, walking up domain", host.get()));

no HSTS entry for %s found

@@ +441,5 @@
> +      pbEntry = mPrivateModeHostTable.GetEntry(subdomain);
> +    }
> +
> +    rv = NS_NewURI(getter_AddRefs(domainWalkURI),
> +                   NS_LITERAL_CSTRING("http://") + Substring(host, offset));

Are we supposed to be using "http://" or "https://" here?

@@ +489,5 @@
> +        break;
> +      }
> +    }
> +
> +    STSLOG(("no non-knockout data for %s found, walking up domain", subdomain));

no HSTS data for %s found...

@@ +630,4 @@
>      NS_ENSURE_SUCCESS(rv, rv);
>  
>      nsCOMPtr<nsIPrincipal> principal;
>      rv = GetPrincipalForURI(uri, getter_AddRefs(principal));

Normalization of https/http URIs here too.

@@ +646,5 @@
>      // rolled back when exiting private mode.
>      nsSTSHostEntry* entry = mPrivateModeHostTable.GetEntry(aHost.get());
>  
> +    if (!entry) {
> +      entry = mPrivateModeHostTable.PutEntry(aHost.get());

can entry become null here? If so, we have null pointer deref below. Ideally we'd use an infallable data structure to avoid this issue (if there is one).

::: security/manager/boot/src/nsStrictTransportSecurityService.h
@@ +28,5 @@
>  // nsPermissionManager.cpp, but specific to private-mode caching of STS
>  // permissions.
>  //
>  // Each nsSTSHostEntry contains:
> +//  - Expiry time (PRInt64, milliseconds)

PRTime, not PRInt64

@@ +30,5 @@
>  //
>  // Each nsSTSHostEntry contains:
> +//  - Expiry time (PRInt64, milliseconds)
> +//  - Expired flag (bool, default false)
> +//  - STS permission (PRUint32, default nsIPermissionManager::UNKNOWN_ACTION)

s/UKNOWN_ACTION/...

@@ +34,5 @@
> +//  - STS permission (PRUint32, default nsIPermissionManager::UNKNOWN_ACTION)
> +//  - Include subdomains flag (bool, default false)
> +//
> +// Note: the subdomains flag has no meaning if the STS permission is
> +// nsIPermissionManager::UNKNOWN_ACTION.

rename to easier to understand name you already did elsewhere.

@@ +105,5 @@
> +      if (mExpired || mExpireTime == 0) {
> +        return mExpired;
> +      }
> +
> +      PRInt64 now = PR_Now() / PR_USEC_PER_MSEC;

PRTime

::: security/manager/ssl/tests/unit/test_sts_preloadlist.js
@@ +1,1 @@
> +var Cc = Components.classes;

Please add new tests for the bugs we found during the code review, regarding how includeSubdomains is inherited.

::: security/manager/tools/getHSTSPreloadList.py
@@ +1,1 @@
> +#!/usr/bin/python

License header.

@@ +24,5 @@
> +static const nsSTSPreload kSTSPreloadList[] = {
> +"""
> +POSTFIX = """};
> +
> +static const PRUint32 kSTSPreloadListSize = PR_ARRAY_SIZE(kSTSPreloadList);

Please remove this.

@@ +68,5 @@
> +        if 'include_subdomains' in entry and entry['include_subdomains']:
> +          line = line + "true },\n"
> +        else:
> +          line = line + "false },\n"
> +        lines.append(line)

# We must sort the list by full domain name because we do a binary search on it when searching.

        Attachment #650682 -
        Flags: review?(bsmith)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch v10 (obsolete)
        — 
          — Splinter Review
      

    


  
Addressed comments, but stuck with original functionality regarding the max-age=0/includeSubdomains issue discussed on the mailing list. Requesting review again just as a signal that we can go ahead with this.

        Attachment #650682 -
        Attachment is obsolete: true

        Attachment #654414 -
        Flags: review?(bsmith)

    
    

    
  
Comment on attachment 654414 [details] [diff] [review]
patch v10

Review of attachment 654414 [details] [diff] [review]:
-----------------------------------------------------------------

Good work.

::: security/manager/boot/src/nsStrictTransportSecurityService.cpp
@@ +23,5 @@
> +// regarding the sts state of this host" (any ancestor of "this host" can still
> +// influence its sts status via include subdomains, however).
> +// This prevents the preload list from overriding the site's current
> +// desired sts status. Knockout values are indicated by permission values of
> +// STS_KNOCKOUT (nsIPermissionManager::DENY_ACTION).

Nit: I would remove the parenthetical comment (nsIPermissionManager::DENY_ACTION).

@@ +125,5 @@
> +  nsCOMPtr<nsIScriptSecurityManager> securityManager =
> +     do_GetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID, &rv);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  // We have to normalize the scheme of the URIs we're using, so just use https

Add:
// HSTS information is shared across all ports for a given host.

@@ +362,5 @@
>    // Should be called on the main thread (or via proxy) since the permission
>    // manager is used and it's not threadsafe.
>    NS_ENSURE_TRUE(NS_IsMainThread(), NS_ERROR_UNEXPECTED);
>  
> +  // set default for if we can't find any STS information

Nit: s/for if/in case/

        Attachment #654414 -
        Flags: review?(bsmith) → review+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch v11
        — 
          — Splinter Review
      

    


  
Fixed nits; carrying over r+.
Try run: https://tbpl.mozilla.org/?tree=Try&rev=87208277228c

        Attachment #654414 -
        Attachment is obsolete: true

        Attachment #655161 -
        Flags: review+

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/ce222ba667f2
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Depends on: 785860

    
  
Depends on: 786417

    
  
Keywords: sec-want

    
  
Depends on: 801828
Depends on: 803236

    
    

    
  
Is this enabled in Firefox 17 beta? I just tested with several of the sites listed in

https://hg.mozilla.org/mozilla-central/rev/ce222ba667f2

and in all cases the initial connection was still made over HTTP.

    
    

    
  
The current content of this list is:
http://mxr.mozilla.org/mozilla-central/source/security/manager/boot/src/nsSTSPreloadList.inc

We removed the entries for the sites that were not publishing an >=18-week max-age on their Strict-Transport-Security header. See bug 786417.

    
  
Alias: preload-hsts

    
  
Depends on: 836898
Depends on: 828873

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: