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

Isolate the DNS resolver into its own compartment. #50

Merged
merged 2 commits into from
Nov 22, 2024

Conversation

hlef
Copy link
Collaborator

@hlef hlef commented Nov 10, 2024

In the current implementation, the TCP/IP compartment is responsible for DNS, which allows it to spoof translations to bypass connection capabilities and create arbitrary firewall entries.

To prevent this, we must isolate the DNS resolver into its own compartment that plugs directly at the firewall-level.

Since neither the FreeRTOS+TCP resolver nor existing resolver libraries are meant to run directly at the Ethernet level, and most are too heavyweight, we write our own one.

Similarly to the FreeRTOS+TCP resolver, this implementation is a stub resolver: it only supports IPv4 recursive servers for A, AAAA, and CNAME queries. This resolver also assumes that the recursive resolver recurses into CNAME records. Support for IPv6 is left for future works.

The address of the DNS server is provided by the DHCP server. This may be overly restrictive - we should evolve it in future PRs or eventually replace it with something more featureful as the need arises.

Since the resolver plugs directly with the firewall, it needs to know its own IP address, the IP address of the DNS server, its own MAC address, as well as the MAC address of the DNS server (or that of the gateway if the server is outside of the local network). The MAC address of the device is obtained from the firewall. The IP address of the device, of the DNS server, and the MAC address of the server/gateway are obtained from DHCP and ARP, whose corresponding packets are also forwarded to the DNS compartment.

This resolver is fault-resilient, but will not recover from faults that happen in the initialization phase (when parsing the first DHCP and ARP messages). Supporting reset at this stage is left for future works.

Testing

I tested this with a variety of A, AAAA, and CNAME queries for various host names.

I also tested crashes in the firewall thread as well as in the user thread.

One command which I found useful to test this code:

arping -I ${interface used by the device} -c 1 -s ${IP of the gateway} -A ${IP of the device}

This generates an ARP packet that sets/changes the MAC address of the gateway. It is necessary to test the ARP-handling code if the DHCP server has the same IP as the DNS server (in which case the MAC address of the server will not be obtained through ARP).

@hlef hlef requested a review from davidchisnall November 10, 2024 06:42
@hlef hlef force-pushed the hlefeuvre/dns-compartment-for-pr branch 2 times, most recently from b668af6 to a41fe34 Compare November 10, 2024 06:53
Copy link
Contributor

@davidchisnall davidchisnall left a comment

Choose a reason for hiding this comment

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

Some early comments, not read everything yet.

lib/dns/dns.cc Show resolved Hide resolved
lib/dns/dns.cc Show resolved Hide resolved
lib/dns/dns.cc Show resolved Hide resolved
lib/dns/dns.cc Outdated Show resolved Hide resolved
Comment on lines +222 to +261
// Use the DNS server port to originate requests, as we are
// sure the TCP/IP stack won't use this one.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have something in the firewall that ensures this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We don't. But I suspect that we want a slightly different check. We don't want the network stack to talk to the DNS server ($DNS server IP : $DNS server port). And conversely we don't want the DNS resolver to do anything else than talking to the DNS server or doing ARP.

I think that this would be easy to add (we just have to check the ForwardFlags in packet_filter_egress), however we would need two versions of ethernet_send_frame, one for the TCP/IP stack and one for the DNS resolver.

Would that make sense? Then we don't have to worry about which port to use.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually I realize that this is harder to implement that I thought, because it is the driver that calls packet_filter_egress, at which point we do not know who (TCP/IP stack or DNS resolver) sent the frame.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Without this check, the TCP/IP stack can send a packet to the DNS server and impersonate the DNS resolver. But I do not think this has any sort of implication - the DNS resolver would simply receive unsolicited answers and drop them. Things are probably fine as they are now.

lib/dns/dns.cc Show resolved Hide resolved
@hlef
Copy link
Collaborator Author

hlef commented Nov 19, 2024

I will also add a comment regarding IEEE 802.1Q VLAN tagging support to the code to mention that we don't support it and where it should be added.

Edit: done.

@hlef
Copy link
Collaborator Author

hlef commented Nov 19, 2024

Some early comments, not read everything yet.

Thanks! I addressed what you pointed at for now 🙂

@hlef hlef force-pushed the hlefeuvre/dns-compartment-for-pr branch 3 times, most recently from bed74c1 to 68f6194 Compare November 19, 2024 22:55
hlef added 2 commits November 19, 2024 15:25
In the current implementation, the TCP/IP compartment is responsible for
DNS, which allows it to spoof translations to bypass connection
capabilities and create arbitrary firewall entries.

To prevent this, we must isolate the DNS resolver into its own
compartment that plugs directly at the firewall-level.

Since neither the FreeRTOS+TCP resolver nor existing resolver libraries
are meant to run directly at the Ethernet level, and most are too
heavyweight, we write our own one.

Similarly to the FreeRTOS+TCP resolver, this implementation is a stub
resolver: it only supports IPv4 recursive servers for A, AAAA, and CNAME
queries. This resolver also assumes that the recursive resolver recurses
into CNAME records.

The address of the DNS server is provided by the DHCP server. This may
be overly restrictive - we should evolve it in future PRs or eventually
replace it with something more featureful as the need arises.

Since the resolver plugs directly with the firewall, it needs to know
its own IP address, the IP address of the DNS server, its own MAC
address, as well as the MAC address of the DNS server (or that of the
gateway if the server is outside of the local network). The MAC address
of the device is obtained from the firewall. The IP address of the
device, of the DNS server, and the MAC address of the server/gateway are
obtained from DHCP and ARP, whose corresponding packets are also
forwarded to the DNS compartment.

Signed-off-by: Hugo Lefeuvre <[email protected]>
@hlef hlef force-pushed the hlefeuvre/dns-compartment-for-pr branch from 68f6194 to d18abd9 Compare November 19, 2024 23:25
@hlef hlef requested a review from davidchisnall November 19, 2024 23:33
@davidchisnall
Copy link
Contributor

What does this PR do to total code size for the network stack?

@hlef
Copy link
Collaborator Author

hlef commented Nov 21, 2024

What does this PR do to total code size for the network stack?

It increases the total binary size from 103.627kB to 104.572kB for the HTTP example (example 2). The size of the TCP/IP compartment shrunk from 50.8125kB to 46.8594kB, but we introduced a 4.77344kB DNS compartment. There is probably room for optimization 🙂

@hlef hlef merged commit f8dbfd9 into main Nov 22, 2024
2 checks passed
@hlef hlef deleted the hlefeuvre/dns-compartment-for-pr branch November 22, 2024 21:10
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.

2 participants