WebCrypto should support spki/pkcs8 formats

[Alexey Proskuryakov]

WebCrypto should support spki/pkcs8 formats for import and export.

[Alexey Proskuryakov]

rdar://problem/21799829

+1

[Greg B]

+1

Is this on a road map in a foreseeable future?

[AliG]

Wondering about the same as Greg. Is support on the roadmap?

[Jiewen Tan]

Created attachment 296180 [details]
Part 1 SPKI

[Jiewen Tan]

Created attachment 296203 [details]
Part 1 SPKI

[Brent Fulgham]

Comment on attachment 296203 [details]
Part 1 SPKI

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

> Source/WebCore/crypto/algorithms/CryptoAlgorithmRSASSA_PKCS1_v1_5.cpp:163
> +        // Since we don't have means to extract that information out of the key, we omit the check for now.

Radar or Bugzilla, please!

> Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:176
> +        // Since we don't have means to extract that information out of the key, we omit the check for now.

Ditto (please add Radar or Bugzilla reference for this future work).

> Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:230
> +        // Since we don't have means to insert that information into the key, we omit it for now.

Ditto.

> Source/WebCore/crypto/mac/CryptoKeyRSAMac.cpp:248
> +    if (length <= 0)

Length can't be less than zero! :-)

> Source/WebCore/crypto/mac/CryptoKeyRSAMac.cpp:334
> +    // O(size) = Sequence(1) + Length(3) + Integer(1) + Length(3) + Modulus + Integer(1) + Length(3) + Exponent

If this is being tracked in Radar, can you reference it here so we can (hopefully someday) get rid of this hack?

> LayoutTests/imported/w3c/WebCryptoAPI/encrypt_decrypt/test_rsa_oaep-expected.txt:111
> +FAIL importVectorKeys step: RSA-OAEP with SHA-512 and a label no encrypt usage assert_unreached: importVectorKeys failed for RSA-OAEP with SHA-512 and a label. Message: ''A required parameter was missing or out-of-range'' Reached unreachable code

It's sad that these still don't pass. Are we missing features that prevent these from working? Will Step 2 (or some future step) get these passing?

[Brent Fulgham]

BTW: The build failures do not seem related to your patch.

[Jiewen Tan]

Created attachment 296212 [details]
Part 1 SPKI

[Jiewen Tan]

Comment on attachment 296203 [details]
Part 1 SPKI

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

Thanks Brent for reviewing my patch.

>> Source/WebCore/crypto/algorithms/CryptoAlgorithmRSASSA_PKCS1_v1_5.cpp:163
>> +        // Since we don't have means to extract that information out of the key, we omit the check for now.
> 
> Radar or Bugzilla, please!

https://bugs.webkit.org/show_bug.cgi?id=165436

>> Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:176
>> +        // Since we don't have means to extract that information out of the key, we omit the check for now.
> 
> Ditto (please add Radar or Bugzilla reference for this future work).

https://bugs.webkit.org/show_bug.cgi?id=165436

>> Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:230
>> +        // Since we don't have means to insert that information into the key, we omit it for now.
> 
> Ditto.

https://bugs.webkit.org/show_bug.cgi?id=165437

>> Source/WebCore/crypto/mac/CryptoKeyRSAMac.cpp:248
>> +    if (length <= 0)
> 
> Length can't be less than zero! :-)

Fixed.

>> Source/WebCore/crypto/mac/CryptoKeyRSAMac.cpp:334
>> +    // O(size) = Sequence(1) + Length(3) + Integer(1) + Length(3) + Modulus + Integer(1) + Length(3) + Exponent
> 
> If this is being tracked in Radar, can you reference it here so we can (hopefully someday) get rid of this hack?

<rdar://problem/29523286>

>> LayoutTests/imported/w3c/WebCryptoAPI/encrypt_decrypt/test_rsa_oaep-expected.txt:111
>> +FAIL importVectorKeys step: RSA-OAEP with SHA-512 and a label no encrypt usage assert_unreached: importVectorKeys failed for RSA-OAEP with SHA-512 and a label. Message: ''A required parameter was missing or out-of-range'' Reached unreachable code
> 
> It's sad that these still don't pass. Are we missing features that prevent these from working? Will Step 2 (or some future step) get these passing?

I don't know the answer for now. I am hoping adding PKCS8 support can help us pass these tests.

[Jiewen Tan]

Created attachment 296223 [details]
Part 1 SPKI

[Build Bot]

Comment on attachment 296223 [details]
Part 1 SPKI

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

