search

hathach / tinyusb

An open source cross-platform USB stack for embedded system
https://www.tinyusb.org
MIT License
4.84k stars 1.03k forks source link

Add fuzz testing harnesses #1715

Open silvergasp opened 1 year ago

silvergasp commented 1 year ago

Related area

Fuzz testing

Hardware specification

Host only

Is your feature request related to a problem?

No

Describe the solution you'd like

I'd like to improve the security and robustness of the tinyusb stack by adding a set of fuzz tests.

I have checked existing issues, dicussion and documentation

silvergasp commented 1 year ago

I'm happy to spend some time contributing to this one. I've got some general questions first though;

  1. Is it acceptable to you to have the fuzz harnesses written in a combo of c/c++. These fuzz harnesses would be run from the host only, so I think the overhead of c++ is acceptable given the ease of use of the fuzzing libs available in c++.
  2. Do you have the bandwidth to give some guidance on WIP PRs?

Here is a list of the things that I would like to add;

silvergasp commented 1 year ago

I'd like to add an additional question. Would you be open to using google's OSS-fuzz service for continuous fuzzing? It's a free service and will fuzz and send you updates when bugs are found.

https://github.com/google/oss-fuzz

hathach commented 1 year ago

Awesome, thank you very much for spending time and effort to adding fuzzing support.

  1. Is it acceptable to you to have the fuzz harnesses written in a combo of c/c++. These fuzz harnesses would be run from the host only, so I think the overhead of c++ is acceptable given the ease of use of the fuzzing libs available in c++.

Sure thing, I have no problems with C/C++ combo at all as long as it is easy to use/maintain.

  1. Do you have the bandwidth to give some guidance on WIP PRs?

Sure things, though sometime I have to do lots of work switching and catching up due to other works. But I absolutely have time for this.

I'd like to add an additional question. Would you be open to using google's OSS-fuzz service for continuous fuzzing? It's a free service and will fuzz and send you updates when bugs are found.

https://github.com/google/oss-fuzz

Yeah sure, why not, I am happy if we could be ale to use this service. Though I am new to fuzzing and still learn on how to use/run/test with it.

Thank you again for this super helpful issue and sorry for super late response :)

silvergasp commented 1 year ago

Sweet, I'll open a WIP progress PR over at OSS-fuzz to integrate tinyusb. Is your '@tinyusb.org` email address associated with a google account? I'm just going over the requirements for accepting new projects with OSS-fuzz and one of them is a google email address.

https://google.github.io/oss-fuzz/getting-started/accepting-new-projects/#accepting-new-projects

hathach commented 1 year ago

Sweet, I'll open a WIP progress PR over at OSS-fuzz to integrate tinyusb. Is your '@tinyusb.org` email address associated with a google account? I'm just going over the requirements for accepting new projects with OSS-fuzz and one of them is a google email address.

https://google.github.io/oss-fuzz/getting-started/accepting-new-projects/#accepting-new-projects

unfortunately not, it is an office 365 email provided by hosting company. I have just created an new gmail for this purpose, I will try to make a few commit with this new email if that make verification steps easier. Thank you.

silvergasp commented 1 year ago

Easy I've added a draft initial integration here https://github.com/google/oss-fuzz/pull/9143 but I'll wait until the harnesses are merged here otherwise there isn't much point integrating.

silvergasp commented 1 year ago

I'm going to work on an HID device fuzzer next up. I'll keep you posted here as to what I'm working on so that we don't overlap.

silvergasp commented 1 year ago

Also I'll see if I can get the oss-fuzz integration working shortly too!

hathach commented 1 year ago

@silvergasp we probably need to update the oss-fuzz pr to match the new folder location of fuzz in test/fuzz

silvergasp commented 1 year ago

@silvergasp we probably need to update the oss-fuzz pr to match the new folder location of fuzz in test/fuzz

Yeah I'll take a better look tonight

silvergasp commented 1 year ago

Just to confirm have you made any commits with that gmail account?

silvergasp commented 1 year ago

I've marked the OSS-fuzz integration as ready to merge https://github.com/google/oss-fuzz/pull/9143. Hopefully everything is in order and it get's merged relatively soon.

hathach commented 1 year ago

Just to confirm have you made any commits with that gmail account?

yeah, I did, just made all recent merged and commits with that gmail account.

I've marked the OSS-fuzz integration as ready to merge google/oss-fuzz#9143. Hopefully everything is in order and it get's merged relatively soon.

thank you

silvergasp commented 1 year ago

So I've got some CI feedback from the OSS-fuzz repo, and it looks like there was a few build failures. I've put up a PR here that should mostly address these problems.

