Closed
Bug 1011269
Opened 10 years ago
Closed 10 years ago
add a pinning enforcement mode that also enforces test mode pins
Categories
(Core :: Security: PSM, defect)
Core
Security: PSM
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: keeler, Assigned: mmc)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
13.38 KB,
patch
|
keeler
:
review+
|
Details | Diff | Splinter Review |
For testing and/or dogfooding purposes, we decided it would be useful to have a pinning enforcement mode that enforces everything, regardless of if a pin is in test mode.
Assignee | ||
Comment 1•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → mmc
Status: NEW → ASSIGNED
Assignee | ||
Updated•10 years ago
|
Attachment #8424992 -
Flags: review?(dkeeler)
Assignee | ||
Comment 2•10 years ago
|
||
Comment on attachment 8424992 [details] [diff] [review] Add CertVerifier::pinningEnforceTestMode ( Review of attachment 8424992 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/manager/ssl/tests/unit/test_pinning.js @@ +92,5 @@ > } > > +function test_enforce_test_mode() { > + // In strict mode, we always evaluate pinning data, regardless of whether the > + // issuer is a built-in trust anchor. Will fix comment
Reporter | ||
Comment 3•10 years ago
|
||
Comment on attachment 8424992 [details] [diff] [review] Add CertVerifier::pinningEnforceTestMode ( Review of attachment 8424992 [details] [diff] [review]: ----------------------------------------------------------------- Looks great, but we should probably still handle the pref-to-enforcement-mode conversion the old-fashioned way (or something equivalent). ::: security/certverifier/CertVerifier.cpp @@ +187,5 @@ > } > } > } > > + const bool enforceTestMode = (callbackState->pinningEnforcementLevel == This doesn't need to be const, right? ::: security/manager/boot/src/PublicKeyPinningService.h @@ +24,1 @@ > * Note: if an alt name is a wildcard, it won't necessarily find a pinset nit: while we're here, let's get rid of this double space that got left in ::: security/manager/ssl/src/nsNSSComponent.cpp @@ +998,5 @@ > CertVerifier::pinning_enforcement_config > + pinningEnforcementLevel = > + static_cast<CertVerifier::pinning_enforcement_config> > + (Preferences::GetInt("security.cert_pinning.enforcement_level", > + CertVerifier::pinningDisabled)); Hmmm - but what happens if someone decides to store a value that's not 0-3 in "security.cert_pinning.enforcement_level"? It should default to disabled for now. ::: security/manager/ssl/tests/unit/test_pinning.js @@ +92,5 @@ > } > > +function test_enforce_test_mode() { > + // In strict mode, we always evaluate pinning data, regardless of whether the > + // issuer is a built-in trust anchor. We should probably fix the test_strict() comment as well (i.e. it doesn't actually enforce everything).
Attachment #8424992 -
Flags: review?(dkeeler) → review+
Assignee | ||
Comment 4•10 years ago
|
||
> Hmmm - but what happens if someone decides to store a value that's not 0-3 in > "security.cert_pinning.enforcement_level"? It should default to disabled for now. The logic of cert verifier said that it would be enforced at strict, so I added a check to default back to 0 if level > CertVerifier::pinningEnforceTestMode. I'm not sure that's strictly necessary, but that's ok. Fixed comments and double-spaces. https://hg.mozilla.org/integration/mozilla-inbound/rev/1f5b5d9cbf72
Assignee | ||
Comment 5•10 years ago
|
||
Argh, I forgot to qref to pickup the comment fixes before importing into inbound. https://hg.mozilla.org/integration/mozilla-inbound/rev/776e1fd3824f
Comment 6•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1f5b5d9cbf72 https://hg.mozilla.org/mozilla-central/rev/776e1fd3824f
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in
before you can comment on or make changes to this bug.
Description
•