Make deriveBits length parameter optional and nullable

[twiss]

Fixes #322, fixes #329. This is marked as a draft since there is not yet consensus in those issues that this is the solution we should go with, but having a concrete proposal might help move the process along.

---

This change allows omitting the length parameter from calls to deriveBits, defaulting to null, and also allows passing null explicitly (as the web platform tests already do).

The "derive bits" operations already handle null as it can also be returned by the "get key length" operations.

In the case of ECDH, the operation returns the entire derived key; in the case of HKDF and PBKDF2, the operation returns an error.

This is technically speaking a breaking change, since currently passing null explicitly should cause it to be converted to 0, causing an empty ArrayBuffer to be returned. However, the only implementation that actually does so (Chromium) is willing to change this. Additionally, returning the entire value (for ECDH) seems more expected and more useful than returning an empty value.

---

Preview | Diff

[javifernandez]

What else is pending to merge this PR ?

As far as I understand we have reached an agreement in #322 about the use of ` optional unsigned long and the addition of ```?=null``` would avoid the need to introducing changes in the algorithm's steps.

[twiss]

@javifernandez since #324 was arguably merged prematurely, I wanted to make doubly sure that we have consensus on this before it's merged :)

@annevk, @martinthomson and @saschanaz, it would be great if you could confirm whether you're OK with this solution, as I think you were at one point arguing against changing the WebIDL in #322. Though note that this PR also addresses #329, and that's not possible without changing the WebIDL - but also there we haven't discussed much whether it's worth it, and in scope for the "maintenance" charter. I do personally think this would be a nice change for developer ergonomics, but arguably it's debatable whether that's in scope. Let me know what you think!

[annevk]

I'm not against changing Web IDL, but I still don't see the point in allowing null. The argument being optional already gives you the value space you need.

[twiss]

@annevk It's not so much that I think allowing null is useful for new applications (that indeed can just omit the parameter), it's rather that given the question of what to do if an existing application does pass null, returning the entire value seems safer than returning an empty Uint8Array.

It's admittedly unlikely any existing web app does so since only Safari behaves "as expected" (per this PR, not per the current spec) in that case, but perhaps not entirely impossible since the web platform tests do test for it, so someone may have gotten the idea from there (or a loose reading of the current spec), and written a Safari-only application that depends on this, perhaps.

Also note that if such code would run today on Chrome, it would silently return an empty value, potentially leading to a security issue. So this change would fix a security issue in such a web app, whereas changing Safari to return the empty value instead of the full value could actually cause a security issue instead (strictly speaking not by fault of Safari, but nevertheless it seems better to be cautious here).

[annevk]

I'm personally not really concerned about that. I'll defer to @martinthomson and @davidben as to whether to accept this PR as-is.

[davidben]

I think have a convenient way to return an untruncated value is good. Better would have been for WebCrypto to not jam Diffie-Hellman and KDFs into the same function, but I don't have a time machine. Thus, I like making it optional to mean default size.

However, the discussion in #322 (comment) suggests that undefined, not null, is the idiomatic JavaScript pattern for an optional value. Given this is not idiomatic, and given both Chromium's and the spec's long-standing behavior, I'm also not convinced by the concern that existing websites might be relying null or zero being interpreted as default.

Thus, I think idiomatic patterns should win out and we should not have the = null business. I'm also a little surprised at this being an IDL-only change... it seems we ought to have some supporting prose, but it's been a while since I'd looked carefully at the spec formulation. That does, AIUI, preserve the existing behavior of coercing null to zero, but type coercion is just deeply part of this language and trying to work around it in every function that takes a number is just going to leave a confusing, inconsistent mess.

[saschanaz]

(Wrong argument)

It's not so much that I think allowing null is useful for new applications (that indeed can just omit the parameter), it's rather that given the question of what to do if an existing application does pass null, returning the entire value seems safer than returning an empty Uint8Array.

I'm not sure I understand the argument here, passing null for optional unsigned long length will behave like passing undefined, not 0. So I still don't see why it should be nullable.

[twiss]

@davidben:

and given both Chromium's and the spec's long-standing behavior