https://github.com/hathach/tinyusb/pull/1823

hathach commented 1 year ago

@silvergasp thank you very much, tinyusb is accepted to oss-fuzz. Though strangely, I would expect an notification email from oss-fuzz but received none so far. Also the document said we could check build status after 1 day or so, but couldn't find it https://oss-fuzz-build-logs.storage.googleapis.com/index.html .

There is no rush, I just wonder if there is anything we should do or just simply wait for a few more days for it to get run.

silvergasp commented 1 year ago

Yeah, I actually think this is an upstream bug in oss fuzz. It turns out that many new projects are not working properly. There is a similar tracking issue here;

https://github.com/google/oss-fuzz/issues/9408

While the name of the issue is not descriptive of tinyusb it appears to be an identical problem.

hathach commented 1 year ago

thank you @silvergasp , seems like they fix the issue already. tinyusb build log can be found at https://oss-fuzz-build-logs.storage.googleapis.com/index.html#tinyusb

silvergasp commented 1 year ago

Yeah, it's looking like things are on track again. I don't think the fuzzer's have been run yet, but I'll check again tomorrow to see what's going on. We should be able to see some statistics once it's been run here;

https://oss-fuzz.com/fuzzer-stats?project=tinyusb&fuzzer=afl&job=afl_asan_tinyusb&group_by=by-fuzzer

You'll need to log in with your tinyusb.org@gmail.com email account to get access to that dashboard.

silvergasp commented 1 year ago

Looks like it's running well. I've got a little bit of tweaking to get it working well with the AFL fuzzing engine. But other than that I'm super impressed that it hasn't found any new bugs. Its already done more than 1000x the iterations than i ever did on my laptop. Their servers are a lot quicker. You've done well writing tinyusb to be so robust against fuzzing!

hathach commented 1 year ago

ah thanks, the dashboard starts to show some interesting statistic, though to be honest, I don't know these well enough.

Looks like it's running well. I've got a little bit of tweaking to get it working well with the AFL fuzzing engine. But other than that I'm super impressed that it hasn't found any new bugs. Its already done more than 1000x the iterations than i ever did on my laptop. Their servers are a lot quicker. You've done well writing tinyusb to be so robust against fuzzing!

I hope so, but there is chance that we didn't have enough fuzzing. I will try to learn more of fuzzing to add more testing in the future. Thank you again, this will hugely improve the stability of tinyusb.

silvergasp commented 1 year ago

Not a problem :)

Well here's a more verbose rundown. There are multiple sanitizers (you can think of these as bug detectors). The ones that we have enabled with OSS-fuzz are;

Then we've got multiple fuzzing engines enabled. Each fuzzing engine behaves a little differently and will mutate the inputs differently. The overall concept is the same it's just how the fuzzing is performed that is different. We have a few of these enabled;

So when you look at the dashboard you'll see a kind of matrix expansion of sanitizers x fuzzing engine.

For the most part, these are all working great for the mass storage, cdc, and net classes. However, if we look at the stats for the AFL fuzzer you'll see that the "statibility" is pretty low, (27.9 out of 100). This isn't fantastic and it should really be closer to 90/100, to get good results from AFL. The reason why stability is so low is that AFL makes the assumption that it can run, multiple iterations in parallel threads. However, in the case of tinyusb this is a poor assumption, as there is a fairly significant amount of global state (variables). This means that there are instead a bunch of different threads all modifying the same global state rather than having their own independent instance of tinyusb. I have a bit of an idea of how we can address this e.g. something like this;

#ifdef FUZZ
   // Make a thread-local static global so that each thread has it's only local instance of tinyusb
   #define TU_STATIC __thread static
#else 
   #define TU_STATIC static
#endif

It'll then just be a process of going through all the static vars and replacing them with TU_STATIC. I'm part way through this process, so I'll have a PR up soon.

hathach commented 1 year ago

thank you very much for the very detail explanation. Things are much clearer to me now. TU_STATIC look good, we can then wrap all variable into a struct to make it easier to add TU_STATIC. I definitely need to learn more about fuzzing in the future.

silvergasp commented 1 year ago

Yeah there are two options;

silvergasp commented 1 year ago

On a different note, the code coverage for the msc and net class fuzzers is not fantastic and i might be missing something in my fuzzing stubs. I'm going to take a closer look over the next couple of days, but if you've got time to take a quick look at the fuzz harnesses/callbacks that would be fantastic.

https://storage.googleapis.com/oss-fuzz-coverage/tinyusb/reports/20230119/linux/src/tinyusb/src/class/report.html