Skip to content

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

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.

Sign up for GitHub

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

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

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

Changes from 4 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
a982da5
DHCPAM: Make_reply to use raw to inject IP.
mzen17 Jan 11, 2025
23f9dfc
REFACT: Removed whitespace.
mzen17 Jan 11, 2025
20f5d0b
REFACT: Minimize make_reply routes.
mzen17 Jan 11, 2025
cd8de29
BUG: namesaver not referencing self
mzen17 Jan 11, 2025
d6d7465
IMPL: Utilize IP to solve multiple ns
mzen17 Jan 16, 2025
d42fc74
REFACT: Fix style
mzen17 Jan 16, 2025
d3d8713
IMPL: DHCP_am tests
mzen17 Jan 20, 2025
c79516a
Merge branch 'secdev:master' into master
mzen17 Jan 20, 2025
2a62d6c
REFAC: Move flatten to parseopt
mzen17 Feb 10, 2025
56b4cf8
IMPL: unpack+None handle
mzen17 Feb 10, 2025
ecf7f44
Merge branch 'secdev:master' into master
mzen17 Feb 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 26 additions & 1 deletion scapy/layers/dhcp.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
StrField,
StrFixedLenField,
XIntField,
RawVal
)
from scapy.layers.inet import UDP, IP
from scapy.layers.l2 import Ether, HARDWARE_TYPES
Expand Down Expand Up @@ -669,6 +670,23 @@ def make_reply(self, req):
class DHCP_am(BOOTP_am):
function_name = "dhcpd"

def ip_to_bytes(self, ip_string):
"""Concat a IP str of form IP,IP,IP and returns it as bytes
Backcompatible if IP is a single IP.
:param ip_string: String of the IP to be packed"""
ip_string = ip_string.replace(" ", "")

# Split IPs by commas and filter out empty strings
ip_list = [ip.strip() for ip in ip_string.split(',') if ip.strip()]

# Convert each IP to packed format
packed_ips = []
for ip in ip_list:
packed_ips.append(socket.inet_aton(ip))

# Concatenate packed IPs into a single byte string
return b''.join(packed_ips)

def make_reply(self, req):
resp = BOOTP_am.make_reply(self, req)
if DHCP in req:
Expand All @@ -677,12 +695,19 @@ def make_reply(self, req):
for op in req[DHCP].options
if isinstance(op, tuple) and op[0] == "message-type"
]

if ',' in self.nameserver:
# Use if statement to reduce how much changes there are.
ns_val = IP(len=RawVal(self.ip_to_bytes(self.nameserver)))
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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'

Copy link
Author

Choose a reason for hiding this comment

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

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".

Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link

@fon1105 fon1105 Feb 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It works but the default second nameserver will be 192.168.0.50 when the input value is blank.
How can it keep blank value when we don't input the second nameserver?

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'

Copy link
Contributor

Choose a reason for hiding this comment

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

How can it keep blank value when we don't input the second nameserver?

It can't be empty because it's set to gw by default in

scapy/scapy/layers/dhcp.py

Line 624 in 3e00f42

self.nameserver = nameserver or gw
. It's documented in

scapy/scapy/layers/dhcp.py

Line 608 in 3e00f42

:param nameserver: the DNS server IP (by default, same than gw)
. It has nothing to do with this PR though.

FWIW I think it would be better to set nameserver in parse_options instead and then flatten it in make_reply.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah. I would agree that parse options makes more sense as a location to flatten it. I did use the comma seperated values because that was what the original issue mentioned (#4431).

else:
ns_val = self.nameserver

dhcp_options += [
x for x in [
("server_id", self.gw),
("domain", self.domain),
("router", self.gw),
("name_server", self.nameserver),
("name_server", ns_val),
("broadcast_address", self.broadcast),
("subnet_mask", self.netmask),
("renewal_time", self.renewal_time),
Expand Down