Fix #16944: Invalid filtering of IPv6 with FILTER_FLAG_NO_RES_RANGE #17111
Conversation
|
/cc @nielsdos @nicolas-grekas — It makes most sense to review this commit-by-commit. |
derickr
force-pushed
the
fix-ip-ranges
branch
from
1096e89 to
0a5aab6
Compare
| *private = false; | ||
|
|
||
| if (ip[0] == 0) { | ||
| /* RFC 1122 - This host on this network */ |
There was a problem hiding this comment.
The errata for RFC6890 changes this, see https://www.rfc-editor.org/errata_search.php?rfc=6890&rec_status=1&presentation=records
Should instead be: /* RFC 0791 - This network */.
There was a problem hiding this comment.
It actually splits up the record into two, so I've made this change instead:
}
/* }}} */
-/* From the tables in RFC 6890 - Special-Purpose IP Address Registries */
-static bool ipv4_get_status_flags(int ip[8], bool *global, bool *reserved, bool *private)
+/* From the tables in RFC 6890 - Special-Purpose IP Address Registriesi
+ * Including errata: https://www.rfc-editor.org/errata_search.php?rfc=6890&rec_status=1 */
+static bool ipv4_get_status_flags(const int ip[8], bool *global, bool *reserved, bool *private)
{
*global = false;
*reserved = false;
*private = false;
if (ip[0] == 0) {
+ /* RFC 0791 - This network */
+ *reserved = true;
+ } else if (ip[0] == 0 && ip[1] == 0 && ip[2] == 0 && ip[3] == 0) {
/* RFC 1122 - This host on this network */
*reserved = true;
} else if (ip[0] == 10) {
There was a problem hiding this comment.
Seems good to me. The compiler will optimize that anyway so it's okay to be verbose 🙂
ext/filter/logical_filters.c
Outdated
| *private = true; | ||
| } else if (ip[0] == 192 && ip[1] == 0 && ip[2] == 0) { | ||
| /* RFC 6890 - IETF Protocol Assignments */ | ||
| } else if (ip[0] == 192 && ip[1] >= 0 && ip[1] <= 31) { |
There was a problem hiding this comment.
The range is 192.0.0.0/29. So this seems wrong because it is 192.0.0.0 until 192.0.0.7, because only the 3 bottom bits are free to use. So both the range and indices of the ip in this check are not correct.
There was a problem hiding this comment.
Thanks! It looks like the original code didn't even check for this one.
ext/filter/logical_filters.c
Outdated
| @@ -869,6 +869,106 @@ static int _php_filter_validate_ipv6(const char *str, size_t str_len, int ip[8]) | |||
| } | |||
| /* }}} */ | |||
|
|
|||
| /* From the tables in RFC 6890 - Special-Purpose IP Address Registries */ | |||
| static bool ipv4_get_status_flags(int ip[8], bool *global, bool *reserved, bool *private) | |||
There was a problem hiding this comment.
Could be const int ip[8] in theory, but doesn't matter much.
|
Also note that this should target branch PHP-8.3, see https://externals.io/message/125995 |
ext/filter/logical_filters.c
Outdated
| *private = true; | ||
| } else if (ip[0] == 192 && ip[1] == 0 && ip[2] == 0) { | ||
| /* RFC 6890 - IETF Protocol Assignments */ | ||
| } else if (ip[0] == 192 && ip[1] >= 0 && ip[1] <= 31) { |
There was a problem hiding this comment.
Thanks! It looks like the original code didn't even check for this one.
| *private = false; | ||
|
|
||
| if (ip[0] == 0) { | ||
| /* RFC 1122 - This host on this network */ |
There was a problem hiding this comment.
It actually splits up the record into two, so I've made this change instead:
}
/* }}} */
-/* From the tables in RFC 6890 - Special-Purpose IP Address Registries */
-static bool ipv4_get_status_flags(int ip[8], bool *global, bool *reserved, bool *private)
+/* From the tables in RFC 6890 - Special-Purpose IP Address Registriesi
+ * Including errata: https://www.rfc-editor.org/errata_search.php?rfc=6890&rec_status=1 */
+static bool ipv4_get_status_flags(const int ip[8], bool *global, bool *reserved, bool *private)
{
*global = false;
*reserved = false;
*private = false;
if (ip[0] == 0) {
+ /* RFC 0791 - This network */
+ *reserved = true;
+ } else if (ip[0] == 0 && ip[1] == 0 && ip[2] == 0 && ip[3] == 0) {
/* RFC 1122 - This host on this network */
*reserved = true;
} else if (ip[0] == 10) {
ext/filter/logical_filters.c
Outdated
| /* RFC 1122 - Reserved */ | ||
| *reserved = true; | ||
| } else if (ip[0] == 255 && ip[1] == 255 && ip[2] == 255 && ip[3] == 255) { | ||
| /* RFC 0919 - Limited Broadcast */ |
There was a problem hiding this comment.
I did another pass over the PR, and I only have one question for this line here.
RFC8190 updates some parts of RFC6890, but the only change relevant to this RP seems to be "2.2. Updates to the IPv4 Special-Purpose Address Registry". It states:
Limited Broadcast prefix (255.255.255.255/32) - The Reserved-by-
Protocol value has changed from False to True. This change was
made to align the registry with reservation of the limited
broadcast address with Section 7 of [RFC919].
So this likely now needs to set reserved to true.
…turned to IANA (RFC 3701)
derickr
force-pushed
the
fix-ip-ranges
branch
from
103c9ae to
50f99af
Compare
It also refactors the code to use the actual tables from RFC 6890, and fixed other bugs with these ranges that were introduced with #7893