Closed Bug 1285052 Opened 3 years ago Closed 3 years ago

Enforce a maximum max-age for HPKP

Categories

(Core :: Security: PSM, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox49 --- fixed
firefox50 --- fixed

People

(Reporter: rbarnes, Assigned: rbarnes)

References

(Blocks 1 open bug)

Details

(Whiteboard: [psm-assigned])

Attachments

(2 files)

Comment on attachment 8768590 [details]
Bug 1285052 - Enforce a maximum max-age for HPKP

Keeler: Does this look like the right approach?  Suggestions for how to improve the testing?
Attachment #8768590 - Flags: feedback?(dkeeler)
https://reviewboard.mozilla.org/r/62748/#review59582

This seems like the right approach. 60 days seems a little short to me, though (perhaps we should add telemetry for how often sites are trying to set longer pins).

::: security/manager/ssl/nsSiteSecurityService.h:148
(Diff revision 1)
>                              uint32_t* aFailureResult);
>    nsresult SetHPKPState(const char* aHost, SiteHPKPState& entry, uint32_t flags);
>  
>    const nsSTSPreload *GetPreloadListEntry(const char *aHost);
>  
> +  int64_t mMaxMaxAge;

This should probably be unsigned.

::: security/manager/ssl/nsSiteSecurityService.cpp:207
(Diff revision 1)
>    }
>  }
>  
>  ////////////////////////////////////////////////////////////////////////////////
>  
> +const int64_t kSixtyDaysInSeconds = 60 * 24 * 60 * 60;

Same here.

::: security/manager/ssl/tests/unit/test_pinning_header_parsing.js:127
(Diff revision 1)
>    checkPassSettingPin(GOOD_MAX_AGE + VALID_PIN1 + BACKUP_PIN2 + INCLUDE_SUBDOMAINS);
>    checkPassSettingPin(VALID_PIN1 + GOOD_MAX_AGE + BACKUP_PIN2 + INCLUDE_SUBDOMAINS);
>    checkPassSettingPin(VALID_PIN1 + GOOD_MAX_AGE + BACKUP_PIN2 + REPORT_URI + INCLUDE_SUBDOMAINS);
>    checkPassSettingPin(INCLUDE_SUBDOMAINS + VALID_PIN1 + GOOD_MAX_AGE + BACKUP_PIN2);
>    checkPassSettingPin(GOOD_MAX_AGE + VALID_PIN1 + BACKUP_PIN1 + UNRECOGNIZED_DIRECTIVE);
> +  checkPassSettingPin(LONG_MAX_AGE + VALID_PIN1 + BACKUP_PIN1);

Since the parsed maxAge is an optional out parameter for processHeader, we could pass the expected clamped maxAge to checkPassSettingPin or something.
Attachment #8768590 - Flags: feedback?(dkeeler) → feedback+
Assignee: nobody → rlb
Priority: -- → P1
Whiteboard: [psm-assigned]
> pref("security.cert_pinning.max_max_age_seconds", 60 * 24 * 60 * 60);

Using multiplication in a pref isn't going to work. It /looks/ like JavaScript, originally it /was/ parsed and executed as JavaScript (in Netscape days), but no longer:
https://dxr.mozilla.org/mozilla-central/source/modules/libpref/prefread.cpp#192


Re: keeler's review comment about 60 days seeming short. Yes, but 1) it's recommended in the spec, and 2) it's what Chrome is doing (see bug 1279662). At this point 60 days is the default expectation and we need to justify any other value.
Comment on attachment 8768590 [details]
Bug 1285052 - Enforce a maximum max-age for HPKP

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62748/diff/1-2/
Attachment #8768590 - Flags: feedback+ → review?(dkeeler)
Comment on attachment 8768590 [details]
Bug 1285052 - Enforce a maximum max-age for HPKP

https://reviewboard.mozilla.org/r/62748/#review59764

Cool - r=me.

::: security/manager/ssl/nsSiteSecurityService.cpp:211
(Diff revision 2)
>  
> +const uint64_t kSixtyDaysInSeconds = 60 * 24 * 60 * 60;
> +
>  nsSiteSecurityService::nsSiteSecurityService()
>    : mUsePreloadList(true)
>    , mPreloadListTimeOffset(0)

