1270599 - crypto.subtle.generateKey throws wrong exception for bad algorithm argument

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P3)

Description

[Charles Engelke]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/50.0.2661.86 Safari/537.36

Steps to reproduce:

Open the developer console and enter the following code:

window.crypto.subtle.generateKey({name: "AES", length: 128}, true, ["encrypt", "decrypt"]).then(function(key) {console.log("It should not have worked.");}).catch(function(err) {console.log("Error is " + err.name);})


Actual results:

"Error is SyntaxError" is displayed.


Expected results:

"Error is NotSupportedError" should have been displayed.

The Web Cryptography API specification section "14.3.6 generateKey" has "normalize an algorithm" as step 2. Section "18.4.4. Normalizing an algorithm" starts by finding the algorithm name in the list of registered algorithm names ("AES" is not in that list). If it is not found, the specification says "Return a new NotSupportedError and terminate this algorithm.".

Comment 1

[Boris Zbarsky [:bzbarsky]]

Looks to me like GetAlgorithmName() in WebCryptoTask.cpp should be returning NS_ERROR_DOM_NOT_SUPPORTED_ERR if NormalizeToken() returns false.  The other callers of NormalizeToken seem to do that.  Tim, can you take a look?

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Boris Zbarsky [:bz] from comment #1)
> Looks to me like GetAlgorithmName() in WebCryptoTask.cpp should be returning
> NS_ERROR_DOM_NOT_SUPPORTED_ERR if NormalizeToken() returns false.  The other
> callers of NormalizeToken seem to do that.  Tim, can you take a look?

Yes, we should do that.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1270599-Return-a-NotSupportedError-when-encounte.patch

Comment 4

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8754404 [details] [diff] [review]
0001-Bug-1270599-Return-a-NotSupportedError-when-encounte.patch

Review of attachment 8754404 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good, but there are a few places in WebCryptoTask.cpp where a failing return value from GetAlgorithmName is converted to NS_ERROR_DOM_SYNTAX_ERR or NS_ERROR_DOM_DATA_ERR. Are these correct?

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1270599-Return-a-NotSupportedError-when-encounte.patch, v2

(In reply to David Keeler [:keeler] (use needinfo?) from comment #4)
> This looks good, but there are a few places in WebCryptoTask.cpp where a
> failing return value from GetAlgorithmName is converted to
> NS_ERROR_DOM_SYNTAX_ERR or NS_ERROR_DOM_DATA_ERR. Are these correct?

Good point, this should cover everything now.

Comment 6

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8754828 [details] [diff] [review]
0001-Bug-1270599-Return-a-NotSupportedError-when-encounte.patch, v2

Review of attachment 8754828 [details] [diff] [review]:
-----------------------------------------------------------------

Cool - this makes more sense. Just a couple of questions and some suggestions for a few more testcases.

::: dom/crypto/WebCryptoTask.cpp
@@ +1117,5 @@
>        }
>  
>        mEarlyRv = GetAlgorithmName(aCx, params.mHash, hashAlgName);
>        if (NS_FAILED(mEarlyRv)) {
> +        mEarlyRv = NS_ERROR_DOM_NOT_SUPPORTED_ERR;

Do we even want to change mEarlyRv here? What if it's a failure unrelated to whether or not it's a known algorithm?

@@ +1256,5 @@
>      nsString algName;
>      mEarlyRv = GetAlgorithmName(aCx, aAlgorithm, algName);
>      if (NS_FAILED(mEarlyRv)) {
> +      MOZ_ASSERT(false); // This shouldn't happen.
> +      mEarlyRv = NS_ERROR_DOM_NOT_SUPPORTED_ERR;

Same question for all of these, really. (Although for the ones where we assert false, are we sure GetAlgorithmName can't fail for reasons like OOM?)

::: dom/crypto/test/test_WebCrypto_Errors.html
@@ +23,5 @@
> +"use strict";
> +
> +// -----------------------------------------------------------------------------
> +TestArray.addTest(
> +  "generateKey() should return NotSupportedError for unknown algorithms",

We should probably test deriveKey as well, right?

@@ +144,5 @@
> +);
> +
> +// -----------------------------------------------------------------------------
> +TestArray.addTest(
> +  "ECDSA sign should return NotSupportedError for unknown hash algorithms",

Should we also have a case where crypto.subtle.sign is given some alg = { name: "SomeAlgorithm" }? (Same with verify)

Comment 7

[R. Martinho Fernandes [:rmf]]

Attachment: Bug 1270599 - Add test for already-fixed bug r=keeler

Depends on D79510

Comment 8

[R. Martinho Fernandes [:rmf]]

Attachment: Bug 1270599 - add test to ensure generateKey returns the correct error given an invalid algorithm argument r=keeler

Depends on D79455

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2ae604119d7b
add test to ensure generateKey returns the correct error given an invalid algorithm argument r=keeler

Comment 10

[Razvan Maries]

Backed out for perma failures.

See https://bugzilla.mozilla.org/show_bug.cgi?id=1270634#c9

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c97c21a56392
add test to ensure generateKey returns the correct error given an invalid algorithm argument r=keeler

Comment 12

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/c97c21a56392