Closed Bug 1011269 Opened 6 years ago Closed 6 years ago

add a pinning enforcement mode that also enforces test mode pins

Categories

(Core :: Security: PSM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: keeler, Assigned: mmc)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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: nobody → mmc
Status: NEW → ASSIGNED
Attachment #8424992 - Flags: review?(dkeeler)
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
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+
> 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
Argh, I forgot to qref to pickup the comment fixes before importing into inbound.

https://hg.mozilla.org/integration/mozilla-inbound/rev/776e1fd3824f
https://hg.mozilla.org/mozilla-central/rev/1f5b5d9cbf72
https://hg.mozilla.org/mozilla-central/rev/776e1fd3824f
Status: ASSIGNED → RESOLVED
Closed: 6 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.