Skip to content

Commit

Permalink
Fix GH-16959: snmpget modifies the object_id (as array).
Browse files Browse the repository at this point in the history
Instead of modifying the zval, we use the zend_try_get_string.

close GH-16969
  • Loading branch information
devnexen committed Dec 1, 2024
1 parent aab7842 commit 73ebc92
Show file tree
Hide file tree
Showing 3 changed files with 133 additions and 20 deletions.
4 changes: 4 additions & 0 deletions NEWS
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,10 @@ PHP NEWS
. Fixed bug GH-16808 (Segmentation fault in RecursiveIteratorIterator
->current() with a xml element input). (nielsdos)

- SNMP:
. Fixed bug GH-16959 (snmget modifies the object_id array).
(David Carlier)

- Standard:
. Fixed bug GH-16905 (Internal iterator functions can't handle UNDEF
properties). (nielsdos)
Expand Down
80 changes: 60 additions & 20 deletions ext/snmp/snmp.c
Original file line number Diff line number Diff line change
Expand Up @@ -626,6 +626,31 @@ static void php_snmp_internal(INTERNAL_FUNCTION_PARAMETERS, int st,
}
/* }}} */

static void php_snmp_zend_string_release_from_char_pointer(char *ptr) {
if (ptr) {
zend_string *pptr = (zend_string *)(ptr - XtOffsetOf(zend_string, val));
zend_string_release(pptr);
}
}

static void php_free_objid_query(struct objid_query *objid_query, HashTable* oid_ht, const HashTable *value_ht, int st) {
if (oid_ht) {
uint32_t count = zend_hash_num_elements(oid_ht);

for (uint32_t i = 0; i < count; i ++) {
snmpobjarg *arg = &objid_query->vars[i];
if (!arg->oid) {
break;
}
if (value_ht) {
php_snmp_zend_string_release_from_char_pointer(arg->value);
}
php_snmp_zend_string_release_from_char_pointer(arg->oid);
}
}
efree(objid_query->vars);
}