I guess we should also initialize mMaxMaxAge here.

::: security/manager/ssl/nsSiteSecurityService.cpp:234
(Diff revision 2)
>      return NS_ERROR_NOT_SAME_THREAD;
>    }
>  
>    mUsePreloadList = mozilla::Preferences::GetBool(
>      "network.stricttransportsecurity.preloadlist", true);
> +  mMaxMaxAge = mozilla::Preferences::GetInt(

nit: it looks like the style here is to set the member variable and then add the observer just after (so this bit shouldn't come between mUsePreloadList and its observer, and the observer for this should be just after it)
Attachment #8768590 - Flags: review?(dkeeler) → review+
Comment on attachment 8768590 [details]
Bug 1285052 - Enforce a maximum max-age for HPKP

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62748/diff/2-3/
https://reviewboard.mozilla.org/r/62748/#review59778

Ok - making that type change was less disruptive than I thought. ExpireTimeFromMaxAge needs to be updated as well, though.

::: security/manager/ssl/nsSiteSecurityService.cpp:605
(Diff revisions 2 - 3)
> -      if (PR_sscanf(directive->mValue.get(), "%lld", &maxAge) != 1) {
> +      if (PR_sscanf(directive->mValue.get(), "%llu", &maxAge) != 1) {
>          SSSLOG(("SSS: could not parse delta-seconds"));
>          return nsISiteSecurityService::ERROR_INVALID_MAX_AGE;
>        }
>  
>        SSSLOG(("SSS: parsed delta-seconds: %lld", maxAge));

Presumably this should be %llu as well.

::: security/manager/ssl/nsSiteSecurityService.cpp:797
(Diff revisions 2 - 3)
>        *aFailureResult = nsISiteSecurityService::ERROR_NO_BACKUP_PIN;
>      }
>      return NS_ERROR_FAILURE;
>    }
>  
>    int64_t expireTime = ExpireTimeFromMaxAge(maxAge);

Looks like this needs to be updated as well (relatedly, there's probably an overflow concern in ExpireTimeFromMaxAge...)

::: security/manager/ssl/nsSiteSecurityService.cpp:800
(Diff revisions 2 - 3)
>    }
>  
>    int64_t expireTime = ExpireTimeFromMaxAge(maxAge);
>    SiteHPKPState dynamicEntry(expireTime, SecurityPropertySet,
>                               foundIncludeSubdomains, sha256keys);
>    SSSLOG(("SSS: about to set pins for  %s, expires=%ld now=%ld maxAge=%ld\n",

I guess the maxAge format should be %llu here as well.
Comment on attachment 8768590 [details]
Bug 1285052 - Enforce a maximum max-age for HPKP

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62748/diff/3-4/
Pushed by rlb@ipv.sx:
https://hg.mozilla.org/integration/autoland/rev/d75db8a749e7
Enforce a maximum max-age for HPKP r=keeler
https://hg.mozilla.org/mozilla-central/rev/d75db8a749e7
https://hg.mozilla.org/mozilla-central/rev/4a6d64033813
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Duplicate of this bug: 1280417
I'd like to see this in 49 as well. Can we get a Beta branch patch made and nominated?
Flags: needinfo?(rlb)
Comment on attachment 8768590 [details]
Bug 1285052 - Enforce a maximum max-age for HPKP

Approval Request Comment
[Feature/regressing bug #]: Greater safety for HPKP
[User impact if declined]: Risk that a site can DOS itself with a long-lived bad HPKP header
[Describe test coverage new/current, TreeHerder]: Inbound testing
[Risks and why]: Minimal risk, just imposing a policy on a fairly niche use case
[String/UUID change made/needed]: None
Flags: needinfo?(rlb)
Attachment #8768590 - Flags: approval-mozilla-beta?
Patch for beta
Comment on attachment 8768590 [details]
Bug 1285052 - Enforce a maximum max-age for HPKP

This patch fixes a HPKP issue to be more secure. Take it in 49 beta.
Attachment #8768590 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
And a delayed followup on beta to fix up an eslint failure: https://hg.mozilla.org/releases/mozilla-beta/rev/77414dfaa7d6
You need to log in before you can comment on or make changes to this bug.