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 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 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.
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.
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: 126.96.36.199; 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]
Thanks, Wan-Teh. I like your choice of comment.