1357226 - avoid reimporting and thus revalidating EC keys in certificate transparency

Status: RESOLVED FIXED

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

Description

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

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 1

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

Attachment: bug 1357226 - work around a library inefficiency with EC keys when verifying ECDSA signatures

Calling VFY_VerifyDigestDirect causes the provided SECKEYPublicKey to be
reimported to the softoken regardless of if it already exists on it. EC keys
must be verified upon import (to see if the point is on the curve to avoid some
small subgroup attacks), and so repeatedly doing this with a static key (say,
for example, a key corresponding to a built-in certificate transparency log) is
inefficient. This patch alters the certificate transparency implementation to
import these keys each once and then use PK11_Verify for ECDSA signature
verification, which doesn't have the same drawback.

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

Comment 2

[J.C. Jones [:jcj] (he/they)]

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.

Comment 3

[Franziskus Kiefer [:franziskus]]

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

Comment 4

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

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.

Comment 5

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

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 6

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

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

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

Comment 7

[J.C. Jones [:jcj] (he/they)]

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

Comment 8

[Franziskus Kiefer [:franziskus]]

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?

Comment 9

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

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

Comment 10

[Franziskus Kiefer [:franziskus]]

Ah, I should've looked at all the code. Never mind that comment.

Comment 11

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

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

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

Comment 12

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

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

[Pulsebot]

Pushed by [e-mail]:
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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c631c917016b