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

secp256k1::Message::from_slice fails on all-0x00 hash #7

Open
junderw opened this issue Jun 9, 2020 · 4 comments
Open

secp256k1::Message::from_slice fails on all-0x00 hash #7

junderw opened this issue Jun 9, 2020 · 4 comments

Comments

@junderw
Copy link
Owner

junderw commented Jun 9, 2020

In the upstream tests, we test the extreme bounds of most of the parameters.

We accept 32 0x00 bytes for input as a hash message for signing.

https://github.com/rust-bitcoin/rust-secp256k1/blob/a5147bbf015ca61bb0e4e09adb8e9b324965c291/src/lib.rs#L490-L504

Need to find some way to get around this:

        if data == [0; constants::MESSAGE_SIZE] {
            return Err(Error::InvalidMessage);
        }
@junderw
Copy link
Owner Author

junderw commented Jun 10, 2020

Temp fix:
junderw/rust-secp256k1@e657814

However, this brings up a good point... perhaps we should throw on all 0x00 in tiny-secp256k1 as well???

@yuki-js
Copy link
Collaborator

yuki-js commented Jun 12, 2020

Did you see rust-bitcoin/rust-secp256k1#207 ?
I think all zero hash should be treated as invalid.

@junderw
Copy link
Owner Author

junderw commented Jun 12, 2020

Yes. I've known that for a while.

The problem is:

32 0x00 bytes is a valid hash. and they allow the curve order, even though since it is modulo, it is equivalent to 0 in a mathmatical sense (and therefore just as dangerous)

1 is also a valid private key (whose public key is the generator point)

If someone tried to rewrite bitcoin-core in rust, and used this library, any tx whose sighash happens to be 0x00 32 bytes will cause a hardfork.

@junderw
Copy link
Owner Author

junderw commented Jun 12, 2020

Also see this comment:

rust-bitcoin/rust-secp256k1#211 (comment)

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

Successfully merging a pull request may close this issue.

2 participants