Bug 144938 - WebCrypto: RSASSA-PKCS1-v1_5 generated keys can not be exported
Summary: WebCrypto: RSASSA-PKCS1-v1_5 generated keys can not be exported
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 128749 (view as bug list)
Depends on:
Blocks:
 
Reported: 2015-05-12 16:57 PDT by Ogi Enc
Modified: 2015-11-27 15:44 PST (History)
7 users (show)

See Also:


Attachments
Patch (29.67 KB, patch)
2015-11-04 20:10 PST, Scott Valentine
no flags Details | Formatted Diff | Diff
Patch (26.21 KB, patch)
2015-11-05 17:02 PST, Scott Valentine
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews114 for mac-yosemite (915.60 KB, application/zip)
2015-11-05 17:54 PST, Build Bot
no flags Details
Patch (27.25 KB, patch)
2015-11-05 18:59 PST, Scott Valentine
no flags Details | Formatted Diff | Diff
Patch (31.23 KB, patch)
2015-11-06 14:02 PST, Scott Valentine
no flags Details | Formatted Diff | Diff
Patch (31.18 KB, patch)
2015-11-06 14:42 PST, Scott Valentine
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ogi Enc 2015-05-12 16:57:29 PDT
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.
Comment 1 Scott Valentine 2015-11-04 20:10:09 PST
Created attachment 264838 [details]
Patch
Comment 2 WebKit Commit Bot 2015-11-04 20:12:46 PST
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.
Comment 3 Alexey Proskuryakov 2015-11-04 20:51:25 PST
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.
Comment 4 Scott Valentine 2015-11-05 16:51:25 PST
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.
Comment 5 Alexey Proskuryakov 2015-11-05 16:56:34 PST
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 ]
Comment 6 Scott Valentine 2015-11-05 17:02:26 PST
Created attachment 264902 [details]
Patch
Comment 7 WebKit Commit Bot 2015-11-05 17:04:51 PST
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.
Comment 8 Scott Valentine 2015-11-05 17:12:30 PST
(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.
Comment 9 Build Bot 2015-11-05 17:54:42 PST
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
Comment 10 Build Bot 2015-11-05 17:54:45 PST
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
Comment 11 Scott Valentine 2015-11-05 18:59:18 PST
Created attachment 264914 [details]
Patch
Comment 12 WebKit Commit Bot 2015-11-05 19:00:17 PST
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.
Comment 13 Scott Valentine 2015-11-05 19:04:12 PST
(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
Comment 14 Alexey Proskuryakov 2015-11-05 21:47:33 PST
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.
Comment 15 Scott Valentine 2015-11-06 14:02:58 PST
Created attachment 264956 [details]
Patch
Comment 16 Scott Valentine 2015-11-06 14:05:46 PST
Cleaned up the remaining nits, and remembered to "git add" the unit test this time.
Comment 17 WebKit Commit Bot 2015-11-06 14:08:03 PST
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.
Comment 18 Alexey Proskuryakov 2015-11-06 14:08:06 PST
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.
Comment 19 Scott Valentine 2015-11-06 14:40:46 PST
Good point... sending it up now.
Comment 20 Scott Valentine 2015-11-06 14:42:43 PST
Created attachment 264958 [details]
Patch
Comment 21 WebKit Commit Bot 2015-11-06 14:49:10 PST
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.
Comment 22 Alexey Proskuryakov 2015-11-06 17:11:56 PST
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.
Comment 23 Scott Valentine 2015-11-06 19:27:44 PST
(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?
Comment 24 Alexey Proskuryakov 2015-11-06 19:52:11 PST
Comment on attachment 264958 [details]
Patch

Yes, it will work despite the style queue being unhappy.
Comment 25 WebKit Commit Bot 2015-11-06 20:43:58 PST
Comment on attachment 264958 [details]
Patch

Clearing flags on attachment: 264958

Committed r192126: <http://trac.webkit.org/changeset/192126>
Comment 26 WebKit Commit Bot 2015-11-06 20:44:02 PST
All reviewed patches have been landed.  Closing bug.
Comment 27 Daniel Bates 2015-11-06 21:30:05 PST
(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>.
Comment 28 Daniel Bates 2015-11-06 21:31:26 PST
(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>.
Comment 29 Scott Valentine 2015-11-06 22:38:12 PST
(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.
Comment 31 Alexey Proskuryakov 2015-11-09 14:42:11 PST
Perhaps marking it a Slow would help?
Comment 32 Ryan Haddad 2015-11-09 14:47:22 PST
Alright, marked as slow in <https://trac.webkit.org/r192182>
Comment 33 Scott Valentine 2015-11-09 17:57:15 PST
It might help to change the generated key size to 2048.
Comment 34 Alexey Proskuryakov 2015-11-10 16:30:26 PST
Changed it to 2048 in r192279, and removed the Slow expectation, as this should make it fast enough indeed.
Comment 35 Alexey Proskuryakov 2015-11-27 15:44:56 PST
*** Bug 128749 has been marked as a duplicate of this bug. ***