From 9b32d54dab94010ee48f6633c9f7745ddc342ed7 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Fri, 6 Mar 2026 12:21:24 -0800 Subject: [PATCH 1/3] ext/snmp: hardening: replace unbounded strcat with offset-tracked snprintf The suffix-as-keys code path in php_snmp builds a dotted OID suffix string by appending with strcat into a fixed 2048-byte buf2 array. The overflow is not reachable in practice: net-snmp's BER decoder enforces MAX_OID_LEN=128, and the worst-case suffix (128 subidentifiers at 11 bytes each) is 1408 bytes, within the buffer. Replace the strcat loop with direct snprintf into buf2 at a tracked size_t offset, breaking out of the loop if the buffer would be exhausted. Defense-in-depth against future net-snmp parser regressions. Signed-off-by: Thomas Vincent --- ext/snmp/snmp.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/ext/snmp/snmp.c b/ext/snmp/snmp.c index 0ff8d41c1e6ce..797ae011b7420 100644 --- a/ext/snmp/snmp.c +++ b/ext/snmp/snmp.c @@ -525,14 +525,21 @@ static void php_snmp_internal(INTERNAL_FUNCTION_PARAMETERS, int st, } else if (st & SNMP_USE_SUFFIX_AS_KEYS && st & SNMP_CMD_WALK) { snprint_objid(buf2, sizeof(buf2), vars->name, vars->name_length); if (rootlen <= vars->name_length && snmp_oid_compare(root, rootlen, vars->name, rootlen) == 0) { - buf2[0] = '\0'; + size_t pos = 0; count = rootlen; - while(count < vars->name_length){ - snprintf(buf, sizeof(buf), "%lu.", vars->name[count]); - strcat(buf2, buf); + while (count < vars->name_length) { + int written = snprintf(buf2 + pos, sizeof(buf2) - pos, "%lu.", vars->name[count]); + if (written < 0 || (size_t)written >= sizeof(buf2) - pos) { + break; + } + pos += (size_t)written; count++; } - buf2[strlen(buf2) - 1] = '\0'; /* remove trailing '.' */ + if (pos > 0) { + buf2[pos - 1] = '\0'; /* remove trailing '.' */ + } else { + buf2[0] = '\0'; + } } add_assoc_zval(return_value, buf2, &snmpval); } else { From 729e45be2fd250221ab0b1625ad668b09d4ec673 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Fri, 6 Mar 2026 12:25:32 -0800 Subject: [PATCH 2/3] ext/snmp: add test for GH-21341 suffix-as-keys snprintf loop Signed-off-by: Thomas Vincent --- ext/snmp/tests/gh21341.phpt | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) create mode 100644 ext/snmp/tests/gh21341.phpt diff --git a/ext/snmp/tests/gh21341.phpt b/ext/snmp/tests/gh21341.phpt new file mode 100644 index 0000000000000..2e4e3aac059eb --- /dev/null +++ b/ext/snmp/tests/gh21341.phpt @@ -0,0 +1,37 @@ +--TEST-- +GH-21341: suffix-as-keys walk uses offset-tracked snprintf (no strcat overflow) +--EXTENSIONS-- +snmp +--SKIPIF-- + +--FILE-- +walk('.1.3.6.1.2.1.1', /* suffix_as_keys */ true); + +var_dump(is_array($result)); +var_dump(count($result) > 0); + +/* Every key must be a bare dotted-numeric suffix, not a full OID. + * Full OIDs start with '.'; suffixes do not. */ +$bad = array_filter(array_keys($result), static function (string $k): bool { + return $k[0] === '.' || !preg_match('/^\d[\d.]*$/', $k); +}); +var_dump(count($bad) === 0); + +var_dump($session->close()); +?> +--EXPECT-- +bool(true) +bool(true) +bool(true) +bool(true) From f502e94e875ff52a4f4244ccb11f8993f3e2b828 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Fri, 6 Mar 2026 12:29:13 -0800 Subject: [PATCH 3/3] ext/snmp: improve GH-21341 test: wider walk subtree, trailing-dot check Signed-off-by: Thomas Vincent --- ext/snmp/tests/gh21341.phpt | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/ext/snmp/tests/gh21341.phpt b/ext/snmp/tests/gh21341.phpt index 2e4e3aac059eb..291c83e0633c6 100644 --- a/ext/snmp/tests/gh21341.phpt +++ b/ext/snmp/tests/gh21341.phpt @@ -12,11 +12,12 @@ if (getenv('SKIP_ASAN')) die('skip Timeouts under ASAN'); require_once(__DIR__.'/snmp_include.inc'); /* Exercise the suffix-as-keys code path (SNMP_USE_SUFFIX_AS_KEYS | - * SNMP_CMD_WALK). The walk OID is chosen so the returned subidentifiers - * form a multi-component suffix, driving multiple iterations of the - * snprintf loop that GH-21341 replaced the strcat loop with. */ + * SNMP_CMD_WALK). Walking .1.3.6.1.2.1 (mib-2) returns entries whose + * suffixes have multiple components (e.g. "1.1.0", "2.1.1.0"), driving + * several iterations of the snprintf loop that GH-21341 replaced the + * strcat loop with. */ $session = new SNMP(SNMP::VERSION_2c, $hostname, $community, $timeout, $retries); -$result = $session->walk('.1.3.6.1.2.1.1', /* suffix_as_keys */ true); +$result = $session->walk('.1.3.6.1.2.1', /* suffix_as_keys */ true); var_dump(is_array($result)); var_dump(count($result) > 0); @@ -28,6 +29,13 @@ $bad = array_filter(array_keys($result), static function (string $k): bool { }); var_dump(count($bad) === 0); +/* No key may end with '.': the trailing-dot removal (buf2[pos-1] = '\0') + * must fire correctly on every entry. */ +$trailing = array_filter(array_keys($result), static function (string $k): bool { + return $k !== '' && $k[-1] === '.'; +}); +var_dump(count($trailing) === 0); + var_dump($session->close()); ?> --EXPECT-- @@ -35,3 +43,4 @@ bool(true) bool(true) bool(true) bool(true) +bool(true)