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

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Andreas Sterbenz]

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

[Andreas Sterbenz]

Attachment: Patch against NSS_3_11_BRANCH

Comment 2

[Wan-Teh Chang]

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.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

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

sr=nelson

>-    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.

Comment 4

[Wan-Teh Chang]

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.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

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

[Wan-Teh Chang]

Attachment: 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
done

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

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]

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Thanks, Wan-Teh.  I like your choice of comment.