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

Request OSS-Fuzz project owner approval before importing OSS-Fuzz reports #2176

Open
davidben opened this issue May 8, 2024 · 14 comments
Open
Assignees

Comments

@davidben
Copy link

davidben commented May 8, 2024

Automating vulnerability reports from OSS-Fuzz is great, but the unfortunate fact is that OSS-Fuzz is very noisy. It's quite frequent that infrastructure changes cause false positives (see google/oss-fuzz#11881).

Prior to OSV, this was fine because false positive reports could simply be ignored, and then OSS-Fuzz will automatically close them later. Fuzzing is super valuable, so we were more than happy to tolerate a moderate false positive rate, in exchange for avoiding false negatives, provided that false positives were low cost.

OSV completely throws this equation off. As soon as a bad report is imported into OSV, a ton of busywork is triggered downstream. The more OSV is used, the worse this will become. See, for example, google/oss-fuzz-vulns#37 where every imported report was incorrect.

Fuzzers are incredibly valuable, and we do not want to be forced to shut off fuzzers because OSS-Fuzz's false vs true positive tradeoffs are no longer viable.

I think the most natural fix here is that, prior to importing from OSS-Fuzz, OSV should ping the project owners and provide an automated way for project owners to modify the imported report. If the maintainers don't respond after a short period of time, it can import automatically. But maintainers who are active need to have some way to deal with this noise, or we'll have to stop fuzzing.

@evverx
Copy link
Contributor

evverx commented May 9, 2024

I came here from google/oss-fuzz#11925 and while I agree that it certainly makes sense to let maintainers vet OSV entries before they are published I think it would be nice if all the entries were published anyway. There are projects that can reject things left and right and it would still be useful to be able get the "raw" feed to double-check.

