Tests for the deriveBits operation with extreme values in the 'length' parameter

[javifernandez]

There is some inconsistency in the spec regarding the 'length' parameter in the deriveBits operations of different algorithms, such as ECDH, HKDF, PBKDF2 or X25519. This PR adds a few tests to evaluate the behavior of these algorithms with extreme values, like 0, null or undefined.

There is an ongoing discussion in the issues 322 and 399 about whether the parameter may be optional or accept null values, since it's currently defined as unsigned long. This PR considers the null value as tentative tests while these issues are not solved.

[javifernandez]

@twiss @martinthomson if there isn't anything else to change on the new tests, could you please review the PR so that I can merge it ? Thanks.

[twiss]

Hey 👋 Apologies for the delayed response.

I think the new (non-tentative) tests are correct in that they match the spec, however, for ECDH and X25519, I'm not 100% sure whether we should start testing that passing 0 leads to an empty array at this time, before a decision has been reached on how to handle the parameter.

One of the proposals I made in w3c/webcrypto#345 (comment) was to ignore the length parameter entirely. If we'd go with that option, even the non-tentative tests would have to change again.

Obviously, we can always do so, and in a sense that reflects the fact that it would be a backwards-incompatible change (and hopefully the counters from Chromium will inform us whether that's possible). But, perhaps it's easier to wait for that? (I'll also try to ping the people in that thread early next week, to try to get to a decision / conclusion.)

Also because, Safari currently returns the entire value when passing 0, and on the (admittedly very unlikely) off-chance that someone relies on this, returning an empty array could lead to a security issue. So if we do end up going with that option, perhaps we don't want to encourage Safari to change the behavior to returning an empty array now (and then back later).

[javifernandez]

Hey 👋 Apologies for the delayed response.

Don't worry, I'm the one that should apologize for being so insistent when asking for the review. I was waiting for this tests to land this Chrome CL to make the X25519 implementation matching the current (and being discussed) spec, so that zero length returns an empty string, updating the WPT accordingly, since
we have a few cases with 'null' that seem wrong.

I think the new (non-tentative) tests are correct in that they match the spec, however, for ECDH and X25519, I'm not 100% sure whether we should start testing that passing 0 leads to an empty array at this time, before a decision has been reached on how to handle the parameter.

We are now in a weird situation, where there are tests that use 'null' when it's not allowed in the current spec, with the fill array as expected result; the only way the implementations can do that is to make '0' equivalent to 'null' (this is Chrome's current behavior) which goes against the spec.

The CL I mentioned before tries to make the X25519 implementation to mach the current spec, returning an empty array for a '0' length; it causes the mentioned test to fail, though. With this PR I tried to fix also the mess we have in the tests, but I understand your concerns about it; I'm just afraid the agreement on the spec issue takes too long.

Unless anybody has good arguments against, I think landing the mentioned CL to that Chrome's X25519 matches the current spec (0 returns an empty string) is the best option now, although I really wanted to land it with the proper testing coverage for the 'zero' length case; if there is no agreement, I may have remove the new test cases there for that case.

One of the proposals I made in w3c/webcrypto#345 (comment) was to ignore the length parameter entirely. If we'd go with that option, even the non-tentative tests would have to change again.

Obviously, we can always do so, and in a sense that reflects the fact that it would be a backwards-incompatible change (and hopefully the counters from Chromium will inform us whether that's possible). But, perhaps it's easier to wait for that? (I'll also try to ping the people in that thread early next week, to try to get to a decision / conclusion.)

Also because, Safari currently returns the entire value when passing 0, and on the (admittedly very unlikely) off-chance that someone relies on this, returning an empty array could lead to a security issue. So if we do end up going with that option, perhaps we don't want to encourage Safari to change the behavior to returning an empty array now (and then back later).

In the case of Chrome, the X25519 algorithm is still behind an experimental runtime flag, so I don't think it's a problem to change the implementation and align it with the current spec (zero length returns an empty array), but I guess we can wait until the spec issue is resolved to merge the PR with the definitive tests.

[twiss]

Yeah, I understand. I'm also not really worried about adding tests for X25519 (tentative or otherwise) that need to be changed later, but I'm more hesitant about doing that for ECDH.

But, if you don't want to wait for the spec decision, perhaps we can also mark the tests for 0 as being tenative, and merge them like that, for now?

[javifernandez]

Yeah, I understand. I'm also not really worried about adding tests for X25519 (tentative or otherwise) that need to be changed later, but I'm more hesitant about doing that for ECDH.

Makes sense.

But, if you don't want to wait for the spec decision, perhaps we can also mark the tests for 0 as being tenative, and merge them like that, for now?

The CL changed the Chrome's X25519 implementation to change its behavior for the '0' length case that's why I wanted to land this PR under the assumption that there was more agreement on that case, leaving as tentative the 'null' and 'undefined' ones. If we want to mark all as tentative, perhaps I can land the CL without new tests and can wait until we solve the issue in the spec, at least for the '0' case.

[twiss]

Right. Yeah, I don't think there's full agreement on that yet either, as e.g. we have this open PR too: w3c/webcrypto#351. (Which, if we merge something like that, we should do the same thing for X25519 and X448, IMO.)

[panva]

@javifernandez Can you update this for the merged state of w3c/webcrypto#345?

[panva]

I don't believe the -1 tests are correct, they should all result in a type error instead

https://webidl.spec.whatwg.org/#abstract-opdef-converttoint

If x < lowerBound or x > upperBound, thenthrow a TypeError.

[javifernandez]

@twiss @martinthomson could you please review this ? I'm already working on the changes for the web engines, but waiting for this PR to be merged. Thanks.

[twiss]

👍 Thanks!