From ee63174d6ff0b3b7d9b255fc47c72a82ae63bc09 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20W=C3=BClker?= Date: Sat, 16 Nov 2024 00:15:32 +0100 Subject: [PATCH] subtlecrypto: Don't throw exceptions twice when converting to Algorithm object (#34239) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Don't throw exceptions twice when converting to Algorithm object Removes match statements like ```rust let Ok(ConversionResult::Success(algorithm)) = Algorithm::new(cx, value.handle()) else { return Err(Error::Syntax); }; ``` These don't cause issues if `Algorithm::new` returns `Ok(ConversionResult::Failure`, but in the case of `Err(())` the implementation already called `throw_type_error` and we must not throw an additional Syntax error, otherwise we'll crash. Luckily, this case is already handled elsewhere by the `value_from_js_object` macro. Signed-off-by: Simon Wülker * Test that calling subtlecrypto methods with empty algorithm objects throws a TypeError The WebCryptoAPI spec does not tell us which error to throw exactly, but according to https://webidl.spec.whatwg.org/ it should be a TypeError. This previously crashed servo. Signed-off-by: Simon Wülker --------- Signed-off-by: Simon Wülker --- components/script/dom/subtlecrypto.rs | 32 ++++--------------- tests/wpt/meta/MANIFEST.json | 6 ++-- .../WebCryptoAPI/digest/digest.https.any.js | 14 ++++++++ .../WebCryptoAPI/generateKey/failures.js | 8 +++++ .../import_export/importKey_failures.js | 15 +++++++++ 5 files changed, 47 insertions(+), 28 deletions(-) diff --git a/components/script/dom/subtlecrypto.rs b/components/script/dom/subtlecrypto.rs index efb3009a831..26c415b04c7 100644 --- a/components/script/dom/subtlecrypto.rs +++ b/components/script/dom/subtlecrypto.rs @@ -1126,10 +1126,7 @@ fn normalize_algorithm_for_get_key_length( match algorithm { AlgorithmIdentifier::Object(obj) => { rooted!(in(*cx) let value = ObjectValue(obj.get())); - let Ok(ConversionResult::Success(algorithm)) = Algorithm::new(cx, value.handle()) - else { - return Err(Error::Syntax); - }; + let algorithm = value_from_js_object!(Algorithm, cx, value); let name = algorithm.name.str(); let normalized_algorithm = if name.eq_ignore_ascii_case(ALG_AES_CBC) || @@ -1162,10 +1159,7 @@ fn normalize_algorithm_for_digest( let name = match algorithm { AlgorithmIdentifier::Object(obj) => { rooted!(in(*cx) let value = ObjectValue(obj.get())); - let Ok(ConversionResult::Success(algorithm)) = Algorithm::new(cx, value.handle()) - else { - return Err(Error::Syntax); - }; + let algorithm = value_from_js_object!(Algorithm, cx, value); algorithm.name.str().to_uppercase() }, @@ -1191,10 +1185,7 @@ fn normalize_algorithm_for_import_key( let name = match algorithm { AlgorithmIdentifier::Object(obj) => { rooted!(in(*cx) let value = ObjectValue(obj.get())); - let Ok(ConversionResult::Success(algorithm)) = Algorithm::new(cx, value.handle()) - else { - return Err(Error::Syntax); - }; + let algorithm = value_from_js_object!(Algorithm, cx, value); let name = algorithm.name.str().to_uppercase(); if name == ALG_HMAC { @@ -1230,9 +1221,7 @@ fn normalize_algorithm_for_derive_bits( }; rooted!(in(*cx) let value = ObjectValue(obj.get())); - let Ok(ConversionResult::Success(algorithm)) = Algorithm::new(cx, value.handle()) else { - return Err(Error::Syntax); - }; + let algorithm = value_from_js_object!(Algorithm, cx, value); let normalized_algorithm = if algorithm.name.str().eq_ignore_ascii_case(ALG_PBKDF2) { let params = value_from_js_object!(Pbkdf2Params, cx, value); @@ -1260,9 +1249,7 @@ fn normalize_algorithm_for_encrypt_or_decrypt( }; rooted!(in(*cx) let value = ObjectValue(obj.get())); - let Ok(ConversionResult::Success(algorithm)) = Algorithm::new(cx, value.handle()) else { - return Err(Error::Syntax); - }; + let algorithm = value_from_js_object!(Algorithm, cx, value); let name = algorithm.name.str(); let normalized_algorithm = if name.eq_ignore_ascii_case(ALG_AES_CBC) { @@ -1287,10 +1274,7 @@ fn normalize_algorithm_for_sign_or_verify( let name = match algorithm { AlgorithmIdentifier::Object(obj) => { rooted!(in(*cx) let value = ObjectValue(obj.get())); - let Ok(ConversionResult::Success(algorithm)) = Algorithm::new(cx, value.handle()) - else { - return Err(Error::Syntax); - }; + let algorithm = value_from_js_object!(Algorithm, cx, value); algorithm.name.str().to_uppercase() }, @@ -1316,9 +1300,7 @@ fn normalize_algorithm_for_generate_key( }; rooted!(in(*cx) let value = ObjectValue(obj.get())); - let Ok(ConversionResult::Success(algorithm)) = Algorithm::new(cx, value.handle()) else { - return Err(Error::Syntax); - }; + let algorithm = value_from_js_object!(Algorithm, cx, value); let name = algorithm.name.str(); let normalized_algorithm = diff --git a/tests/wpt/meta/MANIFEST.json b/tests/wpt/meta/MANIFEST.json index de7136868a3..2ad9a96984e 100644 --- a/tests/wpt/meta/MANIFEST.json +++ b/tests/wpt/meta/MANIFEST.json @@ -358512,7 +358512,7 @@ }, "generateKey": { "failures.js": [ - "e0f0279a69bb885eb30cbe086796c281db0245bb", + "deaac636a99be570b118d8e8f8096b36a4a0b28f", [] ], "successes.js": [ @@ -358526,7 +358526,7 @@ [] ], "importKey_failures.js": [ - "bba48401e616a564200570dddb2fe06d762347fe", + "077ae076c648b097977cdbc484eaf79db884f7b2", [] ], "okp_importKey.js": [ @@ -516888,7 +516888,7 @@ }, "digest": { "digest.https.any.js": [ - "379d9311f30247b85e2e5ea8981d1d180fe783a1", + "3b0972b1f2bf7d5a285765ab7cf2447acca21467", [ "WebCryptoAPI/digest/digest.https.any.html", { diff --git a/tests/wpt/tests/WebCryptoAPI/digest/digest.https.any.js b/tests/wpt/tests/WebCryptoAPI/digest/digest.https.any.js index 379d9311f30..3b0972b1f2b 100644 --- a/tests/wpt/tests/WebCryptoAPI/digest/digest.https.any.js +++ b/tests/wpt/tests/WebCryptoAPI/digest/digest.https.any.js @@ -118,6 +118,20 @@ }); }); + // Call digest() with empty algorithm object + Object.keys(sourceData).forEach(function(size) { + promise_test(function(test) { + var promise = subtle.digest({}, sourceData[size]) + .then(function(result) { + assert_unreached("digest() with missing algorithm name should have thrown a TypeError"); + }, function(err) { + assert_equals(err.name, "TypeError", "Missing algorithm name should cause TypeError") + }); + + return promise; + }, "empty algorithm object with " + size); + }); + done(); diff --git a/tests/wpt/tests/WebCryptoAPI/generateKey/failures.js b/tests/wpt/tests/WebCryptoAPI/generateKey/failures.js index e0f0279a69b..deaac636a99 100644 --- a/tests/wpt/tests/WebCryptoAPI/generateKey/failures.js +++ b/tests/wpt/tests/WebCryptoAPI/generateKey/failures.js @@ -166,6 +166,14 @@ function run_test(algorithmNames) { }); }); + // Empty algorithm should fail with TypeError + allValidUsages(["decrypt", "sign", "deriveBits"], true, []) // Small search space, shouldn't matter because should fail before used + .forEach(function(usages) { + [false, true, "RED", 7].forEach(function(extractable){ + testError({}, extractable, usages, "TypeError", "Empty algorithm"); + }); + }); + // Algorithms normalize okay, but usages bad (though not empty). // It shouldn't matter what other extractable is. Should fail diff --git a/tests/wpt/tests/WebCryptoAPI/import_export/importKey_failures.js b/tests/wpt/tests/WebCryptoAPI/import_export/importKey_failures.js index bba48401e61..077ae076c64 100644 --- a/tests/wpt/tests/WebCryptoAPI/import_export/importKey_failures.js +++ b/tests/wpt/tests/WebCryptoAPI/import_export/importKey_failures.js @@ -192,4 +192,19 @@ function run_test(algorithmNames) { }); }); }); + + // Missing mandatory "name" field on algorithm + testVectors.forEach(function(vector) { + var name = vector.name; + // We just need *some* valid keydata, so pick the first available algorithm. + var algorithm = allAlgorithmSpecifiersFor(name)[0]; + getValidKeyData(algorithm).forEach(function(test) { + validUsages(vector, test.format, test.data).forEach(function(usages) { + [true, false].forEach(function(extractable) { + testError(test.format, {}, test.data, name, usages, extractable, "TypeError", "Missing algorithm name"); + }); + }); + }); + }); + }