(more generally I think that the problem is that OSV consumers can be a bit overzealous in that everything is treated as "vulnerability" without vetting anything. Maybe there should be a way to say that some entries are generated based on automated tools and should be vetted. If they are confirmed by maintainers consumers can be confident that entries are vulnerabilities. I proposed "confidence" in another context #918 but it can probably be applied here)

@andrewpollock
Copy link
Contributor

Maybe there should be a way to say that some entries are generated based on automated tools and should be vetted.

This is a very interesting thought: being able to weight (or even filter) vulnerabilities by their source

@davidben
Copy link
Author

davidben commented May 10, 2024

That would not be acceptable to us, no. Unfortunately, that is not the reality of how these data sources are consumed. No matter how much weighting you apply, if the system claims that completely fabricated things as vulnerabilities, it will cause a lot of downstream churn, with the churn increasing the more successful you all are at getting your data source to be used.

If there is no way for BoringSSL team to withdraw false positives before they ever make it to OSV, we will be forced to remove our project from OSS-Fuzz and rely on fuzzing in other systems. This would be a significant security loss for everyone. Weighting is not sufficient unless the weight can be set to zero, and done so before it ever makes it to OSV.

To be clear, false positives here doesn't mean there was a subjective disagreement over whether an issue had security consequences. False positives here means the issue did not exist. OSS-Fuzz has false positives all the time when the compiler infrastructure needed to check for uninitialized memory, or other errors, broke. Most recently, it looks like the instrumented C/C++ runtime did not get picked up, so it mistakenly believed all strings were uninitialized. A BoringSSL user could not apply a fix for this issue if they wanted to, because there is nothing to fix.

@evverx
Copy link
Contributor

evverx commented May 10, 2024

Unfortunately, that is not the reality of how these data sources are consumed. No matter how much weighting you apply, if the system claims that completely fabricated things as vulnerabilities, it will cause a lot of downstream churn

I completely agree. It annoys upstream projects too when it reaches them. I stopped integrating new projects like google/oss-fuzz#8699 into OSS-Fuzz when I can fuzz them elsewhere to avoid dealing with all that.

withdraw false positives before they ever make it to OSV

My concern is that it offloads that onto upstream projects. I believe that OSV consumers (or whoever relies on that feed) should be able to deal with false positives and bogus vulnerabilities in general.

(Anyway I agree with you that it's not perfect and I'm glad that it has been brought up because I gave up at some point)

@davidben
Copy link
Author

davidben commented May 10, 2024

Ah yeah, I guess I filed this as a thing for OSV to notify project owners, and that may not be the best option. (I filed a lot of bugs here and mentally mixed them up. 😄) That might be a little annoying. I think I also was assuming OSS-Fuzz was immutable and the fix would have to be in OSV, which introduced the bad assumption on OSS-Fuzz data quality, but perhaps we can do better.

My general feeling is that the earlier a mistake is fixed, the less fanout there is, and the less of a pain everything is. If bad statuses are fixed in OSS-Fuzz, they never make it to OSV. If bad statuses are fixed in OSV, they never make it to OSV consumers. Etc.

How about this as the roadmap?

  1. OSS-Fuzz needs to provide an easy way for project owners to adjust issue status and severity without having to summon an OSS-Fuzz maintainer (Project owners need some way to adjust severity of OSS-Fuzz reports oss-fuzz#11939 and Project owners need some way to mark bugs as false positives oss-fuzz#11925)
  2. OSS-Fuzz keeps track of whether each issue has been triaged by the project owner, or if it's just totally automated.
  3. To make sure project owners know these now matter (they previously did not), perhaps there should be an automated reply to the untriaged issues some time before the issue is made public or imported to OSV or something else. It was a huge unpleasant surprise to me to learn that OSV even existed and had been claiming all these false positive issues as vulnerabilities. (I found out because some tooling at Google started monitoring this and caused me a ton of busywork.)
  4. When OSV imports an issue, if the issue was not triaged, include a big warning that this may not be a real security issue due to frequent OSS-Fuzz false positives
  5. If the project owner later triages the OSS-Fuzz report, OSV should automatically update its entry
  6. OSV should not remove the explicit notice about OSS-Fuzz false positives until something like Better pre-commit monitoring for MSan false positives oss-fuzz#11941 is implemented.

@evverx
Copy link
Contributor

evverx commented May 10, 2024

It was a huge unpleasant surprise to me to learn that OSV even existed and had been claiming all these false positive issues as vulnerabilities

I think new projects should also be made aware of this integration with OSV upfront because there are projects that just don't want to be involved in any sort of vulnerability management (they can barely fix bugs fuzz targets throw at them).

As far as I can remember in https://seclists.org/oss-sec/2019/q2/165 some things about autogenerated vulnerabilities were discussed. It was kind of controversial but maybe some ideas can be borrowed.

@andrewpollock
Copy link
Contributor

I think there's some blurriness between the OSS-Fuzz and OSV.dev lines here, and I admit I'm not intimately familiar with the lines myself because they are blurry...

Generally speaking, OSV.dev is an aggregator of upstream data sources, and OSS-Fuzz should be able to be thought of as a data source like any other. I think this issue is more appropriate to OSS-Fuzz directly, rather than OSV.dev.

We get feedback like this often enough, where OSV.dev is the messenger, not the originator of the message.

@andrewpollock
Copy link
Contributor

@oliverchang should this be moved over to OSS-Fuzz?

@davidben
Copy link
Author

Well, it's a little subtle because OSV is interpreting OSS-Fuzz data in a way that does not match reality. Whether explicitly designed as such or not, OSS-Fuzz is set up so that you cannot assume the data in it works as a source of vulnerabilities. That was fine up until OSV existed. So the immediate cause of the problem is OSV.

But the best fix isn't always necessarily at the immediate cause, so if OSS-Fuzz is on board with doing the dev work needed to support OSV's interpretation (i.e. dramatically shore up project owners' ability to triage issues and correct statuses), that works for me!

That said, I don't think there's any solution that doesn't involve some OSV changes. I think we do need OSV to be much, much more explicit about when issues are unconfirmed, and probably to notify on OSS-Fuzz so that project owners know that the new ramifications of untriaged are clearer.

@evverx
Copy link
Contributor

evverx commented May 11, 2024

OSS-Fuzz should be able to be thought of as a data source like any other

It's true but OSV consumers go to upstream projects either directly or via, say, RedHat Product Security to complain about unfixed bugs. In the past bogus CVEs were filed based on OSV data.

I really like the plan proposed in #2176 (comment) because it should help to reduce the number of false positives and things like that on the OSS-Fuzz side that but I don't think it's ever going to be safe to assume that every OSV entry coming from OSS-Fuzz is a vulnerability so it should be easy for upstream projects to withdraw/correct entries and consumers should be prepared to vet things themselves.

@evverx
Copy link
Contributor

evverx commented May 18, 2024

Turns out it's possible to turn this off so I opened #2222 (I'll probably add more projects later).

OSV completely throws this equation off. As soon as a bad report is imported into OSV, a ton of busywork is triggered downstream

Given that to judge from ossf/scorecard#3903 half-baked SBOMs are on their way and I presume they should be integrated with OSV/GUAC (though it's hard to tell because as usual PRs are merged with no rationale and details there) I really think this import should be off by default and projects should be made aware of this integration.

@oliverchang
Copy link
Collaborator

Thanks for all the feedback. It's important for us to not turn this off by default for all OSS-Fuzz projects, because we've more often than not observed projects fixing vulnerabilities without ever notifying downstream consumers or requesting a CVE (because it's a lot of work). Hopefully, by adding some metadata to indicate an OSS-Fuzz entry is fully automated and had zero human triage, we can address some of the concerns around quality.

That said, there's a lot of gaps with our current UX, and we'll start planning suggested improvements on #2176 (comment) and others soon.

@evverx
Copy link
Contributor

evverx commented May 20, 2024

I'd still appreciated it if #2222 was merged. When the boringssl maintainers are OK with the UX I think it should be fine to unblock those projects too.

It's important for us to not turn this off by default for all OSS-Fuzz projects

That would really be a bummer but with the UX improvements it's still based on the assumption that someone is going to vet those bug reports and I don't think it's going to happen. As mentioned elsewhere I think one option would be to separate the OSS-Fuzz feed from OSV and let consumers who are actually prepared to deal with those bug reports deal with those bug reports.

@davidben
Copy link
Author

Hopefully, by adding some metadata to indicate an OSS-Fuzz entry is fully automated and had zero human triage, we can address some of the concerns around quality.

On the OSV consumer side, what's the intended use of this metadata? E.g. would you advise OSV imports at Google to ignore the entries with the bit set? If not, the bit alone isn't very effective.

That would really be a bummer but with the UX improvements it's still based on the assumption that someone is going to vet those bug reports and I don't think it's going to happen.

Not only that, it's currently not possible to vet bug reports. We (BoringSSL) generally keep up with our OSS-Fuzz reports, but there is no mechanism in OSS-Fuzz for us to triage them at all. When OSS-Fuzz is wrong, our only option is to ignore the report and leave the incorrect data alone.

That option was fine before OSV. After OSV, it's a problem.

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

4 participants