Last Comment Bug 327384 - Buffer too small for NIST K-571 and B-571 curves
: Buffer too small for NIST K-571 and B-571 curves
Status: RESOLVED FIXED
:
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.11
: All All
: P1 major (vote)
: 3.11.1
Assigned To: Wan-Teh Chang
: Jason Reid
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-02-15 17:05 PST by Andreas Sterbenz
Modified: 2006-02-16 15:00 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch against NSS_3_11_BRANCH (1.01 KB, patch)
2006-02-15 17:06 PST, Andreas Sterbenz
wtc: review+
nelson: superreview+
Details | Diff | Splinter Review
Patch as checked in (604 bytes, patch)
2006-02-16 14:38 PST, Wan-Teh Chang
no flags Details | Diff | Splinter Review

Description Andreas Sterbenz 2006-02-15 17:05:40 PST
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 2006-02-15 17:06:20 PST
Created attachment 212062 [details] [diff] [review]
Patch against NSS_3_11_BRANCH
Comment 2 Wan-Teh Chang 2006-02-15 17:17:54 PST
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) 2006-02-15 18:12:54 PST
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 2006-02-15 18:47:29 PST
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) 2006-02-15 19:49:33 PST
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 2006-02-16 14:38:57 PST
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
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) 2006-02-16 15:00:50 PST
Thanks, Wan-Teh.  I like your choice of comment.

Note You need to log in before you can comment on or make changes to this bug.