deriveBits length idl does not allow null

[panva]

webcrypto/spec/Overview.html

Lines 1258 to 1260 in 004f26a

	Promise<ArrayBuffer> deriveBits(AlgorithmIdentifier algorithm,
	CryptoKey baseKey,
	unsigned long length);

The SubtleCrypto.deriveBits function length argument's IDL does not allow null to be passed despite there being a special handling for its possible null value in all deriveBits operations, for most it throws but for ECDH, X25519, and X448 it is defined as returning all generated bits.

I believe the length argument of deriveBits should be made nullable like so?

Promise<ArrayBuffer> deriveBits(AlgorithmIdentifier algorithm, 
                         CryptoKey baseKey, 
                         unsigned long? length); 
                                      ^ nullable?

In WPT there are even tests for this behaviour.

https://github.com/web-platform-tests/wpt/blob/3174b85610804424ac4c6c2a194a08220402a00e/WebCryptoAPI/derive_bits_keys/ecdh_bits.js#L58-L66

[twiss]

Hey 👋 It seems you're right. If you feel like making a PR when you're back, it'd be welcome, otherwise I can also do it :)

[panva]

Feel free to do so.

[saschanaz]

But no one is using nullable type for deriveBits, wouldn't changing the IDL also change the behavior? Is this tested by WPT?

• Gecko: https://searchfox.org/mozilla-central/rev/7dffbe62b2caea25ed3b221626d0b82238ce2e9e/dom/webidl/SubtleCrypto.webidl#220
• WebKit: https://searchfox.org/wubkat/rev/f8166f50d036560b0a2553800d1de26b6302b83d/Source/WebCore/crypto/SubtleCrypto.idl#43
• Blink: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/crypto/subtle_crypto.idl;l=49;drc=c3b7c5877ef5262668ee9b5fce7f862326eee7c1

[saschanaz]

In WPT there are even tests for this behaviour.

https://github.com/web-platform-tests/wpt/blob/3174b85610804424ac4c6c2a194a08220402a00e/WebCryptoAPI/derive_bits_keys/ecdh_bits.js#L58-L66

Oops, missed this part.

https://wpt.fyi/results/WebCryptoAPI/derive_bits_keys/ecdh_bits.https.any.html?label=experimental&label=master&aligned says Chrome and Firefox fail the null length tests while Safari passes (how?). Interesting that Safari's implementation is also definitely using non-null length.

[saschanaz]

Eh, so the IDL passes null as 0 to C++ in every browser, and then Safari uses 0 to mimic If length is null: part.

Interesting, should this be specced? 🤔

[panva]

But no one is using nullable type for deriveBits, wouldn't changing the IDL also change the behavior?

Node, CF Workers, Deno, and now Bun as well, correctly implement the null behaviour for ECDH deriveBits.

We're beyond a point where we could remove the behaviour.

I also suspect chrome and firefox could have the correct ECDH deriveBits operation implementation but the webidl conversion coerces null to 0 and we just don't know ;)

[martinthomson]

@saschanaz is correct in that there is no "null" behaviour. The WebIDL definition does not allow for the value to be null. Any value is coerced into a number, with an absent value being turned into a 0.

I think that omitting the argument is a useful bit of ergonomics (not everyone knows the native output size of this function), but I don't see any reason to change the IDL to allow for a null value to achieve that. Using 0 as a sentinel value works perfectly fine in this case, as asking for 0 derived bits is pointless.

I would suggest that we just change the algorithm for ECDH so that it conditions the return of the whole secret on length == 0 rather than length is null. That change is here:

webcrypto/spec/Overview.html

Lines 9662 to 9663 in 004f26a

	<dt>If |length| is null:</dt>
	<dd>Return |secret|</dd>

s/null/0/

This would effectively formalize the Safari behaviour as noted above.

[panva]

@martinthomson There is an interaction to be considered with the get key length op used in deriveKey which also returns null in HKDF and PBKDF2 cases. Such a simple sed is not the way.

I believe changing the IDL is the correct course of action. We have WPTs for this behaviour, indicating the original intention and there are more implementations conforming to it than the opposite.

