Closed Bug 1077790 Opened 10 years ago Closed 10 years ago

mozilla::pkix::CheckPublicKeySize() should check for correct ECC curves

Categories

(Core :: Security: PSM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: Cykesiopka, Assigned: Cykesiopka)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 7 obsolete files)

      No description provided.
Depends on: 622859
Here's an initial pass at this, mostly based on:
https://hg.mozilla.org/mozilla-central/annotate/cc77c762d6a0/dom/crypto/CryptoKey.cpp#l762
Assignee: nobody → cykesiopka.bmo
Status: NEW → ASSIGNED
Attachment #8535721 - Flags: feedback?(brian)
And some initial tests (not entirely sure if these are sufficient though).

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=927f22d4cd01
Attachment #8535723 - Flags: feedback?(brian)
Comment on attachment 8535721 [details] [diff] [review]
bug1077790_mozpkix-check-curves_WIPv1.patch

Review of attachment 8535721 [details] [diff] [review]:
-----------------------------------------------------------------

Great. Thanks for working on this! Just some minor issues below. Please let me know if you need any more help.

::: security/pkix/include/pkix/Result.h
@@ +128,5 @@
>                       MOZILLA_PKIX_ERROR_V1_CERT_USED_AS_CA) \
>      MOZILLA_PKIX_MAP(ERROR_BAD_CERT_DOMAIN, 42, \
>                       SSL_ERROR_BAD_CERT_DOMAIN) \
> +    MOZILLA_PKIX_MAP(ERROR_UNSUPPORTED_ELLIPTIC_CURVE, 43, \
> +                     SEC_ERROR_UNSUPPORTED_ELLIPTIC_CURVE) \

When you add an error code, you need to add the error message text in pkixnss.cpp's RegisterErrorTable and the localization text in security/manager/locales/en-US/chrome/pinnss.properties.

::: security/pkix/lib/pkixnss.cpp
@@ +55,5 @@
>      return MapPRErrorCodeToResult(PR_GetError());
>    }
>  
>    switch (publicKey.get()->keyType) {
> +    case ecKey: {

"{" should be on its own line.

@@ +56,5 @@
>    }
>  
>    switch (publicKey.get()->keyType) {
> +    case ecKey: {
> +      SECKEYECParams* encodedParams = &publicKey.get()->u.ec.DEREncodedParams;

Check for null.

@@ +64,5 @@
> +          encodedParams->data[0] != SEC_ASN1_OBJECT_ID ||
> +          encodedParams->data[1] >= 128 ||
> +          encodedParams->len != (unsigned)encodedParams->data[1] + 2) {
> +        return Result::ERROR_UNSUPPORTED_ELLIPTIC_CURVE;
> +      }

Please use mozilla::pkix::der::Reader for all parsing in mozilla::pkix. This is how we guarantee memory safety. In particular, you can construct an Input, then a Reader, and then use:
   
   Input value;
   rv = der::ExpectTagAndGetValue(der::OIDTag, value);

It would be nice if you could copy the style of DigestAlgorithmIdentifier and AlgorithmIdentifier in pkixder.cpp. It would be even better if you could write this curve ID parsing code as a separate function within pkixder.cpp so that it can be used by the non-NSS code too. However, this isn't necessary to do in this bug; I can do it in a follow-up if you don't want to.

@@ +68,5 @@
> +      }
> +
> +      SECItem oid = { siBuffer, nullptr, 0 };
> +      oid.len = encodedParams->data[1];
> +      oid.data = encodedParams->data + 2;

If you don't want to do things the pkixder.cpp way, then you can convert "Input value;" to a const SECItem* using UnsafeMapInputToSECItem.

@@ +70,5 @@
> +      SECItem oid = { siBuffer, nullptr, 0 };
> +      oid.len = encodedParams->data[1];
> +      oid.data = encodedParams->data + 2;
> +
> +      // Allow only the NIST P-256, P-384 and P-521 curves.

Nit: Please put a comma after P-384 (Oxford comma).
Attachment #8535721 - Flags: feedback?(brian) → feedback+
Comment on attachment 8535723 [details] [diff] [review]
bug1077790_mozpkix-check-curves_tests_WIPv1.patch

Review of attachment 8535723 [details] [diff] [review]:
-----------------------------------------------------------------

This looks reasonable to me, but I've forgotten a lot about this JS-based testing and the python test data scripts. David would probably be able to provide better feedback for this.

Please let me know if you're interested in learning how to write the C++-based unit tests, like those found in security/pkix/test/gtest. The nice thing about those is that they will also work for uses of mozilla::pkix outside of Gecko. However, you definitely don't need to write tests in C++ for this bug to get landed. And the JS-based tests are useful even if there are C++ tests, since they test different things. (The C++ tests are more like unit tests, and the JS tests are more like integration tests.)

Again, thanks for taking this on.

::: security/manager/ssl/tests/unit/test_keysize.js
@@ +120,5 @@
>    checkForKeyType("rsa", 1016, 1024);
>    checkForKeyType("dsa", 960, 1024);
>  
> +  checkEllipticCurve("secp224r1", 224, false);
> +  checkEllipticCurve("prime256v1", 256, true);

Please check that the k1 variants are NOT accepted.
Attachment #8535723 - Flags: feedback?(dkeeler)
Attachment #8535723 - Flags: feedback?(brian)
Attachment #8535723 - Flags: feedback+
In addition, please add the documentation to CheckPublicKey() and to the related member functions in TrustDomain. Thanks!
Comment on attachment 8535723 [details] [diff] [review]
bug1077790_mozpkix-check-curves_tests_WIPv1.patch

Review of attachment 8535723 [details] [diff] [review]:
-----------------------------------------------------------------

This appears to test chains each of a particular curve - what about tests like secp384 -> secp224 -> secp384 or even rsa2048 -> secp224 -> rsa2048?
Anyway, looks good in general.

::: security/manager/ssl/tests/unit/test_keysize.js
@@ +95,5 @@
> + *        command.
> + * @param {Number} keySize
> + *        The key size of the curve.
> + * @param {Boolean} isAcceptableCurve
> + *        Whether the relevant curve is whitelisted for use.

Generally we're not so verbose with documentation in this way, but I guess some is good (things like saying where the curve names come from is good, but we often don't document more self-descriptive things like 'isAcceptableCurve' (although maybe we should call that 'expectedCurveAcceptability' or something)).

