Last Comment Bug 323817 - Truncation of hashes for ECDSA should be done at bit level, not octet level
: Truncation of hashes for ECDSA should be done at bit level, not octet level
Status: RESOLVED FIXED
ECC FIPS
:
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.11.1
: All All
: P1 normal (vote)
: 3.11.1
Assigned To: Robert Relyea
: Jason Reid
Mentors:
Depends on:
Blocks: 320583
  Show dependency treegraph
 
Reported: 2006-01-17 14:46 PST by Wan-Teh Chang
Modified: 2006-03-22 11:05 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
SEC 1.5 draft truncation (section 4.1) (2.28 KB, patch)
2006-03-16 18:06 PST, Robert Relyea
vipul.gupta: review+
douglas: superreview-
Details | Diff | Splinter Review
SEC 1.5 draft truncation, patch targetting 3.11 or ec version 1.5 (2.72 KB, patch)
2006-03-21 13:40 PST, Robert Relyea
douglas: superreview+
Details | Diff | Splinter Review

Description Wan-Teh Chang 2006-01-17 14:46:16 PST
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 2006-03-16 18:06:36 PST
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).
Comment 2 Robert Relyea 2006-03-16 18:09:38 PST
This code continues to work on our interoperability test certs.
Comment 3 Vipul Gupta 2006-03-16 22:08:33 PST
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 2006-03-17 18:21:36 PST
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 2006-03-20 17:55:36 PST
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 2006-03-21 13:40:16 PST
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
Comment 7 Douglas Stebila 2006-03-21 15:25:07 PST
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 2006-03-22 11:05:06 PST
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

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