-
Notifications
You must be signed in to change notification settings - Fork 447
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
Add a basic fuzz testing harness for Dilithium2 #1905
base: main
Are you sure you want to change the base?
Conversation
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Conceptually LGTM. As it only supports a single algorithm (OK for feasibility test, not OK for a release), I'd hold off approving/merging this until 0.11.0 is out. Other opinions welcome.
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. |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I would rather direct users to the CMAKE_C_COMPILER
option than the CC
environment variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me, but I will hold off on approving until we have 0.11.0 out the door.
Co-authored-by: Spencer Wilson <[email protected]> Signed-off-by: Nathaniel Brough <[email protected]>
Signed-off-by: Nathaniel Brough <[email protected]>
I'm new to fuzzing, so one question I was wondering about is how one decides which inputs to fuzz. For example, I notice here that the fuzzing is done only on the message input of the signing and verifying algorithms. Would one ever want to fuzz the other inputs -- public key, secret key, signature? Why or why not? It's okay if the answer is yes, but you're just setting up a basic harness here, I just want to understand what scope of fuzzing is desirable. |
Ideally you'd fuzz as many inputs as possible.
Yes, although this is a little more complicated so I haven't done it yet. The way that fuzzing works is it will generate a set of randomised bytes which is then passed into your fuzz harness a number of times. The fuzzer will then measure the code-coverage for each run, the inputs that produce the highest coverage are then randomly mutated to create new inputs. This is called coverage driven fuzzing. It looks something like this; flowchart LR
A[Random Input] --> B[Fuzz Harness]
B --> C[Collect code coverage information]
C --> D[Mutate orgional input to maximize coverage]
D --> B
The reason that I've opted to NOT fuzz the public/private keys yet is because it would take a really long time for the fuzzer to "guess" at a private/public key pair that actually correspond to each other, so it's very likely that the fuzzer would just be fuzzing the error handling code. Which is still useful and I may follow up with a separate harness that targets that, but in my opinion it is less useful than fuzzing the main code-path. So in short you want to balance the ability for the fuzzer to "guess" at the "correct" inputs that unlock large sections of code-coverage against actually fuzzing all the inputs. There aren't any hard rules as to what should/shouldn't be fuzzed, it's mostly a mix of intuition and experimentation e.g. when you run a fuzzer you can see in real-time the number of "code-blocks" (the actual code between jump instructions e.g. if/else/function_calls) that the fuzzer has been able to run under the
Let me know if you have any further/follow-up questions. |
Thanks for the very helpful explanation, @silvergasp! |
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.