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

ci: move the fuzz target #4441

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

ci: move the fuzz target #4441

wants to merge 2 commits into from

Conversation

evverx
Copy link
Contributor

@evverx evverx commented Jun 23, 2024

as discussed in google/oss-fuzz#12050

(It's a draft because I'm not sure if it's OK to make that licence change. I asked it in google/oss-fuzz#12114 just in case)

Copy link

codecov bot commented Jun 23, 2024

Codecov Report

Attention: Patch coverage is 0% with 19 lines in your changes missing coverage. Please review.

Project coverage is 81.61%. Comparing base (18b3d6c) to head (475f96e).
Report is 1 commits behind head on master.

Files Patch % Lines
scapy/tools/pcap_fuzzer.py 0.00% 19 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4441      +/-   ##
==========================================
+ Coverage   81.42%   81.61%   +0.18%     
==========================================
  Files         355      356       +1     
  Lines       84815    84834      +19     
==========================================
+ Hits        69062    69234     +172     
+ Misses      15753    15600     -153     
Files Coverage Δ
scapy/tools/pcap_fuzzer.py 0.00% <0.00%> (ø)

... and 20 files with indirect coverage changes

@guedou
Copy link
Member

guedou commented Jun 23, 2024

Thanks! Could you move the file into scapy/tools ?

@gpotter2
Copy link
Member

gpotter2 commented Jul 3, 2024

Could you explain a bit what the fuzzing architecture is ? I must say I didn't really read the doc of oss-fuzz, and the assumptions I had about what it did appear to be wrong.

#4378 said:

It downloads the corpus OSS-Fuzz has accumulated so far (including the test cases that triggered issues in the past) and runs the fuzz target with it. It should help to catch most regressions when PRs are opened.

But I now realize we're doing some sort of fuzzing ourself in our test suite (or something that 'builds fuzzers' and other stuff that isn't at all what I thought it would do https://github.com/secdev/scapy/actions/runs/9786056051/job/27020223993). I don't like that at all, although I probably have an incorrect understanding of what it does. I thought it would get done on some other instance, and we would only get the crashing results via email, or something like that. We don't want to slow down the test suite doing fuzzing...

So what did #4378 do ? And what does this PR do?

@evverx
Copy link
Contributor Author

evverx commented Jul 4, 2024

So what did #4378 do ?

The idea behind #4378 is to run the fuzz target when PRs are opened. It should help to catch issues like #4373, #4396 (comment), 9946ef1 and so on before they make it to the repository. It can catch "shallow" issues that can be found in, say, less than a minute. To catch the other issues the fuzz target is run on OSS-Fuzz too.

And what does this PR do?

This PR should make it easier to change the fuzz target to cover new layers for example. Currently it's necessary to clone the OSS-Fuzz repository, make changes there, sign the CLA, cc the maintainers and so on.

@evverx
Copy link
Contributor Author

evverx commented Jul 4, 2024

we would only get the crashing results via email, or something like that

FWIW OSS-Fuzz can open issues on GitHub (for example systemd/systemd#28237) to make it easier to keep track of them. It's possible to make those reports public too.

(They are restricted because OSS-Fuzz follows Google’s standard disclosure policy but I'm not sure it makes sense to apply it here. Other than that as discussed in google/oss-fuzz#8921 (comment) it doesn't help much in general because fuzz targets are public and run outside of OSS-Fuzz anyway)

@evverx
Copy link
Contributor Author

evverx commented Aug 1, 2024

I'll go ahead and close this since it hasn't been confirmed that it's OK to move the fuzz target.

On a somewhat related note having read https://security.googleblog.com/2024/06/hacking-for-defenders-approaches-to.html I'm kind of curious how all those patches and fuzz targets are going to be licensed.

@evverx evverx closed this Aug 1, 2024
@gpotter2
Copy link
Member

gpotter2 commented Aug 1, 2024

I don't mind keeping this until we get the go from OSS-FUZZ. I think it's still a good idea

@guedou
Copy link
Member

guedou commented Aug 1, 2024 via email

@evverx
Copy link
Contributor Author

evverx commented Aug 1, 2024

I'll reopen it then.

I closed it here because it doesn't seem that OSS-Fuzz is going to approve google/oss-fuzz#12114. I closed that PR but I'll reopen it there too.

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

Successfully merging this pull request may close these issues.

None yet

3 participants