Closed Bug 257693 Opened 16 years ago Closed 15 years ago

EC_ValidatePublicKey needs actual checks in security/nss/lib/freebl/ec.c

Categories

(NSS :: Libraries, enhancement, P1)

Sun
SunOS
enhancement

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: susan.margulies, Assigned: wtc)

Details

Attachments

(3 files, 2 obsolete files)

User-Agent:       Mozilla/5.0 (X11; U; SunOS sun4u; en-US; rv:1.4) Gecko/20040414
Build Identifier: Mozilla/5.0 (X11; U; SunOS sun4u; en-US; rv:1.4) Gecko/20040414

The ANSI X9.63 spec states that after a point is decoded, it should be verified
as actually being a point on the specific curve being used.

Those checks are not implemented.

Reproducible: Always
Steps to Reproduce:
1. Go to security/nss/lib/freebl/ec.c
2. Search for EC_ValidatePublicKey
3. Notice /* XXX Add actual checks here */ :)

Actual Results:  
I was sad.

Expected Results:  
It should have made me happy.
I verified that EC_ValidatePublicKey is not implemented.
Julien, Vipul, could you look at this?
Assignee: wchang0222 → julien.pierre.bugs
Status: UNCONFIRMED → NEW
Ever confirmed: true
Re-assigning to Vipul .
Assignee: julien.pierre.bugs → vipul.gupta
Target Milestone: --- → 3.10
The attached patch implements EC_ValidatePublicKey for NSS 3.9.2.  I've created
a test case in the standalone build of the elliptic curve library (i.e., when
you run "make test" from within security/nss/lib/freebl/ecl) but have not
created a system-wide NSS test case.
QA Contact: bishakhabanerjee → jason.m.reid
Comment on attachment 175378 [details] [diff] [review]
NSS 3.9.2 patch to enable EC_ValidatePublicKey

r=wtc.	I checked in this patch (with two or three minor whitespace
changes) on the NSS trunk for NSS 3.11.

I will address the issues I found in a patch.

Questions:

1. The code says "Section 5.2.2 of X9.63", but isn't X9.62 the
ECDSA standard?

2. In the following code,

>+     /* NOTE: We only support uncompressed points for now */
>+     len = (ecParams->fieldID.size + 7) >> 3;
>+     if ((publicValue->data[0] != EC_POINT_FORM_UNCOMPRESSED) ||
>+     (publicValue->len != (2 * len + 1))) {
>+ 	    return SECFailure;
>+     };

Just wanted to check my understanding: I believe the second test
publicValue->len != (2 * len + 1) means publicValue is an uncompressed
point but its length is wrong.	Am I right?  What would be a good
error message for the second test?
Attachment #175378 - Flags: review+
Assignee: vipul.gupta → wtchang
Severity: normal → enhancement
Priority: -- → P1
Target Milestone: 3.10 → 3.11
Attached patch Code cleanup patch (obsolete) — Splinter Review
This patch is mostly code cleanup.

1. Change "X9.63" to "X9.62", unless X9.63 also has a Section
5.22 with the title "Public Key Validation (Optional)".

2. In EC_ValidatePublicKey, handle a NULL return from ECGroup_fromName.

3. In the ECGroupStr structure, move the validate_point field up.
It looks nicer to have the "extra*" fields at the end.

4. In the test cases, if the tests that should fail, passed, say so
in the error messages.
Attachment #192456 - Flags: review?(douglas)
Comment on attachment 192456 [details] [diff] [review]
Code cleanup patch

Patch doesn't apply cleanly for me (there is a conflict between this patch and
the previous one on the ECPoint_validate function in ecl/ecl.c).  I will submit
a new patch that applies cleanly.
Attachment #192456 - Flags: review?(douglas) → review-
1. Revises the previous patch to apply cleanly to the code after it has been
patched with the first patch.  

2. Addresses question 2 of comment 4 by adding separate error messages for a)
the case where the point is not in uncompressed form and b) the case where the
point is in compressed form but is not of the correct length.
Attachment #192456 - Attachment is obsolete: true
Attachment #193114 - Flags: review?(wtchang)
Comment on attachment 193114 [details] [diff] [review]
Code cleanup patch (revised)

r=wtc.

