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

[bitcoin-core] Add differential cryptography fuzzer #5717

Merged

Conversation

guidovranken
Copy link
Contributor

Please don't merge this yet..

This PR incorporates the changes submitted in #5716 so no merge conflicts should arise if this one is merged later..

At @MarcoFalke's request I'm submitting a separate PR for this, so it can be commented on separately by the Bitcoin Core devs.

This PR builds an extra fuzz target based using Cryptofuzz which tests the following:

Bitcoin Core src/crypto primitives:

  • MOD 2**256 bignums (arith_uint256.cpp)
  • Hashes: RIPEMD160, SHA1, SHA256, SHA512, SHA3
  • KDFs: SHA256 HKDF
  • MACs: SIPHASH, SHA256 HMAC
  • Symmetric ciphers: AES CBC, ChaCha20

libsecp256k1-specific:

  • Convert private key to public key
  • Test if given pubkey is valid (point on curve etc)
  • ECDSA signing (both explicit nonce and RFC6979 are supported)
  • ECDSA verification
  • ECDSA pubkey recovery

Botan and Trezor firmware are used as oracles (e.g. used for comparing the output of Bitcoin/secp256k1 against).

@sipa Do you have any suggestions as for additional secp256k1 compilation flags? E.g. particular --with-ecmult-window or --with-ecmult-gen-precision settings that would be useful to test?

@maflcko
Copy link
Contributor

maflcko commented May 6, 2021

Does cryptofuzz support msan? Eventually we should build with that sanitizer enabled: #5699 (comment)

@guidovranken
Copy link
Contributor Author

Does cryptofuzz support msan? Eventually we should build with that sanitizer enabled: #5699 (comment)

Yes this works with MSAN.

Copy link
Contributor

@jonathanmetzman jonathanmetzman left a comment

Choose a reason for hiding this comment

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

LGTM.
Will merge when requested.

@maflcko
Copy link
Contributor

maflcko commented May 7, 2021

Needs rebase

@sipa
Copy link

sipa commented May 10, 2021

  • I think the most interesting thing to test with is the hidden option --with-test-override-wide-multiply= (which can be set to int64 or int128, or auto). Setting it to int64 will test all the 32-bit specific code (which would be used on 32-bit platforms) without needing to actually build for 32-bit.
  • Enabling/disabling x86_64 asm may be useful so both the C and asm implementations get tested (--with-asm=x86_64 and --with-asm=no).
  • --enable-module-schnorrsig is only useful if you're going to be testing the schnorrsig API, which is not the case as far I can tell. I guess there is some marginal benefit in testing that enabling it doesn't break anything else, but I think that's better covered using libsecp256k1's own CI rather than fuzzing. So I'd drop that.
  • Regarding, --ecmult-window and --ecmult-gen-precision: I'd say test the default and the minimal/maximal values (2,4,8 for ecmult-gen-precision, 2,15,24 for ecmult-window), if you have that option? 24 needs 0.5 GB of memory, so feel free to reduce.
  • --enable-ecmult-static-precomputation and its opposite do affect some things internally; it may be useful to test with and without.

Thanks!

@guidovranken
Copy link
Contributor Author

* I think the most interesting thing to test with is the hidden option `--with-test-override-wide-multiply=` (which can be set to `int64` or `int128`, or `auto`). Setting it to `int64` will test all the 32-bit specific code (which would be used on 32-bit platforms) without needing to actually build for 32-bit.

The fuzzer is built for and run on both 64 bit and 32 bit. I suppose this makes invoking --with-test-override-wide-multiply= unnecessary?

* Enabling/disabling x86_64 asm may be useful so both the C and asm implementations get tested (`--with-asm=x86_64` and `--with-asm=no`).

OK. This is already the case: the MemorySanitizer build is built without assembly, the other sanitizers are built with assembly.

* `--enable-module-schnorrsig` is only useful if you're going to be testing the schnorrsig API, which is not the case as far I can tell. I guess there is some marginal benefit in testing that enabling it doesn't break anything else, but I think that's better covered using libsecp256k1's own CI rather than fuzzing. So I'd drop that.

OK, I'll remove it.

* Regarding, --ecmult-window and --ecmult-gen-precision: I'd say test the default and the minimal/maximal values (2,4,8 for ecmult-gen-precision, 2,15,24 for ecmult-window), if you have that option? 24 needs 0.5 GB of memory, so feel free to reduce.

* --enable-ecmult-static-precomputation and its opposite do affect some things internally; it may be useful to test with and without.

3 * 3 * 2 = 18 different configuration options. It is possible to build 18 different binaries, but I think that the CPU time allotted to this project by OSS-Fuzz will be divided across 18 targets, which may impede fuzzer progress compared to just one or a few targets. Perhaps there is a good middle ground we can decide on?

@sipa
Copy link

sipa commented May 18, 2021

--ecmult-window and --ecmult-gen-precision don't interact really; the first is used only for variable-time functions, while the latter is for constant-time ones (roughly). I don't think there is any API call that is affected by both. That means just 3 combinations (e.g. both small, both default, both large) should be sufficient.

@maflcko
Copy link
Contributor

maflcko commented May 18, 2021

Still needs rebase or merge with master?

@guidovranken
Copy link
Contributor Author

Should be good to merge now @jonathanmetzman

@inferno-chromium inferno-chromium merged commit 1385553 into google:master May 19, 2021
@guidovranken
Copy link
Contributor Author

@MarcoFalke @sipa I don't receive bug reports for this project. Please reach out if there's a crash that you can't diagnose.

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.

None yet

6 participants