Fix GH-25137: Reject malformed SNMP ports

NEWS

@@ -145,6 +145,10 @@ PHP                                                                        NEWS
   . Fixed bug GH-12171 (SimpleXML property-proxy equality ignores element
     names). (Ilia Alshanetsky)
 
+- SNMP:
+  . Fixed bug GH-25137 (SNMP accepts malformed remote ports).
+    (somethingwithproof)
+
 - SOAP:
   . Fixed the SOAP client accepting a response body shorter than its
     Content-Length and reusing the connection. (Ilia Alshanetsky)

UPGRADING

@@ -44,6 +44,12 @@ PHP 8.7 UPGRADE NOTES
     ReflectionZendExtension, ReflectionAttribute and ReflectionConstant are
     now readonly.
 
+- SNMP:
+  . A remote port in the hostname must now be one or more decimal digits
+    with no other characters. An empty, nonnumeric or partially numeric
+    port, such as "host:", "host:abc" or "host:123abc", now throws a
+    ValueError instead of being read as 0 or as its leading digits.
+
 - SOAP:
   . SoapClient no longer follows an HTTP redirect to a different origin
     (scheme, host, or port) and throws a SoapFault instead. Previously the

ext/snmp/snmp.c

@@ -843,6 +843,30 @@ static bool php_snmp_parse_oid(
 }
 /* }}} */
 
+/* Parse a remote port: one or more decimal digits, nothing else, at most USHRT_MAX. */
+static bool snmp_parse_port(const char *port_string, unsigned short *port)
+{
+	unsigned int value = 0;
+
+	if (*port_string == '\0') {
+		return false;
+	}
+
+	do {
+		if (!ZEND_IS_DIGIT(*port_string)) {
+			return false;
+		}
+
+		value = value * 10 + (unsigned int) (*port_string - '0');
+		if (value > USHRT_MAX) {
+			return false;
+		}
+	} while (*++port_string != '\0');
+
+	*port = (unsigned short) value;
+	return true;
+}
+
 /* {{{ snmp_session_init
 	allocates memory for session and session->peername, caller should free it manually using snmp_session_free() and efree()
 */
@@ -884,7 +908,6 @@ static bool snmp_session_init(php_snmp_session **session_p, int version, zend_st
 
 	// TODO: Do not strip and re-add the port in peername?
 	unsigned short remote_port = SNMP_PORT;
-	int tmp_port;
 
 	*session_p = (php_snmp_session *)emalloc(sizeof(php_snmp_session));
 	session = *session_p;
@@ -905,13 +928,10 @@ static bool snmp_session_init(php_snmp_session **session_p, int version, zend_st
 		host_ptr++;
 		if ((pptr = strchr(host_ptr, ']'))) {
 			if (pptr[1] == ':') {
-				char *pport = pptr + 2;
-				tmp_port = atoi(pport);
-				if (tmp_port < 0 || tmp_port > USHRT_MAX) {
+				if (!snmp_parse_port(pptr + 2, &remote_port)) {
 					zend_argument_value_error(hostname_arg_num, "remote port must be between 0 and %u", USHRT_MAX);
 					return false;
 				}
-				remote_port = (unsigned short)tmp_port;
 			}
 			*pptr = '\0';
 		} else {
@@ -920,13 +940,10 @@ static bool snmp_session_init(php_snmp_session **session_p, int version, zend_st
 		}
 	} else { /* IPv4 address */
 		if ((pptr = strchr(host_ptr, ':'))) {
-			char *pport = pptr + 1;
-			tmp_port = atoi(pport);
-			if (tmp_port < 0 || tmp_port > USHRT_MAX) {
+			if (!snmp_parse_port(pptr + 1, &remote_port)) {
 				zend_argument_value_error(hostname_arg_num, "remote port must be between 0 and %u", USHRT_MAX);
 				return false;
 			}
-			remote_port = (unsigned short)tmp_port;
 			*pptr = '\0';
 		}
 	}

ext/snmp/tests/gh25137.phpt

@@ -0,0 +1,49 @@
+--TEST--
+GH-25137 (SNMP accepts malformed remote ports)
+--EXTENSIONS--
+snmp
+--FILE--
+<?php
+
+$hostnames = [
+	'127.0.0.1:',
+	'127.0.0.1:not-a-port',
+	'127.0.0.1:123junk',
+	'127.0.0.1:-1',
+	'127.0.0.1:65536',
+	'127.0.0.1:99999999999999999999',
+	'[::1]:',
+	'[::1]:123junk',
+];
+
+foreach ($hostnames as $hostname) {
+	try {
+		new SNMP(SNMP::VERSION_1, $hostname, 'public');
+	} catch (Throwable $e) {
+		echo $e::class, ': ', $e->getMessage(), PHP_EOL;
+	}
+}
+
+foreach (['127.0.0.1:0', '127.0.0.1:65535'] as $hostname) {
+	var_dump(new SNMP(SNMP::VERSION_1, $hostname, 'public') instanceof SNMP);
+}
+
+try {
+	snmpget('127.0.0.1:not-a-port', 'public', '1.3.6.1.2.1.1.1.0');
+} catch (Throwable $e) {
+	echo $e::class, ': ', $e->getMessage(), PHP_EOL;
+}
+
+?>
+--EXPECT--
+ValueError: SNMP::__construct(): Argument #2 ($hostname) remote port must be between 0 and 65535
+ValueError: SNMP::__construct(): Argument #2 ($hostname) remote port must be between 0 and 65535
+ValueError: SNMP::__construct(): Argument #2 ($hostname) remote port must be between 0 and 65535
+ValueError: SNMP::__construct(): Argument #2 ($hostname) remote port must be between 0 and 65535
+ValueError: SNMP::__construct(): Argument #2 ($hostname) remote port must be between 0 and 65535
+ValueError: SNMP::__construct(): Argument #2 ($hostname) remote port must be between 0 and 65535
+ValueError: SNMP::__construct(): Argument #2 ($hostname) remote port must be between 0 and 65535
+ValueError: SNMP::__construct(): Argument #2 ($hostname) remote port must be between 0 and 65535
+bool(true)
+bool(true)
+ValueError: snmpget(): Argument #1 ($hostname) remote port must be between 0 and 65535