Douglas, the reason my patch doesn't apply to your
source tree is that my patch was generated against
the tip of the NSS source tree, whereas you are using
NSS 3.9.  Please do your work on the NSS tip for now
because I only check in new ECC code on the tip

>     /* NOTE: We only support uncompressed points for now */
>     len = (ecParams->fieldID.size + 7) >> 3;
>-    if ((publicValue->data[0] != EC_POINT_FORM_UNCOMPRESSED) ||
>-    (publicValue->len != (2 * len + 1))) {
>+    if (publicValue->data[0] != EC_POINT_FORM_UNCOMPRESSED) {
>+	    PORT_SetError(SEC_ERROR_UNSUPPORTED_EC_POINT_FORM);
>+	    return SECFailure;
>+    } else if (publicValue->len != (2 * len + 1)) {
>+	    PORT_SetError(SEC_ERROR_INPUT_LEN);
> 	    return SECFailure;
>     };

In the publicValue->len != (2 * len + 1) case, is the
public key invalid?  If so, we should use SEC_ERROR_BAD_KEY.
I will submit a patch to set the error codes properly.
Attachment #193114 - Flags: review?(wtchang) → review+
(In reply to comment #8)
> >+    } else if (publicValue->len != (2 * len + 1)) {
> >+          PORT_SetError(SEC_ERROR_INPUT_LEN);
> >           return SECFailure;
> >     };
> 
> In the publicValue->len != (2 * len + 1) case, is the
> public key invalid?  If so, we should use SEC_ERROR_BAD_KEY.
> I will submit a patch to set the error codes properly.

Yes, in that case the public key is invalid, so SEC_ERROR_BAD_KEY is also appropriate.
This bug is P1 for 3.11.  What is its status?
The most recently attached patch has r+.  Is it checked in?
Wan-Teh offered to add another patch to set error codes.
Is this bug waiting on that?  
Attached patch Set the error code correctly (obsolete) — Splinter Review
Douglas, this patch should be applied to the tip of NSS.

In EC_ValidatePublicKey, I set the error code to SEC_ERROR_BAD_KEY
if the public key is invalid.  Unfortunately if ECGroup_fromName
fails, I don't know what the right error code should be because
ECGroup_fromName doesn't return an error code to its caller.  I'm
setting 'err' to MP_UNDEF now, which will result in NSS setting
the error code SEC_ERROR_LIBRARY_FAILURE.

I added a comment to describe the return values of ECPoint_validate.

I changed the tests to compare the return value of ECPoint_validate
with MP_YES for positive test cases and with MP_NO for negative test
cases.	This is to allow us to detect ECPoint_validate failures due
to other errors such as out of memory condition.
Attachment #198726 - Flags: review?(douglas)
Attachment #198726 - Flags: superreview?(rrelyea)
Comment on attachment 198726 [details] [diff] [review]
Set the error code correctly

(In reply to comment #11)

> Unfortunately if ECGroup_fromName
> fails, I don't know what the right error code should be because
> ECGroup_fromName doesn't return an error code to its caller.  I'm
> setting 'err' to MP_UNDEF now, which will result in NSS setting
> the error code SEC_ERROR_LIBRARY_FAILURE.

Because of the design of the functions that ECGroup_fromName depends on, there
is no way to tell the reason for the failure of ECGroup_fromName.  I don't
think that rearchitecting all the underlying functions is worthwhile simply to
return a better error code, but it's up to you, Wan-Teh.  

There is a way to get slightly more information in terms of error reason.

It could be that the requested curve name is <= ECCurve_noName or >=
ECCurve_pastLastCurve.	If this is the case, then a reasonable error code would
be SEC_ERROR_INVALID_ARGS, since the argument is out of bounds.  

Otherwise, it could be that some memory allocation failed, or whatever else. 
Without rearchitecting the underlying part of ECL to return more error
information, your decision to return SEC_ERROR_LIBRARY_FAILURE is a good one.

I will mark the patch as review+, though it you want to change the code for the
additional error value as I just described, that would be fine too.
Attachment #198726 - Flags: review?(douglas) → review+
Attachment #198726 - Flags: superreview?(rrelyea)
Douglas, I implemented the additional error code
for an invalid ecParams->name as you suggested.
You can use Bugzilla's patch interdiff to see
this change.
Attachment #198726 - Attachment is obsolete: true
All patches have been checked in.  Marked the bug fixed.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.