Skip to content

ext/snmp: various internals rewrite. #17368

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 10 commits into from
Closed

ext/snmp: various internals rewrite. #17368

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
d3d8859
ext/snmp: various internals rewrite.
devnexen Jan 5, 2025
29d56a5
add test
devnexen Jan 5, 2025
dc963f3
to exception
devnexen Jan 5, 2025
8dfc82d
further initialisation type check and segfault fix.
devnexen Jan 25, 2025
305631d
revert renaming
devnexen Jan 26, 2025
4d076cf
changes from review
devnexen Jan 26, 2025
051a82a
renaming
devnexen Jan 26, 2025
b466f12
update comment
devnexen Jan 26, 2025
0fead78
last remaining changes.
devnexen Jan 26, 2025
dc02a8f
fix test
devnexen Jan 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
101 changes: 66 additions & 35 deletions ext/snmp/snmp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ static PHP_GINIT_FUNCTION(snmp)
} \
}

static void netsnmp_session_free(php_snmp_session **session) /* {{{ */
static void snmp_session_free(php_snmp_session **session) /* {{{ */
{
if (*session) {
PHP_SNMP_SESSION_FREE(peername);
Expand All @@ -174,7 +174,7 @@ static void php_snmp_object_free_storage(zend_object *object) /* {{{ */
return;
}

netsnmp_session_free(&(intern->session));
snmp_session_free(&(intern->session));

zend_object_std_dtor(&intern->zo);
}
Expand Down Expand Up @@ -829,31 +829,50 @@ static bool php_snmp_parse_oid(
}
/* }}} */

