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

RESOLVED FIXED in 3.11.1

Status

NSS
Libraries
P1
normal
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: Wan-Teh Chang, Assigned: Robert Relyea)

Tracking

3.11.1
3.11.1

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: ECC FIPS)

Attachments

(2 attachments)

(Reporter)

Description

12 years ago
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.
Blocks: 320583
Priority: -- → P1
Whiteboard: ECC FIPS
Target Milestone: --- → 3.11.1
(Assignee)

Comment 1

11 years ago
Created attachment 215359 [details] [diff] [review]
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).
(Assignee)

Updated

11 years ago
Attachment #215359 - Flags: review?(vipul.gupta)
(Assignee)

Comment 2

11 years ago
This code continues to work on our interoperability test certs.

Comment 3

11 years ago
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
Attachment #215359 - Flags: review?(vipul.gupta) → review+

Updated

11 years ago
Attachment #215359 - Flags: superreview?(douglas)

Comment 4

11 years ago
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'
Attachment #215359 - Flags: superreview?(douglas) → superreview-
(Assignee)

Comment 5

11 years ago
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
(Assignee)

Comment 6

11 years ago
Created attachment 215807 [details] [diff] [review]
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
Attachment #215807 - Flags: superreview?(douglas)

Comment 7

11 years ago
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.
Attachment #215807 - Flags: superreview?(douglas) → superreview+
(Assignee)

Comment 8

11 years ago
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
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.