Bug 173647 - [GCrypt] Implement CryptoKeyEC PKCS#8 imports
Summary: [GCrypt] Implement CryptoKeyEC PKCS#8 imports
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: InRadar
Depends on: 174091 174420
Blocks: 133122
  Show dependency treegraph
 
Reported: 2017-06-21 06:08 PDT by Zan Dobersek
Modified: 2017-08-03 23:14 PDT (History)
4 users (show)

See Also:


Attachments
WIP patch (8.92 KB, patch)
2017-06-21 06:29 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (9.00 KB, patch)
2017-06-21 07:20 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (11.62 KB, patch)
2017-06-21 07:57 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (11.00 KB, patch)
2017-07-31 04:30 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
WIP (12.57 KB, patch)
2017-08-01 05:19 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (13.61 KB, patch)
2017-08-01 12:37 PDT, Zan Dobersek
jiewen_tan: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews117 for mac-elcapitan (2.06 MB, application/zip)
2017-08-02 04:51 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Zan Dobersek 2017-06-21 06:08:27 PDT
SSIA.
Comment 1 Zan Dobersek 2017-06-21 06:29:21 PDT
Created attachment 313513 [details]
WIP patch
Comment 2 Build Bot 2017-06-21 06:37:00 PDT
Attachment 313513 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Zan Dobersek 2017-06-21 07:20:52 PDT
Created attachment 313517 [details]
Patch

Ready for review
Comment 4 Build Bot 2017-06-21 07:22:27 PDT
Attachment 313517 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Zan Dobersek 2017-06-21 07:57:19 PDT
Created attachment 313518 [details]
Patch

Filled out the WebCore ChangeLog entry.
Comment 6 Jiewen Tan 2017-07-11 09:40:26 PDT
Comment on attachment 313518 [details]
Patch

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

Looks good to me. Please address the following comments.

> Source/WebCore/crypto/gcrypt/CryptoKeyECGCrypt.cpp:353
> +        if (version->size() != 1 || version->at(0) != 0x00)

We should have 0x00 as a constant as well. Will memcpy be more straightforward for readability?

> Source/WebCore/crypto/gcrypt/CryptoKeyECGCrypt.cpp:403
> +        if (version->size() != 1 || version->at(0) != 0x01)

Ditto.

