Throw on truncation in the ECDH derive bits operation

[twiss]

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]

This should not be merged. See #369 (comment)

[twiss]

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

[annevk]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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