pi-hole/web

savesettings.php doesn't properly handle MAC-whitelisted DHCP pools

HealsCodes opened this issue · 6 comments

HealsCodes commented

Versions

  • Pi-hole: v5.18.3
  • AdminLTE: 5.21
  • FTL: 5.25.2

Platform

  • OS and version: Arch Linux
  • Platform: Docker

Expected behavior

A user should be allowed to use the special keyword static as value for the "To" field of the configured DHCP range on the DHCP-Settings page.

Reason why this should be allowed

dnsmasq (and pihole-FTL) support static DHCP pools for which only clients that have a defined MAC/IP pair configured will receive a DHCPOFFER.

All unknown clients are rejected.

A pool like this is configured using the IPv4 base address as DHCP-Start and the keyword static as DHCP-End like so:

dhcp-range:192.168.178.1,static,24h

Actual behavior / bug

The web interface refuses static as a valid To-IP-address and reports "To IP ('static') is invalid!".

Steps to reproduce

Steps to reproduce the behavior:

  1. Got to http://pi.hole and log in
  2. Click on 'Settings' in the left side-bar
  3. Click on the 'DHCP' tab
  4. Check the box for 'DHCP Enabled'
  5. Enter a valid IP into the "From" Textfield (e.g. 192.168.178.1)
  6. Enter the keyword static into the "To" Textfield
  7. Click the 'Save'-Button at the bottom of the page
  8. The error message "To IP ('static') is invalid!" is displayed at the top of the page.

Debug Token

  • URL: --

Screenshots

Additional context

The error is generated by the check in savesettings.php:495 which - plausibly - checks for a valid IP address.
However in this special case the check should also accept the keyword static, specifically only for the TO: field.
I tried to extend the check in my install to read if (!validIP($to) and $to != 'static') { and pi-hole generates a valid dnsmasq / pihole-FTL configuration that serves only known MAC-Addresses.

yubiuser commented

All unknown clients are rejected.

Pi-hole v6 will offer such a feature
2024-09-15_12-46

HealsCodes commented

Pi-hole v6 will offer such a feature

That feature is sadly not the same as what I describe - having a pool defined with static allows an advanced user to also add additional non-static pools for clients that do not have a known MAC address.

My configuration pre-pi-hole looks something like this (ranges changed):

# only known MAC addresses get access to the real local net
dhcp-range=192.168.20.1,static,24h

# honeypot for unkown devices (same people just can't help plugging in random LAN devices..)
dhcp-range=set:tag1,192.168.178.200,192.168.178.210,255.255.0.0,1h
dhcp-option=tag:tag1,option:router,192.168.178.1
dhcp-option=tag:tag1,option:netmask,255.255.255.255
dhcp-option=tag:tag1,6,192.168.178.1

Here all known client that are allowed to be on the network get DHCP IP addresses based on der MAC.

Any unknown clients - e.G. devices plugged into LAN ports that people shouldn't plug in - get handled by a honeypot DHCP-pool.

pihole-FTL supports this kind of setup without issue, but since I'm not allowed to use the static keyword "To" part of my DHCP-Rang in the WebUI I'm forced to either manually path one line in savesettings.php or completely give up managing my known-goood clients via the webui.

yubiuser commented

Ok, I see.

Then you might workaround this by using those two options where you either enabling loading your custom config from /etc/dnsmas.d or enter it line-by-line in the web interface.

2024-09-15_15-01

If this is not want you want, consider it a feature request which should be opened at https://discourse.pi-hole.net/c/feature-requests/8

HealsCodes commented

Thanks for showing these - I'm already using additional files in me /etc/dnsmasq.d/ right now :)

I'd have been happy to just create a PR it's literally a very specific one-line change that allows the web-ui to accept a valid input which it currently - falsely - rejects as "invalid" - which feels more like a bug in the web-ui's input validation than a feature request.

yubiuser commented

Feel free to submit a PR, but note that all PRs should be based on and targeting the development branch. Also note: there are many, many changes between v5 and v6 (currently in development branch)

  • 👍1
HealsCodes commented

Thank you, I appreciate the feedback - v6 sounds like it has a lot of improvements already so maybe it's smarter to wait for it to release and test it properly.

If anything I can still open a feature-request or submit a PR for it afterwards 👍

  • 👍1