[martinthomson]

For HKDF, "If length is null or zero" becomes simply "If length is zero". Same for PBKDF2. I don't see any major problem there.

A nullable value makes the IDL and the algorithm more complex. This is simpler.

[panva]

@martinthomson That's not dealing with the fact that get key length returns null for HKDF and PBKDF2. Meaning this is no longer a simple one-liner like change you're suggesting as opposed to fixing the webidl to reflect the original intention as well as a larger number of existing implementations with an actual one-liner.

[martinthomson]

Sorry, I thought that was clear: if you pass the output from getting the length as an argument to deriveBits, then it will coerce to zero happily, no problem.

[twiss]

I think checking for zero instead of null is technically speaking not backwards-compatible. It's a bit of an edge-case (and not really a useful operation to do), but as currently specified, passing 0 should cause an empty ArrayBuffer to be returned, and that's in fact what some implementations do. So I think we should change the IDL, to what seems to have been the original intention.

[martinthomson]

To be clear, what I'm proposing requires a change in Firefox, but I think that it is the better change. A zero-length array can be obtained more simply by other means. And implementations are already non-compatible.

Edit: Note that there is no option here that is strictly backwards-compatible.

[panva]

@martinthomson what you're proposing makes every currently conforming implementation non-conform since they already return 0-length array as specified currently when 0 is passed.

[twiss]

To be clear, what I'm proposing requires a change in Firefox, but I think that it is the better change. A zero-length array can be obtained more simply by other means. And implementations are already non-compatible.

Yeah, that's true, but I don't think checking for 0 is better than checking for null, especially if the outcome that we want to support is that developers can pass null - checking for that explicitly seems like the clearer and more obvious way to go.

Edit: Note that there is no option here that is strictly backwards-compatible.

You're right that changing the behavior for null is strictly speaking not backwards-compatible either. However, if we change the behavior for 0, we're changing the behavior for both null and 0, so it's strictly less backwards-compatible 😅

Finally, if developer ergonomics are the goal, I think we could (at some point) consider making the parameter optional entirely (i.e. allow passing undefined or two parameters instead of three), and I think making the parameter nullable is a step in that direction in a way that checking for zero isn't.

[martinthomson]

@martinthomson what you're proposing makes every currently conforming implementation non-conform since they already return 0-length array as specified currently when 0 is passed.

There is no conformance because the spec is incoherent.

[...] I don't think checking for 0 is better than checking for null, especially if the outcome that we want to support is that developers can pass null - checking for that explicitly seems like the clearer and more obvious way to go.

I think that this is the central disagreement. I assert that the null -> 0 coercion is fine and that anyone passing null should be getting an ECDH secret in full rather than a zero-length array. That is, the two are treated as equivalent. @twiss considers this distinction between null and 0 to be significant, such that 0 produces an error instead, where null produces the full ECDH secret.

(Note that HKDF and PBKDF2 will be the same in both cases.)

I probably haven't pointed this out, but changing the IDL is more disruptive.

[twiss]

There is no conformance because the spec is incoherent.

The spec isn't really incoherent on what to do when 0 is passed, only on what to do when null is passed. That's also part of why I think changing the former to address the latter isn't necessarily the best way to go.

I think that this is the central disagreement. I assert that the null -> 0 coercion is fine and that anyone passing null should be getting an ECDH secret in full rather than a zero-length array. That is, the two are treated as equivalent. @twiss considers this distinction between null and 0 to be significant, such that 0 produces an error instead, where null produces the full ECDH secret.

To be pedantic, 0 doesn't produce an error, as currently specced - perhaps it should, but I think that's orthogonal - but you're right that I see null and 0 as distinct.

I probably haven't pointed this out, but changing the IDL is more disruptive.

What disruption would changing the IDL cause?

[martinthomson]

Implementations that build on WebIDL often require a lot more change when IDL changes as it affects the type of arguments, codegen, automated tests generated from IDL, and other things.

[panva]

There is no conformance because the spec is incoherent.

There are existing implementations that conform to the language specified in the ECDH derive bits operation as well as the existing WPTs and there's likely userland code relying on that as well.

changing the IDL is more disruptive

Changing the IDL isn't going to make any existing implementation less conform. OTOH changing the behaviour would only further fragment the implementations to a point where this behaviour would not be interoperable for a long while before all implementations and installed versions align.

Changing the IDL fixes the spec incoherency problem and allows the two non-conform implementations to proceed to fix a problem they had for a long time (chrome, firefox), changing the behaviour would require every implementation to align to new behaviours.

[twiss]

@martinthomson Alright, understood. I personally think it's more important to minimize disruption to developers, than to implementations, though. Even though admittedly the chance that anyone is actually relying on the behavior for passing 0 is very low, it seems better to me to fix this in the way that's minimally breaking backwards-compatibility, especially if it seems that's the behavior that was intended originally.

[martinthomson]

Given that Safari implements the logic I'm proposing, from a web standpoint, that would point toward that solution rather than making the argument nullable.

[twiss]

You're right that I can't really cite backwards compatibility concerns if Safari already implements what you're proposing ^.^ However, I think what Safari did is somewhere between "accidentally working", and a hack to work around a spec bug. I still think it's more straightforward to just fix the IDL in the spec to what was intended, rather than adopt that workaround.

Changing the IDL won't make Safari any more nonconformant than it is today, either - it will make it conformant for null, but not for 0 (which it already isn't).

Note also that Chrome does implement the behavior for 0 exactly as written in the spec, and some other implementations do as well.

[twiss]

@annevk Do you (or others from WebKit) have an opinion on this? Would you be OK with changing the IDL to address this issue?

[annevk]

I can ask. I'd personally defer to @martinthomson on this.

The PR here "restores" nullability, but doesn't deal with making the argument optional as suggested upthread. Was that feedback missed?

[saschanaz]

but doesn't deal with making the argument optional as suggested upthread. Was that feedback missed?

Can we make it optional when other algorithms require it? 🤔

https://w3c.github.io/webcrypto/#hkdf-operations

If length is null or zero, or is not a multiple of 8, then throw an OperationError.

[twiss]

I think we could make it optional in the WebIDL, and then (still) throw in the operation if it's required for that specific algorithm, however, I think we should consider that separately from this issue, which is about an inconsistency in the spec. (However, I do think that making it nullable here is in a way a step towards making it optional, as I said upthread 😊)

[davidben]

I don't think one can say the "magic zero" behavior is what was "originally intended". The WPTs came from web-platform-tests/wpt#3305, long after WebCrypto was originally shipped the browsers. Sadly, WebCrypto is from a slightly earlier era of web platform development, and hasn't seen significant effort in browsers since.

I suspect the spec indeed meant for null and zero to be different values, some folks misread the spec (understandably—it's confusing that "null" exists in an internal call to the operation), that misreading made its way into WPTs, which begat further misreadings, and then no one noticed this giant mess until now.

As for the caller-level API, I definitely agree optionality in some form makes sense here. All this mess is really a symptom of WebCrypto inexplicably believing that HKDF and ECDH are the same operation. HKDF needs a length. ECDH does not want a length, but WebCrypto then decided it means truncation, despite this being incoherent and unsafe. And so, for the sake of ECDH, there should be a convenient way to say the correct length.

optional unsigned long length? = null; would be very straightforward for Chromium and, yeah, would not be a breaking change for us or the spec. I also think, were there no existing implementations, we wouldn't want zero to secretly mean "use the original length" as it adds a weird hole in the abstraction. We wouldn't want str.substr(0, 0) to silently mean "return the whole string", even though we want str.substr(0) to mean that, thus String.substr maps the missing parameter to undefined, not zero.

Discontinuities like this mean callers that pass in a variable input need to check for zero or risk weird behaviors. Even if it's a degenerate case, boundary inputs should behave continuously with the rest of the function. Thus, I think a separate null is the safest and best option for this API.

I recognize, however, that due to the WPT mess, some implementations ended up non-compliant here, and so the safe option is an interop risk for Firefox and Safari, just as the unsafe one is a risk for Chrome. Have we done measurements on the web for how often things are called with zero? That will tell us what the risk is, and whether to make the decision based on this risk or based on what would be a good API design.

[twiss]

@javifernandez:

The issue about 0 not throwing is that it leads to "zero length is silently reinterpreted as the untruncated length" which I think it's confusing. Isn't it ?

I agree that would be confusing but I don't think it should lead to that, if we only change the WebIDL (and even today, according to a strict reading of the spec); passing 0 should return an empty array buffer, which is useless but not confusing, IMHO.

---

@davidben:

I don't think one can say the "magic zero" behavior is what was "originally intended". The WPTs came from web-platform-tests/wpt#3305, long after WebCrypto was originally shipped the browsers. Sadly, WebCrypto is from a slightly earlier era of web platform development, and hasn't seen significant effort in browsers since.

I suspect the spec indeed meant for null and zero to be different values, some folks misread the spec (understandably—it's confusing that "null" exists in an internal call to the operation), that misreading made its way into WPTs, which begat further misreadings, and then no one noticed this giant mess until now.

I agree with your reading of the intention of the spec, and I think the WPTs do actually match that as well - they only test null (to return the entire value), there is no test for the behavior of 0.

So I don't think you and @panva are in disagreement either, since he was talking about making the behavior for null work, not changing the behavior for 0 ☺️ And I think everyone agrees on what passing null should do, just not on what's the best way to achieve it, and (indirectly) on what passing 0 should do.

The behavior for 0 is currently inconsistent between implementations (Safari returns the entire value, Chrome and Node.js return an empty array buffer, Firefox throws an error), so I think there should be no significant web compat risks to changing the behavior (but @annevk, let me know if you disagree on this point) - and IMHO it would make the most sense to standardize on the behavior already required by the spec, i.e. return an empty array buffer.

[davidben]

Ah, yeah. I agree null meaning the default length is sensible.

I suppose even that would be a breaking change for Chrome (and the spec) because null binds to unsigned long length in WebIDL by coercing to zero. (Refs: 1, 2, 3). And so the spec currently believes null => zero.

But I think that breaking change gets us to a better place. Hopefully it's unlikely that people had been passing in null and expecting the zero behavior. (I suppose that too is something we can UseCounter. E.g. we could do the IDL change, just so the C/C++ code can see the difference, temporarily just map null to zero, and measure how often zero and null are passed in.)

What a mess! All this because we decided HKDF and ECDH were the same thing. 😄

[annevk]

I have to say I'm still not sold on having special semantics for null. Even if we wanted distinct semantics between 0 and omitted (i.e., undefined), there's no reason to make omitted be null. The modern approach is to just make it optional and check for it being omitted instead of coercing undefined to null.

But I also think @martinthomson is right that you never went 0 to begin with which makes me question the need for an additional value (i.e., instead of undefined or number just have number exactly as Safari does today, but do make the argument optional and default to 0 for ease-of-use).

[twiss]

I have to say I'm still not sold on having special semantics for null. Even if we wanted distinct semantics between 0 and omitted (i.e., undefined), there's no reason to make omitted be null. The modern approach is to just make it optional and check for it being omitted instead of coercing undefined to null.

I agree if we were to create the API from scratch, that would be the best approach. But since there are WPTs for passing null, and a bunch of implementations that implement it according to the intention of the spec (and thus pass the tests), it seems better to make passing null be equivalent to omitting it (and thus the other way around as well)?

[davidben]

Ah good point about null vs undefined. I suppose then the right solution in a vacuum would have been that the default is spelled undefined and not zero or null.

Why would defaulting to 0 give ease-of-use? If you don't want to specify a length, you omit the argument. If you are in a weird situation[*] where you sometimes want to and sometimes don't want to specify a length, you can and should spell "don't specify a length" as undefined, not 0.

It seems to me the only real argument for applying this behavior on null or 0 is existing callers and implementations. Some existing callers (outside the web, presumably, but I gather in non-web WebCrypto implementations) seem to want null to mean undefined. Some existing implementations make both null and 0 mean undefined, while the spec and others do not. And then we add API design considerations: null meaning default is, while unidiomatic, not completely wild. 0 meaning default is a discontinuity.

It sounds like, above all, we need some measurements for what's on the web today. Given browsers disagree, I highly doubt the behavior on zero (or null because of coercion) matters for web interop. The behavior on null seems to matter for non-web interop. Anywhere it doesn't matter, we should pick based on what would be good API design.

[*] You will never be in this situation. Specifying a length with ECDH is always wrong, and not specifying a length with HKDF doesn't work. But it never makes sense to have a function that doesn't know whether it's calling ECDH and HKDF because these operations have nothing to do with each other. They're only put together out of WebCrypto design mistakes.

BTW, a minor nitpick from earlier:

I agree with your reading of the intention of the spec, and I think the WPTs do actually match that as well - they only test null (to return the entire value), there is no test for the behavior of 0.

The spec doesn't match the WPTs in that the spec doesn't believe you'll allowed to omit the length. That seems to have been the original intention, for better or worse. But I think we all agree that was wrong for an ECDH API.

[twiss]

The spec doesn't match the WPTs in that the spec doesn't believe you'll allowed to omit the length. That seems to have been the original intention, for better or worse. But I think we all agree that was wrong for an ECDH API.

But the WPTs don't omit the length, they pass null explicitly. You're right though that a strict reading of the spec would suggest that the implementation should return an empty array buffer in that case, not the entire value, but the WPTs seem to match the intention of the spec (to return the entire value).

[annevk]

The intention of the spec is unclear and I wish we'd stop using that line of reasoning. It's clearly in conflict with itself.

Also, we often test passing JS values that either result in coercion or throwing due to IDL. And we should as sometimes bindings have bugs. You can't derive intent from that either, and in this case the tests were written much later and presumably different people so that is not a correct line of reasoning either.

Why would defaulting to 0 give ease-of-use?

You can't argue with only part of that sentence.

Anyway, I agree that interop is unlikely to be a problem here and it seems like a lot of people want 0 to be mean 0-length so that would argue for

optional unsigned long length;

in Web IDL and replacing the null check in prose with a "length is not given" check.

[twiss]

Also, we often test passing JS values that either result in coercion or throwing due to IDL.

Sure, but the test expects the function to return the entire value; if it was testing the IDL, it should've expected an empty array buffer.

Anyway, even ignoring intent, what's the advantage of null being converted to 0? I agree that your proposed API is a more elegant solution to #329, but if we consider in isolation what to do for passing null explicitly, there are a bunch of implementations that return the entire value, and only one implementation which returns the empty array buffer (and is willing to change that).

On the off chance that there is some (Safari-only) web app or server-side code that passes null and expects the entire value, changing that to mean 0 would actually introduce a security issue (if the empty key is then used for something sensitive), whereas changing null to mean the entire value should never cause a security issue, and should in fact be what the developer wanted (since the empty value is useless and you always want the entire value for ECDH, as both @martinthomson and @davidben pointed out).

[panva]

Wouldn't optional without a default in deriveBits method WebIDL throw off the ergonomics for the HKDF and PBKDF2 algorithms? Any optionality like so would also mean changes to the Derive Bits operation steps for all three (or five when considering https://wicg.github.io/webcrypto-secure-curves/) algorithms that implement this operation.

OTOH unsigned long? length requires no other change in the specification since all Derive Bits operations already have a handling for null which, contrary to what has been said in this thread, is valid even given the existing non-nullable WebIDL - null is returned by the get key length operation called from the deriveKey method which in turn calls the Derive Bits operation (not method) with the null returned from get key length.

It's only the deriveBits method WebIDL that stands in the way here, not the operations' steps, those are already in the right state.

[javifernandez]

It'd be useful if we all could settle on some fundamental agreements, so that we make some progress towards a final solution to this issue. If I've understood it correctly, I believe we can agree on the following points:

• the optional value is the best approach for facing the different inconsistencies we currently have
• we don't want to default to 0
• a value of 0 should return an empty array

Then we have other considerations and possible approaches that have both, pros and cons, but I'd be useful for me to at least conform we all agree on the 3 points listed above.

[twiss]

I've created a draft PR making it

optional unsigned long? length = null

which I think is the most straightforward way to address both this (taking into account the points above) and #329. But let me know if anyone objects to this ☺️

[javifernandez]

I've created a draft PR making it

optional unsigned long? length = null

which I think is the most straightforward way to address both this (taking into account the points above) and #329. But let me know if anyone objects to this relaxed

I agree that this seems the easier approach to address this issue. It wouldn't break Chromium actually, since once it can receive null, it will return the full-length secret as it's specified (or the Exception if it's the case).

However, and I'd agree to address this in a separate issue if we want to follow an incremental approach, there are some questions that remain unclear.

1. Safari and Firefox are handling the 0 case that is not spec complaint.
2. Extend the value space with 'undefined'

I think we've got an agreement regarding 1) which would require changes in WebKit and Firefox implementations (not in the spec, though) and additional test (no changes required in WPT).

The point 2) is perhaps more complex and would require more discussion. I'd be sad, though, about delaying what it'd be a step forward cleared spec and tests and better interop between browsers, but as @davidben commented before, we should base our decision on what we consider a good API, instead of focusing on specific and isolated cases.

So, summarizing, I'm fine with going ahead with the change described in PR#345, together with the improvements of the 0 length case, but I'd defer to the ones with longer experience in specs the decision of whether wait for a more complete proposal that covers the rest of the cases.

[twiss]

1. Safari and Firefox are handling the 0 case that is not spec complaint.

Right. In the case of Firefox, it currently returns an error for 0, so handling that case properly shouldn't lead to backwards compatibility issues there, I think. Safari currently returns the entire value, and it would be nice to know if they'd be willing to change that. However, I do think it's a bit orthogonal to both this issue and #329; and even the change in #345 doesn't really affect it. And personally I think having consistent behavior for null (and undefined, and omitting the parameter) is more important than that for 0, though it would be nice to have consistent behavior for all values.

2. Extend the value space with 'undefined'

Yeah, that's a good point. If I understand the overload resolution algorithm correctly, passing undefined to an optional argument should cause the default value (null, if we go with #345) to be passed to the algorithm. To me that seems reasonable, but let me know if you disagree or if you think my interpretation is wrong :)

[javifernandez]

2. Extend the value space with 'undefined'

Yeah, that's a good point. If I understand the overload resolution algorithm correctly, passing undefined to an optional argument should cause the default value (null, if we go with #345) to be passed to the algorithm. To me that seems reasonable, but let me know if you disagree or if you think my interpretation is wrong :)

I don't have objections on defaulting in case of passing 'undefined', so if we have an agreement, lets go for it.

[annevk]

null is returned by the get key length operation

It looks like "get key length" is an internal operation, is that correct?

Also from at least https://w3c.github.io/webcrypto/#hmac-registration it's not clear that it can return null. And when looking at its definition at the end of https://w3c.github.io/webcrypto/#aes-ctr-operations it references a length dictionary member that also cannot be null. So I'm not sure I follow this line of reasoning.

[davidben]

It returns null for HKDF:
https://w3c.github.io/webcrypto/#hkdf-registration

This is all a bit silly and ultimately comes from WebCrypto incorrectly jamming unrelated operations into a single function, but we're a bit stuck with it now.

[panva]

And PBKDF2 has the same return null definition.

[annevk]

Okay, but it still seems like that's an internal API and not a public API and as such shouldn't really influence our decision here.

[davidben]

That's right. In the spec formulation (and in Chromium), deriveBits does not take null at all and has no way to say "I want the default". Null is currently purely a spec-internal construction.

[twiss]

(I'll repeat most of my comment from #345 (comment):)

I agree the internal API shouldn't influence the external API, but regardless, when considering what to do if an application passes 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 #345, 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).