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

[mzen17]

Checklist:

• If you are new to Scapy: I have checked CONTRIBUTING.md (esp. section submitting-pull-requests)
• I squashed commits belonging together
• I added unit tests or explained why they are not relevant
• I executed the regression tests (using cd test && ./run_tests or tox)
• 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 converts a list of IPs to bytes
• Use RawVal to pass these bytes to override processing.

Notes

• List was used to store IPs during packing after a .split from ',' (this shouldn't have any impact on performance since it is only on answering machine, and also users should not put in large amounts of name servers).

fixes #4431

[gpotter2]

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.

[mzen17]

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?

[gpotter2]

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

[codecov]

Codecov Report

Attention: Patch coverage is 50.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 70.08%. Comparing base (92925da) to head (d42fc74).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
scapy/layers/dhcp.py	50.00%	2 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (92925da) and HEAD (d42fc74). Click for more details.

HEAD has 8 uploads less than BASE

Flag	BASE (92925da)	HEAD (d42fc74)
	9	1

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #4638       +/-   ##
===========================================
- Coverage   80.71%   70.08%   -10.64%     
===========================================
  Files         359      334       -25     
  Lines       86052    81040     -5012     
===========================================
- Hits        69461    56797    -12664     
- Misses      16591    24243     +7652     

Files with missing lines	Coverage Δ
scapy/layers/dhcp.py	83.01% <50.00%> (-0.49%)	⬇️

... and 297 files with indirect coverage changes

[evverx]

I'm not sure how it was tested but it doesn't work in the sense that the option isn't included in DHCP replies at all.

I'm not sure it's necessary to add ip_to_bytes either. scapy can turn IP sequences into DHCP options out of the box. self.nameserver should be unpacked with

("name_server", *self.nameserver)

to get it to work when something like DHCP_am(nameserver=('1.2.3.4', '5.6.7.8')) is used.

Just out of curiosity looking at the comments I wonder if the code is autogenerated with some sort of an AI assistant?

[mzen17]

Interesting.

I was playing around with your suggestion to unpack it, however, it was retrieving only the first nameserver rather than both the nameserver. Like, if I used ('1.2.3.4', '5.6.7.8'), it would only get ('1.2.3.4'). The previous method I was using didn't work after examining the output again

I was testing it by inserting this to the end of the make_reply (before it returns).

dhcp_options = resp[DHCP].options
        for opt in dhcp_options:
            if isinstance(opt, tuple) and opt[0] == "name_server":
                dns_servers = opt[1]
                print("[+] dns server: ", dns_servers)

Just the ip_to_bytes function was AI generated. It was generated with the function comment and a force to use inet_ion and concat.

[evverx]

it was retrieving only the first nameserver rather than both the nameserver

I think it's how it currently works so maybe make_reply wasn't edited. With the following patch applied

diff --git a/scapy/layers/dhcp.py b/scapy/layers/dhcp.py
index 031fe34d..6b3cf065 100644
--- a/scapy/layers/dhcp.py
+++ b/scapy/layers/dhcp.py
@@ -682,7 +682,7 @@ class DHCP_am(BOOTP_am):
                     ("server_id", self.gw),
                     ("domain", self.domain),
                     ("router", self.gw),
-                    ("name_server", self.nameserver),
+                    ("name_server", *self.nameserver),
                     ("broadcast_address", self.broadcast),
                     ("subnet_mask", self.netmask),
                     ("renewal_time", self.renewal_time),

it should work with more than one IP address (but it doesn't work with one IP address). The idea is to flatten the list. For example here is how it works outside the answering machine:

>>> raw(DHCP(options=[('name_server', '1.2.3.4', '5.6.7.8')]))
b'\x06\x08\x01\x02\x03\x04\x05\x06\x07\x08'

[mzen17]

So I adjusted the if statement so that the string is transformed into a tuple if an IP such as "1.1.1.1,2.2.2.2" is used. It also works on sole strings such as "1.1.1.1".

[evverx]

Personally I don't think split is needed. I think it should support 2 use cases like

DHCP_am(nameserver='1.2.3.4')

and

DHCP_am(nameserver=('1.2.3.4', '5.6.7.8'))

but I'd wait for the second opinion.

(It would probably be great to add a test to https://github.com/secdev/scapy/blob/master/test/answering_machines.uts)