Replace verification source with a SAN in the content signature verifier interface

RESOLVED FIXED in Firefox 48

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mgoodwin, Assigned: mgoodwin)

Tracking

unspecified
mozilla48
Points:
---

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
Change the ContentSignatureVerifier interface such that a required SAN could be passed in instead of the verification source.

Rationale; it enables us to add new verification sources (e.g. for browser features we ship as addons) without having to wait for a new verification source to ride the trains.
(Assignee)

Comment 1

3 years ago
Created attachment 8742295 [details]
MozReview Request: Bug 1265085 - Replace verification source with a SAN in the content signature verifier interface. r?Cykesiopka,r?fkiefer

This change replaces the hardcoded 'sourceis' in nsIContentSignatureVerifier and
ContentSignatureVerifier.cpp with a string parameter which allows the caller
to specify which hostname the signing certificate must be valid for. This allows
us to create and use new signing certificates without having to wait for new
sources to ride the trains.

Review commit: https://reviewboard.mozilla.org/r/47111/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/47111/
Attachment #8742295 - Flags: review?(franziskuskiefer)
Attachment #8742295 - Flags: review?(cykesiopka.bmo)
Comment on attachment 8742295 [details]
MozReview Request: Bug 1265085 - Replace verification source with a SAN in the content signature verifier interface. r?Cykesiopka,r?fkiefer

https://reviewboard.mozilla.org/r/47111/#review43611

lgtm. But I'd like to see some additional tests now that check for invalid (randome) SANs.
Attachment #8742295 - Flags: review?(franziskuskiefer)
(Assignee)

Comment 3

3 years ago
Comment on attachment 8742295 [details]
MozReview Request: Bug 1265085 - Replace verification source with a SAN in the content signature verifier interface. r?Cykesiopka,r?fkiefer

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47111/diff/1-2/
Attachment #8742295 - Flags: review?(franziskuskiefer)
(Assignee)

Comment 4

3 years ago
https://reviewboard.mozilla.org/r/47111/#review43625

::: security/manager/ssl/ContentSignatureVerifier.cpp:128
(Diff revision 2)
>    }
>    return NS_OK;
>  }
>  
>  // Create a context for a content signature verification.
> -// It sets signature, certificate chain, and context that shold be used to
> +// It sets signature, certificate chain and name that shold be used to verify

s/shold/should

::: security/manager/ssl/ContentSignatureVerifier.cpp
(Diff revision 2)
>      // the chain is bad
>      CSVerifier_LOG(("CSVerifier: The supplied chain is bad\n"));
>      return NS_ERROR_INVALID_SIGNATURE;
>    }
>  
> -  // Check the SAN

The comment should stay
(Assignee)

Comment 5

3 years ago
Comment on attachment 8742295 [details]
MozReview Request: Bug 1265085 - Replace verification source with a SAN in the content signature verifier interface. r?Cykesiopka,r?fkiefer

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47111/diff/2-3/
(Assignee)

Comment 6

3 years ago
Comment on attachment 8742295 [details]
MozReview Request: Bug 1265085 - Replace verification source with a SAN in the content signature verifier interface. r?Cykesiopka,r?fkiefer

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47111/diff/3-4/
Comment on attachment 8742295 [details]
MozReview Request: Bug 1265085 - Replace verification source with a SAN in the content signature verifier interface. r?Cykesiopka,r?fkiefer

https://reviewboard.mozilla.org/r/47111/#review43807

lgtm
Attachment #8742295 - Flags: review?(franziskuskiefer) → review+
https://reviewboard.mozilla.org/r/47111/#review43815

::: security/manager/ssl/ContentSignatureVerifier.cpp:199
(Diff revision 4)
>    if (result != Success) {
>      return NS_ERROR_FAILURE;
>    }
>  
> +  // Check we have the EKU extension - since a missing EKU extension prevents
> +  // CheckCertHostname from matching the required EKU

BuildCertChain is what processes EKUs - CheckCertHostname doesn't. In any case, this change seems unrelated to this bug.
(Assignee)

