search

w3c / webcrypto

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

Throw on truncation in the ECDH derive bits operation #351

Open twiss opened 1 year ago

twiss commented 1 year ago

Throw on truncation in the ECDH derive bits operation, as discussed in #369.

Truncating the derived value in ECDH does not really make sense, and from Chromium's usage statistics, there is very low usage of truncation, so this change should be relatively low risk.

Preview | Diff

davidben commented 2 months ago

This should not be merged. See https://github.com/w3c/webcrypto/issues/369#issuecomment-2353175465

twiss commented 2 months ago

I've updated the MR to throw on all truncation instead of only for 0, as discussed in #369 :)

annevk commented 2 months ago

This seems reasonable, modulo what I wrote in the corresponding issue:

[S]omeone has to be willing to commit the time to see it through so we don't end up with a specification requiring something that's not web compatible in the end (or requiring something that nobody actually implements).

It's also a bit surprising to me that it didn't handle the case of length being too long, but maybe that is handled elsewhere. It's hard to find all the callers of the algorithms in this specification.

panva commented 2 months ago

It's also a bit surprising to me that it didn't handle the case of length being too long, but maybe that is handled elsewhere.

It is indeed

If the length of secret in bits is less than length: throw an OperationError.

javifernandez commented 1 month ago

It's also a bit surprising to me that it didn't handle the case of length being too long, but maybe that is handled elsewhere.

It is indeed

If the length of secret in bits is less than length: throw an OperationError.

This PR is removing this statement, though; we are requiring now that the value of the 'length' argument is exactly the expected key size for the used key algorithm.

panva commented 1 month ago

It's also a bit surprising to me that it didn't handle the case of length being too long, but maybe that is handled elsewhere.

It is indeed

If the length of secret in bits is less than length: throw an OperationError.

This PR is removing this statement, though; we are requiring now that the value of the 'length' argument is exactly the expected key size for the used key algorithm.

Correct. The case is handled before and after the PR.

panva commented 1 month ago

A change to reject truncated ECDH breaks deriveKey, wrapKey, and unwrapKey where the length sent to deriveBits is a result of the target key's get key length op.

javifernandez commented 1 month ago

A change to reject truncated ECDH breaks deriveKey, wrapKey, and unwrapKey where the length sent to deriveBits is a result of the target key's get key length op.

Are there any WPT tests to cover that case ? If not, would you mind adding them ?

panva commented 1 month ago

A change to reject truncated ECDH breaks deriveKey, wrapKey, and unwrapKey where the length sent to deriveBits is a result of the target key's get key length op.

Are there any WPT tests to cover that case ? If not, would you mind adding them ?

Yeah there are. And they promptly start failing once you introduce this PR to an implementation.

twiss commented 1 month ago

A change to reject truncated ECDH breaks deriveKey, wrapKey, and unwrapKey where the length sent to deriveBits is a result of the target key's get key length op.

Right, we might want to start by throwing in the crypto.subtle.deriveBits function only, rather than all other functions that depend on the derive bits operation.

Though, I'm not sure if there's a very legitimate use case for truncating ECDH output in any of those other uses, either?

(But, we'd need to double check whether we can compatibly break those, of course. But, I think they're also covered by https://chromestatus.com/metrics/feature/timeline/popularity/4746, i.e. very low usage.)

panva commented 1 month ago

Though, I'm not sure if there's a very legitimate use case for truncating ECDH output in any of those other uses, either?

Definitely not.

(But, we'd need to double check whether we can compatibly break those, of course. But, I think they're also covered by chromestatus.com/metrics/feature/timeline/popularity/4746, i.e. very low usage.)

Looking at the code that seems to be the case. @davidben could confirm.

javifernandez commented 1 month ago

A change to reject truncated ECDH breaks deriveKey, wrapKey, and unwrapKey where the length sent to deriveBits is a result of the target key's get key length op.

Are there any WPT tests to cover that case ? If not, would you mind adding them ?

Yeah there are. And they promptly start failing once you introduce this PR to an implementation.

I may have understanding the wrap/unwrap tests wrongly, but my prototype to implement the "throw on truncation" behavior in x25519 only shows failures when using the deriveBits function in the 'equalKeys' function, and the failure comes from the fact that it's hardcoding 128 as the deriveBits's length argument. The wrap/unwrapp calls in previous test cases pass as expected.

twiss commented 1 month ago

Right, actually the wrapKey and unwrapKey methods don't call out to the derive bits operation so it shouldn't cause failures there.