325683 - EC param parsing error not propagated correctly

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Andreas Sterbenz]

When the application specifies unsupported EC params, the parsing error is not correctly propagated. CKR_OK is returned to the app, but the key object is not fully initialized, which may lead to crashes later on.

See attached patch. I don't what the best error to report is in this case, I picked CKR_ATTRIBUTE_VALUE_INVALID.

Comment 1

[Andreas Sterbenz]

Attachment: Patch against NSS_3_11_BRANCH

Comment 2

[Wan-Teh Chang]

According to the last paragraph of PKCS #11 v2.20, Sec.
12.3 (the second paragraph on page 216), the error code
should be CKR_DOMAIN_PARAMS_INVALID.  Other than that,
the patch is good.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Andreas's patch with revised CKR error code

Comment 4

[Julien Pierre]

Comment on attachment 212361 [details] [diff] [review]
Andreas's patch with revised CKR error code

This patch looks fine, but are these the only 2 instances that we want to return that error code ?

There are other calls to EC_FillParams in keydb.c and lowcert.c where we don't set that error in case of failure. Also, EC_DecodeParams calls EC_FillParams. Should all the callers of EC_DecodeParams be changed as well ?

As a side note, why is EC_FillParams redeclared in each C file ? Shouldn't it be in a header file ?

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Adding Vipul.  Vipul, any answers to Julien's questions in the previous comment?

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Patch checked in.  Checkin comment:

Bug 325683. EC param parsing error not propagated correctly. 
Fix the cases that Andreas identified.  Patch by [e-mail]
r=Julien,wtchang,nelson

On 3.11 branch:
Checking in pkcs11.c;  new revision: 1.112.2.1; previous revision: 1.112

On trunk:
Checking in pkcs11.c;  new revision: 1.114;     previous revision: 1.113

I'm not sure if we should mark this bug fixed or not, in light of Julien's
good questions in comment 4 above.  

Bob, this is assigned to you.  I'll let you decide if this fix is sufficient.

Comment 7

[Robert Relyea]

re Julien's comment 4

The usage in keydb.c is incorrect. The error code is not checked at all. In the other 2 cases, the functions are returning conventional NSS error codes, not PKCS #11 error codes so the error checking is correct.

Julien is also correct about the multiple function prototypes. There should be one in a common header file.

bob

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Bob, please elaborate on your comment 7.
I found it to be somewhat ambiguous.  Did you mean to suggest (as you 
apparently did) that some functions are returning the wrong type of error codes?
After reading comment 7, I cannot tell what, if anything, needs to be done to 
complete this bug, other than moving the declaration of EC_FillParams to a 
header file.

Comment 9

[Robert Relyea]

Different functions within softoken return different types of error codes. The patch corrects the case where the function is returning CK_RV error codes, but those errors were not being set.

The additional functions that julien identified return SECStatus type error codes. In those cases the proper error code is set (even though the code looks a lot like the orginal incorrect code that Andreas fixed).

The only problematic one is the case julien identified in keydb.c. In keydb.c the error return from the function is not checked at all and it should be.

bob

Comment 10

[Robert Relyea]

Attachment: Fix handling EC_Fill Error in keydb.c

This is in addition to Andrea's patch.

Comment 11

[Robert Relyea]

Comment on attachment 215338 [details] [diff] [review]
Fix handling EC_Fill Error in keydb.c

asking for reviewers who are familiar with the problem;).

Comment 12

[Andreas Sterbenz]

(In reply to comment #11)
> (From update of attachment 215338 [details] [diff] [review] [edit])
> asking for reviewers who are familiar with the problem;).

I believe the intention of this patch is to add a function prototype for EC_FillParams to softoken.h, but softoken.h is missing from the patch.

Bob, could you add that or am I confused?

Comment 13

[Robert Relyea]

Attachment: Softoken.h potion of the patch.

this patch and patch 215338 are one unit.

Comment 14

[Andreas Sterbenz]

(In reply to comment #13)
> Created an attachment (id=215463) [edit]
> Softoken.h potion of the patch.
> 
> this patch and patch 215338 are one unit.

It appears my bugzilla account is does not have permission to review bugs ("You are not authorised to edit attachment 215463 [details] [diff] [review].")

Comment 15

[Robert Relyea]

Checked into the tip (after a false start):
Checking in softoken.h;
/cvsroot/mozilla/security/nss/lib/softoken/softoken.h,v  <--  softoken.h
new revision: 1.10; previous revision: 1.9
done
Checking in keydb.c;
/cvsroot/mozilla/security/nss/lib/softoken/keydb.c,v  <--  keydb.c
new revision: 1.43; previous revision: 1.42
done
Checking in pkcs11.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11.c,v  <--  pkcs11.c
new revision: 1.119; previous revision: 1.118
done
Checking in lowcert.c;
/cvsroot/mozilla/security/nss/lib/softoken/lowcert.c,v  <--  lowcert.c
new revision: 1.23; previous revision: 1.22
done

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

Is this bug fixed now?

Comment 17

[Wan-Teh Chang]

The main bug fix patch ("Andrea's patch with revised CKR error code")
has been checked in on the trunk and the NSS_3_11_BRANCH.

The other two patches have been checked in on the trunk.  I just
checked them in on the NSS_3_11_BRANCH.

Checking in keydb.c;
/cvsroot/mozilla/security/nss/lib/softoken/keydb.c,v  <--  keydb.c
new revision: 1.40.2.1; previous revision: 1.40
done
Checking in lowcert.c;
/cvsroot/mozilla/security/nss/lib/softoken/lowcert.c,v  <--  lowcert.c
new revision: 1.20.2.1; previous revision: 1.20
done
Checking in pkcs11.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11.c,v  <--  pkcs11.c
new revision: 1.112.2.4; previous revision: 1.112.2.3
done
Checking in softoken.h;
/cvsroot/mozilla/security/nss/lib/softoken/softoken.h,v  <--  softoken.h
new revision: 1.7.30.2; previous revision: 1.7.30.1
done