WebCrypto: RSASSA-PKCS1-v1_5 generated keys can not be exported

• Component: New Bugs
• Status: RESOLVED FIXED

[Ogi Enc]

Generating RSASSA key and trying to export JWK of either public or private key, fails with:

"Key algorithm and size do not map to any JWK algorithm identifier"

Unlike the current revision of WebCrypto standard, version Webkit implements did not specify hash during key generation, but during "sign" and "verify" functions.
However, exportKey function requires hash to be specified for JWK export. Current unit tests only validate exportKey with keys imported with importKey (which properly set hash values), but not with generateKey.

This is related to bug 128749, since if a hash could be specified during key generation, this would not be an issue. Otherwise, maybe hash could be specified as argument to exportKey.

[Scott Valentine]

Created attachment 264838 [details]
Patch

[WebKit Commit Bot]

Attachment 264838 [details] did not pass style-queue:


ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSAES_PKCS1_v1_5.cpp:99:  CryptoAlgorithmRSAES_PKCS1_v1_5::importKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexey Proskuryakov]

Comment on attachment 264838 [details]
Patch

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

The approach looks good to me, however EWS detects crashes (assertion failures?)

> Source/WebCore/bindings/js/JSCryptoAlgorithmDictionary.cpp:181
> +    if (!getHashAlgorithm(jsDictionary, result->hash, true)) {

We try very hard to not have mysterious true/false arguments in new code. Please add either an enum class for the value, or a separate function.

[Scott Valentine]

I see some failures running with or without this patch. Here is the output without the patch:

************************************************************************************

Expected to timeout, but passed: (3)
  fast/frames/lots-of-objects.html
  imported/w3c/web-platform-tests/html/semantics/document-metadata/the-base-element/base_multiple.html
  imported/w3c/web-platform-tests/html/semantics/embedded-content/the-object-element/object-handler.html

Expected to fail, but passed: (4)
  fast/hidpi/filters-blur.html
  fast/hidpi/image-srcset-svg-canvas.html
  imported/w3c/web-platform-tests/html/semantics/text-level-semantics/the-wbr-element/wbr-element.html
  plugins/navigator-plugins.html


Unexpected flakiness: timeouts (7)
  accessibility/accessibility-node-memory-management.html [ Timeout Pass ]
  accessibility/mac/abbr-acronym-tags.html [ Timeout Pass ]
  accessibility/mac/select-text/select-text-1.html [ Timeout Pass ]
  animations/3d/change-transform-in-end-event.html [ ImageOnlyFailure Timeout Pass ]
  animations/added-while-suspended.html [ Timeout Pass ]
  canvas/philip/tests/2d.canvas.readonly.html [ Timeout Pass ]
  imported/w3c/web-platform-tests/XMLHttpRequest/event-readystatechange-loaded.htm [ Timeout Pass ]


Regressions: Unexpected text-only failures (1)
  http/tests/media/hls/video-controller-getStartDate.html [ Failure ]

Regressions: Unexpected timeouts (1)
  crypto/subtle/rsa-export-generated-keys.html [ Timeout ]

************************************************************************************


And here is the output after the patch:


************************************************************************************

Expected to timeout, but passed: (3)
  fast/frames/lots-of-objects.html
  imported/w3c/web-platform-tests/html/semantics/document-metadata/the-base-element/base_multiple.html
  imported/w3c/web-platform-tests/html/semantics/embedded-content/the-object-element/object-handler.html

Expected to fail, but passed: (4)
  fast/hidpi/filters-blur.html
  fast/hidpi/image-srcset-svg-canvas.html
  imported/w3c/web-platform-tests/html/semantics/text-level-semantics/the-wbr-element/wbr-element.html
  plugins/navigator-plugins.html


Unexpected flakiness: timeouts (13)
  accessibility/accessibility-node-memory-management.html [ Timeout Pass ]
  accessibility/mac/abbr-acronym-tags.html [ Timeout Pass ]
  accessibility/mac/select-text/select-text-1.html [ Timeout Pass ]
  animations/3d/change-transform-in-end-event.html [ ImageOnlyFailure Timeout Pass ]
  animations/added-while-suspended.html [ Timeout Pass ]
  canvas/philip/tests/2d.canvas.readonly.html [ Timeout Pass ]
  http/tests/notifications/events.html [ Crash Timeout Pass ]
  imported/w3c/indexeddb/idbcursor-advance-invalid.htm [ Timeout Pass ]
  imported/w3c/indexeddb/idbdatabase_createObjectStore8-parameters.htm [ Timeout Pass ]
  storage/indexeddb/mozilla/cursor-update-updates-indexes.html [ Failure Timeout Pass ]
  storage/indexeddb/mozilla/cursors.html [ Timeout Pass ]
  webgl/1.0.2/conformance/context/context-lost-restored.html [ Timeout Pass ]
  webgl/1.0.2/conformance/rendering/point-size.html [ Timeout Pass ]


Regressions: Unexpected text-only failures (1)
  http/tests/media/hls/video-controller-getStartDate.html [ Failure ]

************************************************************************************

So things are pretty much behaving as expected WRT the patch. The regression failure is due to a buggy test... here's the output diff:

-EXPECTED (video.getStartDate() == 'Wed Nov 03 2010 01:00:00 GMT-0700 (PDT)') OK
+EXPECTED (video.getStartDate() == 'Wed Nov 03 2010 01:00:00 GMT-0700 (PDT)'), OBSERVED 'Tue Nov 02 2010 22:00:00 GMT-1000 (HST)' FAIL

So that is only failing because I'm in the wrong timezone.

In any case, I did a git pull today and just re-ran these to verify, I promoted the boolean flag to an enum as requested, and will be submitting a new patch momentarily.

Let me know what I can do if there are still failures on EWS.

[Alexey Proskuryakov]

Are you building debug or release? Assertions are disabled in release builds.

It is unfortunate that you are getting multiple failures that bots don't have (these could be hardware dependent), but this is what the problem detected by the bot was:

Regressions: Unexpected crashes (2)
  crypto/subtle/rsaes-pkcs1-v1_5-decrypt.html [ Crash ]
  crypto/subtle/rsaes-pkcs1-v1_5-wrap-unwrap-aes.html [ Crash ]

[Scott Valentine]

Created attachment 264902 [details]
Patch

[WebKit Commit Bot]

Attachment 264902 [details] did not pass style-queue:


ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSAES_PKCS1_v1_5.cpp:99:  CryptoAlgorithmRSAES_PKCS1_v1_5::importKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Scott Valentine]

