1011269 - add a pinning enforcement mode that also enforces test mode pins

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Dana Keeler (she/her) [:keeler]]

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.

Comment 1

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Add CertVerifier::pinningEnforceTestMode (

Comment 2

[[:mmc] Monica Chew (no longer reading bugmail)]

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 3

[Dana Keeler (she/her) [:keeler]]

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).

Comment 4

[[:mmc] Monica Chew (no longer reading bugmail)]

> 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

Comment 5

[[:mmc] Monica Chew (no longer reading bugmail)]

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

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1f5b5d9cbf72
https://hg.mozilla.org/mozilla-central/rev/776e1fd3824f