/* {{{ php_snmp_parse_oid
*
* OID parser (and type, value for SNMP_SET command)
Expand Down Expand Up @@ -674,10 +699,15 @@ static bool php_snmp_parse_oid(
return false;
}
objid_query->vars = (snmpobjarg *)safe_emalloc(sizeof(snmpobjarg), zend_hash_num_elements(oid_ht), 0);
memset(objid_query->vars, 0, sizeof(snmpobjarg) * zend_hash_num_elements(oid_ht));
objid_query->array_output = (st & SNMP_CMD_SET) == 0;
ZEND_HASH_FOREACH_VAL(oid_ht, tmp_oid) {
convert_to_string(tmp_oid);
objid_query->vars[objid_query->count].oid = Z_STRVAL_P(tmp_oid);
zend_string *tmp = zval_try_get_string(tmp_oid);
if (!tmp) {
php_free_objid_query(objid_query, oid_ht, value_ht, st);
return false;
}
objid_query->vars[objid_query->count].oid = ZSTR_VAL(tmp);
if (st & SNMP_CMD_SET) {
if (type_str) {
pptr = ZSTR_VAL(type_str);
Expand All @@ -701,18 +731,24 @@ static bool php_snmp_parse_oid(
}
}
if (idx_type < type_ht->nNumUsed) {
convert_to_string(tmp_type);
if (Z_STRLEN_P(tmp_type) != 1) {
zend_string *type = zval_try_get_string(tmp_type);
if (!type) {
php_free_objid_query(objid_query, oid_ht, value_ht, st);
return false;
}
size_t len = ZSTR_LEN(type);
char ptype = *ZSTR_VAL(type);
zend_string_release(type);
if (len != 1) {
zend_value_error("Type must be a single character");
efree(objid_query->vars);
php_free_objid_query(objid_query, oid_ht, value_ht, st);
return false;
}
pptr = Z_STRVAL_P(tmp_type);
objid_query->vars[objid_query->count].type = *pptr;
objid_query->vars[objid_query->count].type = ptype;
idx_type++;
} else {
php_error_docref(NULL, E_WARNING, "'%s': no type set", Z_STRVAL_P(tmp_oid));
efree(objid_query->vars);
php_error_docref(NULL, E_WARNING, "'%s': no type set", ZSTR_VAL(tmp));
php_free_objid_query(objid_query, oid_ht, value_ht, st);
return false;
}
}
Expand All @@ -738,12 +774,16 @@ static bool php_snmp_parse_oid(
}
}
if (idx_value < value_ht->nNumUsed) {
convert_to_string(tmp_value);
objid_query->vars[objid_query->count].value = Z_STRVAL_P(tmp_value);
zend_string *tmp = zval_try_get_string(tmp_value);
if (!tmp) {
php_free_objid_query(objid_query, oid_ht, value_ht, st);
return false;
}
objid_query->vars[objid_query->count].value = ZSTR_VAL(tmp);
idx_value++;
} else {
php_error_docref(NULL, E_WARNING, "'%s': no value set", Z_STRVAL_P(tmp_oid));
efree(objid_query->vars);
php_error_docref(NULL, E_WARNING, "'%s': no value set", ZSTR_VAL(tmp));
php_free_objid_query(objid_query, oid_ht, value_ht, st);
return false;
}
}
Expand All @@ -756,14 +796,14 @@ static bool php_snmp_parse_oid(
if (st & SNMP_CMD_WALK) {
if (objid_query->count > 1) {
php_snmp_error(object, PHP_SNMP_ERRNO_OID_PARSING_ERROR, "Multi OID walks are not supported!");
efree(objid_query->vars);
php_free_objid_query(objid_query, oid_ht, value_ht, st);
return false;
}
objid_query->vars[0].name_length = MAX_NAME_LEN;
if (strlen(objid_query->vars[0].oid)) { /* on a walk, an empty string means top of tree - no error */
if (!snmp_parse_oid(objid_query->vars[0].oid, objid_query->vars[0].name, &(objid_query->vars[0].name_length))) {
php_snmp_error(object, PHP_SNMP_ERRNO_OID_PARSING_ERROR, "Invalid object identifier: %s", objid_query->vars[0].oid);
efree(objid_query->vars);
php_free_objid_query(objid_query, oid_ht, value_ht, st);
return false;
}
} else {
Expand All @@ -775,7 +815,7 @@ static bool php_snmp_parse_oid(
objid_query->vars[objid_query->offset].name_length = MAX_OID_LEN;
if (!snmp_parse_oid(objid_query->vars[objid_query->offset].oid, objid_query->vars[objid_query->offset].name, &(objid_query->vars[objid_query->offset].name_length))) {
php_snmp_error(object, PHP_SNMP_ERRNO_OID_PARSING_ERROR, "Invalid object identifier: %s", objid_query->vars[objid_query->offset].oid);
efree(objid_query->vars);
php_free_objid_query(objid_query, oid_ht, value_ht, st);
return false;
}
}
Expand Down Expand Up @@ -1252,12 +1292,12 @@ 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)) {
efree(objid_query.vars);
php_free_objid_query(&objid_query, oid_ht, value_ht, st);
netsnmp_session_free(&session);
RETURN_FALSE;
}
if (version == SNMP_VERSION_3 && !netsnmp_session_set_security(session, a3, a4, a5, a6, a7, NULL, NULL)) {
efree(objid_query.vars);
php_free_objid_query(&objid_query, oid_ht, value_ht, st);
netsnmp_session_free(&session);
/* Warning message sent already, just bail out */
RETURN_FALSE;
Expand All @@ -1268,7 +1308,7 @@ static void php_snmp(INTERNAL_FUNCTION_PARAMETERS, int st, int version)
session = snmp_object->session;
if (!session) {
zend_throw_error(NULL, "Invalid or uninitialized SNMP object");
efree(objid_query.vars);
php_free_objid_query(&objid_query, oid_ht, value_ht, st);
RETURN_THROWS();
}

Expand All @@ -1294,7 +1334,7 @@ static void php_snmp(INTERNAL_FUNCTION_PARAMETERS, int st, int version)

php_snmp_internal(INTERNAL_FUNCTION_PARAM_PASSTHRU, st, session, &objid_query);

efree(objid_query.vars);
php_free_objid_query(&objid_query, oid_ht, value_ht, st);

if (session_less_mode) {
netsnmp_session_free(&session);
Expand Down
69 changes: 69 additions & 0 deletions ext/snmp/tests/gh16959.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
--TEST--
snmpget() modifies object_id array source
--EXTENSIONS--
snmp
--SKIPIF--
<?php
require_once(__DIR__.'/skipif.inc');
?>
--FILE--
<?php
require_once(__DIR__.'/snmp_include.inc');

$bad_object_ids = array (
077 => 077, -066 => -066, -0345 => -0345, 0 => 0
);
var_dump($bad_object_ids);
var_dump(snmpget($hostname, "", $bad_object_ids) === false);
// The array should remain unmodified
var_dump($bad_object_ids);
try {
snmpget($hostname, "", [0 => new stdClass()]);
} catch (Throwable $e) {
echo $e->getMessage() . PHP_EOL;
}

try {
snmp2_set($hostname, $communityWrite, $bad_object_ids, array(new stdClass()), array(null));
} catch (Throwable $e) {
echo $e->getMessage() . PHP_EOL;
}
try {
snmp2_set($hostname, $communityWrite, $bad_object_ids, array("toolongtype"), array(null));
} catch (Throwable $e) {
echo $e->getMessage() . PHP_EOL;
}
try {
snmp2_set($hostname, $communityWrite, $bad_object_ids, array(str_repeat("onetoomuch", random_int(1, 1))), array(null));
} catch (Throwable $e) {
echo $e->getMessage();
}
?>
--EXPECTF--
array(4) {
[63]=>
int(63)
[-54]=>
int(-54)
[-229]=>
int(-229)
[0]=>
int(0)
}

Warning: snmpget(): Invalid object identifier: -54 in %s on line %d
bool(true)
array(4) {
[63]=>
int(63)
[-54]=>
int(-54)
[-229]=>
int(-229)
[0]=>
int(0)
}
Object of class stdClass could not be converted to string
Object of class stdClass could not be converted to string
Type must be a single character
Type must be a single character

0 comments on commit 73ebc92

Please sign in to comment.