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

API/docs: Return value if ARG_CHECK fires #811

Open
real-or-random opened this issue Sep 11, 2020 · 6 comments
Open

API/docs: Return value if ARG_CHECK fires #811

real-or-random opened this issue Sep 11, 2020 · 6 comments

Comments

@real-or-random
Copy link
Contributor

For some functions handling public keys, the API docs claim that these functions always return 1. But when a zeroed pubkey is passed, the ARG_CHECK fires and they return 0.

Examples:

  • secp256k1_ec_pubkey_negate
  • secp256k1_ec_pubkey_serialize

Perhabs we can fix this as part of #783 .

@real-or-random
Copy link
Contributor Author

Anyway, we say for the illegal callback: "It will only trigger for violations that are mentioned explicitly in the header."

I guess zeroed public keys are never really explicit in the header. That's a little bit unfortunate. We have a special invalid value in our pubkey type for enhanced safety but it makes the API more complex.

@sipa
Copy link
Contributor

sipa commented Sep 11, 2020

I think any time you end up with a zeroed public key, it's a sign you're really using the API incorrectly? They're not part of the public interface, but an additional safety check added by the implementation.

@gmaxwell
Copy link
Contributor

Right, if you operate on a zeroed pubkey you've failed to perform required error handling.

@elichai
Copy link
Contributor

elichai commented Sep 14, 2020

FWIW, I wrote bindings in Go for libsecp and because of weird idioms in Go(all objects are zero init and there are no constructors) I'm checking if the pubkey is zero before calling these functions to prevent a user of the lib accidentally aborting the program because of it (specifically trying to serialize an empty pubkey for some reason)

(a better fix might be to set a different illegal callback that will return an error or something like that)

@real-or-random
Copy link
Contributor Author

Right, if you operate on a zeroed pubkey you've failed to perform required error handling.

Yes, my point is simply that this is not very clear from the docs.

@gmaxwell
Copy link
Contributor

gmaxwell commented Sep 14, 2020

@elichai if they've written their code like that-- (i agree, easy mistakes to make, -- not just in go but in C too) then they're potentially vulnerable to worse things... accepting invalid signatures, leaking private keys (e.g. a buffer for a key gets reused for a key and serialized), or even RCEs (there are function pointers used in this library). So it is much better to have a reliable abort, because aborting essentially always safer than an RCE, and also that sort of misuse will likely end up aborting closer to every time, rather than looking kinda like it worked.

(it would even be better if all the structures had canaries, so that invalid but coincidentally all zeros also got detected)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants