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

NEW
Unassigned

Status

()

P3
normal
3 years ago
5 months ago

People

(Reporter: bugzilla, Unassigned)

Tracking

46 Branch
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [domsecurity-backlog2])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

3 years ago
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.".
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?
Flags: needinfo?(ttaubert)
Status: UNCONFIRMED → NEW
Component: Untriaged → DOM: Security
Ever confirmed: true
Product: Firefox → Core
Component: DOM: Security → Security
(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.
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Flags: needinfo?(ttaubert)
Created attachment 8754404 [details] [diff] [review]
0001-Bug-1270599-Return-a-NotSupportedError-when-encounte.patch
Attachment #8754404 - Flags: review?(dkeeler)
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?
Attachment #8754404 - Flags: review?(dkeeler) → review+
Created attachment 8754828 [details] [diff] [review]
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.
Attachment #8754404 - Attachment is obsolete: true
Attachment #8754828 - Flags: review?(dkeeler)
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)
Attachment #8754828 - Flags: review?(dkeeler) → review+
Component: Security → DOM: Security
Priority: -- → P3
Whiteboard: [domsecurity-backlog2]
Assignee: ttaubert → nobody
Status: ASSIGNED → NEW
You need to log in before you can comment on or make changes to this bug.