Throw for all truncation in ECDH?

[twiss]

@davidben has expressed that throwing only on 0 doesn't go far enough, as no truncation ever really makes sense for ECDH. So, we could throw for any truncation?

However, it seems there's some (very) small amount of usage of this (as opposed to 0), so we might want to be a bit careful here, and e.g. warn about it first?

@Frosne / @martinthomson and @annevk, do you have an opinion here?

[davidben]

More accurately:

1. The specification defined ECDH to be ECDH + truncate. This was a bad idea, but so long as that is the definition of the operation, truncate(0) is well-defined.
2. Due to misreading the spec and mixing up null and zero, some implementations were not spec-compliant and did weird things at zero. Those were simply bugs in those implementations and not a reason for the spec to special-case zero. Zero means zero.
3. Truncating ECDH secrets is a bad idea. Obviously truncating to zero is a degenerate case, but truncating to one byte or really any other length is also a bad idea. There is no security argument for special-casing truncation to zero. That means Throw on truncation in the ECDH derive bits operation #351 should not be merged.
4. If one wishes to make a security argument, it should be to forbid all truncation. This is because the entire truncation behavior stems from the specification mixing up ECDH and HKDF, when they have nothing to do with each other. However, as this is an existing API with existing shipped behavior, such a change should only be done if compatible.
5. If (4) is not compatible, we should leave it alone and keep truncate(0) meaning truncate(0). Again, Throw on truncation in the ECDH derive bits operation #351 should not be merged.
6. Whether or not we can make the incremental API improvement in (4), WebCrypto jamming unrelated operations into the same verb was clearly a problem. This is not a question of "high-level" vs "low-level" API or bad defaults or anything. This was simply an API that was never idiomatic for the web or cryptography. Any high-effort or incompatible WebCrypto work would be better spent on correcting that mistake and making a more sensible API.

[twiss]

@davidben Thanks for clarifying your position.

Re. point 4, do you think we can compatibly throw for all truncation despite the small amount of usage indicated here? And/or, that we could deprecate & warn about truncation with the goal of eventually throwing?

---

Even if the answer ends up being no, there is a (small) security argument to be made for throwing for 0, specifically for the implementation that returned the entire value until now. Even though that was because of a bug, that's not really a reason to punish the users of that implementation with a potential vulnerability. Though, it's admittedly unlikely that web apps depended on this since every implementation behaved differently; it's theoretically possible for internal applications or some such to depend on this.

[javifernandez]

Re. point 4, do you think we can compatibly throw for all truncation despite the small amount of usage indicated here? And/or, that we could deprecate & warn about truncation with the goal of eventually throwing?

@davidben should be the one approving this for Chrome, but in my opinion, with the proper warning about deprecation and given its low usage, changing the spec to throw at any value smaller than the key's default value looks sensible to me.

I'd like to know @annevk position on this issue, since Webkit is the only one returning the full length when passing 0, so it might be theoretically affected by a potential vulnerability. However, I'd say that the only cases where WebKit treated 0 as requesting the key's full length would be when the argument is omitted. Now that we have implemented it as an optional argument, this case is already safe.

I think a deprecation warning for any attempt to truncate the derived key material would also address the WebKit s potential vulnerability on the cases where the app is explicitly passing 0 expecting the API to return the full-length bits.

[annevk]

I don't really understand how this is a WebKit vulnerability. It seems more like a vulnerability in any websites that are already holding the API incorrectly. I'm not too concerned about those.

It seems reasonable to attempt to throw for truncation, but someone 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).

cc @nmahendru

[javifernandez]

I don't really understand how this is a WebKit vulnerability. It seems more like a vulnerability in any websites that are already holding the API incorrectly. I'm not too concerned about those.

True, I expressed it very bad; I mean a vulnerability in websites passing 0 explicitly, which is indeed an incorrect way of using the API.

It seems reasonable to attempt to throw for truncation, but someone 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).

I'm planing to work on the implementation of this change, but any help from browser vendors would be really appreciated, as always.

cc @nmahendru

[panva]

I am likewise ready to update as many server(less) js runtimes as possible

[davidben]

I don't really understand how this is a WebKit vulnerability. It seems more like a vulnerability in any websites that are already holding the API incorrectly. I'm not too concerned about those.

I also don't think this is a WebKit vulnerability. If you don't either that's great. It sounds like we should go ahead and close #351, decide zero means zero, truncate(0) means truncate(0), and move on from that topic. And then we can separately decide whether anyone has the energy to push through this low-but-non-zero-compat-risk question of whether to allow truncation at all, zero or otherwise.

[twiss]

@davidben This issue is already about whether we should allow truncation at all, zero or otherwise. Your point that we shouldn't treat 0 specially has been taken :) But we can also modify #351 if we reach a consensus here - and if not I'll close it of course.

You haven't answered my main question which is, do you think we can compatibly throw on truncation? (Whether you have the energy to implement it is a separate question, of course.)

[davidben]

I mean, I don't have any more data than what you linked to. The shape of the graph in https://chromestatus.com/metrics/feature/timeline/popularity/4746 is concerning, though there's only one site listed there. (Slightly curious what they're doing...) I know there was a bug in the metrics initially, but I think that would have cycled into the metrics by then, so the increase may just be some site starting to use it.

But also 0.000005% is very, very, very small, so that seems a doable deprecation? Like @annevk said, it would require someone having the time to seeing it through. (I'm personally much more concerned about closing #351 so we stop hinting to folks that the spec is interested in treating zero special.)

[twiss]

The shape of the graph in https://chromestatus.com/metrics/feature/timeline/popularity/4746 is concerning

Yeah. Could me experimenting in the browser console contribute to those metrics? 😅

But also 0.000005% is very, very, very small, so that seems a doable deprecation? Like @annevk said, it would require someone having the time to seeing it through. (I'm personally much more concerned about closing #351 so we stop hinting to folks that the spec is interested in treating zero special.)

OK, I've changed #351 now to stop hinting at that, and to reflect the aspiration of throwing for all truncation :)

[Frosne]

I don't really understand how this is a WebKit vulnerability. It seems more like a vulnerability in any websites that are already holding the API incorrectly. I'm not too concerned about those.

I also don't think this is a WebKit vulnerability. If you don't either that's great. It sounds like we should go ahead and close #351, decide zero means zero, truncate(0) means truncate(0), and move on from that topic. And then we can separately decide whether anyone has the energy to push through this low-but-non-zero-compat-risk question of whether to allow truncation at all, zero or otherwise.

I agree here.

[javifernandez]

I don't really understand how this is a WebKit vulnerability. It seems more like a vulnerability in any websites that are already holding the API incorrectly. I'm not too concerned about those.

I also don't think this is a WebKit vulnerability. If you don't either that's great. It sounds like we should go ahead and close #351, decide zero means zero, truncate(0) means truncate(0), and move on from that topic. And then we can separately decide whether anyone has the energy to push through this low-but-non-zero-compat-risk question of whether to allow truncation at all, zero or otherwise.

I agree here.

Does this mean that FF prefers to update its implementation to return an empty string when length = 0 now ?

I have a PR for WebKit to follow this approach, so that the test cases defined would be green for the 3 browsers.

@annevk do you also agree on postponing this issue and fix WebKit implementation now ?

I'm asking this because the deriveBits interop issues was one of the blockers to merge X25519 into the WebCryptoAPi spec, so I wonder whether this decision would unblock it.