Are you concerned about changing Chromium's behavior because you're worried that someone might be relying on passing null returning an empty value? Or some other reason?

(IMHO we shouldn't be worried about "the spec's long-standing behavior" because the spec's intention on this point is debatable, as evidenced by the web platform tests.)

However, the discussion in #322 (comment) suggests that undefined, not null, is the idiomatic JavaScript pattern for an optional value.

Would you be happier if it said optional unsigned long? length = undefined, and then the spec prose said "if length is undefined or null"?

I'm also a little surprised at this being an IDL-only change... it seems we ought to have some supporting prose, but it's been a while since I'd looked carefully at the spec formulation.

The reason it's an IDL-only change is because the prose saying "if length is null" is already there.

That does, AIUI, preserve the existing behavior of coercing null to zero, but type coercion is just deeply part of this language and trying to work around it in every function that takes a number is just going to leave a confusing, inconsistent mess.

In general I would agree but I think we should make an exception for security concerns 😅

---

@saschanaz:

I'm not sure I understand the argument here, passing null for optional unsigned long length will behave like passing undefined, not 0.

Are you sure? https://webidl.spec.whatwg.org/#es-overloads only mentions special behavior of undefined for optional arguments, not null. See also the references in #322 (comment).

[saschanaz]

(Me quickly checking with new DOMException(null)) Ah yes, you are right, I guess I have long had a wrong understanding about optional.

[martinthomson]

I'm OK with this outcome. Like @davidben, this is a consequence of deeper structural issues, but the way this works seems fine.

I'm not 100% on the outcome for length == 0. ECDH seems to return an empty array in that case, but HKDF and PBKDF2 throw. That seems wrong, at least for ECDH.

[twiss]

Right, we could throw for length == 0 as well, that seems like a reasonable protection mechanism.

[davidben]

I'm not 100% on the outcome for length == 0. ECDH seems to return an empty array in that case, but HKDF and PBKDF2 throw. That seems wrong, at least for ECDH.

Ah, fun. I think we should treat that separately, as it's not related to how to handle the default.

I really don't like making 0 silently turn into default, but I feel less strongly about empty vs throw. TBH, I probably would have picked consistently empty over consistently throw, but shrug. I'm not opposed to trying to align that, though as it'd be a breaking change, we should get some metrics for that.

Keep in mind that, say, 1-byte or 2-byte output is also insecure, so throwing on zero is not actually meaningfully enforcing any kind of secure use of the algorithm. Indeed for ECDH, truncation at all is a bad idea. The whole output should be passed to a KDF. Given we've already gone down the truncation route, it's just a question of whether you believe "truncate to zero" is a defined operation.

If we want to throw, we should instead be seeing whether we can compatibly throw on any truncation at all.

[davidben]

(IMHO we shouldn't be worried about "the spec's long-standing behavior" because the spec's intention on this point is debatable, as evidenced by the web platform tests.)

We've discussed this thoroughly in #322 already, but the WPTs came long after the spec and original Chromium implementation. The spec was also originally written in large part by someone from Chromium. Whether or not the intended semantics were right (like I said, I'm extremely unhappy with how the spec handles ECDH overall), it's quite clear what the spec's intent was.

[martinthomson]

FWIW, 0 = empty seems obvious but it's a nonsensical thing to ask for, so I'm OK with throwing. A separate PR might be needed there.

Edit: I agree with David about truncation for ECDH, at any number of bytes. But I have found many uses for very short KDF outputs.

[twiss]

Ah, fun. I think we should treat that separately, as it's not related to how to handle the default.

Yeah, I'll make a separate PR for that.

Keep in mind that, say, 1-byte or 2-byte output is also insecure

Sure, I meant more that it could be a protection mechanism against accidentally getting an empty value instead of the full value. For example, Safari currently returns the full value when returning 0, it might be better to change that to throw rather than returning the empty value (which could theoretically cause a security issue).

If we want to throw, we should instead be seeing whether we can compatibly throw on any truncation at all.

Can we not let perfect be the enemy of good? 😅 We already know we can compatibly throw for 0, as Firefox does so, so it would seem reasonable to me to start with that.

[davidben]

If we want to throw, we should instead be seeing whether we can compatibly throw on any truncation at all.

Can we not let perfect be the enemy of good? 😅 We already know we can compatibly throw for 0, as Firefox does so, so it would seem reasonable to me to start with that.

The tradeoff is between trying to enforce some security criteria (which rejecting zero does a bad job of) vs having an extra case of fallibility in the function. The point of my comment isn't striving towards perfection, but that we're sacrificing continuity of semantics for a security check that doesn't actually do much. The continuity doesn't mean much, but measured against a security check that doesn't do much, this is all silly.

Since starting to throw would be a spec-incompatible change, and a breaking change for Chromium, we'd need some code to measure things. If we're measuring things anyway, we should also validate whether we can just remove this truncation behavior altogether, as it was wrong to begin with.

[twiss]

(I've created #351 for this)

The tradeoff is between trying to enforce some security criteria (which rejecting zero does a bad job of)

Again, the security check wouldn't be to protect web applications in general, just any code that happens to rely on WebKit returning the entire value for zero. Rejecting zero does do a good job of protecting against that 🙃

Since starting to throw would be a spec-incompatible change, and a breaking change for Chromium, we'd need some code to measure things. If we're measuring things anyway, we should also validate whether we can just remove this truncation behavior altogether, as it was wrong to begin with.

But yes, fair enough. If you do want to do some measurements that would be great, let us know what the outcome is 😊

[davidben]

Ah fair, yeah zero is a bit special because of this mess. :-)

[twiss]

@martinthomson and @saschanaz could you please explicitly approve this PR, if you do indeed approve of it (with or without #351)?

And @davidben I'll repeat this part as it was a genuine question:

Would you be happier if it said optional unsigned long? length = undefined, and then the spec prose said "if length is undefined or null"?

And perhaps if everyone else wants to weigh in on that alternative option that'd be great 😊

[martinthomson]

I really don't think that = undefined changes anything in this case.

[davidben]

Certainly there's no difference between = null and = undefined with prose for null. The question in my mind is whether passing in an explicit null should be interpreted as using a default.

My preference would be what was already discussed in the issue:
#322 (comment)
#322 (comment)
#322 (comment)

That is, we should just do whatever would be idiomatic on the web for an optional parameter. That seems to be plain optional unsigned long length, which I understand from the discussion to not permit null as a stand-in for an omitted value.

But if we're really set on ignoring convention for some reason, I can live with null being a stand-in. (WebCrypto is very much not a hill worth dying on. 😛) But then there should be a clear reason for it, and it doesn't seem like there is one, beyond the WPTs which were always just a mistake in the tests.

[javifernandez]

Just to clarify, this PR doesn't change the behavior of passing 0, it only changes the behavior of passing null, which currently gets converted to 0 and then returns the empty value (although only Chrome implements this per spec), but after this PR will return the entire value (as e.g. Safari already does).

The metrics are still valuable in that they should demonstrate (AFAIU) that nobody is passing 0-converted-from-null, and so we shouldn't have to worry about changing the behavior of passing null (and tbh I also think it would be a bit strange if someone did that to get an empty Uint8Array :)).

Yeah, that was the point I tried to make. After merging this PR we can probably change WebKit's implementation to avoid '0' returning the entire key and rely on the 'null' value for that behavior. That would make the deriveBits operations become interoperable in the three major browsers.

[twiss]

Oh I see, yeah makes sense. So you want to close #351, then?

[davidben]

Oops, this got buried in my inbox. Yeah, this change SGTM.

[javifernandez]

Oh I see, yeah makes sense. So you want to close #351, then?

I think we can do that later, once we manage to solve the lack of interoperability of the deriveOperations in several algorithms, I think returning an empty string when length is 0 is consistent with the spec and what it's already implemented in some engines.

If we want to change the spec of a specific algorithm to raise an exception and forbid that case, we may discuss it independently of this PR.

[javifernandez]

Hi @annevk, would you mind to explicitly approve the change on behalf of Safari now that Chrome and Firefox have ?

[annevk]

Sure.

[twiss]

Thanks, everyone!