Comment 9

3 years ago
(In reply to David Keeler [:keeler] (use needinfo?) from comment #8)
> > +  // Check we have the EKU extension - since a missing EKU extension prevents
> > +  // CheckCertHostname from matching the required EKU
> 
> BuildCertChain is what processes EKUs - CheckCertHostname doesn't. In any
> case, this change seems unrelated to this bug.

Good catch; that's a typo (I meant BuildCertChain).

Yes. Franziskus identified some more missing tests which identified a problem in the case of missing EKU extensions. Would you prefer I file a new bug and move this (and the corresponding test change) to it?
Flags: needinfo?(dkeeler)
Yes, I think that would be best.
Flags: needinfo?(dkeeler)
(Assignee)

Updated

3 years ago
Blocks: 1265499
(Assignee)

Comment 11

3 years ago
Comment on attachment 8742295 [details]
MozReview Request: Bug 1265085 - Replace verification source with a SAN in the content signature verifier interface. r?Cykesiopka,r?fkiefer

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47111/diff/4-5/

Comment 12

3 years ago
Comment on attachment 8742295 [details]
MozReview Request: Bug 1265085 - Replace verification source with a SAN in the content signature verifier interface. r?Cykesiopka,r?fkiefer

https://reviewboard.mozilla.org/r/47111/#review44001

Mostly looks good!

::: security/manager/ssl/ContentSignatureVerifier.cpp:45
(Diff revision 5)
>    }
>    destructorSafeDestroyNSSReference();
>    shutdown(calledFromObject);
>  }
>  
>  nsresult

Nit: Looks like |nsresult| here should be |NS_IMETHODIMP| instead.

::: security/manager/ssl/ContentSignatureVerifier.cpp:49
(Diff revision 5)
>  
>  nsresult
>  ContentSignatureVerifier::VerifyContentSignature(
>    const nsACString& aData, const nsACString& aCSHeader,
> -  const nsACString& aCertChain, const uint32_t aSource, bool* _retval)
> +  const nsACString& aCertChain, const nsACString& aName, bool* _retval)
>  {

There should be a check like |NS_ENSURE_ARG_POINTER(_reval)| just after this line.

::: security/manager/ssl/ContentSignatureVerifier.cpp:129
(Diff revision 5)
> -// verify the data. The optional data parameter is added to the data to verify.
> +// the data. The optional data parameter is the first part of the data to
> +// verify.

Optional: "optional data parameter" leads me to think that a caller can omit the parameter entirely, which isn't true.

Maybe the comment could mention that |data| is optional in the sense that it can be the empty string?

Maybe it's just me though, up to you.

::: security/manager/ssl/nsIContentSignatureVerifier.idl:49
(Diff revision 5)
>     * @param aData                   The first chunk of data to be tested.
>     *                                This parameter is optional.

Same "optional data parameter" comment I made previously.

::: security/manager/ssl/tests/unit/test_content_signing.js:64
(Diff revision 5)
> +  let wrongEKUChain = loadChain(TEST_DATA_DIR + "content_signing",
> +                                ["onecrl_wrong_EKU_ee", "int", "root"]);

Looks like this should be moved to the patch for Bug 1265499.

::: security/manager/ssl/tests/unit/test_content_signing.js:119
(Diff revision 5)
> -  // Check good signatures from good certificates with incorrect SANs
> +  // Check good signatures from good certificates with the wrong SANs
>    chain1 = oneCRLChain.join("\n");
>    verifier = getSignatureVerifier();
>    ok(!verifier.verifyContentSignature(DATA, GOOD_SIGNATURE, chain1,
> -                                      verifier.ABOUT_NEWTAB),
> -     "A OneCRL signature should not verify if the signer has the newtab SAN");
> +                                      ABOUT_NEWTAB_NAME),
> +     "A signature should not verify if the SANs do not match a supplied name");

Nit: This should be probably be reverted since ideally subtests should have unique messages to allow subtests to be differentiated in the logs.

