RESOLVED FIXED 171535
[GCrypt] ECDSA signing results can be smaller than the EC key size 
https://bugs.webkit.org/show_bug.cgi?id=171535
Summary [GCrypt] ECDSA signing results can be smaller than the EC key size
Zan Dobersek
Reported 2017-05-01 22:36:48 PDT
See https://bugs.webkit.org/show_bug.cgi?id=171103#c8.
Attachments
Patch (4.36 KB, patch)
2017-06-06 14:07 PDT, Zan Dobersek 		no flags
DetailsFormatted DiffDiff
Patch (4.68 KB, patch)
2017-06-08 05:59 PDT, Zan Dobersek 		no flags
DetailsFormatted DiffDiff
Patch for landing (4.65 KB, patch)
2017-06-09 00:45 PDT, Zan Dobersek 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Zan Dobersek
Comment 1 2017-06-06 14:07:55 PDT
Created attachment 312119 [details] Patch
Jiewen Tan
Comment 2 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.
Zan Dobersek
Comment 3 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.
Zan Dobersek
Comment 4 2017-06-08 05:59:14 PDT
Created attachment 312297 [details] Patch
Jiewen Tan
Comment 5 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)?
Zan Dobersek
Comment 6 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.
Zan Dobersek
Comment 7 2017-06-09 00:45:53 PDT
Created attachment 312404 [details] Patch for landing
Zan Dobersek
Comment 8 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>
Zan Dobersek
Comment 9 2017-06-09 01:47:50 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.