> Source/WebCore/crypto/gcrypt/CryptoKeyECGCrypt.cpp:410
> +        if (parameters) {

I wonder if this part of code ever gets executed.
Comment 7 Zan Dobersek 2017-07-12 04:49:35 PDT
Comment on attachment 313518 [details]
Patch

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

>> Source/WebCore/crypto/gcrypt/CryptoKeyECGCrypt.cpp:353
>> +        if (version->size() != 1 || version->at(0) != 0x00)
> 
> We should have 0x00 as a constant as well. Will memcpy be more straightforward for readability?

You probably mean memcmp. Problem with using that is if an empty Vector is (somehow) returned. If that happens, we'd be testing uninitialized memory.

>> Source/WebCore/crypto/gcrypt/CryptoKeyECGCrypt.cpp:410
>> +        if (parameters) {
> 
> I wonder if this part of code ever gets executed.

Probably not, I don't think there's any test cases covering it, in WebKit or web-platform-tests. Same I think for the optional `privateKey.publicKey` member.

I've opened bug #174420 to add such a test.
Comment 8 Zan Dobersek 2017-07-12 04:50:42 PDT
(In reply to Jiewen Tan from comment #6)
> Comment on attachment 313518 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=313518&action=review
> 
> Looks good to me. Please address the following comments.
> 

Please r+ the patch if you're OK with the replies to the comments.
Comment 9 Zan Dobersek 2017-07-12 06:19:41 PDT
Comment on attachment 313518 [details]
Patch

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

>>> Source/WebCore/crypto/gcrypt/CryptoKeyECGCrypt.cpp:353
>>> +        if (version->size() != 1 || version->at(0) != 0x00)
>> 
>> We should have 0x00 as a constant as well. Will memcpy be more straightforward for readability?
> 
> You probably mean memcmp. Problem with using that is if an empty Vector is (somehow) returned. If that happens, we'd be testing uninitialized memory.

... otherwise yes, after the size is validated we'd be able to call memcmp and compare the vector data against an array constant.
Comment 10 Jiewen Tan 2017-07-12 09:26:16 PDT
Comment on attachment 313518 [details]
Patch

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

>>>> Source/WebCore/crypto/gcrypt/CryptoKeyECGCrypt.cpp:353
>>>> +        if (version->size() != 1 || version->at(0) != 0x00)
>>> 
>>> We should have 0x00 as a constant as well. Will memcpy be more straightforward for readability?
>> 
>> You probably mean memcmp. Problem with using that is if an empty Vector is (somehow) returned. If that happens, we'd be testing uninitialized memory.
> 
> ... otherwise yes, after the size is validated we'd be able to call memcmp and compare the vector data against an array constant.

I would prefer your second comment.

>>> Source/WebCore/crypto/gcrypt/CryptoKeyECGCrypt.cpp:410
>>> +        if (parameters) {
>> 
>> I wonder if this part of code ever gets executed.
> 
> Probably not, I don't think there's any test cases covering it, in WebKit or web-platform-tests. Same I think for the optional `privateKey.publicKey` member.
> 
> I've opened bug #174420 to add such a test.

Awesome! Please enable them just for GTK+ now.
Comment 11 Jiewen Tan 2017-07-25 15:17:31 PDT
Comment on attachment 313518 [details]
Patch

Any plans on this patch?
Comment 12 Zan Dobersek 2017-07-26 01:59:50 PDT
(In reply to Jiewen Tan from comment #11)
> Comment on attachment 313518 [details]
> Patch
> 
> Any plans on this patch?

The constants patch is ready for review in bug #17409. I'm setting this bug to depend on it, after it lands this patch can be updated to properly test for specific constants.

I'll also make this bug depend on bug #174420 so that the test case is provided first, and then this patch can simply unskip it for the GTK+ and WPE ports.
Comment 13 Zan Dobersek 2017-07-26 02:00:33 PDT
(In reply to Zan Dobersek from comment #12)
> (In reply to Jiewen Tan from comment #11)
> > Comment on attachment 313518 [details]
> > Patch
> > 
> > Any plans on this patch?
> 
> The constants patch is ready for review in bug #17409.

Bug #174091 actually.
Comment 14 Zan Dobersek 2017-07-31 04:30:08 PDT
Created attachment 316765 [details]
Patch

Corrected implementation after the test in bug #174420 revealed some issues. Still depends on #174091 to land, will be updated after that.
Comment 15 Zan Dobersek 2017-08-01 05:19:07 PDT
Created attachment 316845 [details]
WIP
Comment 16 Zan Dobersek 2017-08-01 12:37:31 PDT
Created attachment 316884 [details]
Patch
Comment 17 Build Bot 2017-08-02 04:51:42 PDT
Comment on attachment 316884 [details]
Patch

Attachment 316884 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4239273

New failing tests:
fast/dom/StyleSheet/detached-sheet-owner-node-link.html
Comment 18 Build Bot 2017-08-02 04:51:44 PDT
Created attachment 316953 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 19 Jiewen Tan 2017-08-02 16:49:19 PDT
Comment on attachment 316884 [details]
Patch

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

> Source/WebCore/crypto/gcrypt/CryptoKeyECGCrypt.cpp:468
> +        // Retrieve the `q` point. Since only `d` is provided, libgcrypt will compute it on-the-fly.

Isn't the public key provided in the previous step if any? Why is computation still needed for that case?
Comment 20 Zan Dobersek 2017-08-03 03:52:30 PDT
Comment on attachment 316884 [details]
Patch

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

>> Source/WebCore/crypto/gcrypt/CryptoKeyECGCrypt.cpp:468
>> +        // Retrieve the `q` point. Since only `d` is provided, libgcrypt will compute it on-the-fly.
> 
> Isn't the public key provided in the previous step if any? Why is computation still needed for that case?

Pubic key is optional. If it's present and of valid form, it's set above, using the gcry_mpi_t object. Here it's retrieved into a gcry_mpi_point_t object, which is the type used to represent EC points, but it has essentially the same value as publicKeyMPI if that was provided, or libgcrypt will have to compute it from scratch for the specific EC curve.

I can improve the comment upon landing, or upon next upload if there's anything more substantial to polish.
Comment 21 Jiewen Tan 2017-08-03 12:12:54 PDT
Comment on attachment 316884 [details]
Patch

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

Looks good to me. r=me.

>>> Source/WebCore/crypto/gcrypt/CryptoKeyECGCrypt.cpp:468
>>> +        // Retrieve the `q` point. Since only `d` is provided, libgcrypt will compute it on-the-fly.
>> 
>> Isn't the public key provided in the previous step if any? Why is computation still needed for that case?
> 
> Pubic key is optional. If it's present and of valid form, it's set above, using the gcry_mpi_t object. Here it's retrieved into a gcry_mpi_point_t object, which is the type used to represent EC points, but it has essentially the same value as publicKeyMPI if that was provided, or libgcrypt will have to compute it from scratch for the specific EC curve.
> 
> I can improve the comment upon landing, or upon next upload if there's anything more substantial to polish.

Sure, please improve the comments.
Comment 22 Zan Dobersek 2017-08-03 23:13:27 PDT
Committed r220253: <http://trac.webkit.org/changeset/220253>
Comment 23 Radar WebKit Bug Importer 2017-08-03 23:14:17 PDT
<rdar://problem/33717915>