New failing tests:
crypto/subtle/rsa-import-spki-key-export-spki-key.html
crypto/subtle/rsaes-pkcs1-v1_5-import-spki-key.html
crypto/subtle/rsa-import-spki-key-export-jwk-key.html
crypto/subtle/rsa-oaep-import-spki-key.html
imported/w3c/WebCryptoAPI/encrypt_decrypt/test_rsa_oaep.html
crypto/subtle/rsassa-pkcs1-v1_5-import-spki-key.html
crypto/workers/subtle/rsa-import-spki-key.html

[Build Bot]

Created attachment 296236 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 296223 [details]
Part 1 SPKI

Attachment 296223 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2630355

New failing tests:
crypto/subtle/rsa-import-spki-key-export-spki-key.html
crypto/subtle/rsaes-pkcs1-v1_5-import-spki-key.html
crypto/subtle/rsa-import-spki-key-export-jwk-key.html
crypto/subtle/rsa-oaep-import-spki-key.html
imported/w3c/WebCryptoAPI/encrypt_decrypt/test_rsa_oaep.html
crypto/subtle/rsassa-pkcs1-v1_5-import-spki-key.html
crypto/workers/subtle/rsa-import-spki-key.html

[Build Bot]

Created attachment 296237 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 296223 [details]
Part 1 SPKI

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

New failing tests:
crypto/subtle/rsa-import-spki-key-export-spki-key.html
crypto/subtle/rsaes-pkcs1-v1_5-import-spki-key.html
crypto/subtle/rsa-import-spki-key-export-jwk-key.html
crypto/subtle/rsa-oaep-import-spki-key.html
imported/w3c/WebCryptoAPI/encrypt_decrypt/test_rsa_oaep.html
crypto/subtle/rsassa-pkcs1-v1_5-import-spki-key.html
crypto/workers/subtle/rsa-import-spki-key.html

[Build Bot]

Created attachment 296239 [details]
Archive of layout-test-results from ews115 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Jiewen Tan]

Created attachment 296264 [details]
Part 1 of 2 SPKI

[Build Bot]

Comment on attachment 296264 [details]
Part 1 of 2 SPKI

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

New failing tests:
crypto/subtle/rsa-import-spki-key-export-spki-key.html
crypto/subtle/rsaes-pkcs1-v1_5-import-spki-key.html
crypto/subtle/rsa-import-spki-key-export-jwk-key.html
crypto/subtle/rsa-oaep-import-spki-key.html
imported/w3c/WebCryptoAPI/encrypt_decrypt/test_rsa_oaep.html
crypto/subtle/rsassa-pkcs1-v1_5-import-spki-key.html
crypto/workers/subtle/rsa-import-spki-key.html

[Build Bot]

Created attachment 296273 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 296264 [details]
Part 1 of 2 SPKI

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

New failing tests:
crypto/subtle/rsa-import-spki-key-export-spki-key.html
crypto/subtle/rsaes-pkcs1-v1_5-import-spki-key.html
crypto/subtle/rsa-import-spki-key-export-jwk-key.html
crypto/subtle/rsa-oaep-import-spki-key.html
imported/w3c/WebCryptoAPI/encrypt_decrypt/test_rsa_oaep.html
crypto/subtle/rsassa-pkcs1-v1_5-import-spki-key.html
crypto/workers/subtle/rsa-import-spki-key.html

[Build Bot]

Created attachment 296274 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 296264 [details]
Part 1 of 2 SPKI

Attachment 296264 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2631946

New failing tests:
crypto/subtle/rsa-import-spki-key-export-spki-key.html
crypto/subtle/rsaes-pkcs1-v1_5-import-spki-key.html
crypto/subtle/rsa-import-spki-key-export-jwk-key.html
crypto/subtle/rsa-oaep-import-spki-key.html
imported/w3c/WebCryptoAPI/encrypt_decrypt/test_rsa_oaep.html
crypto/subtle/rsassa-pkcs1-v1_5-import-spki-key.html
crypto/workers/subtle/rsa-import-spki-key.html

[Build Bot]

Created attachment 296275 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Brent Fulgham]

Comment on attachment 296264 [details]
Part 1 of 2 SPKI

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

This looks good, but seems to be causing some test failures.

> Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:177
> +        // https://bugs.webkit.org/show_bug.cgi?id=165436

Ditto.

> Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:232
> +        // https://bugs.webkit.org/show_bug.cgi?id=165437

Ditto.

> Source/WebCore/crypto/mac/CryptoKeyRSAMac.cpp:333
> +    // <rdar://problem/29523286>

"This hack can be removed when <rdar://problem/29523286> is resolved."

[Jiewen Tan]

Comment on attachment 296264 [details]
Part 1 of 2 SPKI

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