/* {{{ netsnmp_session_init
allocates memory for session and session->peername, caller should free it manually using netsnmp_session_free() and efree()
/* {{{ snmp_session_init
allocates memory for session and session->peername, caller should free it manually using session_free() and efree()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
allocates memory for session and session->peername, caller should free it manually using session_free() and efree()
allocates memory for session and session->peername, caller should free it manually using snmp_session_free() and efree()

*/
static bool netsnmp_session_init(php_snmp_session **session_p, int version, zend_string *hostname, zend_string *community, int timeout, int retries)
static bool snmp_session_init(php_snmp_session **session_p, int version, zend_string *hostname, zend_string *community, zend_long timeout, zend_long retries)
{
php_snmp_session *session;
char *pptr, *host_ptr;
bool force_ipv6 = false;
int n;
struct sockaddr **psal;
struct sockaddr **res;

*session_p = 0;

if (ZSTR_LEN(hostname) >= MAX_NAME_LEN) {
zend_value_error("hostname length must be lower than %d", MAX_NAME_LEN);
return false;
}

if (timeout < -1 || timeout > LONG_MAX) {
zend_value_error("timeout must be between -1 and %ld", LONG_MAX);
return false;
}

if (retries < -1 || retries > INT_MAX) {
zend_value_error("retries must be between -1 and %d", INT_MAX);
return false;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we pass the arg_nums to these parameters? Does the community zend_string need to be checked too? Is Hostname checked to not contain null bytes prior to calling this function? If yes please add an assertion.


// TODO: Do not strip and re-add the port in peername?
unsigned remote_port = SNMP_PORT;
unsigned short remote_port = SNMP_PORT;
int tmp_port;

*session_p = (php_snmp_session *)emalloc(sizeof(php_snmp_session));
session = *session_p;
memset(session, 0, sizeof(php_snmp_session));

snmp_sess_init(session);

session->version = version;
session->version = (long)version;

session->peername = emalloc(MAX_NAME_LEN);
/* we copy original hostname for further processing */
strlcpy(session->peername, ZSTR_VAL(hostname), MAX_NAME_LEN);
memcpy(session->peername, ZSTR_VAL(hostname), ZSTR_LEN(hostname) + 1);
host_ptr = session->peername;

/* Reading the hostname and its optional non-default port number */
Expand All @@ -862,7 +881,13 @@ static bool netsnmp_session_init(php_snmp_session **session_p, int version, zend
host_ptr++;
if ((pptr = strchr(host_ptr, ']'))) {
if (pptr[1] == ':') {
remote_port = atoi(pptr + 2);
char *pport = pptr + 2;
tmp_port = atoi(pport);
if (tmp_port < 0 || tmp_port > USHRT_MAX) {
zend_value_error("remote port must be between 0 and %u", USHRT_MAX);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use the argument version for it and use the host arg number, as the error message does not point where the error is.

return false;
}
remote_port = (unsigned short)tmp_port;
}
*pptr = '\0';
} else {
Expand All @@ -871,7 +896,13 @@ static bool netsnmp_session_init(php_snmp_session **session_p, int version, zend
}
} else { /* IPv4 address */
if ((pptr = strchr(host_ptr, ':'))) {
remote_port = atoi(pptr + 1);
char *pport = pptr + 1;
tmp_port = atoi(pport);
if (tmp_port < 0 || tmp_port > USHRT_MAX) {
zend_value_error("remote port must be between 0 and %u", USHRT_MAX);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

return false;
}
remote_port = (unsigned short)tmp_port;
*pptr = '\0';
}
}
Expand Down Expand Up @@ -920,7 +951,7 @@ static bool netsnmp_session_init(php_snmp_session **session_p, int version, zend
if (remote_port != SNMP_PORT) {
size_t peername_length = strlen(session->peername);
pptr = session->peername + peername_length;
snprintf(pptr, MAX_NAME_LEN - peername_length, ":%d", remote_port);
snprintf(pptr, MAX_NAME_LEN - peername_length, ":%u", remote_port);
}

php_network_freeaddresses(psal);
Expand All @@ -935,14 +966,14 @@ static bool netsnmp_session_init(php_snmp_session **session_p, int version, zend
session->community_len = ZSTR_LEN(community);
}

session->retries = retries;
session->timeout = timeout;
session->retries = (int)retries;
session->timeout = (long)timeout;
return true;
}
/* }}} */

/* {{{ Set the security level in the snmpv3 session */
static bool netsnmp_session_set_sec_level(struct snmp_session *s, zend_string *level)
static bool snmp_session_set_sec_level(struct snmp_session *s, zend_string *level)
{
if (zend_string_equals_literal_ci(level, "noAuthNoPriv") || zend_string_equals_literal_ci(level, "nanp")) {
s->securityLevel = SNMP_SEC_LEVEL_NOAUTH;
Expand All @@ -959,7 +990,7 @@ static bool netsnmp_session_set_sec_level(struct snmp_session *s, zend_string *l
/* }}} */

/* {{{ Set the authentication protocol in the snmpv3 session */
static bool netsnmp_session_set_auth_protocol(struct snmp_session *s, zend_string *prot)
static bool snmp_session_set_auth_protocol(struct snmp_session *s, zend_string *prot)
{
#ifndef DISABLE_MD5
if (zend_string_equals_literal_ci(prot, "MD5")) {
Expand Down Expand Up @@ -1011,7 +1042,7 @@ static bool netsnmp_session_set_auth_protocol(struct snmp_session *s, zend_strin
/* }}} */

/* {{{ Set the security protocol in the snmpv3 session */
static bool netsnmp_session_set_sec_protocol(struct snmp_session *s, zend_string *prot)
static bool snmp_session_set_sec_protocol(struct snmp_session *s, zend_string *prot)
{
#ifndef NETSNMP_DISABLE_DES
if (zend_string_equals_literal_ci(prot, "DES")) {
Expand Down Expand Up @@ -1048,7 +1079,7 @@ static bool netsnmp_session_set_sec_protocol(struct snmp_session *s, zend_string
/* }}} */

/* {{{ Make key from pass phrase in the snmpv3 session */
static bool netsnmp_session_gen_auth_key(struct snmp_session *s, zend_string *pass)
static bool snmp_session_gen_auth_key(struct snmp_session *s, zend_string *pass)
{
int snmp_errno;
s->securityAuthKeyLen = USM_AUTH_KU_LEN;
Expand All @@ -1063,7 +1094,7 @@ static bool netsnmp_session_gen_auth_key(struct snmp_session *s, zend_string *pa
/* }}} */

/* {{{ Make key from pass phrase in the snmpv3 session */
static bool netsnmp_session_gen_sec_key(struct snmp_session *s, zend_string *pass)
static bool snmp_session_gen_sec_key(struct snmp_session *s, zend_string *pass)
{
int snmp_errno;

Expand All @@ -1079,7 +1110,7 @@ static bool netsnmp_session_gen_sec_key(struct snmp_session *s, zend_string *pas
/* }}} */

/* {{{ Set context Engine Id in the snmpv3 session */
static bool netsnmp_session_set_contextEngineID(struct snmp_session *s, zend_string * contextEngineID)
static bool snmp_session_set_contextEngineID(struct snmp_session *s, zend_string * contextEngineID)
{
size_t ebuf_len = 32, eout_len = 0;
uint8_t *ebuf = (uint8_t *) emalloc(ebuf_len);
Expand All @@ -1102,40 +1133,40 @@ static bool netsnmp_session_set_contextEngineID(struct snmp_session *s, zend_str
/* }}} */

/* {{{ Set all snmpv3-related security options */
static bool netsnmp_session_set_security(struct snmp_session *session, zend_string *sec_level,
static bool snmp_session_set_security(struct snmp_session *session, zend_string *sec_level,
zend_string *auth_protocol, zend_string *auth_passphrase, zend_string *priv_protocol,
zend_string *priv_passphrase, zend_string *contextName, zend_string *contextEngineID)
{

/* Setting the security level. */
if (!netsnmp_session_set_sec_level(session, sec_level)) {
if (!snmp_session_set_sec_level(session, sec_level)) {
/* ValueError already generated, just bail out */
return false;
}

if (session->securityLevel == SNMP_SEC_LEVEL_AUTHNOPRIV || session->securityLevel == SNMP_SEC_LEVEL_AUTHPRIV) {

/* Setting the authentication protocol. */
if (!netsnmp_session_set_auth_protocol(session, auth_protocol)) {
if (!snmp_session_set_auth_protocol(session, auth_protocol)) {
/* ValueError already generated, just bail out */
return false;
}

/* Setting the authentication passphrase. */
if (!netsnmp_session_gen_auth_key(session, auth_passphrase)) {
if (!snmp_session_gen_auth_key(session, auth_passphrase)) {
/* Warning message sent already, just bail out */
return false;
}

if (session->securityLevel == SNMP_SEC_LEVEL_AUTHPRIV) {
/* Setting the security protocol. */
if (!netsnmp_session_set_sec_protocol(session, priv_protocol)) {
if (!snmp_session_set_sec_protocol(session, priv_protocol)) {
/* ValueError already generated, just bail out */
return false;
}

/* Setting the security protocol passphrase. */
if (!netsnmp_session_gen_sec_key(session, priv_passphrase)) {
if (!snmp_session_gen_sec_key(session, priv_passphrase)) {
/* Warning message sent already, just bail out */
return false;
}
Expand All @@ -1149,7 +1180,7 @@ static bool netsnmp_session_set_security(struct snmp_session *session, zend_stri
}

/* Setting contextEngineIS if specified */
if (contextEngineID && ZSTR_LEN(contextEngineID) && !netsnmp_session_set_contextEngineID(session, contextEngineID)) {
if (contextEngineID && ZSTR_LEN(contextEngineID) && !snmp_session_set_contextEngineID(session, contextEngineID)) {
/* Warning message sent already, just bail out */
return false;
}
Expand Down Expand Up @@ -1289,14 +1320,14 @@ static void php_snmp(INTERNAL_FUNCTION_PARAMETERS, int st, int version)
}

if (session_less_mode) {
if (!netsnmp_session_init(&session, version, a1, a2, timeout, retries)) {
if (!snmp_session_init(&session, version, a1, a2, timeout, retries)) {
php_free_objid_query(&objid_query, oid_ht, value_ht, st);
netsnmp_session_free(&session);
snmp_session_free(&session);
RETURN_FALSE;
}
if (version == SNMP_VERSION_3 && !netsnmp_session_set_security(session, a3, a4, a5, a6, a7, NULL, NULL)) {
if (version == SNMP_VERSION_3 && !snmp_session_set_security(session, a3, a4, a5, a6, a7, NULL, NULL)) {
php_free_objid_query(&objid_query, oid_ht, value_ht, st);
netsnmp_session_free(&session);
snmp_session_free(&session);
/* Warning message sent already, just bail out */
RETURN_FALSE;
}
Expand Down Expand Up @@ -1335,7 +1366,7 @@ static void php_snmp(INTERNAL_FUNCTION_PARAMETERS, int st, int version)
php_free_objid_query(&objid_query, oid_ht, value_ht, st);

if (session_less_mode) {
netsnmp_session_free(&session);
snmp_session_free(&session);
} else {
netsnmp_ds_set_boolean(NETSNMP_DS_LIBRARY_ID, NETSNMP_DS_LIB_PRINT_NUMERIC_ENUM, glob_snmp_object.enum_print);
netsnmp_ds_set_boolean(NETSNMP_DS_LIBRARY_ID, NETSNMP_DS_LIB_QUICK_PRINT, glob_snmp_object.quick_print);
Expand Down Expand Up @@ -1590,10 +1621,10 @@ PHP_METHOD(SNMP, __construct)

/* handle re-open of snmp session */
if (snmp_object->session) {
netsnmp_session_free(&(snmp_object->session));
snmp_session_free(&(snmp_object->session));
}

if (!netsnmp_session_init(&(snmp_object->session), version, a1, a2, timeout, retries)) {
if (!snmp_session_init(&(snmp_object->session), version, a1, a2, timeout, retries)) {
return;
}
snmp_object->max_oids = 0;
Expand All @@ -1618,7 +1649,7 @@ PHP_METHOD(SNMP, close)
RETURN_THROWS();
}

netsnmp_session_free(&(snmp_object->session));
snmp_session_free(&(snmp_object->session));

RETURN_TRUE;
}
Expand Down Expand Up @@ -1669,7 +1700,7 @@ PHP_METHOD(SNMP, setSecurity)
RETURN_THROWS();
}

if (!netsnmp_session_set_security(snmp_object->session, a1, a2, a3, a4, a5, a6, a7)) {
if (!snmp_session_set_security(snmp_object->session, a1, a2, a3, a4, a5, a6, a7)) {
/* Warning message sent already, just bail out */
RETURN_FALSE;
}
Expand Down
49 changes: 49 additions & 0 deletions ext/snmp/tests/snmp_session_error.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
--TEST--
SNMP::__construct checks
--CREDITS--
Boris Lytochkin
--EXTENSIONS--
snmp
--SKIPIF--
<?php
require_once(__DIR__.'/skipif.inc');
?>
--FILE--
<?php
require_once(__DIR__.'/snmp_include.inc');

$longhostname = str_repeat($hostname4, 1_000_000);
try {
new SNMP(SNMP::VERSION_1, "$hostname4:-1", $community, $timeout, $retries);
} catch (\ValueError $e) {
echo $e->getMessage(), PHP_EOL;
}
try {
new SNMP(SNMP::VERSION_1, "$hostname4:65536", $community, $timeout, $retries);
} catch (\ValueError $e) {
echo $e->getMessage(), PHP_EOL;
}
try {
new SNMP(SNMP::VERSION_1, "$longhostname:$port", $community, $timeout, $retries);
} catch (\ValueError $e) {
echo $e->getMessage(), PHP_EOL;
}
try {
new SNMP(SNMP::VERSION_1, "$hostname:$port", $community, PHP_INT_MIN, $retries);
} catch (\ValueError $e) {
echo $e->getMessage(), PHP_EOL;
}
try {
new SNMP(SNMP::VERSION_1, "$hostname:$port", $community, $timeout, PHP_INT_MAX);
} catch (\ValueError $e) {
echo $e->getMessage(), PHP_EOL;
}
echo "OK";
?>
--EXPECTF--
remote port must be between 0 and 65535
remote port must be between 0 and 65535
hostname length must be lower than 128
timeout must be between -1 and %d
retries must be between -1 and %d
OK
Loading