@@ +119,5 @@
>  function run_test() {
>    checkForKeyType("rsa", 1016, 1024);
>    checkForKeyType("dsa", 960, 1024);
>  
> +  checkEllipticCurve("secp224r1", 224, false);

The key size seems redundant with the curve name. Am I missing something? Or is that just an artifact of how this test is set up?

::: security/manager/ssl/tests/unit/test_keysize/generate.py
@@ +168,5 @@
>          dsaOK_param_filename,
>          adequate_key_size,
>          generate_ev)
>  
> +    # If the two sizes are equal, there is nothing else to generate.

Maybe instead we could pass 'None' for adequate_key_size/inadequate_key_size, and if inadequate_key_size is None, we don't generate an additional cert.
Attachment #8535723 - Flags: feedback?(dkeeler) → feedback+
(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #3)
> ::: security/pkix/include/pkix/Result.h
> @@ +128,5 @@
> >                       MOZILLA_PKIX_ERROR_V1_CERT_USED_AS_CA) \
> >      MOZILLA_PKIX_MAP(ERROR_BAD_CERT_DOMAIN, 42, \
> >                       SSL_ERROR_BAD_CERT_DOMAIN) \
> > +    MOZILLA_PKIX_MAP(ERROR_UNSUPPORTED_ELLIPTIC_CURVE, 43, \
> > +                     SEC_ERROR_UNSUPPORTED_ELLIPTIC_CURVE) \
> 
> When you add an error code, you need to add the error message text in
> pkixnss.cpp's RegisterErrorTable and the localization text in
> security/manager/locales/en-US/chrome/pinnss.properties.

I'll check again, but since I'm reusing a NSS error code, I don't think this is needed. In any case, the l10n entries are already present.

