Buffer too small for NIST K-571 and B-571 curves



12 years ago
12 years ago


(Reporter: Andreas Sterbenz, Assigned: Wan-Teh Chang)


Firefox Tracking Flags

(Not tracked)



(1 attachment, 1 obsolete attachment)

604 bytes, patch
Details | Diff | Splinter Review


12 years ago
The genenc buffer in EC_FillParams() does not leave space for the 0x00 string termination byte, which can lead to overflow when curves of the maximum size are used. This can cause errors or crashes when they are used in e.g. a C_GenerateKeyPair call.

The size of the genenc buffer should be (2 ['04'] + MAX_ECKEY_LEN * 2 [x,y] * 2 [hex string] + 1 [0x00]) bytes.

Comment 1

12 years ago
Created attachment 212062 [details] [diff] [review]
Patch against NSS_3_11_BRANCH

Comment 2

12 years ago
Comment on attachment 212062 [details] [diff] [review]
Patch against NSS_3_11_BRANCH

Good catch, Andreas.  Would be nice to replace
the strcat calls with code that can detect the
'genenc' buffer is too small.
Attachment #212062 - Flags: superreview?(nelson)
Attachment #212062 - Flags: review+
Priority: -- → P1
Comment on attachment 212062 [details] [diff] [review]
Patch against NSS_3_11_BRANCH


>-    char genenc[2 + 2 * 2 * MAX_ECKEY_LEN];
>+    char genenc[3 + 2 * 2 * MAX_ECKEY_LEN];

Hey, let's be generous and give it 4 extra bytes.  :-)

I see 3 magic numbers in that expression, and no explanation for them.
How are future maintainers of this code gonna know why we're 
multiplying the max len by 4, and adding a small prime fudge factor?

But that's a complaint about the original code. 
The patch seems correct, and is no worse than the original with
respect to the lack of comments.
Attachment #212062 - Flags: superreview?(nelson) → superreview+

Comment 4

12 years ago
Someone who can't figure out what those magic numbers
mean by reading the code is probably not qualified to
maintain NSS.  But I agree a comment would be helpful.
How about this:

 '0' + '4' + genx + geny + '\0', where genx and geny are each
 <= MAX_ECKEY_LEN bytes, or
 <= 2 * MAX_ECKEY_LEN hex digits.
I like Andreas's comment line:
   (2 ['04'] + MAX_ECKEY_LEN * 2 [x,y] * 2 [hex string] + 1 [0x00]) 
I think the crucial parts of the explanation are that 
- one 2x multiplier is because there are two variables of equal potential length,
- one 2x multiplier is because each variable byte is encoded as two hex chars.
- the 3 is for the string "04" including the trailing NUL character.

Wan-Teh, assuming you're going to check this patch in, please add whatever 
comment you think best.  Thanks.

Comment 6

12 years ago
Created attachment 212152 [details] [diff] [review]
Patch as checked in

I checked in Andreas' patch with his comment on the NSS
trunk (3.12) and NSS_3_11_BRANCH (3.11.1).

Checking in ecdecode.c;
/cvsroot/mozilla/security/nss/lib/softoken/ecdecode.c,v  <--  ecdecode.c
new revision: 1.5; previous revision: 1.4

Checking in ecdecode.c;
/cvsroot/mozilla/security/nss/lib/softoken/ecdecode.c,v  <--  ecdecode.c
new revision:; previous revision: 1.4

I edited Andreas' comment slightly because I thought
'04' was a character or byte with value 04.  We could
also say:

3 ["04"] + MAX_ECKEY_LEN * 2 [x,y] * 2 [hex string]
Attachment #212062 - Attachment is obsolete: true


12 years ago
Last Resolved: 12 years ago
Resolution: --- → FIXED
Thanks, Wan-Teh.  I like your choice of comment.
You need to log in before you can comment on or make changes to this bug.