Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validation criteria: verify_strict might be accepting non-canonical R #267

Open
rozbb opened this issue Jan 17, 2023 · 3 comments
Open

Validation criteria: verify_strict might be accepting non-canonical R #267

rozbb opened this issue Jan 17, 2023 · 3 comments
Labels
do-for-2.0 This should be resolved before a 2.0 release

Comments

@rozbb
Copy link
Contributor

rozbb commented Jan 17, 2023

It appears that verify_strict might permit non-canonical R, while verify does not. Notice the difference in the code is that verify_strict does a point comparison, while verify does a byte comparison.

This is pretty subtle, and I think it's not detected by our validation tests because the validation tests never do a non-canonical R that isn't also low-order. Three things I wanna do:

  1. Confirm that this behavior is happening
  2. Update verify_strict to reject non-canonical encoding
  3. Write a regression test similar to what zebra has

This is technically breaking, but I consider it a bugfix rather than a validation change. Nobody would reasonably expect verify_strict to be more permissive than verify on any axis.

@rozbb rozbb added the do-for-2.0 This should be resolved before a 2.0 release label Jan 17, 2023
@rozbb
Copy link
Contributor Author

rozbb commented Jan 20, 2023

Ok I think this is a non-issue. Yes, it may be the case that verify_strict accepts prime-order non-canonical R, but the reason it's not an issue is because it's extremely difficult to generate a signature with such an R. That's because the ability to produce such a signature is the same as proving knowledge of of dlog(R), which is probably hard.

My proposed solution: let's just make this explicit, and do verification by byte comparison in all verify_* functions. This will simplify the code and only change behavior in a computationally infeasible edge case.

cc @tarcieri

P.S. This doesn't necessarily address the fact that verify_* might all accept a prime-order non-canonical pubkey A as input. But for the same reason as above, it should be computationally infeasible to come up with a valid signature under such a pubkey.

@tarcieri
Copy link
Contributor

My proposed solution: let's just make this explicit, and do verification by byte comparison in all verify_* functions.

Agree with this, and I wonder if you could potentially wrap it up a little more, perhaps a function that calls your proposed recompute_r function and compares it with an expected R in a single place which can be reused?

@rozbb
Copy link
Contributor Author

rozbb commented Jan 20, 2023

My thought was to have recompute_r just return the compressed form, and then all the callers have no choice but to do a byte comparison. This looks nice enough imo, since everything all the verify functions look like expected_R == computed_R.

But #251 needs to land before that happens.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-for-2.0 This should be resolved before a 2.0 release
Projects
None yet
Development

No branches or pull requests

2 participants