Add a basic fuzz testing harness for Dilithium2 #1905
Conversation
silvergasp
force-pushed
the
main
branch
2 times, most recently
from
0571f45
to
e27055c
Compare
|
I'm pretty happy with with where the fuzz harness is at. It's not anything particularly complicated, just essentially throws a bunch of messages at the dilithium api. I've got a little bit more work to do in integrating oss-fuzz before I want to merge this. I plan on following up with more fuzzer's and more in depth testing e.g. constant time/algorithm fuzzing etc. But I'll just start with getting all the automation up and running with something super simple like this. |
Do you also plan on adding more algorithms to the fray?
Sounds like a good plan. Also some documentation (how to start and read results, say) would be very welcome. |
Yeah for sure. I plan on slowly chipping away at it, one algorithm at a time. I admit that I'm not familiar with most of these algorithms so it may take me some time. But I'm very much on top of how to write efficient fuzz-harnesses. So hopefully we'll end up with something that's useful at the end of it all.
Sure does the markdown docs under |
You shouldn't have to be -- they're all behind the same API. In fact, lots of code in
Nope. Our semi-formal agreement is to include documentation that is dependent on scripts in the repo there (in the repo, under "docs") and only leave code-independent stuff (procedures for release etc) in the Wiki. |
|
Thanks for the great work, Nathaniel! This is a great start. I don't have any experience with fuzzing, but your plan in #1215 (comment) sounds promising. Obviously we would eventually want fuzzing against as many algorithms as possible, and as Michael points out we have a code generation system that can help with that.
@baentsch I don't think it harms the 0.11.0 release to have this landed even if it's only for one algorithm, though we'd want to be careful to make sure we word things in a way that users don't think all algorithms have been fuzzed. E.g. change the title of the PR/commit to "Add a basic fuzz testing harness for Dilithium2". I wouldn't hold 0.11.0 for this, but we're not in a code-freeze yet either so I don't want to block progress on this. |
With that (and CI passing) I'd agree to merge, too. |
silvergasp
changed the title
Done
Done, at least I think I've got it right this time.
I've only given it a cursory read for now, and it's not entirely obvious to me how I would integrate this with fuzzing harnesses. But I'll have time to take a deeper dive with code generation next week.
Done. I've also added a 'current status section' to the fuzzing docs. |
There was a problem hiding this comment.
Thanks very much for this proposed contribution, @silvergasp! It would help to resolve one of our oldest outstanding issues.
I've commented on a few minor style points. I'd also like to see if we can have this running in CI. Probably the easiest way to do this is to add a python wrapper in the tests directory (similar to, say, test_leaks.py) and trigger it with a "fuzzing" CI job, which can be added to the big matrix in .github/workflows/linux.yml. Let me know if you need help with that. We just landed a big CI refactor this morning, so you will want to sync your fork before taking this on.
CONFIGURE.md
Outdated
|
|
||
| ## OQS_BUILD_FUZZ_TESTS | ||
| Can be `ON` or `OFF`. When `ON` liboqs the fuzz test-suite will be enabled. This option is only available if `$CC` is set to clang. |
There was a problem hiding this comment.
Nit: I would rather direct users to the CMAKE_C_COMPILER option than the CC environment variable.
There was a problem hiding this comment.
I think I've got this right now, but please confirm this is what you are after as I'm not a cmake wizard.
Whoops, I should have read through the related issue (#1215) more carefully before suggesting this. If you have a different plan for getting this running in CI, please feel free to disregard this suggestion. |
Yeah I've got a plan in mind :) I've just accepted all your requested edit's but squashed them back into the original commit, adding you as co-author. |
|
Is there anything more that I need to address here? |
Sorry for dropping the ball on my side, @silvergasp . I just triggered CI running and it failed with an error that seems triggered by the PR: Please check it out: Goal should be to get CI to green. Then, what about adding a CI test enabling the new option? Can there be a "short" test run to ascertain the fuzzing build is OK without spending hours in CI? |
|
I suggest we hold off on merging this until after the 0.11.0 release. |
Not a problem. I've fixed the CI and tested it using a personal branch so that I could confirm that it indeed works as I don't have permissions to run the CI here. As requested I've also added a CI build stage that builds the fuzz harness and runs it for 30s. I will follow up later with more sophisticated CI integration once the oss-fuzz stuff has gone ahead. |
Easy, there is no immediate rush on my end. I'll be taking on new responsibilities in about a months time, so after that I may be a little slower to respond. |
Co-authored-by: Spencer Wilson <spencer.wilson@uwaterloo.ca> Signed-off-by: Nathaniel Brough <nathaniel.brough@gmail.com>
Signed-off-by: Nathaniel Brough <nathaniel.brough@gmail.com>
The purpose of this PR is to improve the overall security of liboqs. This is by adding a very basic fuzz as described here #1215.
This PR just adds a new style of test i.e. a fuzz test and therefore doesn't need additional unit tests. Integrating this into the CI isn't currently possible but I've got a bit of a plan as to how to go ahead with it which I've described here.