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

Bundle standalone 'manufdb' file to comply with Wireshark changes. #4351

Merged
merged 1 commit into from
Apr 28, 2024

Conversation

gpotter2
Copy link
Member

@gpotter2 gpotter2 commented Apr 13, 2024

This PR:

Fun fact: a first version of this PR converted Manuf to a python dictionary. Apparently loading a file containing a very, very big Python dictionnary takes infinetly longer than loading text/binary and parsing it.

@gpotter2 gpotter2 added this to the 2.6.0 milestone Apr 13, 2024
Copy link

codecov bot commented Apr 13, 2024

Codecov Report

Merging #4351 (1602125) into master (cd2fed9) will decrease coverage by 0.03%.
The diff coverage is 59.44%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4351      +/-   ##
==========================================
- Coverage   82.13%   82.10%   -0.03%     
==========================================
  Files         350      352       +2     
  Lines       83136    83224      +88     
==========================================
+ Hits        68280    68334      +54     
- Misses      14856    14890      +34     
Files Coverage Δ
scapy/arch/windows/__init__.py 61.73% <100.00%> (-0.99%) ⬇️
scapy/config.py 79.81% <100.00%> (+1.70%) ⬆️
scapy/dadict.py 92.64% <100.00%> (+0.84%) ⬆️
scapy/libs/manuf.py 100.00% <ø> (ø)
scapy/main.py 70.95% <78.26%> (-0.05%) ⬇️
scapy/data.py 90.52% <86.88%> (-0.81%) ⬇️
scapy/tools/generate_manuf.py 0.00% <0.00%> (ø)
scapy/tools/generate_ethertypes.py 0.00% <0.00%> (ø)

... and 1 file with indirect coverage changes

if cachepath.exists():
try:
with cachepath.open("rb") as fd:
data = pickle.load(fd)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it’s quite dangerous to pickle.load from an arbitrary file. Would it be a possibility to use serialisation to json instead ?

Copy link
Member Author

@gpotter2 gpotter2 Apr 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@polybassa @guedou This file is local. If you can edit it you can also add malicious code to .config/scapy/prestart.py.

Considering this I don't see how it's an issue

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I totally agree on that point. I just wanted to mention it.

@gpotter2 gpotter2 force-pushed the nomanuf branch 2 times, most recently from 471de77 to 78bc2dc Compare April 14, 2024 13:17
polybassa
polybassa previously approved these changes Apr 16, 2024
@gpotter2
Copy link
Member Author

gpotter2 commented Apr 20, 2024

Hm, should we gzip the blob ? It's a bit big.
I can't recall how supported gzip is on all plateforms. If I'm not mistaken zlib is often not available...

Edit: we're already using gzip as a hard dependency, so I ended up using it.

@evverx
Copy link
Contributor

evverx commented Apr 21, 2024

It took me a while but I figured out why Packit failed in https://download.copr.fedorainfracloud.org/results/packit/evverx-scapy-2/fedora-rawhide-x86_64/07331856-scapy/builder-live.log.gz. There is no ~/.config there. I reproduced it locally by launching a stripped container and running the test there. Once I manually created the ~/.config directory the tests started passing.

@gpotter2
Copy link
Member Author

gpotter2 commented Apr 21, 2024

Thanks for looking into it.

I didn't fully read the XDG spec so I kinda assumed one shouldn't create the .config, apparently as I was wrong and it is actually standardized:

https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html

If, when attempting to write a file, the destination directory is non-existent an attempt should be made to create it with permission 0700. If the destination directory exists already the permissions should not be changed.

@gpotter2
Copy link
Member Author

/packit build

Loading the dictionnary takes 0.5s by itsef, so we have to cache it in
order to keep reasonable boot times.
@gpotter2
Copy link
Member Author

/packit build

@gpotter2 gpotter2 merged commit 601dbde into secdev:master Apr 28, 2024
33 checks passed
@gpotter2 gpotter2 deleted the nomanuf branch April 28, 2024 18:04
evverx added a commit to evverx/scapy that referenced this pull request May 1, 2024
now that secdev#4351 is merged and
secdev#4280 is closed.

and also run the netaccess tests.

It's a follow-up to 86c7a05.
evverx added a commit to evverx/scapy that referenced this pull request May 1, 2024
now that secdev#4351 is merged and
secdev#4280 is closed.

and also run the netaccess tests.

It's a follow-up to 86c7a05.
gpotter2 pushed a commit that referenced this pull request May 1, 2024
now that #4351 is merged and
#4280 is closed.

and also run the netaccess tests.

It's a follow-up to 86c7a05.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

conf.manufdb.* doesn't seem to work because Wireshark and some distros no longer ship the OUI database
3 participants