::: security/manager/ssl/tests/unit/test_content_signing.js:125
(Diff revision 5)
>  
>    chain2 = remoteNewTabChain.join("\n");
>    verifier = getSignatureVerifier();
>    ok(!verifier.verifyContentSignature(DATA, GOOD_SIGNATURE, chain2,
> -                                      verifier.ONECRL),
> -     "A newtab signature should not verify if the signer has the OneCRL SAN");
> +                                      ONECRL_NAME),
> +     "A signature should not verify if the SANs do not match a supplied name");

Nit: This should be probably be reverted since ideally subtests should have unique messages to allow subtests to be differentiated in the logs.

::: security/manager/ssl/tests/unit/test_content_signing.js:129
(Diff revision 5)
> +  ok(!verifier.verifyContentSignature(DATA, GOOD_SIGNATURE, chain1, ""),
> +     "A signature should not verify if the SANs do not match an empty name");

Optional, future clean up: This test file would probably be easier to read if fixed constants for e.g. |oneCRLChain.join("\n")| were used instead of |chain1| and |chain2|.

I'm forgetful, so I find myself having to constantly look up what |chain1| and |chain2| mean in the local context.

::: security/manager/ssl/tests/unit/test_content_signing.js:151
(Diff revision 5)
> +  // check good signature from a chain with the wrong EKU on the end entity
> +  chain1 = wrongEKUChain.join("\n");
> +  verifier = getSignatureVerifier();
> +  ok(!verifier.verifyContentSignature(DATA, GOOD_SIGNATURE, chain1,
> +                                      ONECRL_NAME),
> +     "A signature should not verify if the signer has the wrong EKU");

Looks like this should be moved to the patch for Bug 1265499.

::: security/manager/ssl/tests/unit/test_content_signing/content_signing_onecrl_no_SAN_ee.pem.certspec:3
(Diff revision 5)
> +issuer:int-CA
> +subject:ee-int-CA
> +subjectKey:secp384r1

This should have |extension:extKeyUsage:codeSigning| as well right?

::: security/manager/ssl/tests/unit/test_content_signing/content_signing_onecrl_wrong_EKU_ee.pem:1
(Diff revision 5)
> +-----BEGIN CERTIFICATE-----

Looks like this should be moved to the patch for Bug 1265499.

::: security/manager/ssl/tests/unit/test_content_signing/content_signing_onecrl_wrong_EKU_ee.pem.certspec:1
(Diff revision 5)
> +issuer:int-CA

Looks like this should be moved to the patch for Bug 1265499.

::: security/manager/ssl/tests/unit/test_content_signing/moz.build:13
(Diff revision 5)
>  #test_certificates = (
>  #    'content_signing_root.pem',
>  #    'content_signing_int.pem',
>  #    'content_signing_onecrl_ee.pem',
> +#    'content_signing_onecrl_no_SAN_ee.pem',
> +#    'content_signing_onecrl_wrong_EKU_ee.pem',

Looks like this should be moved to the patch for Bug 1265499.
Attachment #8742295 - Flags: review?(cykesiopka.bmo) → review+
(Assignee)

Comment 14

3 years ago
Comment on attachment 8742295 [details]
MozReview Request: Bug 1265085 - Replace verification source with a SAN in the content signature verifier interface. r?Cykesiopka,r?fkiefer

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47111/diff/5-6/
(Assignee)

Comment 15

3 years ago
https://reviewboard.mozilla.org/r/47111/#review44001

> Nit: This should be probably be reverted since ideally subtests should have unique messages to allow subtests to be differentiated in the logs.

I've changed this to another message that's unique and similar to the old. I changed these because the old message doesn't actually communicate what's being tested - it's a signature with a chain with OneCRL in the EE SAN but we expect verification to fail because we're requiring a newtab SAN.
(Assignee)

Comment 16

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d72b110307dc616e5d16cba45d2702e738c2ab4
Bug 1265085 - Replace verification source with a SAN in the content signature verifier interface. r=Cykesiopka,r=fkiefer

Comment 17

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5d72b110307d
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.