avoid reimporting and thus revalidating EC keys in certificate transparency

RESOLVED FIXED in Firefox 55

Status

()

Core
Security: PSM
P1
normal
RESOLVED FIXED
a month ago
a month ago

People

(Reporter: keeler, Assigned: keeler)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [psm-assigned])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

The certificate transparency implementation currently uses VerifyECDSASignedDigestNSS to verify SCTs. Unfortunately this means it reimports and revalidates the same EC keys over and over again. Since this actually takes a nontrivial amount of time (about as much time as the signature verification itself), we should import/verify the keys once and reuse them with an API that doesn't cause a reverification.
Comment hidden (mozreview-request)

Comment 2

a month ago
mozreview-review
Comment on attachment 8858956 [details]
bug 1357226 - work around a library inefficiency with EC keys when verifying ECDSA signatures

https://reviewboard.mozilla.org/r/130958/#review133546

The parsing code looks good to me. Thanks for this fix! 

That said, I feel that since this is new parsing code, there should be a couple unit tests for AppendFixedSizeInteger and FasterVerifyECDSASignedDigestNSS to make sure that it parses oddly shaped EC pubkeys correctly before we merge it.

::: security/certverifier/CTLogVerifier.h:78
(Diff revision 1)
>    pkix::Result VerifySignature(pkix::Input data, pkix::Input signature);
>    pkix::Result VerifySignature(const Buffer& data, const Buffer& signature);
>  
> +  // mPublicECKey works around an architectural deficiency in NSS. In the case
> +  // of EC, if we don't create, import, and cache this key, NSS will import and
> +  // verify it every signature verification, which is slow.

Add a note that, if the CT key is not EC, this value is left as a `nullptr`?

::: security/manager/ssl/nsNSSComponent.cpp:1820
(Diff revision 1)
>        return;
>      }
> +    // Release the default CertVerifier. This will cause any held NSS resources
> +    // to be released (it's not an nsNSSShutDownObject, so we have to do this
> +    // manually).
> +    mDefaultCertVerifier = nullptr;

Nit: this leak-fix isn't mentioned in the commit message, but probably should be.
Attachment #8858956 - Flags: review?(jjones) → review-

Comment 3

a month ago
mozreview-review
Comment on attachment 8858956 [details]
bug 1357226 - work around a library inefficiency with EC keys when verifying ECDSA signatures

https://reviewboard.mozilla.org/r/130958/#review133638

I guess I could be convinced doing it this way. But I think writing less code and re-using what we have is the better move.

::: security/certverifier/CTLogVerifier.cpp:319
(Diff revision 1)
> +  }
> +  return Success;
> +}
> +
> +static Result
> +FasterVerifyECDSASignedDigestNSS(const SignedDigest& sd,

Having to do all of this here is silly. The NSS API sucks for this (unspurprisingly). But we have all this functionality. To avoid having to write all this new code I'd suggest to export DSAU_DecodeDerSigToLen, which is used in TLS and should be tested well enough.

[1] https://searchfox.org/mozilla-central/source/security/nss/lib/cryptohi/dsautil.c#269

::: security/certverifier/CTLogVerifier.cpp:351
(Diff revision 1)
> +    return rv;
> +  }
> +  if (!rAndS.AtEnd()) {
> +    return Result::ERROR_BAD_DER;
> +  }
> +  size_t signatureLen = SECKEY_SignatureLen(pubkey.get());

If pubkey == nullptr, this will throw a nullpointer error because we access it in SECKEY_SignatureLen (same for PK11_Verify).
Attachment #8858956 - Flags: review?(franziskuskiefer) → review-
(Assignee)

Comment 4

a month ago
mozreview-review-reply
Comment on attachment 8858956 [details]
bug 1357226 - work around a library inefficiency with EC keys when verifying ECDSA signatures

https://reviewboard.mozilla.org/r/130958/#review133546

