1280224 - Initial values for the content signature root pref are ignored

Status: RESOLVED FIXED

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

Description

[Mark Goodwin [:mgoodwin]]

Content signature verifications work in cases where the pref is set explicitly, previously the browser session - but not with the default preference value.

Comment 1

[Mark Goodwin [:mgoodwin]]

Attachment: Bug 1280224 - Initial values for the content signature root pref are ignored.

Review commit: https://reviewboard.mozilla.org/r/59326/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59326/

Comment 2

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

Comment on attachment 8762864 [details]
Bug 1280224 -  Initial values for the content signature root pref are ignored.

https://reviewboard.mozilla.org/r/59326/#review56348

Looks good - just the two comments.

::: security/manager/ssl/nsNSSComponent.cpp:1753
(Diff revision 1)
>      return NS_ERROR_NOT_AVAILABLE;
>    }
>  
> +  // ensure we have an initial value for the content signer root
> +  mContentSigningRootHash =
> +    Preferences::GetString("security.content.signature.root_hash");

Let's add a quick test-case that this does in fact get set before it gets set to something else by Services.prefs.setCharPref (in, e.g. test_content_signing.js).

::: security/manager/ssl/nsNSSComponent.cpp:2188
(Diff revision 1)
>    MOZ_ASSERT(mNSSInitialized);
>  
>    result = false;
>  
>    if (mContentSigningRootHash.IsEmpty()) {
> +    MOZ_LOG(gPIPNSSLog, LogLevel::Debug, ("mContentSigningRootHash is empty\n"));

nit: the trailing \n aren't necessary any more (if they ever were), so let's just remove them.

Comment 3

[Mark Goodwin [:mgoodwin]]

Thanks for your review.

(In reply to David Keeler [:keeler] (use needinfo?) from comment #2)
> Comment on attachment 8762864 [details]
> Let's add a quick test-case that this does in fact get set before it gets
> set to something else by Services.prefs.setCharPref (in, e.g.
> test_content_signing.js).

This could be complicated; the issue is that, short of performing a signature verification, there's no way of checking that the initial value is there. To perform such a verification, we need to create a test signature using a production signer - and that has a fairly short validity. This means we'd need to modify the test each and every time we create new signers.

On the other hand, as this bug suggests, this is one of these cases where the lack of a test means that our unit tests pass when there's a bug that causes breakage in real use.

Please let me know your thoughts.

Comment 4

[Mark Goodwin [:mgoodwin]]

https://reviewboard.mozilla.org/r/59326/#review56348

> Let's add a quick test-case that this does in fact get set before it gets set to something else by Services.prefs.setCharPref (in, e.g. test_content_signing.js).

See comment 3.

Comment 5

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8762864 [details]
Bug 1280224 -  Initial values for the content signature root pref are ignored.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59326/diff/1-2/

Comment 6

[Mark Goodwin [:mgoodwin]]

https://reviewboard.mozilla.org/r/59326/#review56348

> See comment 3.

I've done this by ensuring that we return an error when mContentSignerRoot is empty when the check is performed to see if a cert is a root. I needed to make changes to ensure that the error wasn't swallowed by the ContentSignatureVerifier so you may want to take another look at that bit.

Comment 7

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

https://reviewboard.mozilla.org/r/59326/#review56762

Ok - cool. Just a few additional comments.

::: security/manager/ssl/ContentSignatureVerifier.cpp:186
(Diff revision 2)
>                            KeyPurposeId::id_kp_codeSigning,
>                            CertPolicyId::anyPolicy,
>                            nullptr/*stapledOCSPResponse*/);
>    if (result != Success) {
> -    // the chain is bad
> +    // if there was a library error, return an appropriate error
> +    if (result == Result::FATAL_ERROR_LIBRARY_FAILURE) {

We should use IsFatalError here, I think.

::: security/manager/ssl/tests/unit/test_content_signing.js:65
(Diff revision 2)
>                               ["onecrl_no_SAN_ee", "int", "root"]);
>  
> -  // Check good signatures from good certificates with the correct SAN
> +  // Check signature verification works without error before the root is set
>    let chain1 = oneCRLChain.join("\n");
>    let verifier = getSignatureVerifier();
> +  ok(!verifier.verifyContentSignature(DATA, BAD_SIGNATURE, chain1, ONECRL_NAME),

We could use GOOD_SIGNATURE here, right? (with the explanation that the data won't verify, but neither should it throw).

::: security/manager/ssl/tests/unit/test_content_signing.js:66
(Diff revision 2)
>  
> -  // Check good signatures from good certificates with the correct SAN
> +  // Check signature verification works without error before the root is set
>    let chain1 = oneCRLChain.join("\n");
>    let verifier = getSignatureVerifier();
> +  ok(!verifier.verifyContentSignature(DATA, BAD_SIGNATURE, chain1, ONECRL_NAME),
> +     "A OneCRL signature should verify with the OneCRL chain");

nit: I think this test description string needs an update

Comment 8

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8762864 [details]
Bug 1280224 -  Initial values for the content signature root pref are ignored.

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

Comment 9

[Mark Goodwin [:mgoodwin]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ef354057c1d

Comment 10

[Mark Goodwin [:mgoodwin]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/15b2940c713102b67cf546308d5d3dd4a34a3bdd
Bug 1280224 -  Initial values for the content signature root pref are ignored. r=keeler

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/15b2940c7131