Bug #14702
closed
``ctype_digit()`` returns unexpected result for values <= ``255`` which can break some validation functions/usages
100%
Description
Hello pfSense,
I've noticed that when you create a NAT rule with a port range starting with 20 (e.g. 20-21 or 20-28, etc.) it will erroneously create a corresponding Firewall rule that allows all ports. Please see steps to reproduce and screenshots below for details.
NOTE: This was working in pfsense 2.6 CE, but broke when I upgraded to 23.05.1 plus edition. I can also confirm this bug on another device runing 23.05.01 plus that was previously upgraded from 2.7 CE.
Steps to reproduce:
1. Create a new NAT rule with port range 20 to 21 and target port of 20 (see 1st screenshot below)
2. Ensure "add associated filter rule" is selected at the bottom
3. Save rule and navigate to Firewall rules
4. Expected: A new Firewall rule is generated with a destination port range of 20-21
5. Actual: A new Firewall rule is generated with a destination port of ALL ports! (see 2nd screenshot below)
Files
Updated by Jim Pingle 9 months ago
- Project changed from pfSense Plus to pfSense
- Subject changed from NAT Rules Created With A Port Range Starting With Port 20 Generates Incorrect Firewall Rules to Associated firewall rule for a NAT port forward range involving certain low port ranges does not contain proper port restrictions
- Category changed from Rules / NAT to Rules / NAT
- Status changed from New to Confirmed
- Assignee set to Jim Pingle
- Target version set to 2.8.0
- Affected Plus Version deleted (
23.05.1) - Plus Target Version set to 23.09
This isn't specific to FTP, it happens for a few different ranges I tried (10-11, 20-21, 100-101, etc.) though it doesn't seem to happen for higher ranges (At least >400).
The values are being passed but something in the port calculation is off, like maybe somehow it's failing an is_port() test unexpectedly.
Updated by Jim Pingle 9 months ago
The problem is with the ctype_digit() test used in is_port():
https://www.php.net/manual/en/function.ctype-digit.php
Note:
If an int between -128 and 255 inclusive is provided, it is interpreted as the ASCII value of a single character (negative values have 256 added in order to allow characters in the Extended ASCII range). Any other integer is interpreted as a string containing the decimal digits of the integer.Warning
As of PHP 8.1.0, passing a non-string argument is deprecated. In the future, the argument will be interpreted as a string instead of an ASCII codepoint. Depending on the intended behavior, the argument should either be cast to string or an explicit call to chr() should be made.
Updated by Jim Pingle 9 months ago
- Subject changed from Associated firewall rule for a NAT port forward range involving certain low port ranges does not contain proper port restrictions to ``ctype_digit()`` returns unexpected result for values <= ``255`` which can break some validation functions/usages
- Status changed from Confirmed to In Progress
Looks like this could also break things in a few other places since we use that function ~10 times in various files.
Adding a strval() around the parameter seems to make it work as we want, so ctype_digit(strval($whatever)).
Fix coming shortly.
Updated by Jim Pingle 9 months ago
- Status changed from In Progress to Feedback
- % Done changed from 0 to 100
Applied in changeset 28e2b61100b0f1cf81de5e73fd579bb6bd36afb5.
Updated by Danilo Zrenjanin 9 months ago
- Status changed from Feedback to Resolved
The patch fixes it.
I am marking this ticket resolved.