(In reply to comment #5)
> Are you building debug or release? Assertions are disabled in release builds.
> 
> It is unfortunate that you are getting multiple failures that bots don't
> have (these could be hardware dependent), but this is what the problem
> detected by the bot was:
> 
> Regressions: Unexpected crashes (2)
>   crypto/subtle/rsaes-pkcs1-v1_5-decrypt.html [ Crash ]
>   crypto/subtle/rsaes-pkcs1-v1_5-wrap-unwrap-aes.html [ Crash ]

I'll look into those further. I was just doing a build-webkit without any options, I will do a --debug build. I'm running on an older macbook pro with El Capitan.

[Build Bot]

Comment on attachment 264902 [details]
Patch

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

New failing tests:
crypto/subtle/rsaes-pkcs1-v1_5-wrap-unwrap-aes.html
crypto/subtle/rsaes-pkcs1-v1_5-decrypt.html

[Build Bot]

Created attachment 264910 [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

[Scott Valentine]

Created attachment 264914 [details]
Patch

[WebKit Commit Bot]

Attachment 264914 [details] did not pass style-queue:


ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSAES_PKCS1_v1_5.cpp:99:  CryptoAlgorithmRSAES_PKCS1_v1_5::importKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Scott Valentine]

(In reply to comment #6)
> Created attachment 264902 [details]
> Patch

I was able to reproduce the crashes in the debug build, and this version of the patch fixes the issue. I missed one case in JSCryptoKeySerializationJWK.cpp

[Alexey Proskuryakov]

Comment on attachment 264914 [details]
Patch

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

Looks good to me. Marking r- because there are minor nits that needs to be addressed.

> Source/WebCore/bindings/js/JSCryptoAlgorithmDictionary.cpp:49
> +enum HashRequirement {

Please use a strongly typed enum (enum class).

> Source/WebCore/bindings/js/JSCryptoAlgorithmDictionary.cpp:50
> +    HashIsOptional = 0,

No need for a value here.

> Source/WebCore/bindings/js/JSCryptoAlgorithmDictionary.cpp:121
> +        if (HashIsRequired == isRequired)

While it is sometimes recommended to put constants at the left to avoid accidental assignment, this is not a style we use in WebKit. We prefer readability over catching such mistakes, which are caught by compiler warnings and by tests anyway.

[Scott Valentine]

Created attachment 264956 [details]
Patch

[Scott Valentine]

Cleaned up the remaining nits, and remembered to "git add" the unit test this time.

[WebKit Commit Bot]

Attachment 264956 [details] did not pass style-queue:


ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSAES_PKCS1_v1_5.cpp:99:  CryptoAlgorithmRSAES_PKCS1_v1_5::importKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexey Proskuryakov]

Comment on attachment 264956 [details]
Patch

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

> Source/WebCore/bindings/js/JSCryptoAlgorithmDictionary.cpp:52
> +enum class HashRequirement {
> +    HashIsOptional,
> +    HashIsRequired, 
> +};

I'd use shorter values, given that the enum name is now required, e.g. HashRequirement::Optional.

[Scott Valentine]

Good point... sending it up now.

[Scott Valentine]

Created attachment 264958 [details]
Patch

[WebKit Commit Bot]

Attachment 264958 [details] did not pass style-queue:


ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSAES_PKCS1_v1_5.cpp:99:  CryptoAlgorithmRSAES_PKCS1_v1_5::importKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexey Proskuryakov]

Please mark the patch as "commit-queue?" if you'd like it to be automatically landed. This is available through Details link to the right of the patch.

[Scott Valentine]

(In reply to comment #22)
> Please mark the patch as "commit-queue?" if you'd like it to be
> automatically landed. This is available through Details link to the right of
> the patch.

Is that going to work with the style-queue issue?

[Alexey Proskuryakov]

Comment on attachment 264958 [details]
Patch

Yes, it will work despite the style queue being unhappy.

[WebKit Commit Bot]

Comment on attachment 264958 [details]
Patch

Clearing flags on attachment: 264958

Committed r192126: <http://trac.webkit.org/changeset/192126>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Daniel Bates]

(In reply to comment #25)
> Comment on attachment 264958 [details]
> Patch
> 
> Clearing flags on attachment: 264958
> 
> Committed r192126: <http://trac.webkit.org/changeset/192126>

The format of the ChangeLog entries in this commit are not consistent with the format of a ChangeLog entry used by the WebKit OpenSource Project. In particular, the ChangeLog entries did not mention the name of the person that reviewed the patch (Alexey Proskuryakov). You can read more about the format of a ChangeLog entry in section ChangeLog files on <http://www.webkit.org/coding/contributing.html>.

Substituted "Unreviewed initial submission" for "Reviewed by Alexey Proskuryakov" in the ChangeLog entries associated with this patch and committed this change in <http://trac.webkit.org/changeset/192128>.

[Daniel Bates]

(In reply to comment #27)
[...]
> Substituted "Unreviewed initial submission" for "Reviewed by Alexey
> Proskuryakov" in the ChangeLog entries associated with this patch and
> committed this change in <http://trac.webkit.org/changeset/192128>.

I meant to write:

Substituted "Reviewed by Alexey Proskuryakov" for "Unreviewed initial submission" in the ChangeLog entries associated with this patch and committed this change in <http://trac.webkit.org/changeset/192128>.

[Scott Valentine]

(In reply to comment #28)
> (In reply to comment #27)
> [...]
> > Substituted "Unreviewed initial submission" for "Reviewed by Alexey
> > Proskuryakov" in the ChangeLog entries associated with this patch and
> > committed this change in <http://trac.webkit.org/changeset/192128>.
> 
> I meant to write:
> 
> Substituted "Reviewed by Alexey Proskuryakov" for "Unreviewed initial
> submission" in the ChangeLog entries associated with this patch and
> committed this change in <http://trac.webkit.org/changeset/192128>.

Thanks, I wasn't sure if that was automated or if I was supposed to do that.

[Ryan Haddad]

The test added with this patch is frequently timing out on Mac

Most recent timeout:
<https://build.webkit.org/builders/Apple%20Yosemite%20Release%20WK1%20(Tests)/builds/9786>

Flakiness Dashboard:
<http://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=crypto%2Fsubtle%2Frsa-export-generated-keys.html>

[Alexey Proskuryakov]

Perhaps marking it a Slow would help?

[Ryan Haddad]

Alright, marked as slow in <https://trac.webkit.org/r192182>

[Scott Valentine]

It might help to change the generated key size to 2048.

[Alexey Proskuryakov]

Changed it to 2048 in r192279, and removed the Slow expectation, as this should make it fast enough indeed.

[Alexey Proskuryakov]

*** Bug 128749 has been marked as a duplicate of this bug. ***