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

idea: treat a major coverage drop an issue! #11398

Open
bagder opened this issue Dec 22, 2023 · 13 comments
Open

idea: treat a major coverage drop an issue! #11398

bagder opened this issue Dec 22, 2023 · 13 comments

Comments

@bagder
Copy link

bagder commented Dec 22, 2023

The curl code base has been fuzzed by oss-fuzz for many years. These days we rarely get any reports. It chugs away in the background and while it certainly gives us confidence and helps us "stay honest", it also falls off the top of our minds. We don't think about it too much in our daily work. I think that's good. It still tells us when it finds something. Which these days are many months between each time.

With this in mind, when we changed a config detail a year ago, the fuzz coverage for curl dropped considerably but no one noticed because no one checked. We are used to oss-fuzz telling us when there's an issue. Thus, oss-fuzz has the last year been fuzzing a much smaller part of the code base, which then of course could have made us miss problems.

The other day we fixed this issue and the coverage is again back up to levels similar to those a year ago, and nothing new has been reported (yet) so I figure in a way we were lucky and dodged one this time. But it brings up this topic on my agenda: how do I detect this problem the next time?

And perhaps even more broadly: how do we all (all oss-fuzz users) detect this because I presume that even though it happened to us this time, it might happen to others every once in a while. And maybe it has already happened to 22 users who have not even noticed yet and now the fuzzer is missing out?

How do others do? Is there a best practice I have missed? Can oss-fuzz perhaps itself detect huge drops and file an issue about that?

@evverx
Copy link
Contributor

evverx commented Dec 22, 2023

How do others do? Is there a best practice I have missed?

I opened #7190 so that CIFuzz could catch coverage drops as soon as possible but it kind of stalled. The only way to keep track of drops like that I'm aware of is the Fuzz-Introspector dashboard but it works when there are less than ten fuzz targets. With more than 40 fuzz targets it gets blurry.

@DavidKorczynski
Copy link
Collaborator

DavidKorczynski commented Dec 22, 2023

but it works when there are less than ten fuzz targets.

Could you clarify this? It works when there are 10 or more as well?

FWIF it's visible that there was a sudden jump in Curl's coverage: https://introspector.oss-fuzz.com/project-profile?project=curl so I think the drop would have been easy to spot but it looks like it happened at a point before the app was started (April 20 2023)

@evverx
Copy link
Contributor

evverx commented Dec 22, 2023

Could you clarify this? It works when there are 10 or more as well?

The global dashboard itself works with a lot of fuzz targets. The problem is that sometimes coverage drops affect certain fuzz targets and it's hard to spot them because they are hidden behind the global coverage where several fuzz targets can cover the same code a bit differently. Basically I have to go over specific coverage reports one by one.

@phi-go
Copy link
Contributor

phi-go commented Feb 21, 2025

FYI this feature now exists: ossf/fuzz-introspector#2102

I would wait for first results but we could maybe close this issue then.

@bagder @evverx to gauge if this should be implemented, would you want to be notified if there are issues detected for a fuzz target or is the project view enough?

@evverx
Copy link
Contributor

evverx commented Feb 21, 2025

would you want to be notified

I think it depends. If the coverage goes down significantly I think it should be certainly reported but ideally thresholds and things like that should be configurable probably to avoid spurious notifications.

The latest coverage drop was #12519 (comment) (where one fuzz target went down) and as far as I understand with ossf/fuzz-introspector#2102 it should be easier to spot things like that.

@phi-go
Copy link
Contributor

phi-go commented Feb 21, 2025

So would you prefer to be notified by default, let's say if there is a 5% drop for a fuzzer target, or only after configuration for a threshold has been set?

@evverx
Copy link
Contributor

evverx commented Feb 21, 2025

I think there are two (kind of) different use cases here. It's probably safe to say that all the projects should be notified by default when their total coverage goes down significantly. Drops affecting specific fuzz targets/code paths are important too (for example GHSA-5fhj-g3p3-pq9g) but it's not always obvious how to deal with them. Personally I'd look at the dashboards every now and then for the time being to see how it works first.

@phi-go
Copy link
Contributor

phi-go commented Feb 21, 2025

Thank you for the input! So to put this into actionable points:

  • There should be opt-out warnings for significant coverage drops of total project coverage.
  • Have opt-out warnings for the case when a fuzz target ceases to built successfully or if a target starts to be blocked.
  • Have opt-in warnings for significant fuzz target specific coverage drops.
  • Treat a 5% coverage drop or a 1000 line drop in coverage as significant but make these values configurable.
  • Reassess once this feature has been around for some time.

What do you think?

@evverx
Copy link
Contributor

evverx commented Feb 21, 2025

There should be opt-out warnings for significant coverage drops of total project coverage.
Have opt-out warnings for the case when a fuzz target ceases to built successfully or if a target starts to be blocked.

@phi-go I'm a bit reluctant when it comes to notifications because I'm subscribed to a bunch of different projects with different levels of tolerance to false positives so personally I think before adding opt-out notifications it would be great to figure out what it should do when fuzz targets are renamed, new code is added and so on. I don't think those things should trigger notifications to, say, mailing lists with hundreds of people there.

