w3c / webcrypto

The W3C Web Cryptography API
https://w3c.github.io/webcrypto/
Other
267 stars 55 forks source link

AES-CBC decrypt: Unspecified error type for ciphertext of odd length #381

Open BenWiederhake opened 3 weeks ago

BenWiederhake commented 3 weeks ago

Context: https://w3c.github.io/webcrypto/#aes-cbc-operations

Some observations:

Suggestion: Insert between Step 1 and Step 2: "If ciphertext does not have a length that is a multiple of 16 bytes, then throw an OperationError." (Or some other Error type.)

Minimal-ish reproducer:

var keyData = new Uint8Array([
    0x20, 0x21, 0x22, 0x23, 0x24, 0x25, 0x26, 0x27, 0x28, 0x29, 0x2a, 0x2b, 0x2c, 0x2d, 0x2e, 0x2f]);
var ivData = new Uint8Array([
    0x30, 0x31, 0x32, 0x33, 0x34, 0x35, 0x36, 0x37, 0x38, 0x39, 0x3a, 0x3b, 0x3c, 0x3d, 0x3e, 0x3f]);
var ciphertextOdd = new Uint8Array([0x04, 0x05, 0x06, 0x07, 0x08]);
var aesAlgorithmKeyGen = { name: "AES-CBC", length: 128 };
var aesAlgorithmDecrypt = { name: "AES-CBC", iv: ivData };
var aesKey = await window.crypto.subtle.importKey("raw", keyData, aesAlgorithmKeyGen, false, ["decrypt"]);
await window.crypto.subtle.decrypt(aesAlgorithmDecrypt, aesKey, ciphertextOdd)
.then((res) => { console.log("success?!"); }, (err) => { console.log("failed with " + err.name); } );

I can't find a WPT test that covers this edge case. Note that bigPadChar, inconsistentPadChars, and zeroPadChar are different edge cases.

It seems that Firefox and Chrome agree that this should result in an OperationError. However, I'm not entirely confident that all the other implementors agree with that. Given that there is no WPT test for this, I don't want to assume that this as a given, even though OperationError is the obvious choice here.

EDIT: That's why I think this would be a substantive change, because it clears up an […] under-specified part of the specification in such a way that […] an agent whose conformance was once unclear becomes clearly either conforming or non-conforming, even though it's only a hypothetical such agent. For example, that hypothetical agent might raise a DataError.

I tried to sign up using the link you provided to me, @twiss, but it seems I'm not allowed to do so without a special affiliation. (I do not work for a W3C Member organization, and strongly prefer to not pull my employer into this. Therefore my only option is to "participate as an Invited Expert", which sounds bureaucratically and legally intense.)

Therefore, I'm unable to write a PR.

twiss commented 3 weeks ago

Thanks for the report! I also agree that throwing OperationError is the obvious outcome; unfortunately it seems Safari's current implementation doesn't agree and returns an empty ArrayBuffer for your test case (cc @nmahendru).

So, I agree it's technically a substantive change, though a fairly minor and hopefully non-controversial one. And clearly we should have a WPT test for this.

And, makes sense! I'll make a PR.