scapy icon indicating copy to clipboard operation
scapy copied to clipboard

Fix #4431 - Allow DHCP Answering Machine to have multiple namesevers

Open mzen17 opened this issue 11 months ago • 4 comments

Checklist:

  • [x] If you are new to Scapy: I have checked CONTRIBUTING.md (esp. section submitting-pull-requests)
  • [x] I squashed commits belonging together
  • [x] I added unit tests or explained why they are not relevant
  • [x] I executed the regression tests (using cd test && ./run_tests or tox)
  • [x] If the PR is still not finished, please create a Draft Pull Request

This PR allows the DHCP Answering Machine to support multiple nameservers, as specified in RFC 2132 section 3.8. Previously, the program would run into an error if multiple IPs were provided into nameservers due to the way IPFields pack; it uses the entire string as a single IP and sends it through inet_aton to pack.

The summary of the changes are as follows:

  • Create a function in DHCP_am that splits a list of IPs to seperate IPs (e.g, "1.1.1.1,2.2.2.2" => ("1.1.1.1", "2.2.2.2")
  • Populate the nameserver option field to return both.

fixes #4431

mzen17 avatar Jan 10 '25 19:01 mzen17

Hi ! Thanks for the PR. I think that it would be better to simply add support for a list of IPs in the DHCP answering machine, rather than editing the global IPField. This has too many unintended changes as it's used in a lot of places.

gpotter2 avatar Jan 10 '25 19:01 gpotter2

Yeah, that sounds very reasonable. So the issue is that the nameserver field in Answering Machine is created as an IP Field, and I directly changed the IP field to add support to it (which is bad design choice since so much non-DHCP systems rely on it and it is the only one that needs multiple IPs).

So there are two choices I am considering:

  • Create a type MultiIPField seperate from IPField to make the multi IP feature more reusable + solve it for all DHCP, albeit it probably won't do much since its just for DHCP (but will need more maintaince + tests)

  • Set nameserver to be a raw field and just manually do the IP packing inside the make_reply (will congest the DHCP answering machine make_reply code, unreusable, but easy to maintain)

Which choice do you think will work better?

mzen17 avatar Jan 10 '25 20:01 mzen17

I think it's simpler and clearer to just add it to make_reply. Thanks a lot

gpotter2 avatar Jan 10 '25 20:01 gpotter2

Codecov Report

:white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: Project coverage is 78.56%. Comparing base (c15a670) to head (ecf7f44). :warning: Report is 113 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4638      +/-   ##
==========================================
- Coverage   81.55%   78.56%   -2.99%     
==========================================
  Files         359      334      -25     
  Lines       86557    81358    -5199     
==========================================
- Hits        70592    63922    -6670     
- Misses      15965    17436    +1471     
Files with missing lines Coverage Δ
scapy/layers/dhcp.py 83.49% <100.00%> (ø)

... and 286 files with indirect coverage changes

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

codecov[bot] avatar Jan 11 '25 04:01 codecov[bot]