Thanks for the review! As Franziskus pointed out, we can just DSAU_DecodeDerSigToLen, which is what the original code path used anyway.

> Add a note that, if the CT key is not EC, this value is left as a `nullptr`?

Sounds good.

> Nit: this leak-fix isn't mentioned in the commit message, but probably should be.

It's not really a leak fix since before this patch this isn't an issue - CertVerifier doesn't hold any NSS resources. After this patch, though, it does, so it has to be done. I clarified this in the commit message.
(Assignee)

Comment 5

a month ago
mozreview-review-reply
Comment on attachment 8858956 [details]
bug 1357226 - work around a library inefficiency with EC keys when verifying ECDSA signatures

https://reviewboard.mozilla.org/r/130958/#review133638

> Having to do all of this here is silly. The NSS API sucks for this (unspurprisingly). But we have all this functionality. To avoid having to write all this new code I'd suggest to export DSAU_DecodeDerSigToLen, which is used in TLS and should be tested well enough.
> 
> [1] https://searchfox.org/mozilla-central/source/security/nss/lib/cryptohi/dsautil.c#269

Sounds good.

> If pubkey == nullptr, this will throw a nullpointer error because we access it in SECKEY_SignatureLen (same for PK11_Verify).

But it can't be null, because we only call this for ECDSA where we previously ensured we decoded the key correctly. In any case, I added an assertion and a belt-and-suspenders null check.
Comment hidden (mozreview-request)

Comment 7

a month ago
mozreview-review
Comment on attachment 8858956 [details]
bug 1357226 - work around a library inefficiency with EC keys when verifying ECDSA signatures

https://reviewboard.mozilla.org/r/130958/#review133892

Thanks for that pointer, Franziskus!

Yay, previously-tested code.

(Note, this rebase's interdiff contains the OCSP preference code, please ignore in review.)
Attachment #8858956 - Flags: review?(jjones) → review+

Comment 8

a month ago
mozreview-review
Comment on attachment 8858956 [details]
bug 1357226 - work around a library inefficiency with EC keys when verifying ECDSA signatures

https://reviewboard.mozilla.org/r/130958/#review134194

Much better :) Thanks!

::: security/certverifier/CTLogVerifier.cpp:190
(Diff revision 2)
> +                                                   mPublicECKey.get(), false);
> +    if (handle == CK_INVALID_HANDLE) {
> +      return MapPRErrorCodeToResult(PR_GetError());
> +    }
> +  } else {
> +    mPublicECKey.reset(nullptr);

Would it make sense to fail here already?
Attachment #8858956 - Flags: review?(franziskuskiefer) → review+
Thanks!

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #8)
...
> ::: security/certverifier/CTLogVerifier.cpp:190
> (Diff revision 2)
> > +                                                   mPublicECKey.get(), false);
> > +    if (handle == CK_INVALID_HANDLE) {
> > +      return MapPRErrorCodeToResult(PR_GetError());
> > +    }
> > +  } else {
> > +    mPublicECKey.reset(nullptr);
> 
> Would it make sense to fail here already?

I'm not sure I understand - this is just the RSA case where we don't use mPublicECKey (so I'm deliberately setting it to null here). Or is this about returning a failing result if we fail to decode/import the EC key? Since we're using static data, it should never fail unless we give it the wrong information, in which case we would want to know (hmmm - maybe I should put some assertions in?)
Flags: needinfo?(franziskuskiefer)
Ah, I should've looked at all the code. Never mind that comment.
Flags: needinfo?(franziskuskiefer)
Comment hidden (mozreview-request)
Ok - thanks for the reviews!
Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5150b71bb66d
(I had forgotten to add PK11_Verify to nss.symbols, so I fixed that.)

Comment 13

a month ago
Pushed by dkeeler@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c631c917016b
work around a library inefficiency with EC keys when verifying ECDSA signatures r=fkiefer,jcj

Comment 14

a month ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c631c917016b
Status: NEW → RESOLVED
Last Resolved: a month ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.