It looks like that those Yosemite bots are not configured to have internal SDK. I will skip those test cases for Yosemite specifically.

>> Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:177
>> +        // https://bugs.webkit.org/show_bug.cgi?id=165436
> 
> Ditto.

Fixed.

>> Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:232
>> +        // https://bugs.webkit.org/show_bug.cgi?id=165437
> 
> Ditto.

Fixed.

>> Source/WebCore/crypto/mac/CryptoKeyRSAMac.cpp:333
>> +    // <rdar://problem/29523286>
> 
> "This hack can be removed when <rdar://problem/29523286> is resolved."

Fixed.

[Alexey Proskuryakov]

> It looks like that those Yosemite bots are not configured to have internal SDK. I will skip those test cases for Yosemite specifically.

None of webkit.org bots have internal SDK. WebKit and its tests are expected to work in an open source build.

[Brent Fulgham]

(In reply to comment #26)

> It looks like that those Yosemite bots are not configured to have internal
> SDK. I will skip those test cases for Yosemite specifically.

Are you saying these tests should pass on El Capitain and newer? Then I think skipping for Yosemite is okay.

If these tests require some kind of internal SDK feature, we need to create a WebKitSystemInterface implementation that can be used by OpenSource builds.

[Jiewen Tan]

Created attachment 296305 [details]
Part 1 of 2 SPKI

[Jiewen Tan]

Created attachment 296320 [details]
Part 1 of 2 SPKI

[Jiewen Tan]

Comment on attachment 296320 [details]
Part 1 of 2 SPKI

This patch has added some logging into those failing tests to determine potential causes.

[Build Bot]

Comment on attachment 296320 [details]
Part 1 of 2 SPKI

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

New failing tests:
crypto/subtle/rsa-import-spki-key-export-spki-key.html
crypto/subtle/rsaes-pkcs1-v1_5-import-spki-key.html
crypto/subtle/rsa-import-spki-key-export-jwk-key.html
crypto/subtle/rsa-oaep-import-spki-key.html
imported/w3c/WebCryptoAPI/encrypt_decrypt/test_rsa_oaep.html
crypto/subtle/rsassa-pkcs1-v1_5-import-spki-key.html
crypto/workers/subtle/rsa-import-spki-key.html

[Build Bot]

Created attachment 296330 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 296320 [details]
Part 1 of 2 SPKI

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

New failing tests:
crypto/subtle/rsa-import-spki-key-export-spki-key.html
crypto/subtle/rsaes-pkcs1-v1_5-import-spki-key.html
crypto/subtle/rsa-import-spki-key-export-jwk-key.html
crypto/subtle/rsa-oaep-import-spki-key.html
imported/w3c/WebCryptoAPI/encrypt_decrypt/test_rsa_oaep.html
crypto/subtle/rsassa-pkcs1-v1_5-import-spki-key.html
crypto/workers/subtle/rsa-import-spki-key.html

[Build Bot]

Created attachment 296333 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 296320 [details]
Part 1 of 2 SPKI

Attachment 296320 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2636788

New failing tests:
crypto/subtle/rsa-import-spki-key-export-spki-key.html
crypto/subtle/rsaes-pkcs1-v1_5-import-spki-key.html
crypto/subtle/rsa-import-spki-key-export-jwk-key.html
crypto/subtle/rsa-oaep-import-spki-key.html
imported/w3c/WebCryptoAPI/encrypt_decrypt/test_rsa_oaep.html
crypto/subtle/rsassa-pkcs1-v1_5-import-spki-key.html
crypto/workers/subtle/rsa-import-spki-key.html

[Build Bot]

Created attachment 296335 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Jiewen Tan]

Created attachment 296336 [details]
Part 1 of 2 SPKI

[Brent Fulgham]

Comment on attachment 296336 [details]
Part 1 of 2 SPKI

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

> Source/WebCore/crypto/algorithms/CryptoAlgorithmRSASSA_PKCS1_v1_5.cpp:164
> +        // A bug is filed to keep it in track: https://bugs.webkit.org/show_bug.cgi?id=165436

This should just be:

// FIXME: <webkit.org/b/165436>

> Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:177
> +        // A bug is filed to keep it in track: https://bugs.webkit.org/show_bug.cgi?id=165436

Ditto: Just use the standard FIXME format, no need for other words.

> Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:232
> +        // A bug is filed to keep it in track: https://bugs.webkit.org/show_bug.cgi?id=165437

Ditto.

> LayoutTests/ChangeLog:39
> +        * platform/mac/TestExpectations:

I don't think this file is modified anymore.

[Jiewen Tan]

