Less strict ecc_verify_hash_ex (as it was before ecc_recover_key) #443
Conversation
|
Cc: @rmw42 |
|
Ah... this is the checking the length of the r/s values, rather than just dividing the buffer length in 2? Fair enough. There's no great solution in a format which doesn't mark the lengths :( |
tests/ecc_test.c
Outdated
| return err; | ||
| } | ||
|
|
||
| static int _ecc_issue443(void) /* https://github.com/libtom/libtomcrypt/issues/443 */ |
| else if (sigformat == LTC_ECCSIG_RFC7518) { | ||
| /* RFC7518 format - raw (r,s) */ | ||
| i = mp_unsigned_bin_size(key->dp.order); | ||
| if (siglen != (2*i)) { |
There was a problem hiding this comment.
I also preferred this version of the check TBH
There was a problem hiding this comment.
Yeah, I'm not convinced that {0x05, 0x01} should be a valid signature blob for ECDSA, let alone that it should be in a security/proofing library.
There was a problem hiding this comment.
I also don't like that it could be a valid signature, but it's a valid edge-case... :/
... and history has proven that there are always implementations who do worse than your implementation so this (0x000..05...) is a good another example of Postel's Law IMO :)
karel-m
force-pushed
the
pr/ecc-verify-was-too-strict
branch
from
5e7ad04 to
1ac1072
Compare
I just thought that this check could then still be done after the |
|
A rather more radical solution would be to throw the test out entirely, and consider every possible partition of the input string, one by one. That way we make no assumptions at all about the padded size of Would probably want to be a flags parameter to |
|
I do not have a strong opinion on this. Let's give it a day or two to think it over. |
IMO it's good as it is now |
|
Yeah, agreed. I'd say there's a problem with wycheproof but, until/unless they fix it, the test needs to be the way it is now. |
|
In fact there is newer wycheproof test suite than I am using - https://github.com/google/wycheproof |
It's still got that test in there, though :( |
|
BTW my quick check - there is at least 20 new ECDSA edge-cases that we do not implement correctly 😞 |
karel-m
force-pushed
the
pr/ecc-verify-was-too-strict
branch
from
1ac1072 to
59bc3b5
Compare
The recent ECDSA related changes (ecc_recover_key & co.) made
ecc_verify_hash_exa little bit more stricter (I know, it was my idea).However, it turned out that 2 tests from wycheproof test suite started to fail.
Both cases were in category "acceptable" (so basically it is fine to reject them but also to accept them).
The 100% correct signature looks like this (hexa):
The "acceptable" signature looks like this (hexa):
So with this PR we will accept the "acceptable" signature as we did before ecc_recover_key.
I have added both failing test cases to
ecc_test.c