Bug 173647 – [GCrypt] Implement CryptoKeyEC PKCS#8 imports

• Component: New Bugs
• Status: RESOLVED FIXED

[Zan Dobersek]

SSIA.

[Zan Dobersek]

Created attachment 313513 [details]
WIP patch

[Build Bot]

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.

[Zan Dobersek]

Created attachment 313517 [details]
Patch

Ready for review

[Build Bot]

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.

[Zan Dobersek]

Created attachment 313518 [details]
Patch

Filled out the WebCore ChangeLog entry.

[Jiewen Tan]

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.

[Zan Dobersek]

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.

[Zan Dobersek]

(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.

[Zan Dobersek]

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.

[Jiewen Tan]

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.

[Jiewen Tan]

Comment on attachment 313518 [details]
Patch

Any plans on this patch?

[Zan Dobersek]

(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.

[Zan Dobersek]

(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.

[Zan Dobersek]

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.

[Zan Dobersek]

Created attachment 316845 [details]
WIP

[Zan Dobersek]

Created attachment 316884 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Jiewen Tan]

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?

[Zan Dobersek]

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.

[Jiewen Tan]

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.

[Zan Dobersek]

Committed r220253: <http://trac.webkit.org/changeset/220253>

[Radar WebKit Bug Importer]

<rdar://problem/33717915>