Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
BenWiederhake opened this issue Oct 26, 2024 · 1 comment · May be fixed by #382
Open

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

BenWiederhake opened this issue Oct 26, 2024 · 1 comment · May be fixed by #382

Comments

@BenWiederhake
Copy link
Collaborator

BenWiederhake commented Oct 26, 2024

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

Some observations:

  • The decrypt operation does not explain how to handle errors in Step 2 ("… performing the CBC Decryption operation …"), seemingly assuming that no error can occur
  • The decrypt operation goes on to explain step-by-step the details of padding removal, specifying that inconsistent padding should result in an OperationError, but remains silent about other error types.
  • Most other situations that similarly call into a third-party library in order to perform some high-level action explicitly state the error type, e.g. ECDSA generateKey: "If performing the key generation operation results in an error, then throw an OperationError."
  • The description of SubtleCrypto.importKey contains nothing about this specific detail. (As expected)

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
Copy link
Member

twiss commented Oct 28, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants