Bug 171535 - [GCrypt] ECDSA signing results can be smaller than the EC key size
Summary: [GCrypt] ECDSA signing results can be smaller than the EC key size
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Zan Dobersek
URL:
Keywords:
Depends on:
Blocks: 133122
  Show dependency treegraph
 
Reported: 2017-05-01 22:36 PDT by Zan Dobersek
Modified: 2017-06-09 01:47 PDT (History)
2 users (show)

See Also:


Attachments
Patch (4.36 KB, patch)
2017-06-06 14:07 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (4.68 KB, patch)
2017-06-08 05:59 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch for landing (4.65 KB, patch)
2017-06-09 00:45 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zan Dobersek 2017-05-01 22:36:48 PDT
See https://bugs.webkit.org/show_bug.cgi?id=171103#c8.
Comment 1 Zan Dobersek 2017-06-06 14:07:55 PDT
Created attachment 312119 [details]
Patch
Comment 2 Jiewen Tan 2017-06-07 14:25:47 PDT
Comment on attachment 312119 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=312119&action=review

Looks good to me. Please address the following comments.

> Source/WebCore/crypto/gcrypt/CryptoAlgorithmECDSAGCrypt.cpp:101
> +        if (dataSize >= keySizeInBytes) {

It will be good if we can fold this code and the one below into one.

Also, I am wondering if you are sure that the verification process can automatically remove the heading 0s.
Comment 3 Zan Dobersek 2017-06-08 04:38:52 PDT
Comment on attachment 312119 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=312119&action=review

>> Source/WebCore/crypto/gcrypt/CryptoAlgorithmECDSAGCrypt.cpp:101
>> +        if (dataSize >= keySizeInBytes) {
> 
> It will be good if we can fold this code and the one below into one.
> 
> Also, I am wondering if you are sure that the verification process can automatically remove the heading 0s.

It removes the heading zeros while converting the binary data into MPIs.
Comment 4 Zan Dobersek 2017-06-08 05:59:14 PDT
Created attachment 312297 [details]
Patch
Comment 5 Jiewen Tan 2017-06-08 13:53:13 PDT
Comment on attachment 312297 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=312297&action=review

r=me

Looks good. Please address the following comments.

> Source/WebCore/crypto/gcrypt/CryptoAlgorithmECDSAGCrypt.cpp:117
> +        || extractECDSASignatureInteger(signature, signatureSexp, "s", keySizeInBytes))

Should be !extractECDSASignatureInteger(signature, signatureSexp, "s", keySizeInBytes)?
Comment 6 Zan Dobersek 2017-06-09 00:27:15 PDT
Comment on attachment 312297 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=312297&action=review

>> Source/WebCore/crypto/gcrypt/CryptoAlgorithmECDSAGCrypt.cpp:117
>> +        || extractECDSASignatureInteger(signature, signatureSexp, "s", keySizeInBytes))
> 
> Should be !extractECDSASignatureInteger(signature, signatureSexp, "s", keySizeInBytes)?

Yes, was a mistake.
Comment 7 Zan Dobersek 2017-06-09 00:45:53 PDT
Created attachment 312404 [details]
Patch for landing
Comment 8 Zan Dobersek 2017-06-09 01:47:46 PDT
Comment on attachment 312404 [details]
Patch for landing

Clearing flags on attachment: 312404

Committed r217969: <http://trac.webkit.org/changeset/217969>
Comment 9 Zan Dobersek 2017-06-09 01:47:50 PDT
All reviewed patches have been landed.  Closing bug.