@phi-go
Copy link
Contributor

phi-go commented Feb 21, 2025

Yeah, that's why I'm asking. I would expect the reports to be sent to the issue tracker as is already done, are these monitored by large mailing lists? But yes, it is probably better to keep everything opt-in at the beginning, maybe forever.

From my experience renaming fuzz targets does not happen often. This could maybe be detected by code matching, though, this seems a bit error prone and difficult to implement.

Coverage drops due to adding code happens often (obviously). But is this truly a false positive if the fuzzing setup has reduced coverage due to added project code? In a perfect world harnessing should be updated to address new code so a notification seems warranted. Adding code could also break the fuzzer harness. Though, I'm not so eager on having notification when vendoring third-party code, this also regularly causes coverage drops. However, here we would need to separate project code and third-party code, which is likely not trivial, probably another thing that should be configurable. (See here for a bit more discussion: ossf/fuzz-introspector#2054)

Another common reason for coverage changes commonly seen as fluctuations are that the coverage process does not run through cleanly (think OOM or timeout). Should this be reported to maintainers?

@evverx
Copy link
Contributor

evverx commented Feb 21, 2025

are these monitored by large mailing lists?

It depends on the project. For example the elfutils project is notified via the devel mailing list.

From my experience renaming fuzz targets does not happen often.

Agreed. Though I renamed fuzz targets a few times and it would have been weird if OSS-Fuzz had filed bug reports and sent notifications.

is this truly a false positive if the fuzzing setup has reduced coverage due to added project code?

I think it depends. If the whole project is a hypothetical JSON parser it should probably be safe to assume that the new code is somehow related to the parser and should be fuzzed. In "umbrella" projects it's not always clear-cut. With no context I think it should stick to reporting obvious regressions it can be 100% sure about.

See here for a bit more discussion: ossf/fuzz-introspector#2054

I'll take a look. Thanks!

Another #12986 (comment) for coverage changes commonly seen as fluctuations are that the coverage process does not run through cleanly (think OOM or timeout). Should this be reported to maintainers?

I think those things should be more visible but I'm not sure they should always lead to notifications. It seems to me dashboards should be a good place to experiment, collect project-specific data and act on those things.

@phi-go
Copy link
Contributor

phi-go commented Feb 22, 2025

Agreed. Though I renamed fuzz targets a few times and it would have been weird if OSS-Fuzz had filed bug reports and sent notifications.

Yes, fair enough. What do you think of optionally allowing maintainers to specify a list of fuzz targets that should be fuzzed? We can use a heuristic based on fuzzers available in the past if this list is not specified. Renaming a fuzz target would still cause an alert in the later case.

Also personally I would actually like to see that the fuzz targets have changed as the oss-fuzz interaction is pretty much async this would signal to me that my change worked but I guess having a dashboard the same can be confirmed.

In "umbrella" projects it's not always clear-cut. With no context I think it should stick to reporting obvious regressions it can be 100% sure about.

While there is the difference that for a JSON parser the new code should be covered by the existing harness this is not necessarily the case for a more diverse project. However, in both cases new code is added that should be fuzzed, is that not a clear regression? Or do you mean that the new code might not need to be fuzzed?

I'm probably missing something but without context how can we detect obvious regressions?

I think those things should be more visible but I'm not sure they should always lead to notifications. It seems to me dashboards should be a good place to experiment, collect project-specific data and act on those things.

I agree, let's take it slow and extend the dashboard, we can still do notifications later.

@evverx
Copy link
Contributor

evverx commented Feb 22, 2025

What do you think of optionally allowing maintainers to specify a list of fuzz targets that should be fuzzed?

It can probably work if projects are actively involved in maintaining their fuzzing infrastructure but I don't think there are a lot of projects where it happens in practice. That's why I don't think that manual tweaks can work in practice in general. There are FI issues where it's discussed how maintainers can manually specify their threat models, exclude coverage and so on but it's unlikely a lot of projects are going to do that.

the oss-fuzz interaction is pretty much async

Agreed. That's why I think CIFuzz (which is run when PRs are opened) should do as much as possible to make sure stuff works/prevent regressions from landing. It's not there yet though.

However, in both cases new code is added that should be fuzzed, is that not a clear regression? Or do you mean that the new code might not need to be fuzzed?

I was trying to say that in some cases it's hard to tell whether code should be fuzzed at all. It's also worth mentioning that things like static reachability and so on aren't available everywhere because there are projects where FI just doesn't work.

I'm probably missing something but without context how can we detect obvious regressions?

As far as I understand this issue was prompted by a huge coverage drop in the curl project

Image

and I think in cases like this it's enough to warrant a bug report with almost no context. I think there are things that need looking into (like how to auto-close those issues when nobody reacts to them for a year and a lot of things have changed in the projects) but I think there should be a way for projects to say that they are interested in monitoring that metric.

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

No branches or pull requests

5 participants
@bagder @DavidKorczynski @evverx @phi-go and others