323817 - Truncation of hashes for ECDSA should be done at bit level, not octet level

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Wan-Teh Chang]

When performing ECDSA with a hash algorithm other than
SHA-1, if the hash output is longer than the length of
the base point order n, the hash output needs to be
truncated to that length.  This length is a length in
bits, not octets.

In lib/freebl/ec.c, we truncate the hash output by
setting the hash output's length in octets to the base
point order's length in octets.  This is fine if the
base point order's length in bits is a multiple of 8,
but in general this is incorrect.

There is another complication.  Since we store the hash
output in a SECItem as an octet string rather than a bit
string, if we need to truncate the hash output and the
length in bits is not a multiple of 8, we also need to
right-shift the bits in the remaining octets.

Comment 1

[Robert Relyea]

Attachment: SEC 1.5 draft truncation (section 4.1)

Section 4.1 essentially says we take n most significant bits of the hash in our signature where n is log2 of our modulus (field).

Comment 2

[Robert Relyea]

This code continues to work on our interoperability test certs.

Comment 3

[Vipul Gupta]

Comment on attachment 215359 [details] [diff] [review]
SEC 1.5 draft truncation (section 4.1)

I haven't run any tests but the
patch seems fine. 

You might want to include a reference to the spec in the comments that mention digest truncation. Also, the indentation seems off.

vipul

Comment 4

[Douglas Stebila]

Comment on attachment 215359 [details] [diff] [review]
SEC 1.5 draft truncation (section 4.1)

The patch seems to be against version 1.15 of ec.c but there is a new version in the respository, version 1.16.  It doesn't apply cleanly against 1.16, so I tested against 1.15.  This will have to be resolved before check in.

Builds successfully and the all.sh tests all pass.  

However, there is a compiler warning as indicated below:

ec.c: In function `ECDSA_VerifyDigest':
ec.c:881: warning: unused variable `localDigest'

Comment 5

[Robert Relyea]

revision 1.6 on the tip has the patch applied (which is why it wouldn't apply).
NSS checkin rules are 1 review for tip, and 2 reviews for branches
I'll attach a new version with the unused variable removed.

bob

Comment 6

[Robert Relyea]

Attachment: SEC 1.5 draft truncation, patch targetting 3.11 or ec version 1.5

This patch includes removal of the extra localDigest variable.
Most of this patch has already been checked into the trunk.

bob

Comment 7

[Douglas Stebila]

Comment on attachment 215807 [details] [diff] [review]
SEC 1.5 draft truncation, patch targetting 3.11 or ec version 1.5

Can't apply the patch to test it, but looking at the code it looks fine.

Comment 8

[Robert Relyea]

checked in to tip (rev 1.17)

and 3.11 branch
Checking in ec.c;
/cvsroot/mozilla/security/nss/lib/freebl/ec.c,v  <--  ec.c
new revision: 1.9.2.7; previous revision: 1.9.2.6
done