From 6e7a24246639382846bd717300d9927169d85ae2 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Fri, 6 Mar 2026 12:21:24 -0800 Subject: [PATCH 1/4] 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 0c0607392366..79c58987b8c7 100644 --- a/ext/snmp/snmp.c +++ b/ext/snmp/snmp.c @@ -535,14 +535,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 0374d7380eadb991c6e89ebe139a0acda30dee1b Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Fri, 6 Mar 2026 12:25:32 -0800 Subject: [PATCH 2/4] 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 000000000000..2e4e3aac059e --- /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 24e167a76e4f0374c4871ff4718cc8108ab1ca0c Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Fri, 6 Mar 2026 12:29:13 -0800 Subject: [PATCH 3/4] 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 2e4e3aac059e..291c83e0633c 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) From b13d9f8da67893ce9115334e5053e3dd99773873 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Mon, 26 Oct 2026 15:18:36 -0700 Subject: [PATCH 4/4] ext/snmp: drop ineffective suffix walk regression test Signed-off-by: Thomas Vincent --- ext/snmp/tests/gh21341.phpt | 46 ------------------------------------- 1 file changed, 46 deletions(-) delete mode 100644 ext/snmp/tests/gh21341.phpt diff --git a/ext/snmp/tests/gh21341.phpt b/ext/snmp/tests/gh21341.phpt deleted file mode 100644 index 291c83e0633c..000000000000 --- a/ext/snmp/tests/gh21341.phpt +++ /dev/null @@ -1,46 +0,0 @@ ---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', /* 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); - -/* 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-- -bool(true) -bool(true) -bool(true) -bool(true) -bool(true)