> @@ +64,5 @@
> > +          encodedParams->data[0] != SEC_ASN1_OBJECT_ID ||
> > +          encodedParams->data[1] >= 128 ||
> > +          encodedParams->len != (unsigned)encodedParams->data[1] + 2) {
> > +        return Result::ERROR_UNSUPPORTED_ELLIPTIC_CURVE;
> > +      }
> 
> Please use mozilla::pkix::der::Reader for all parsing in mozilla::pkix. This
> is how we guarantee memory safety. In particular, you can construct an
> Input, then a Reader, and then use:
>    
>    Input value;
>    rv = der::ExpectTagAndGetValue(der::OIDTag, value);
> 
> It would be nice if you could copy the style of DigestAlgorithmIdentifier
> and AlgorithmIdentifier in pkixder.cpp. It would be even better if you could
> write this curve ID parsing code as a separate function within pkixder.cpp
> so that it can be used by the non-NSS code too. However, this isn't
> necessary to do in this bug; I can do it in a follow-up if you don't want to.

Sure, I'll make an attempt at least.
(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #4)
> Please let me know if you're interested in learning how to write the
> C++-based unit tests, like those found in security/pkix/test/gtest. The nice
> thing about those is that they will also work for uses of mozilla::pkix
> outside of Gecko. However, you definitely don't need to write tests in C++
> for this bug to get landed. And the JS-based tests are useful even if there
> are C++ tests, since they test different things. (The C++ tests are more
> like unit tests, and the JS tests are more like integration tests.)

Sure. (Probably in another bug though).

> Again, thanks for taking this on.

No problem!
(In reply to David Keeler [:keeler] (use needinfo?) from comment #6)
> This appears to test chains each of a particular curve - what about tests
> like secp384 -> secp224 -> secp384 or even rsa2048 -> secp224 -> rsa2048?

The next thing I was planning on handling.

> ::: security/manager/ssl/tests/unit/test_keysize.js
> @@ +95,5 @@
> > + *        command.
> > + * @param {Number} keySize
> > + *        The key size of the curve.
> > + * @param {Boolean} isAcceptableCurve
> > + *        Whether the relevant curve is whitelisted for use.
> 
> Generally we're not so verbose with documentation in this way, but I guess
> some is good (things like saying where the curve names come from is good,
> but we often don't document more self-descriptive things like
> 'isAcceptableCurve' (although maybe we should call that
> 'expectedCurveAcceptability' or something)).

Noted.

> @@ +119,5 @@
> >  function run_test() {
> >    checkForKeyType("rsa", 1016, 1024);
> >    checkForKeyType("dsa", 960, 1024);
> >  
> > +  checkEllipticCurve("secp224r1", 224, false);
> 
> The key size seems redundant with the curve name. Am I missing something? Or
> is that just an artifact of how this test is set up?

It's an artifact of the current cert name scheme - I'll try and get rid of the redundancy if possible.
+ Add null check
+ Add pkixder curve ID parsing code and use that instead
+ Address other misc comments
Attachment #8535721 - Attachment is obsolete: true
Attachment #8546997 - Flags: feedback?(brian)
Comment on attachment 8546997 [details] [diff] [review]
bug1077790_mozpkix-check-curves_WIPv2.patch

Review of attachment 8546997 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/pkix/lib/pkixder.cpp
@@ +224,5 @@
>    return Success;
>  }
>  
> +Result
> +EllipticCurveOIDValue(Reader& ellipticeCurveID,

Oops, just noticed this typo. I'll fix it in the next iteration of the patch.
+ Add check that secp256k1 is not accepted
+ Address misc comments

... still missing mixed curve / mixed ECC and RSA chain cases.
Attachment #8535723 - Attachment is obsolete: true
Attaching the correct patch this time.
Attachment #8547000 - Attachment is obsolete: true
Comment on attachment 8546997 [details] [diff] [review]
bug1077790_mozpkix-check-curves_WIPv2.patch

Review of attachment 8546997 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/pkix/lib/pkixder.cpp
@@ +223,5 @@
>  
>    return Success;
>  }
>  
> +Result

static Result

@@ +224,5 @@
>    return Success;
>  }
>  
> +Result
> +EllipticCurveOIDValue(Reader& ellipticeCurveID,

OK, don't forget this s/ellipticeCurveID/ellipticCurveID/

@@ +313,5 @@
> +  if (rv != Success) {
> +    return rv;
> +  }
> +
> +  rv = EllipticCurveOIDValue(ellipticCurveID, ellipticCurve);

Can you use AlgorithmIdentifier(EllipticCurveOIDValue, input, ellipticCurve) here? Maybe you 'd need to add an extra parameter (enum class AllowNULLParameter { No = 0, Yes = 1 } allowNULLParameter) to indicate whether or not a NULL parameter is allowed.

If so, then I think that this function can be reduced to:

  return AlgorithmIdentifier(EllipticCurveOIDValue, input, AllowNulParamter::No,
                             ellipticCurve);

which would make it even more like DigestAlgorithmIdentifier and SignatureAlgorithmIdentifier.

::: security/pkix/lib/pkixnss.cpp
@@ +59,5 @@
>    switch (publicKey.get()->keyType) {
>      case ecKey:
> +    {
> +      SECKEYECParams* encodedParams = &publicKey.get()->u.ec.DEREncodedParams;
> +

Drop the blank line here.

@@ +75,5 @@
> +      EllipticCurve ellipticCurve;
> +      rv = mozilla::pkix::der::EllipticCurveIdentifier(reader, ellipticCurve);
> +      if (rv != Success) {
> +        return rv;
> +      }

I would prefer that we be extra careful and add an explicit switch:

switch (ellipticCurve) {
  case p256: // fall through
  case p384: // fall through
  case p521:
    break;
  default:
    return Result::ERROR_UNSUPPORTED_ELLIPTIC_CURVE;
}

That way, we don't have to worry about accidentally adding support for parsing unacceptable elliptic curve OIDs to EllipticCurveIdentifier in the future causing us to accidentally accept those unacceptable curves.
Attachment #8546997 - Flags: feedback?(brian) → feedback+
It's going to take me a couple of days to get to the review of the tests. Sorry for the delay. Good work so far!
Comment on attachment 8546997 [details] [diff] [review]
bug1077790_mozpkix-check-curves_WIPv2.patch

Review of attachment 8546997 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with nits addressed.

::: security/pkix/lib/pkixder.cpp
@@ +313,5 @@
> +  if (rv != Success) {
> +    return rv;
> +  }
> +
> +  rv = EllipticCurveOIDValue(ellipticCurveID, ellipticCurve);

I guess you can't use AlgorithmIdentifier because you don't have the entire AlgorithmIdentifier structure to parse. So, just ignore this comment.
Attachment #8546997 - Flags: review+
Comment on attachment 8547001 [details] [diff] [review]
bug1077790_mozpkix-check-curves_tests_WIPv2.patch

Review of attachment 8547001 [details] [diff] [review]:
-----------------------------------------------------------------

Please make the change I suggested above/below so that there's only one bad cert in each chain being tested. Otherwise, LGTM.

::: security/manager/ssl/tests/unit/test_keysize.js
@@ +116,5 @@
>  function run_test() {
>    checkForKeyType("rsa", 1016, 1024);
>  
> +  checkEllipticCurve("secp224r1", 224, SEC_ERROR_UNSUPPORTED_ELLIPTIC_CURVE);
> +  checkEllipticCurve("secp256k1", 256, SEC_ERROR_UNSUPPORTED_ELLIPTIC_CURVE);

I think these kinds of tests work better when there is only one bad cert in the chain. Above, you'll create a whole chain of bad certs, right?

In particular, it would be better to have a test where the end-entity certificate is P224 (bad) and the root cert is P256 (good), and one where the end-entity is P256 (good) and the root is P224 (bad). Similarly for secp256k1.
Attachment #8547001 - Flags: review-
Attachment #8547001 - Flags: feedback+
+ Add explicit switch statement on which curves to accept
+ Fix misc review comments
Attachment #8546997 - Attachment is obsolete: true
Attachment #8550695 - Flags: review+
+ Make ECC chains contain certs with different curves
+ Add some mixed RSA and ECC chains

A few notes:
- I didn't remove the curve + key size redundancy because I couldn't think of a way to do so without introducing more complexity, but maybe there's something obvious I'm missing.
- I tested to make sure the common methods added here work for the pure RSA cases as well. I'll do the refactoring in another bug along with some other clean ups.
Attachment #8547001 - Attachment is obsolete: true
Attachment #8550696 - Flags: feedback?(dkeeler)
Attachment #8550696 - Flags: feedback?(brian)
Here's a try run for earlier revisions of the two patches:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=90ffb9181931

The OSX failure looks infra related.
The Android failure is a weird issue where verification of a cert generated within a day or so of the push fails with SEC_ERROR_EXPIRED_ISSUER_CERTIFICATE, but is fine afterwards - not sure why this occurs.
Comment on attachment 8550696 [details] [diff] [review]
bug1077790_mozpkix-check-curves_tests_WIPv3.patch

Review of attachment 8550696 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM. I think this is ready for review.
Attachment #8550696 - Flags: review?(dkeeler)
Attachment #8550696 - Flags: feedback?(dkeeler)
Attachment #8550696 - Flags: feedback?(brian)
Attachment #8550696 - Flags: feedback+
Comment on attachment 8550695 [details] [diff] [review]
bug1077790_mozpkix-check-curves_v1.patch

Review of attachment 8550695 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/pkix/lib/pkixnss.cpp
@@ +74,5 @@
> +      EllipticCurve ellipticCurve;
> +      rv = der::EllipticCurveIdentifier(reader, ellipticCurve);
> +      if (rv != Success) {
> +        return rv;
> +      }

Do we need this here?:

   rv = der::End(reader);
   if (rv != Success) {
     return rv;
   }
Attachment #8550695 - Flags: review+
Comment on attachment 8550696 [details] [diff] [review]
bug1077790_mozpkix-check-curves_tests_WIPv3.patch

Review of attachment 8550696 [details] [diff] [review]:
-----------------------------------------------------------------

Great! Just a few comments.

::: security/manager/ssl/tests/unit/test_keysize.js
@@ +4,5 @@
>  // file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  "use strict";
>  
>  // Checks that RSA certs with key sizes below 1024 bits are rejected.
> +// Checks that ECC certs using curves other than the NIST P-256, P-384 or P-521

nit: looks like this line is >80 chars

@@ +141,5 @@
> +             SEC_ERROR_UNSUPPORTED_ELLIPTIC_CURVE);
> +  checkChain("secp256k1", 256,
> +             "prime256v1", 256,
> +             "prime256v1", 256,
> +             SEC_ERROR_UNSUPPORTED_ELLIPTIC_CURVE);

How about a test where the intermediate is an unsupported elliptic curve? (or am I missing it in this list?)

::: security/manager/ssl/tests/unit/test_keysize/generate.py
@@ +82,5 @@
>  
> +    # Reuse the existing RSA EV root
> +    if (generate_ev and key_type == 'rsa' and signer_key_filename == '' and
> +        signer_cert_filename == '' and key_size == '2048'):
> +            cert_name = 'evroot'

Looks like this is indented 8 spaces from the if. Unfortunately, 4 spaces would align the body with 'signer_cert_filename', which might make it hard to read. One thing you could do is remove the parentheses - they're not necessary, right? Anyway, not a big deal here.
Attachment #8550696 - Flags: review?(dkeeler) → review+
Comment on attachment 8550695 [details] [diff] [review]
bug1077790_mozpkix-check-curves_v1.patch

Review of attachment 8550695 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, I noticed some small issues with this, when adapting my patches to work on top of it.

::: security/pkix/lib/pkixder.cpp
@@ +305,5 @@
>    return AlgorithmIdentifier(DigestAlgorithmOIDValue, input, algorithm);
>  }
>  
>  Result
> +EllipticCurveIdentifier(Reader& input, /*out*/ EllipticCurve& ellipticCurve)

I suggest that you rename this function EllipticCurveOID. The functions named XXXIdentifier are parsing entire AlgorithmIdentifiers, but this is just parsing a single OID. (Those other functions probably should get renamed XXXAlgorithmIdentifier).

@@ +318,5 @@
> +  if (rv != Success) {
> +    return rv;
> +  }
> +
> +  return End(input);

This call to End should be made in the caller. In the rest of the code, the code that starts parsing a sequence (e.g. constructs the Reader) is responsible for calling End(...), and we should do that here too.
Attachment #8550695 - Flags: review-
Attachment #8550695 - Flags: review+
Comment on attachment 8550695 [details] [diff] [review]
bug1077790_mozpkix-check-curves_v1.patch

Review of attachment 8550695 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/pkix/lib/pkixder.cpp
@@ +305,5 @@
>    return AlgorithmIdentifier(DigestAlgorithmOIDValue, input, algorithm);
>  }
>  
>  Result
> +EllipticCurveIdentifier(Reader& input, /*out*/ EllipticCurve& ellipticCurve)

Actually, instead of EllipticCurveOID, I suggest NamedCurveOID, since that better matches the name in the spec.
(In reply to David Keeler [:keeler] (use needinfo?) from comment #23)
> ::: security/manager/ssl/tests/unit/test_keysize.js
> @@ +4,5 @@
> >  // file, You can obtain one at http://mozilla.org/MPL/2.0/.
> >  "use strict";
> >  
> >  // Checks that RSA certs with key sizes below 1024 bits are rejected.
> > +// Checks that ECC certs using curves other than the NIST P-256, P-384 or P-521
> 
> nit: looks like this line is >80 chars

Just checked, my IDE says 79.

> @@ +141,5 @@
> > +             SEC_ERROR_UNSUPPORTED_ELLIPTIC_CURVE);
> > +  checkChain("secp256k1", 256,
> > +             "prime256v1", 256,
> > +             "prime256v1", 256,
> > +             SEC_ERROR_UNSUPPORTED_ELLIPTIC_CURVE);
> 
> How about a test where the intermediate is an unsupported elliptic curve?
> (or am I missing it in this list?)

Added.

> ::: security/manager/ssl/tests/unit/test_keysize/generate.py
> @@ +82,5 @@
> >  
> > +    # Reuse the existing RSA EV root
> > +    if (generate_ev and key_type == 'rsa' and signer_key_filename == '' and
> > +        signer_cert_filename == '' and key_size == '2048'):
> > +            cert_name = 'evroot'
> 
> Looks like this is indented 8 spaces from the if. Unfortunately, 4 spaces
> would align the body with 'signer_cert_filename', which might make it hard
> to read. One thing you could do is remove the parentheses - they're not
> necessary, right? Anyway, not a big deal here.

I get a syntax error if I just remove the parentheses - I think I need a backslash to do so.
Anyways, I'll go with a different style suggested by PEP8 (indent the continuation line) so that the other lines are no longer indented 8 spaces in.
(In reply to Cykesiopka from comment #26)
> Just checked, my IDE says 79.

Oh - whoops. I was thinking it was indented :)

> I get a syntax error if I just remove the parentheses - I think I need a
> backslash to do so.
> Anyways, I'll go with a different style suggested by PEP8 (indent the
> continuation line) so that the other lines are no longer indented 8 spaces
> in.

Sounds good.
+ Use NamedCurve instead of EllipticCurve where appropriate to match spec
+ Rename EllipticCurveIdentifier() to NamedCurveOID()
+ Move der::End() check from EllipticCurveIdentifier() to caller
Attachment #8550695 - Attachment is obsolete: true
Attachment #8552769 - Flags: review?(brian)
+ Add test case for chain where the intermediate is an unsupported curve
+ Fix indentation issue
Attachment #8550696 - Attachment is obsolete: true
Attachment #8552771 - Flags: review+
Comment on attachment 8552769 [details] [diff] [review]
bug1077790_mozpkix-check-curves_v2.patch

Review of attachment 8552769 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM. I wasn't expecting you to rename EllipticCurve to NamedCurve, but it is fine with me.
Attachment #8552769 - Flags: review?(brian) → review+
(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #30)
> LGTM. I wasn't expecting you to rename EllipticCurve to NamedCurve, but it
> is fine with me.

I wavered a bit, but I decided to rename it for consistency. Renaming it back would be fine with me as well.
(In reply to Cykesiopka from comment #31)
> (In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment
> #30)
> > LGTM. I wasn't expecting you to rename EllipticCurve to NamedCurve, but it
> > is fine with me.
> 
> I wavered a bit, but I decided to rename it for consistency. Renaming it
> back would be fine with me as well.

I think the way you had it is fine.

Based on your comments in bug 1122841, I removed the "id-" and "id_" prefixes before checking in these patches:

https://hg.mozilla.org/integration/mozilla-inbound/rev/21cec989e617
https://hg.mozilla.org/integration/mozilla-inbound/rev/c504648c4be8
Target Milestone: --- → mozilla38
https://hg.mozilla.org/mozilla-central/rev/21cec989e617
https://hg.mozilla.org/mozilla-central/rev/c504648c4be8
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: