Skip to content

Check added to PcapNg processing #4373

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

Merged
merged 2 commits into from
May 8, 2024
Merged

Check added to PcapNg processing #4373

merged 2 commits into from
May 8, 2024

Conversation

guedou
Copy link
Member

@guedou guedou commented Apr 30, 2024

This PR fixes issues discovered by oss-fuzz.

@guedou guedou added this to the 2.6.0 milestone Apr 30, 2024
@guedou guedou force-pushed the guedou/20240430/oss-fuzz branch from 309dc52 to a86c368 Compare April 30, 2024 18:38
@guedou guedou force-pushed the guedou/20240430/oss-fuzz branch from a86c368 to 1e5bca1 Compare April 30, 2024 18:46
Copy link

codecov bot commented Apr 30, 2024

Codecov Report

Attention: Patch coverage is 59.37500% with 13 lines in your changes are missing coverage. Please review.

Project coverage is 82.21%. Comparing base (ac3d5bb) to head (371aaf6).
Report is 2 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4373   +/-   ##
=======================================
  Coverage   82.20%   82.21%           
=======================================
  Files         353      353           
  Lines       83529    83607   +78     
=======================================
+ Hits        68662    68734   +72     
- Misses      14867    14873    +6     
Files Coverage Δ
scapy/utils.py 73.01% <59.37%> (-0.23%) ⬇️

... and 6 files with indirect coverage changes

@evverx
Copy link
Contributor

evverx commented May 1, 2024

I apologize for bringing this up here but since it's related to OSS-Fuzz I wonder if it would be OK if I opened google/oss-fuzz#9629 again?

@guedou
Copy link
Member Author

guedou commented May 2, 2024

@evverx I am fine with the proposed PR that loads all layers. However, I will prefer to keep the reports to the Scapy maintainers only.

@evverx
Copy link
Contributor

evverx commented May 2, 2024

Fair enough. I fuzz scapy elsewhere so I don't need access to bug reports on OSS-Fuzz (though it would make it easier to filter out duplicates, comment on issues like #3145 (comment) reproducible by that fuzz target in certain environments only and so on). I'll open a PR without my email address then.

@guedou
Copy link
Member Author

guedou commented May 2, 2024

The others maintainers might have a different opinion.

@gpotter2
Copy link
Member

gpotter2 commented May 2, 2024

I'm personally fine with @evverx having their email there.

Two reasons:

  • oss-fuzz has currently never reported anything worth worrying about (due to the nature of Scapy, we can't really have anything much more serious than a DoS), meaning it's actually not that big of a deal.
  • @evverx has provided valuable contributions to Scapy, and I believe they're asking in good faith.

@p-l-
Copy link
Member

p-l- commented May 2, 2024

@guedou Do you think it would be interesting to add some problematic files as test cases here? Or do we consider oss-fuzz to be enough?

I agree w/ @gpotter2 re. adding @evverx mail.

@guedou
Copy link
Member Author

guedou commented May 2, 2024

One test case is small (~100 bytes), while the two others one are big (1k+ bytes). I could try to reduce the big ones if you think that it is worth the effort.

@p-l-
Copy link
Member

p-l- commented May 2, 2024

Not necessarily, that was more a question than a change request to your PR!

p-l-
p-l- previously approved these changes May 2, 2024
@guedou
Copy link
Member Author

guedou commented May 2, 2024

@gpotter2 I managed to strip down the test cases to 12, 100 and 140 bytes. If that's OK for you, I will add the corresponding unit tests.

@evverx
Copy link
Contributor

evverx commented May 2, 2024

Do you think it would be interesting to add some problematic files as test cases here?

FWIW It should be possible to turn on CIFuzz: https://google.github.io/oss-fuzz/getting-started/continuous-integration/. It downloads corpora accumulated by OSS-Fuzz (including test cases that triggered issues in the past) and runs fuzz targets with them. It doesn't catch all the issues but it should be enough to catch most regressions when PRs are opened. Though it looks like OSS-Fuzz is updating the toolchains, interpreters and some other things so I'd wait for them to finish just in case to avoid running into things like google/oss-fuzz#11886.

evverx added a commit to evverx/scapy that referenced this pull request May 3, 2024
using https://google.github.io/oss-fuzz/getting-started/continuous-integration/

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.

Prompted by secdev#4373.
evverx added a commit to evverx/oss-fuzz that referenced this pull request May 6, 2024
@evverx
Copy link
Contributor

evverx commented May 6, 2024

(I opened google/oss-fuzz#11912. It should hopefully make the regression test from #4378 a bit more useful)

jonathanmetzman pushed a commit to google/oss-fuzz that referenced this pull request May 7, 2024
When scapy reads pcap/pcapng files it extracts link layer types from
headers and tries to match them with classes representing those layers.
Those classes are registered in conf.l2types.num2layer when they are
imported so if no layers are imported the only known layer is Raw and it
isn't very interesting in terms of fuzzing because it just wraps bytes
without parsing anything. Apart from populating l2types when the layers
are imported they are bound to make it possible for scapy to guess
payloads and move from lower layers all the way up.

The coverage went from 24% to 39%. It can be bumped further but I think
it would be better to roll out those changes gradually.

* [scapy] fix indentation

Fixes:
```sh
projects/scapy/pcap_fuzzer.py:21:3: E111 indentation is not a multiple of 4
projects/scapy/pcap_fuzzer.py:36:3: E111 indentation is not a multiple of 4
projects/scapy/pcap_fuzzer.py:37:3: E111 indentation is not a multiple of 4
projects/scapy/pcap_fuzzer.py:41:3: E111 indentation is not a multiple of 4
...
```
and makes it easier to change the code using editors expecting Python
code to be compliant with PEP 8.

[scapy] update the CC list

It was discussed in
secdev/scapy#4373 (comment)
evverx added a commit to evverx/scapy that referenced this pull request May 7, 2024
using https://google.github.io/oss-fuzz/getting-started/continuous-integration/

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.

Prompted by secdev#4373.
@gpotter2 gpotter2 merged commit 0d7b148 into master May 8, 2024
@gpotter2 gpotter2 deleted the guedou/20240430/oss-fuzz branch May 8, 2024 10:23
evverx added a commit to evverx/scapy that referenced this pull request May 15, 2024
using https://google.github.io/oss-fuzz/getting-started/continuous-integration/

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.

Prompted by secdev#4373.
evverx added a commit to evverx/scapy that referenced this pull request May 15, 2024
using https://google.github.io/oss-fuzz/getting-started/continuous-integration/

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.

Prompted by secdev#4373.
evverx added a commit to evverx/scapy that referenced this pull request May 22, 2024
using https://google.github.io/oss-fuzz/getting-started/continuous-integration/

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.

Prompted by secdev#4373.
evverx added a commit to evverx/scapy that referenced this pull request May 23, 2024
using https://google.github.io/oss-fuzz/getting-started/continuous-integration/

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.

Prompted by secdev#4373.
evverx added a commit to evverx/scapy that referenced this pull request Jun 1, 2024
using https://google.github.io/oss-fuzz/getting-started/continuous-integration/

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.

Prompted by secdev#4373.
evverx added a commit to evverx/scapy that referenced this pull request Jun 10, 2024
using https://google.github.io/oss-fuzz/getting-started/continuous-integration/

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.

Prompted by secdev#4373.
evverx added a commit to evverx/scapy that referenced this pull request Jun 15, 2024
using https://google.github.io/oss-fuzz/getting-started/continuous-integration/

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.

Prompted by secdev#4373.
evverx added a commit to evverx/scapy that referenced this pull request Jun 21, 2024
using https://google.github.io/oss-fuzz/getting-started/continuous-integration/

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.

Prompted by secdev#4373.
evverx added a commit to evverx/scapy that referenced this pull request Jun 22, 2024
using https://google.github.io/oss-fuzz/getting-started/continuous-integration/

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.

Prompted by secdev#4373.
evverx added a commit to evverx/scapy that referenced this pull request Jun 28, 2024
using https://google.github.io/oss-fuzz/getting-started/continuous-integration/

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.

Prompted by secdev#4373.
evverx added a commit to evverx/scapy that referenced this pull request Jun 30, 2024
using https://google.github.io/oss-fuzz/getting-started/continuous-integration/

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.

Prompted by secdev#4373.
evverx added a commit to evverx/scapy that referenced this pull request Jul 1, 2024
using https://google.github.io/oss-fuzz/getting-started/continuous-integration/

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.

Prompted by secdev#4373.
gpotter2 pushed a commit that referenced this pull request Jul 1, 2024
* ci: run the fuzz target on PRs

using https://google.github.io/oss-fuzz/getting-started/continuous-integration/

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.

Prompted by #4373.

* dcerpc: turn print into log_runtime.warning

to make it possible to turn it off with logging.disable().

(it should help to make the fuzz target less chatty among other things
because it seems to be the only dissector (covered by the fuzz target)
printing messages like that directly)
@evverx evverx mentioned this pull request Jul 4, 2024
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.

4 participants