380351 - Move the EC_ValidatePublicKey call into ECDH_Derive

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P4)

Description

[Wan-Teh Chang]

Attachment: Move the EC_ValidatePublicKey call into ECDH_Derive

NSS only calls ECDH_Derive at one location, and before that call
it calls EC_ValidatePublicKey.

If we move that EC_ValidatePublicKey call into ECDH_Derive, it'll
be easier to verify that NSS always validates the public key before
performing ECDH key derivation.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Bob Relyea asked:

> I believe there are 2 checks one can make on the public key:
> 
> 1) make sure the key is a point on the curve. This is what we do today 
> on all ECDH operations (the only public ECDH function we present is 
> through the PKCS #11 module, which does this check). This involves 
> simply checking that y^2 = x^3+ax+b (some multiplies and divides) (where 
> x and y are the x and y coordinates  of the public key)
> 
> 2) make sure the key is a point in the large subgroup of the curve.  
> This involves calculating Y = nY where Y is the public key, n is the 
> order of the group and nY is an ECC point multiply. Doing so effectively 
> doubles the cost of the operation.
> 
> Which of these checks [should we] do?

And the reply was:

> 1. Regarding ECDSA_VerifyDigest, we recommend that you check that the public 
> key variable "key" is a point on the curve by verifying that it satisfies 
> the Weierstrass equation. [...] ECDSA_VerifyDigest checks only that variable 
> key is not a NULL pointer.  Verifying that it satisfies the curve equation 
> is a cheap way to give this function better security properties.
> 
> We are persuaded that testing whether the point lies in the cryptographic 
> subgroup, [...] is too expensive to do each time this function is called.  
> In fact, this test adds nothing for Suite B curves, which have prime order, 
> so any rational point is in the cryptographic group.  However, we caution
> that if some future application invokes this function for a curve of 
> nonprime order without proper validation of the public key argument,
> then the results may be unpredictable.
> 
> 2.  the signed char array "naf" at 
http://lxr.mozilla.org/security/source/security/nss/lib/freebl/ecl/ecp_fp.c#476
> should be explicitly cleared as part of the CLEANUP (line 533) to  
> avoid divulging the private key.
> 
> This is not a memory leak issue and the fix is simple -- fill it with  
> zeros, i.e. whatever the NSS/NSPR preferred equivalent is for
> memset(naf, 0, sizeof(naf))

Comment 2

[Robert Relyea]

Comment on attachment 264431 [details] [diff] [review]
Move the EC_ValidatePublicKey call into ECDH_Derive

r- I'm rejecting this patch because it is necessar, byt not sufficient.


Before we can validate the public key, we should remove the group order check.

Once that happens, we can always validate the public key without a significant performance hit.

Nelson's post also points out 2 other areas that need to be fixed.

I'll attach a patch will all of these.

Comment 3

[Robert Relyea]

Attachment: Validate public keys on derive and verify.

Comment 4

[Wan-Teh Chang]

Comment on attachment 307116 [details] [diff] [review]
Validate public keys on derive and verify.

Bob, you attached the wrong patch.  This patch has a lot of
unrelated changes.  It seems to have been generated against
a very old CVS tag.

Comment 5

[Robert Relyea]

Attachment: Correct patch...

Oops grabbed the wrong patch file...

Comment 6

[Wan-Teh Chang]

EC_ValidatePublicKey is validated by the FIPS ECDSA algorithm testing.
It has to check nQ = O as specified by ANSI X9.62.  If ECC_VALIDATE_ORDER
is not defined, EC_ValidatePublicKey will fail FIPS ECDSA algorithm testing.

Comment 7

[Wan-Teh Chang]

I checked the FIPS ECDSA validation list at
http://csrc.nist.gov/groups/STM/cavp/documents/dss/ecdsaval.html

There are quite a few ECDSA implementations that didn't have
the public key validation (PKV) feature validated.  So perhaps
it's not a big deal to not have EC_ValidatePublicKey validated
in our next FIPS validation.  I just wanted us to be aware of that.

Is the omission of the nQ = O test for performance reasons?

Comment 8

[Robert Relyea]

Primarily yes. See the comment Nelson posted.

Also, for the nist curves anyway, it's shown to be unnecessary. Any rational point on the curve will have that order, so testing that the point is on the curve is sufficient.

nQ=O will effectively double the cost of any of these operations. On the other hand not checking if the point is on the curve could be an issue for anyone who is testing against unvalidated or self-validated keys.


bob

Comment 9

[Robert Relyea]

Comment on attachment 307153 [details] [diff] [review]
Correct patch...

cancelling old request.

Comment 10

[Tomas Hoger]

The EC code used in Mozilla NSS is also used in OpenJDK / Oracle JDK JCE component.  It seems this exact issue was addressed there via Oracle Critical Patch Update Advisory - July 2015 as CVE-2015-2613.

http://www.oracle.com/technetwork/topics/security/cpujul2015-2367936.html#AppendixJAVA

Applied patch adds EC_ValidatePublicKey() to ECDH_Derive():

http://hg.openjdk.java.net/jdk8u/jdk8u60/jdk/rev/dcc75a75d3a3

Unlike NSS (see comment 0), JCE likely exposes a code path to ECDH_Derive() where EC_ValidatePublicKey() is not called.  Actually, ECDH_Derive() is (now) part of the exposed NSS API, so there may be other exposed use cases.

Comment 11

[Al Billings [:abillings - ex-MoCo]]

Kai, is this something we need to work on?

Comment 12

[Kai Engert [:KaiE:]]

I think this will be addressed as part of the fix for bug 1160139?

Bob, do you agree? Then I suggest to mark it as a duplicate.

Comment 13

[Robert Relyea]

It's one step to prevent 1160139, but the patch there does not include the necessary checks here.

bob

Comment 14

[Robert Relyea]

OK, Actually this bug is about moving the checks from pkcs11c.c to freebl. The requisite check is already being done.

Comment 15

[Franziskus Kiefer [:franziskus]]

https://hg.mozilla.org/projects/nss/rev/1fe149b6e69d1cd253528a68ca72c544132e273d