pfSense

This bug was uncovered in resolving PR #2317
I don't know how pervasive it is or how to efficiently fix it.

BACKGROUND TO THE PR:

util.inc included many IP and network conversion functions. Many of these did no input sanitation or checking in the past. When IPv6 came along, many functions continued to call code that used 32-bit integer manipulation for IPv6 and on x64, which often fails. The code didn't usually detect these, and allowed bad data to be passed back as good data. For example, long2ip based functions often treated a return value of FALSE (signifying bad data) the same as zero (good data) as they didn't check for "===false" first; they just assumed the input was valid. Two reasons this was a problem:

1) conversion/sanitation functions shouldn't return values which make it impossible to distinguish good from invalid data, or which appears to be trustworthy but hasn't actually been checked;
2) past work in this area showed such issues had indeed occurred many times and caused code bugs in practice; it is unlikely all of these are found so far.

So the above PR updated three small IP conversion functions that converted (int)IPv4 to and from (string)IPv4.

EFFECT OF PR:

Chris (correctly) noticed that this broke range validation on the page services_dhcp.php and perhaps others, and asked that it not be resubmitted until fixed. He left it for me to look into as the author.

In fact what this commit revealed was the exact opposite of this initial conclusion. When I looked carefully, the patched code was doing precisely the right thing, that it was intended to do. It was trapping bad data which had previously gone un-noticed, and highlighted a bug in the calling code, which was leading to ip2long() being fed invalid data. So it was (correctly) returning "bad data" rather than allowing this and returning rubbish.

The specific bug which caused the issue Chris noticed, was this line in services_dhcp.php:

$subnet_end = ip2ulong(long2ip32(ip2long($ifcfgip) | (~gen_subnet_mask_long($ifcfgsn))));

Suppose we set a subnet of x.x.x.x/24. Then gen_subnet_mask_long($ifcfgsn) will return 0x000000FF. But the subnet end IP gets calculated using ~(netmask), i.e., ~ 0x000000FF.... which on x64 will be 0xFFFFFFFFFFFFFF00 NOT 0xFFFFFF00. That is why this tightened function began trapping and returning an error as Chris reported- the calling function wasn't in fact providing it with valid IPv4 IPs in the arguments to convert to/from dotted format, and this simply hadn't been detected before now.

The fix is to use the function gen_subnetv4_max() which already exists and works correctly, rather than ~$netmask but it's not clear how widely this problem exists or how to find the places it occurs efficiently. I'm not sure how I'd go about finding all incorrect IUPv4 calcs which might be sending bad data to these functions, if I were asked. So I'm posting it here to see what people think best.

One idea is to scan the codebase for ~$var which often means a subnet calc. Another might be to return the old value but log the error and ask testers to review and send in that log after a while to catch the "big ones". Ignoring it seems a bad idea since we know these calls are being mis-called and return rubbish on bad data rather than detecting it. What's best?