Test Ed25519 with small- and mixed-order points

[javifernandez]

Tests to ensure that the EdDSA Verify implementation performs checks for small-order key sand signature's point R.

[panva]

Tests 1,2,3,4, and 12 all fail on existing implementations while the others pass. Begs the question, are the failing ones correct?

[javifernandez]

Tests 1,2,3,4, and 12 all fail on existing implementations while the others pass. Begs the question, are the failing ones correct?

I have submitted a new PR, with some comments and refactoring that I think it makes the test cases clearer. I have added a new field for the test cases structure so that we can introduce the specific behavior of the engines regarding how to deal with small-order points in signature verification. So now all the test cases should pass in both WebKit and Blink engines, which are the only ones implementing the Ed25519 algorithm for now.

So now, talking about the expected results, it well-known the divergences regarding the different implementations on how to handle small-order points in the signature verification, as it's explained in the issue 19. It seems that both Blink's crypto library (BoringSSL) and WebKit's (Apple CryptoKit) implements the cofactorless signature verification algorithm and allow small components in the key and signature (vectors 0-3),

I still don't understand why the vectors in test case 11 are verified successfully, since the cypto library should have rejected non-cannonical keys, as it happens in test case 10.

[javifernandez]

Now the question is whether these tests matches the spec or not, since in the lat draft it's stated:

If the key data of key represents an invalid point or a small-order element on the Elliptic Curve of Ed25519, return false.

And vectors [0, 1] have small order key data and still are verified by most of the implementations.

If the point R, encoded in the first half of signature, represents an invalid point or a small-order element on the Elliptic Curve of Ed25519, return false.

At least vector 2 has an small-order R point, is validated by most of the implementations.

Should we add additional checks in the WebCrypto API implementation to match the spec, rejecting signatures generated by any small-order public keys ?

[panva]

Should we add additional checks in the WebCrypto API implementation to match the spec, rejecting signatures generated by any small-order public keys ?

I'm not aware of any more OpenSSL APIs we could employ to aid in the process. The status quo is IIRC as far as we can go on the Node.js side of things without resorting to introspecting the signature & key bytes ourselves.

cc @tniessen

[javifernandez]

Should we add additional checks in the WebCrypto API implementation to match the spec, rejecting signatures generated by any small-order public keys ?

I'm not aware of any more OpenSSL APIs we could employ to aid in the process. The status quo is IIRC as far as we can go on the Node.js side of things without resorting to introspecting the signature & key bytes ourselves.

cc @tniessen

According to this paper a check to reject small order key data could be implemented just looking for matches with the 14 small order points defined there. Additionally, given that non-cannonical points relevant for this check are all of small-order, there is no need to add specific checks for this.

[twiss]

Hello 👋 I haven't manually checked the values but if the quoted paper is correct, all the tests in kSmallOrderTestCases should have verified: false, I think.

Indeed the implementation could check for small-order points manually, or perhaps we could request OpenSSL to add a function to check this?

FWIW, I personally don't think it's super urgent to modify existing implementations that would currently fail these tests, since applications that don't require consistent validation criteria are unlikely to be vulnerable in some way because of this.

However, I do think it's best to make the specification explicit about which signatures are expected to be valid; and ideally try to get the implementations to be consistent on this point, otherwise it could inadvertently cause interoperability failures in the future, and/or it might become a fingerprinting signal. And so I would merge these tests with verified: false for all of them, so that implementations can work towards that goal.

---

Note that, technically speaking, an implementation that validates the signature with ID 11 in a TLS context is strictly speaking not compliant with RFC8446, which also refers to RFC8032 (which requires rejecting non-canonical points).

Whether or not that matters for TLS I don't know - I suspect not much, except again perhaps for fingerprinting? - but it might be an argument to get OpenSSL to care about that part.

[javifernandez]

And so I would merge these tests with verified: false for all of them, so that implementations can work towards that goal.

Some of the verification inconsistencies (vectors [4. 5]),come from the cofactored vs cofactorless implementation, which the spec is still not clear about which one is preferred; this is discussed in issue 19.

If we want to expect all these vectors to be verified as false, wouldn't we need to solve issue 19 and change the spec accordingly ?

[twiss]

Oh, I see, I misread the tests; I thought all of them were small-order (coming from Table 1 in the paper).

Then yeah, at the moment the draft doesn't say which verification equation to use, so strictly speaking both false and true would be acceptable in those cases, which is of course not ideal. So yeah, we should discuss that in WICG/webcrypto-secure-curves#19. Perhaps if we want this PR to strictly test WICG/webcrypto-secure-curves#21, we could leave out those cases for now (or allow either result)?

Edit: alternatively, I can make a PR requiring the cofactorless equation, and see if there's any objection to that.

[javifernandez]

Based on this comment I've submitted changes to expect failures in all the test cases, expect vector 5 which would require mixed-point checks. This implies that the WebCrypto API implementation should add checks for rejecting small-order points.

In order to pass the test case 5 we would require the spec to request for a cofactorless implementation, as suggested in PR #25.

These tests provide cases for non-canonical points, but all of them are small-order and definitively some implementations (eg BoringSSL) fail to detect them and refuse to verify the signatures. Adding checks for small-order points would imply we may miss some cases of non-canonical points that are not small-order and should be rejected.

Finally, test cases 12 and 13 detect scenarios that breaking non-repudiation; a canonical key and signature is verified against 2 different messages.

If we accept this tests, we would diverge from what other implementations do, especially regarding the rejection of any small-order point.

[twiss]

Thanks! This looks good to me now, but I'll hold off on approving until WICG/webcrypto-secure-curves#25 is approved and merged.

These tests provide cases for non-canonical points, but all of them are small-order and definitively some implementations (eg BoringSSL) fail to detect them and refuse to verify the signatures. Adding checks for small-order points would imply we may miss some cases of non-canonical points that are not small-order and should be rejected.

I think it would be reasonable to add more test cases for non-canonical points, but we could also do so later in a separate PR, IMO.

If we accept this tests, we would diverge from what other implementations do, especially regarding the rejection of any small-order point.

From these results, it looks like we would match the results of libsodium, libra-crypto, and dalek strict, right? Obviously it's a shame we don't match the others, but the results diverge quite a lot, and we can't match all of them (if we want to exactly define the results, in order to be able to have tests like this at all). And, out of the available options, erring on the side of strictness looks most reasonable to me - also because while it's possible to add a check, it isn't possible to remove one (without patching the underlying library).

[twiss]

WICG/webcrypto-secure-curves#25 has been merged, so I'll merge this as well 😊