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

Reduce cryptography usage in --cfg=fuzzing #282

Merged
merged 4 commits into from
Jun 9, 2021

Conversation

TheBlueMatt
Copy link
Member

This reduces the usage of real cryptography in --cfg=fuzzing,
specifically replacing the secret->public key derivation with a
simple copy and ECDH with XOR of the public and private parts
(plus a stream of 1s to make a test pass that expected non-0
output).

It leaves secret tweak addition/multiplication as-is.

It also changes the context creation to over-allocate and store
the context flags at the end of the context buffer, allowing us
to easily test context flags in each function.

While it would be nice to have something fancier (eg XOR-based),
its not immediately obvious how to accomplish this, and better to
fix the issues I have than spend too much time on it.

Fixes #271.

This partially reverts b811ec1

secp256k1-sys/src/lib.rs Outdated Show resolved Hide resolved
@TheBlueMatt
Copy link
Member Author

Rebased on latest upstream with no changes.

@TheBlueMatt
Copy link
Member Author

Rebased on latest upstream with no changes.

apoelstra
apoelstra previously approved these changes Jun 8, 2021
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack f1e34bf

lol @ the hand-rolled synchronized static context creation. I believe it's correct, though it took me some time to reason through it.

@apoelstra
Copy link
Member

needs rebase now

In the next commit the secret->public key derivation in fuzzing cfg
is changed to be simpler, as well as the validity rules of public
keys relaxed.

This adds a new test to ensure random keys can be added, not just
the hard-coded keys test that exists today.
This reduces the usage of real cryptography in --cfg=fuzzing,
specifically replacing the secret->public key derivation with a
simple copy and ECDH with XOR of the public and private parts
(plus a stream of 1s to make a test pass that expected non-0
output).

It leaves secret tweak addition/multiplication as-is.

It also changes the context creation to over-allocate and store
the context flags at the end of the context buffer, allowing us
to easily test context flags in each function.

While it would be nice to have something fancier (eg XOR-based),
its not immediately obvious how to accomplish this, and better to
fix the issues I have than spend too much time on it.

Fixes rust-bitcoin#271.

This partially reverts b811ec1
@TheBlueMatt
Copy link
Member Author

Rebased, was just a super-trivial new test method that was at the same spot as one that is added here.

Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack 79119e8

@apoelstra apoelstra merged commit bb25ed4 into rust-bitcoin:master Jun 9, 2021
@apoelstra
Copy link
Member

@TheBlueMatt rust-miniscript fuzz tests are now failing because public keys do not round-trip

@apoelstra
Copy link
Member

if you parse

0439033304444444444444444444444444444444444444444444444444444444444444444444444444444444444444444444433333333333333333333323972654

then print it you get

0439033304444444444444444444444444444444444444444444444444444444440003330444444444444444444444444444444444444444444444444444444444

@apoelstra
Copy link
Member

(I think this is straightforward to fix, we just need to tighten up the parsing a bit, I can open a PR later today or maybe @sanket1729 can)

@TheBlueMatt
Copy link
Member Author

Grrr sorry about that, I guess I hardly tested full public key parsing instead of just the compressed public keys. Indeed, just storing more info seems like a good solution.

@sanket1729
Copy link
Member

I tried to fix this. But I think it is impossible to solve this in the current framework. We use byte 32 in public key to overwrite whether the key was compressed or not.

But when doing this, we lose the corresponding information for uncompressed pubkeys. And we would need a call to libsecp again to construct the y-coordinate which would require moving the parsing functions to not(fuzzing) that might break the goal of the PR.

Looking for more input on this issue. @TheBlueMatt @apoelstra

@sanket1729
Copy link
Member

sanket1729 commented Jul 15, 2021

One solution I can think of is to encode 02 compressed keys are x-coordinate``<000......000> and 03 as x-coordinate``<ffff......fffff>. That way, it is improbable that fuzzer would guess these exact values when trying to parse an uncompressed public key.

If the parsers look at the code and then choose the data inputs/control paths, I think this solution will also crash.

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 this pull request may close these issues.

#264 restircted fuzz coverage due to conversion to real private -> public key conversion
5 participants