Record and report better statistics on HSTS cache entries

RESOLVED FIXED in Firefox 56

Status

()

P1
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: kmckinley, Assigned: kmckinley)

Tracking

Trunk
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [domsecurity-active])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

a year ago
The HSTS cache consists of two "groups" of cached entries, the HSTS preload list and the entries seen in the wild.

In order to better understand the impact of HSTS priming, this bug should 1) add the concept of where the HSTS cache entry originates, and 1) report this as daily telemetry. This should be something like number of entries in preload list, number in cache seen organically, number in cache seen as a result of HSTS priming.

See Bug 1359987
(Assignee)

Comment 1

a year ago
Also update the HTTP_SCHEME_UPGRADE (or create new telemetry probe) to understand where HSTS upgrades originate, from preload, organically, HSTS priming.
Assignee: nobody → kmckinley
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: [domsecurity-active]
(Assignee)

Updated

a year ago
Blocks: 1365432
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8875479 - Flags: review?(francois)

Comment 3

a year ago
mozreview-review
Comment on attachment 8875479 [details]
Bug 1363546 - Store and report HSTS upgrade source  p=francois

https://reviewboard.mozilla.org/r/146920/#review151008

::: toolkit/components/telemetry/Histograms.json:10139
(Diff revision 1)
>      "description": "The amount of time required for HSTS priming requests (ms), keyed by success or failure of the priming request. (success, failure)"
>    },
> +  "HSTS_UPGRADE_SOURCE": {
> +    "record_in_processes": [ "main" ],
> +    "alert_emails": ["seceng-telemetry@mozilla.com"],
> +    "bug_numbers": [1359987],

Did you mean to use bug 1359987 or bug 1363546?

::: toolkit/components/telemetry/Histograms.json:10143
(Diff revision 1)
> +    "alert_emails": ["seceng-telemetry@mozilla.com"],
> +    "bug_numbers": [1359987],
> +    "expires_in_version": "62",
> +    "kind": "enumerated",
> +    "n_values": 8,
> +    "description": "0: HSTS preload list, 1: HSTS Header seen naturally, 2: HSTS priming"

Can you please expand on that description and make it clear what it's measuring and when the measurement is taken?

Do you mean something like: "Note the HSTS status of a TLD every time we open a channel (0: ..."
Attachment #8875479 - Flags: review?(francois) → review-
Comment hidden (mozreview-request)

Comment 5

a year ago
mozreview-review
Comment on attachment 8875479 [details]
Bug 1363546 - Store and report HSTS upgrade source  p=francois

https://reviewboard.mozilla.org/r/146920/#review151026

In general I'm on board with this, but I think we shouldn't overload the API in this way. See in particular the third comment, but long story short is I think we should just add another parameter (as well as some constants in nsISiteSecurityService) that describes the source of the information.

::: security/manager/ssl/nsISiteSecurityService.idl:142
(Diff revision 2)
>                               in nsIURI aSourceURI,
>                               in ACString aHeader,
>                               in nsISSLStatus aSSLStatus,
>                               in uint32_t aFlags,
>                               in const_OriginAttributesRef aOriginAttributes,
> +                             in boolean aIsHSTSPriming,

See below, but I think this should be a more generalized parameter (backed by uint32_t constants because idl) that tells the backend where this data came from.

::: security/manager/ssl/nsISiteSecurityService.idl:206
(Diff revision 2)
>       */
>      [binaryname(IsSecureURI), noscript, must_use]
>      boolean isSecureURINative(in uint32_t aType, in nsIURI aURI,
>                                in uint32_t aFlags,
>                                in const_OriginAttributesRef aOriginAttributes,
> -                              [optional] out boolean aCached);
> +                              [optional] out uint32_t aSecureFlags);

I don't think having this flags field mean both "was this cached?" as well as "where did this come from?" will serve us well in the future in terms of maintaining and extending this code. Let's keep the cached boolean and just add a new parameter like "source" that can be one of {preloaded, priming, organic}.

::: security/manager/ssl/nsSiteSecurityService.cpp:669
(Diff revision 2)
>    const OriginAttributes& aOriginAttributes)
>  {
>    nsAutoCString hostname;
>    nsresult rv = GetHost(aSourceURI, hostname);
>    NS_ENSURE_SUCCESS(rv, rv);
> +  // SecurityPropertyNegative results only come rom HSTS priming

"from"

::: security/manager/ssl/nsSiteSecurityService.cpp:1415
(Diff revision 2)
> -        if (aCached) {
> +        if (aSecureFlags) {
>            // Only set cached if this includes subdomains
> -          *aCached = aRequireIncludeSubdomains ? siteState->mHSTSIncludeSubdomains
> +          bool cached = aRequireIncludeSubdomains ? siteState->mHSTSIncludeSubdomains
> -                                               : true;
> +                                                  : true;
> +          SSSLOG(("Marking HSTS as %s and %s from priming",
> +                (cached) ? "cached" : "not cached",

nit: indent these two lines two more spaces

::: security/manager/ssl/nsSiteSecurityService.cpp:1417
(Diff revision 2)
> -          *aCached = aRequireIncludeSubdomains ? siteState->mHSTSIncludeSubdomains
> +          bool cached = aRequireIncludeSubdomains ? siteState->mHSTSIncludeSubdomains
> -                                               : true;
> +                                                  : true;
> +          SSSLOG(("Marking HSTS as %s and %s from priming",
> +                (cached) ? "cached" : "not cached",
> +                (siteState->mIsHSTSPriming) ? "is" : "is not"
> +                ));

nit: put this on the previous line

::: security/manager/ssl/nsSiteSecurityService.cpp:1430
(Diff revision 2)
> -        return true;
>        } else if (siteState->mHSTSState == SecurityPropertyNegative) {
>          *aResult = false;
> -        if (aCached) {
> +        if (aSecureFlags) {
>            // if it's negative, it is always cached
> -          *aCached = true;
> +          SSSLOG(("Marking HSTS as as cached (SecurityPropertyNegative)"));

nit: extra 'as'

::: security/manager/ssl/nsSiteSecurityService.cpp:1475
(Diff revision 2)
> -        if (aCached) {
> +        if (aSecureFlags) {
>            // Only set cached if this includes subdomains
> -          *aCached = aRequireIncludeSubdomains ? dynamicState->mHSTSIncludeSubdomains
> +          bool cached = aRequireIncludeSubdomains ? siteState->mHSTSIncludeSubdomains
> -                                               : true;
> +                                                  : true;
> +          SSSLOG(("Marking HSTS as %s and is from preload",
> +                (cached) ? "cached" : "not cached"));

nit: indentation

::: security/manager/ssl/nsSiteSecurityService.cpp:1487
(Diff revision 2)
>          return true;
>        } else if (dynamicState->mHSTSState == SecurityPropertyNegative) {
>          *aResult = false;
> -        if (aCached) {
> +        if (aSecureFlags) {
>            // if it's negative, it is always cached
> -          *aCached = true;
> +          SSSLOG(("Marking HSTS as as cached (SecurityPropertyNegative)"));

nit: extra 'as'

::: security/manager/ssl/nsSiteSecurityService.cpp
(Diff revision 2)
>                   aType == nsISiteSecurityService::HEADER_HPKP,
>                   NS_ERROR_NOT_IMPLEMENTED);
>  
>    // set default in case if we can't find any STS information
>    *aResult = false;
> -  if (aCached) {

I think aSecureFlags can be uninitialized in some cases without something like this.
Attachment #8875479 - Flags: review?(dkeeler) → review-

Comment 6

a year ago
mozreview-review
Comment on attachment 8875479 [details]
Bug 1363546 - Store and report HSTS upgrade source  p=francois

https://reviewboard.mozilla.org/r/146920/#review151406

::: security/manager/ssl/nsISiteSecurityService.idl:47
(Diff revision 2)
>     * SECURITY_PROPERTY_SET and SECURITY_PROPERTY_UNSET correspond to indicating
>     * a site has or does not have the security property in question,
>     * respectively.
>     * SECURITY_PROPERTY_KNOCKOUT indicates a value on a preloaded
>     * list is being overridden, and the associated site does not have the
>     * security property in question.

Also, on line 199 of this file, a '*' got replaced by what looks like a malformed utf-8 character. I can't comment on it because apparently the bugzilla API rejects it.
rb is broken...

r+ but only for the network bits.  I am not the right reviewer for anything else in the patch.  I agree with david's comments on leaving the bool arg instead of introducing flags.

There is some weird char in the patch (security/manager/ssl/nsISiteSecurityService.idl)
Comment on attachment 8875479 [details]
Bug 1363546 - Store and report HSTS upgrade source  p=francois

See comment 7
Attachment #8875479 - Flags: review?(honzab.moz) → review+

Comment 9

a year ago
mozreview-review
Comment on attachment 8875479 [details]
Bug 1363546 - Store and report HSTS upgrade source  p=francois

https://reviewboard.mozilla.org/r/146920/#review151780

r+ but only for the network bits.  I am not the right reviewer for anything else in the patch.  I agree with david's comments on leaving the bool arg instead of introducing flags.

::: security/manager/ssl/nsSiteSecurityService.h:202
(Diff revision 2)
>                                 uint32_t aFlags, bool aIsPreload,
>                                 const OriginAttributes& aOriginAttributes);
>    bool HostHasHSTSEntry(const nsAutoCString& aHost,
>                          bool aRequireIncludeSubdomains, uint32_t aFlags,
>                          const OriginAttributes& aOriginAttributes,
> -                        bool* aResult, bool* aCached);
> +                        bool* aResult, uint32_t* aSecureFlags);

probably not the best name?
Comment hidden (mozreview-request)
(Assignee)

Comment 11

a year ago
Francois, can you take a look at this one again as well. Thanks.
Flags: needinfo?(francois)
Attachment #8875479 - Flags: review?(dkeeler)

Comment 12

a year ago
mozreview-review
Comment on attachment 8875479 [details]
Bug 1363546 - Store and report HSTS upgrade source  p=francois

https://reviewboard.mozilla.org/r/146920/#review153086

datareview+
Attachment #8875479 - Flags: review+
Flags: needinfo?(francois)

Comment 13

a year ago
mozreview-review
Comment on attachment 8875479 [details]
Bug 1363546 - Store and report HSTS upgrade source  p=francois

https://reviewboard.mozilla.org/r/146920/#review153068

Cool - I think this approach looks good. I still have some questions, though. Also, there are a few nits to address. In particular, I'd appreciate it if you could go through these changes and make sure long argument lists are formatted properly. That is, no line should be more than 80 characters, but each line should be as long as possible. I realize this is tedious, but I think maintaining the visual consistency of the source code is helpful for future ease of understanding. (Don't worry about lines that this patch doesn't change that are already over 80 characters - it's probably not worth touching them.)

::: commit-message-03252:5
(Diff revision 3)
> +Bug 1363546 - Store and report HSTS upgrade source r?keeler,mayhemer
> +
> +Add a flag to the HSTS cache which is true if the entry was first seen
> +by HSTS priming.
> +Change the aCached parameter to uint32_t aSecureFlags to avoid bloating

Looks like the commit message needs updating.

::: netwerk/base/nsNetUtil.cpp:2603
(Diff revision 3)
>      if (isStsHost) {
>        LOG(("nsHttpChannel::Connect() STS permissions found\n"));
>        if (aAllowSTS) {
>          Telemetry::Accumulate(Telemetry::HTTP_SCHEME_UPGRADE, 3);
>          aShouldUpgrade = true;
> +        if (hstsSource & nsISiteSecurityService::PRELOAD_LIST) {

See later comments, but if these are discrete values rather than flags, let's use == instead of the binary &.

::: security/manager/ssl/nsISiteSecurityService.idl:95
(Diff revision 3)
>      const uint32_t ERROR_NO_BACKUP_PIN = 12;
>      const uint32_t ERROR_COULD_NOT_SAVE_STATE = 13;
>      const uint32_t ERROR_ROOT_NOT_BUILT_IN = 14;
>  
>      /**
> +     * nsISiteSecurityService::IsSecureURI can optionally return a set of flags

Are these really flags, though? They seem exclusive in theory and in this implementation (i.e. no more than one bit ever seems to be set).

::: security/manager/ssl/nsISiteSecurityService.idl:99
(Diff revision 3)
>      /**
> +     * nsISiteSecurityService::IsSecureURI can optionally return a set of flags
> +     * indicating the source of the HSTS cache entry, if it comes from the
> +     * preload list, was seen naturally, or is a result of HSTS priming.
> +     */
> +    const uint32_t NO_HEADER       = 0x0;

If we go with the discrete values rather than flags implementation, let's have these be 0, 1, 2, 3.
Also maybe prefix the names with SOURCE_?
Finally, maybe UNKNOWN or UNSET instead of NO_HEADER? (since it could also be that it came from a profile before this change, for example)

::: security/manager/ssl/nsISiteSecurityService.idl:128
(Diff revision 3)
>       *                          (note that this implementation does not isolate
>       *                          by userContextId because of the risk of man-in-
>       *                          the-middle attacks before trust-on-second-use
>       *                          happens).
> +     * @param aSource the source of the header, whether it was from the preload
> +     *                list, an organic header, or HSTS priming.

"... or none of the above"

::: security/manager/ssl/nsISiteSecurityService.idl:202
(Diff revision 3)
>       * @param aOriginAttributes the origin attributes that isolate this origin,
>       *                          (note that this implementation does not isolate
>       *                          by userContextId because of the risk of man-in-
>       *                          the-middle attacks before trust-on-second-use
>       *                          happens).
> -     * @param aCached true if we have cached information regarding whether or not
> +     * @param aCached true if we have cached information about this host, even

Nice - I think this is a good comment update.

::: security/manager/ssl/nsISiteSecurityService.idl:204
(Diff revision 3)
>       *                          by userContextId because of the risk of man-in-
>       *                          the-middle attacks before trust-on-second-use
>       *                          happens).
> -     * @param aCached true if we have cached information regarding whether or not
> -     *                  the host is HSTS, false otherwise.
> +     * @param aCached true if we have cached information about this host, even
> +     *                if the security state is false.
> +     * @param aSourceFlags flags indicating the source of the HSTS entry,

Again, probably not actually flags. Also, include the "none of the above" case.

::: security/manager/ssl/nsSiteSecurityService.cpp:663
(Diff revision 3)
>      SSSLOG(("SSS: storing HSTS site entry for %s", hostname.get()));
> +    nsCString value = mSiteStateStorage->Get(storageKey, storageType);
> +    RefPtr<SiteHSTSState> curSiteState =
> +      new SiteHSTSState(hostname, aOriginAttributes, value);
> +    if (curSiteState->mHSTSState == SecurityPropertyUnset) {
> +      value = mPreloadStateStorage->Get(storageKey, storageType);

I'm not sure I understand the rationale for falling back to looking for the entry's source in the dynamic preload list. Why are we doing this here?

::: security/manager/ssl/nsSiteSecurityService.cpp:851
(Diff revision 3)
> +      return NS_ERROR_INVALID_ARG;
> +  }
>  
>    NS_ENSURE_ARG(aSSLStatus);
>    return ProcessHeaderInternal(aType, aSourceURI, PromiseFlatCString(aHeader),
> -                               aSSLStatus, aFlags, aOriginAttributes, aMaxAge,
> +                               aSSLStatus, aFlags, aOriginAttributes,

nit: these arguments can be reorganized with respect to the 80 character line limit

::: security/manager/ssl/nsSiteSecurityService.cpp:921
(Diff revision 3)
>    }
>  
>    switch (aType) {
>      case nsISiteSecurityService::HEADER_HSTS:
> -      rv = ProcessSTSHeader(aSourceURI, aHeader, aFlags, aOriginAttributes, aMaxAge,
> +      rv = ProcessSTSHeader(aSourceURI, aHeader, aFlags, aOriginAttributes,
> +                            aSource, aMaxAge,

nit: same here

::: security/manager/ssl/nsSiteSecurityService.cpp:1302
(Diff revision 3)
>    NS_ENSURE_SUCCESS(rv, rv);
>  
>    // record the successfully parsed header data.
>    rv = SetHSTSState(aType, hostname.get(), maxAge, foundIncludeSubdomains,
> -                    aFlags, SecurityPropertySet, false, aOriginAttributes);
> +                    aFlags, SecurityPropertySet, aOriginAttributes,
> +                    aSource);

nit: looks like this can go on the previous line

::: security/manager/ssl/nsSiteSecurityService.cpp:1413
(Diff revision 3)
>    const nsAutoCString& aHost, bool aRequireIncludeSubdomains, uint32_t aFlags,
> -  const OriginAttributes& aOriginAttributes, bool* aResult, bool* aCached)
> +  const OriginAttributes& aOriginAttributes, bool* aResult, bool* aCached,
> +  uint32_t* aSource)
>  {
> +  if (aSource) {
> +    *aSource = 0;

SourceNone?

::: security/manager/ssl/nsSiteSecurityService.cpp:1449
(Diff revision 3)
>          *aResult = aRequireIncludeSubdomains ? siteState->mHSTSIncludeSubdomains
>                                               : true;
>          if (aCached) {
>            // Only set cached if this includes subdomains
>            *aCached = aRequireIncludeSubdomains ? siteState->mHSTSIncludeSubdomains
> -                                               : true;
> +                                                  : true;

nit: indentation

::: security/manager/ssl/nsSiteSecurityService.cpp:1462
(Diff revision 3)
> +          SSSLOG(("Marking HSTS as as cached (SecurityPropertyNegative)"));
>            *aCached = true;
>          }
> -        return true;
>        }
> +      return true;

I don't think hoisting this is correct - siteState->mHSTSState could have a value other than SecurityPropertySet or SecurityPropertyNegative, and it would be a change of behavior to unconditionally return early in those cases.

::: security/manager/ssl/nsSiteSecurityService.cpp:1511
(Diff revision 3)
>          return true;
>        } else if (dynamicState->mHSTSState == SecurityPropertyNegative) {
>          *aResult = false;
>          if (aCached) {
>            // if it's negative, it is always cached
> -          *aCached = true;
> +          SSSLOG(("Marking HSTS as as cached (SecurityPropertyNegative)"));

nit: "as as"

::: security/manager/ssl/nsSiteSecurityService.cpp:1605
(Diff revision 3)
>        StringEndsWith(host, NS_LITERAL_CSTRING(".chart.apis.google.com"))) {
>      if (aCached) {
>        *aCached = true;
>      }
> +    if (aSource) {
> +      *aSource |= nsISiteSecurityService::PRELOAD_LIST;

I think just assignment here instead of bit operations.

::: security/manager/ssl/nsSiteSecurityService.cpp:1611
(Diff revision 3)
> +    }
>      return NS_OK;
>    }
>  
>    // First check the exact host.
> -  if (HostHasHSTSEntry(host, false, aFlags, aOriginAttributes, aResult, aCached)) {
> +  if (HostHasHSTSEntry(host, false, aFlags, aOriginAttributes, aResult, aCached, aSource)) {

nit: long line

::: security/manager/ssl/nsSiteSecurityService.cpp:1810
(Diff revision 3)
>  
>    const nsCString& flatHost = PromiseFlatCString(aHost);
>    nsAutoCString host(
>      PublicKeyPinningService::CanonicalizeHostname(flatHost.get()));
>    return SetHSTSState(nsISiteSecurityService::HEADER_HSTS, host.get(), aExpires,
> -                      aIncludeSubdomains, 0, SecurityPropertySet, true,
> +                      aIncludeSubdomains, 0, SecurityPropertySet, 

nit: trailing whitespace
Attachment #8875479 - Flags: review-
Comment hidden (mozreview-request)

Comment 15

a year ago
mozreview-review
Comment on attachment 8875479 [details]
Bug 1363546 - Store and report HSTS upgrade source  p=francois

https://reviewboard.mozilla.org/r/146920/#review154118

Cool - I think this is almost ready to go. My main concern is about the source parameter to processHeader being optional. See the second comment for that. Other than that, just some nits and minor issues.

::: netwerk/base/nsNetUtil.cpp:2587
(Diff revision 4)
>      // enforce Strict-Transport-Security
>      nsISiteSecurityService* sss = gHttpHandler->GetSSService();
>      NS_ENSURE_TRUE(sss, NS_ERROR_OUT_OF_MEMORY);
>  
>      bool isStsHost = false;
> +    bool isCached = false;

nit: doesn't look like we're actually using the result of isCached, so we probably don't need it

::: security/manager/ssl/nsISiteSecurityService.idl:158
(Diff revision 4)
>                         in nsIURI aSourceURI,
>                         in ACString aHeader,
>                         in nsISSLStatus aSSLStatus,
>                         in uint32_t aFlags,
>                         [optional] in jsval aOriginAttributes,
> +                       [optional] in uint32_t aSource,

Looking at this closer, I'm not sure it makes sense for this to be optional. The caller should probably specify it. Let's take off the optional annotation and put it before aOriginAttributes (we should probably change the order in processHeaderNative to be consistent).

In fact, if this is optional the current implementation makes test_sts_ipv4_ipv6.js fail silently (that is, the test itself isn't sufficient, but these changes make the test stop testing what we think it's testing). We should probably add this to that test:

```
notEqual(parsedMaxAge.value, undefined);
notEqual(parsedIncludeSubdomains.value, undefined);

```

Also note that there is at least one caller of processHeader that isn't in tests or the platform itself:
https://dxr.mozilla.org/mozilla-central/rev/da66c4a05fda49d457d9411a7092fed87cf9e53a/security/manager/tools/getHSTSPreloadList.js#118

::: security/manager/ssl/nsSiteSecurityService.h:209
(Diff revision 4)
>                                 uint32_t aFlags, bool aIsPreload,
>                                 const OriginAttributes& aOriginAttributes);
>    bool HostHasHSTSEntry(const nsAutoCString& aHost,
>                          bool aRequireIncludeSubdomains, uint32_t aFlags,
>                          const OriginAttributes& aOriginAttributes,
> -                        bool* aResult, bool* aCached);
> +                        bool* aResult, bool* aCached, uint32_t* aSource);

Let's use the underlying type `SecurityPropertySource` here.

::: security/manager/ssl/nsSiteSecurityService.h:214
(Diff revision 4)
> -                        bool* aResult, bool* aCached);
> +                        bool* aResult, bool* aCached, uint32_t* aSource);
>    const nsSTSPreload *GetPreloadListEntry(const char *aHost);
>    nsresult IsSecureHost(uint32_t aType, const nsACString& aHost,
>                          uint32_t aFlags,
>                          const OriginAttributes& aOriginAttributes,
> -                        bool* aCached, bool* aResult);
> +                        bool* aCached, uint32_t* aSource,

Same here.

::: security/manager/ssl/nsSiteSecurityService.cpp:208
(Diff revision 4)
>  
> +  if (tokenizer.CheckChar(',')) {
> +    if (!tokenizer.ReadSource(source)) {
> +      return false;
> +    }
> +  }

Let's just make sure we set source by adding:

```
} else {
  source = SourceUnknown;
}
```

::: security/manager/ssl/nsSiteSecurityService.cpp:668
(Diff revision 4)
> +      // don't override the source
> +      siteState->mHSTSSource = curSiteState->mHSTSSource;
> +      siteState->ToString(stateString);
> +    }
>      rv = mSiteStateStorage->Put(storageKey, stateString, storageType);
> +    if (!NS_SUCCEEDED(rv)) {

nit: NS_FAILED
In any case, I'm not sure this additional logging provides that much utility (particularly since the NS_ENSURE_SUCCESS below will log and since we don't have a corresponding log statement if the mPreloadStateStorage->Put call fails).

::: security/manager/ssl/nsSiteSecurityService.cpp:738
(Diff revision 4)
>      new SiteHSTSState(aHost, aOriginAttributes, value);
>    if (GetPreloadListEntry(aHost.get()) ||
>        dynamicState->mHSTSState != SecurityPropertyUnset) {
>      SSSLOG(("SSS: storing knockout entry for %s", aHost.get()));
>      RefPtr<SiteHSTSState> siteState = new SiteHSTSState(
> -      aHost, aOriginAttributes, 0, SecurityPropertyKnockout, false);
> +      aHost, aOriginAttributes, 0, SecurityPropertyKnockout, false, SourceUnknown);

nit: looks like this line might be a bit long

::: security/manager/ssl/nsSiteSecurityService.cpp:1455
(Diff revision 4)
>          return true;
>        } else if (siteState->mHSTSState == SecurityPropertyNegative) {
>          *aResult = false;
>          if (aCached) {
>            // if it's negative, it is always cached
> +          SSSLOG(("Marking HSTS as as cached (SecurityPropertyNegative)"));

nit: "as as"

::: security/manager/ssl/nsSiteSecurityService.cpp:1509
(Diff revision 4)
>        } else if (dynamicState->mHSTSState == SecurityPropertyNegative) {
>          *aResult = false;
>          if (aCached) {
>            // if it's negative, it is always cached
> -          *aCached = true;
> +          SSSLOG(("Marking HSTS as cached (SecurityPropertyNegative)"));
> +          *aCached = false;

This now contradicts the comment. Which is correct?

::: security/manager/ssl/nsSiteSecurityService.cpp:1535
(Diff revision 4)
>      SSSLOG(("%s is a preloaded HSTS host", aHost.get()));
>      *aResult = aRequireIncludeSubdomains ? preload->mIncludeSubdomains
>                                           : true;
>      if (aCached) {
>        // Only set cached if this includes subdomains
>        *aCached = aRequireIncludeSubdomains ? siteState->mHSTSIncludeSubdomains

I think this might be a preexisting bug - shouldn't this be `preload->mIncludeSubdomains` rather than `siteState->mHSTSIncludeSubdomains`?

::: security/manager/ssl/nsSiteSecurityService.cpp:1539
(Diff revision 4)
>        // Only set cached if this includes subdomains
>        *aCached = aRequireIncludeSubdomains ? siteState->mHSTSIncludeSubdomains
>                                             : true;
>      }
> +    if (aSource) {
> +      *aSource = siteState->mHSTSSource;

Shouldn't this just be `*aSource = SourcePreload;`?
Attachment #8875479 - Flags: review?(dkeeler) → review-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 19

a year ago
mozreview-review
Comment on attachment 8875479 [details]
Bug 1363546 - Store and report HSTS upgrade source  p=francois

https://reviewboard.mozilla.org/r/146920/#review155222

Cool - lgtm with nits addressed.

::: netwerk/protocol/http/nsHttpChannel.cpp:1797
(Diff revision 7)
> +        uint32_t headerSource = nsISiteSecurityService::SOURCE_ORGANIC_REQUEST;
> +        if (mLoadInfo && mLoadInfo->GetIsHSTSPriming()) {
> +            headerSource = nsISiteSecurityService::SOURCE_HSTS_PRIMING;
> +        }
>          rv = sss->ProcessHeader(aType, mURI, securityHeader, aSSLStatus,
> -                                aFlags, originAttributes, nullptr, nullptr,
> +                                aFlags, headerSource, originAttributes, 

nit: trailing whitespace

::: security/manager/ssl/nsSiteSecurityService.h:209
(Diff revision 7)
>                                 uint32_t aFlags, bool aIsPreload,
>                                 const OriginAttributes& aOriginAttributes);
>    bool HostHasHSTSEntry(const nsAutoCString& aHost,
>                          bool aRequireIncludeSubdomains, uint32_t aFlags,
>                          const OriginAttributes& aOriginAttributes,
> -                        bool* aResult, bool* aCached);
> +                        bool* aResult, bool* aCached, SecurityPropertySource* aSource);

nit: long line

::: security/manager/ssl/nsSiteSecurityService.cpp:1453
(Diff revision 7)
>          }
> +        if (aSource) {
> +          *aSource = siteState->mHSTSSource;
> +        }
>          return true;
>        } else if (siteState->mHSTSState == SecurityPropertyNegative) {

Seems like we need to set *aSource in this branch as well? (like in the preload case, below)

::: security/manager/ssl/tests/unit/test_sts_ipv4_ipv6.js:23
(Diff revision 7)
>  
>    let parsedMaxAge = {};
>    let parsedIncludeSubdomains = {};
>    s.processHeader(Ci.nsISiteSecurityService.HEADER_HSTS, uri,
> -                  "max-age=1000;includeSubdomains", sslStatus, 0, {},
> +                  "max-age=1000;includeSubdomains", sslStatus, 0,
> +                  Ci.nsISiteSecurityService.SOURCe_ORGANIC_REQUEST, {},

typo: SOURCE
Attachment #8875479 - Flags: review?(dkeeler) → review+
Comment hidden (mozreview-request)

Comment 21

a year ago
Pushed by kmckinley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a3815f61536e
Store and report HSTS upgrade source r=francois,keeler,mayhemer p=francois

Comment 22

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a3815f61536e
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.