DHCP Server on 1.2.3 complaining that a valid MAC address is invalid

bblacey

I am trying to add a static mapping for an Onkyo receiver on my home network. I copied the mac address from the DHCP leases page and pasted it into the field mac address field on the "Services:DHCP:Edit Static Mapping" page. However, when I click save, I get an error "A valid Mac address must be specified". I even tried changing the lower case hex letter to uppercase but to no avail… Ideas?

wallabybob

Have you inadvertently copied some formatting information (e.g. leading or trailing spaces) with the MAC address?

bblacey

No but I restarted pfsense after having not done so in months and it worked after reboot. Hmmmmm….

cmb

@bblacey:

No but I restarted pfsense after having not done so in months and it worked after reboot. Hmmmmm….

No, it worked because you entered it correctly after the reboot and not before. :) That simply checks the formatting for a legit MAC, there's no way rebooting changes that. Pretty sure it doesn't strip leading or trailing spaces so that may be enough for it to be wrong.

bblacey

So the MAC check is purely formatting and has nothing to do with the state of the dhcp leases cache? Instead of using the plus sign on the leases page to change a dynamic lease to a static lease, I tried to enter a MAC address for an existing dynamic lease into the add static lease page/form. I am positive I entered the same MAC address before and after reboot so figured it had something to do with the dhcp lease state…

cmb

@bblacey:

So the MAC check is purely formatting and has nothing to do with the state of the dhcp leases cache?

Correct. That page uses:

	if (($_POST['mac'] && !is_macaddr($_POST['mac']))) {
		$input_errors[] = "A valid MAC address must be specified.";
	}

with is_macaddr being:

/* returns true if $macaddr is a valid MAC address */
function is_macaddr($macaddr) {
	if (!is_string($macaddr))
		return false;

	$maca = explode(":", $macaddr);
	if (count($maca) != 6)
		return false;

	foreach ($maca as $macel) {
		if (($macel === "") || (strlen($macel) > 2))
			return false;
		if (preg_match("/[^0-9a-f]/i", $macel))
			return false;
	}

	return true;
}

So if it contains anything other than 0-9 and a-f (after removing colons), it fails, if it's not the right length or format (two digits between each : ) it fails.

bblacey

Chris,

Ok, fair enough so I must be losing my mind ;)

I looked at the code you provided - are you sure that you want the regex carat inside the open bracket and not before when checking the tuples returned from explode()? I'm wondering if the tuple check should be ^[0-9a-f][0-9a-f] requiring two hexadecimal characters starting with a hex character or even expressed as ^[0-9a-f]{2} because preg_match() will return 1 on the first match and it looks like the current regex is only checking the first character???

All of that said, and for what it is worth, the following will provide a strict MAC address check with fewer lines of code and should be faster because it performs a single regex call instead of six plus a call to explode() and is_string().


/* returns true if $macaddr is a valid MAC address */
function is_macaddr($macaddr) {
        return preg_match('/^[0-9A-F]{2}(?=([:]?))(?:\\1[0-9A-F]{2}){5}$/i', $macaddr);
}

There is no need for the is_string() check because preg_match() will simply return 0 (no match) if you pass a non-string mac address as the argument.

I'm no php expert, but I think zero is treated as false and any non-zero value is treated as true so the above should work fine but if you want to ensure that only true or false is retuned, the following simple extension will do it.


/* returns true if $macaddr is a valid MAC address */
function is_macaddr($macaddr) {
        return preg_match('/^[0-9A-F]{2}(?=([:]?))(?:\\1[0-9A-F]{2}){5}$/i', $macaddr) == 1 ? true : false;
}

Finally, you can even extend the (?=) lookahead expression to support delimiters other than ':' such as '-' if desired.

Cheers,
Bruce

cmb

Wow, I'm not sure anyone has put that much thought into it. ;D It came from m0n0wall as is, and it does correctly validate, though the one liner you provided does as well so I replaced it with that instead.
https://rcs.pfsense.org/projects/pfsense/repos/mainline/commits/cb847796df756202c4667b68c081a9766f0ce736

thanks!

bblacey

@cmb:

Wow, I'm not sure anyone has put that much thought into it.

You know the old saying, "If I had more time, I would write less code." ;)