Comment on attachment 296336 [details]
Part 1 of 2 SPKI

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

Thanks Brent for r+ my patch.

>> Source/WebCore/crypto/algorithms/CryptoAlgorithmRSASSA_PKCS1_v1_5.cpp:164
>> +        // A bug is filed to keep it in track: https://bugs.webkit.org/show_bug.cgi?id=165436
> 
> This should just be:
> 
> // FIXME: <webkit.org/b/165436>

Fixed.

>> Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:177
>> +        // A bug is filed to keep it in track: https://bugs.webkit.org/show_bug.cgi?id=165436
> 
> Ditto: Just use the standard FIXME format, no need for other words.

Fixed.

>> Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:232
>> +        // A bug is filed to keep it in track: https://bugs.webkit.org/show_bug.cgi?id=165437
> 
> Ditto.

Fixed.

>> LayoutTests/ChangeLog:39
>> +        * platform/mac/TestExpectations:
> 
> I don't think this file is modified anymore.

Fixed.

[Jiewen Tan]

Created attachment 296355 [details]
Patch for landing (1/2)

[Jiewen Tan]

Committed r209437: <http://trac.webkit.org/changeset/209437>

[Jiewen Tan]

Reopen for part 2.

[Jiewen Tan]

Created attachment 296371 [details]
Part 2 of 2 PKCS8

[Brent Fulgham]

Comment on attachment 296371 [details]
Part 2 of 2 PKCS8

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

This patch looks good, but I noticed that potential buffer overrun. I think you need to fix that before landing, and please double-check that we don't have the same mistake elsewhere in the crypto stuff.

r- to correct the buffer overrun issue.

> Source/WebCore/crypto/mac/CryptoKeyRSAMac.cpp:396
> +    if (keyData.size() < headerSize)

If "keyData.size() == headerSize", and then you dereference as "keyData[headerSize]" we have a problem.

You might want to double-check the other places you use this code, because I might not have noticed this previously.

> LayoutTests/crypto/subtle/import-key-malformed-parameters-expected.txt:-18
> -PASS crypto.subtle.importKey("pkcs8", rawKey, "rsaes-pkcs1-v1_5", extractable, ["encrypt", "decrypt"]) rejected promise  with NotSupportedError (DOM Exception 9): The operation is not supported..

Good!

> LayoutTests/crypto/subtle/import-key-malformed-parameters.html:-52
> -shouldReject('crypto.subtle.importKey("pkcs8", rawKey, "rsaes-pkcs1-v1_5", extractable, ["encrypt", "decrypt"])');

Yay!

> LayoutTests/imported/w3c/WebCryptoAPI/encrypt_decrypt/test_rsa_oaep-expected.txt:99
> +PASS RSA-OAEP with SHA-512 and a label using privateKey to encrypt 

Very nice!

[Jiewen Tan]

Comment on attachment 296371 [details]
Part 2 of 2 PKCS8

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

Thanks Brent for reviewing my patch.

>> Source/WebCore/crypto/mac/CryptoKeyRSAMac.cpp:396
>> +    if (keyData.size() < headerSize)
> 
> If "keyData.size() == headerSize", and then you dereference as "keyData[headerSize]" we have a problem.
> 
> You might want to double-check the other places you use this code, because I might not have noticed this previously.

Oops. Forgot to + 1. Sure, I will double check the SPKI one. Fixed.

[Jiewen Tan]

Created attachment 296545 [details]
Part 2 of 2 PKCS8

[Build Bot]

Comment on attachment 296545 [details]
Part 2 of 2 PKCS8

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

New failing tests:
crypto/subtle/rsa-import-key-malformed-parameters.html

[Build Bot]

Created attachment 296551 [details]
Archive of layout-test-results from ews100 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 296545 [details]
Part 2 of 2 PKCS8

Attachment 296545 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2655265

New failing tests:
crypto/subtle/rsa-import-key-malformed-parameters.html

[Build Bot]

Created attachment 296553 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Jiewen Tan]

Created attachment 296565 [details]
Part 2 of 2 PKCS8

[Brent Fulgham]

Comment on attachment 296565 [details]
Part 2 of 2 PKCS8

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

r=me

> LayoutTests/imported/w3c/WebCryptoAPI/encrypt_decrypt/test_rsa_oaep-expected.txt:99
> +PASS RSA-OAEP with SHA-512 and a label using privateKey to encrypt 

Excellent!

[Jiewen Tan]

Comment on attachment 296565 [details]
Part 2 of 2 PKCS8

Thanks Brent for r+ my patch.

[Jiewen Tan]

Committed r209633: <http://trac.webkit.org/changeset/209633>