From f90323c8d4456ff2b9a0f442f6a383d9b58185ae Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Fri, 27 Dec 2024 17:58:50 +0100 Subject: [PATCH] Fix GH-17280: ldap_search() fails when $attributes array has holes (#17284) We relax the constraint that the array must be a list. What really matters is that it only has numeric keys. As shown in the example code, it's really easy to accidentally create a non-list array, so it makes sense to relax the constraint. There are 3 cases left where the array is checked to be a list, in php_ldap_do_search, but I believe this makes sense to keep because the indices of those arrays have a meaning because they should match between different arrays. In that case it will prevent programmer errors. --- ext/ldap/ldap.c | 42 +++++++++++++------ ext/ldap/tests/gh17280.phpt | 18 ++++++++ ..._add_modify_delete_programming_errors.phpt | 2 +- ...p_list_read_search_programming_errors.phpt | 2 +- .../ldap_modify_batch_programming_error.phpt | 20 ++------- ext/ldap/tests/ldap_search_error.phpt | 4 +- 6 files changed, 55 insertions(+), 33 deletions(-) create mode 100644 ext/ldap/tests/gh17280.phpt diff --git a/ext/ldap/ldap.c b/ext/ldap/ldap.c index e728f3049d958..f13cc71248998 100644 --- a/ext/ldap/ldap.c +++ b/ext/ldap/ldap.c @@ -234,6 +234,22 @@ static void ldap_result_entry_free_obj(zend_object *obj) } \ } +static bool php_ldap_is_numerically_indexed_array(const zend_array *arr) +{ + if (zend_hash_num_elements(arr) == 0 || HT_IS_PACKED(arr)) { + return true; + } + + zend_string *str_key; + ZEND_HASH_MAP_FOREACH_STR_KEY(arr, str_key) { + if (str_key) { + return false; + } + } ZEND_HASH_FOREACH_END(); + + return true; +} + /* An LDAP value must be a string, however it defines a format for integer and * booleans, thus we parse zvals to the corresponding string if possible * See RFC 4517: https://datatracker.ietf.org/doc/html/rfc4517 */ @@ -1481,8 +1497,8 @@ static void php_ldap_do_search(INTERNAL_FUNCTION_PARAMETERS, int scope) /* We don't allocate ldap_attrs for an empty array */ goto process; } - if (!zend_array_is_list(attributes)) { - zend_argument_value_error(4, "must be a list"); + if (!php_ldap_is_numerically_indexed_array(attributes)) { + zend_argument_value_error(4, "must be an array with numeric keys"); RETURN_THROWS(); } /* Allocate +1 as we need an extra entry to NULL terminate the list */ @@ -1490,7 +1506,7 @@ static void php_ldap_do_search(INTERNAL_FUNCTION_PARAMETERS, int scope) zend_ulong attribute_index = 0; zval *attribute_zv = NULL; - ZEND_HASH_FOREACH_NUM_KEY_VAL(attributes, attribute_index, attribute_zv) { + ZEND_HASH_FOREACH_VAL(attributes, attribute_zv) { ZVAL_DEREF(attribute_zv); if (Z_TYPE_P(attribute_zv) != IS_STRING) { zend_argument_type_error(4, "must be a list of strings, %s given", zend_zval_value_name(attribute_zv)); @@ -1503,7 +1519,7 @@ static void php_ldap_do_search(INTERNAL_FUNCTION_PARAMETERS, int scope) ret = 0; goto cleanup; } - ldap_attrs[attribute_index] = ZSTR_VAL(attribute); + ldap_attrs[attribute_index++] = ZSTR_VAL(attribute); } ZEND_HASH_FOREACH_END(); ldap_attrs[num_attribs] = NULL; } @@ -2302,8 +2318,8 @@ static void php_ldap_do_modify(INTERNAL_FUNCTION_PARAMETERS, int oper, int ext) RETVAL_FALSE; goto cleanup; } - if (!zend_array_is_list(Z_ARRVAL_P(attribute_values))) { - zend_argument_value_error(3, "must be a list of attribute values"); + if (!php_ldap_is_numerically_indexed_array(Z_ARRVAL_P(attribute_values))) { + zend_argument_value_error(3, "must be an array of attribute values with numeric keys"); RETVAL_FALSE; goto cleanup; } @@ -2314,7 +2330,7 @@ static void php_ldap_do_modify(INTERNAL_FUNCTION_PARAMETERS, int oper, int ext) zend_ulong attribute_value_index = 0; zval *attribute_value = NULL; - ZEND_HASH_FOREACH_NUM_KEY_VAL(Z_ARRVAL_P(attribute_values), attribute_value_index, attribute_value) { + ZEND_HASH_FOREACH_VAL(Z_ARRVAL_P(attribute_values), attribute_value) { zend_string *value = php_ldap_try_get_ldap_value_from_zval(attribute_value); if (UNEXPECTED(value == NULL)) { RETVAL_FALSE; @@ -2324,6 +2340,7 @@ static void php_ldap_do_modify(INTERNAL_FUNCTION_PARAMETERS, int oper, int ext) /* The string will be free by php_ldap_zend_string_release_from_char_pointer() during cleanup */ ldap_mods[attribute_index]->mod_bvalues[attribute_value_index]->bv_val = ZSTR_VAL(value); ldap_mods[attribute_index]->mod_bvalues[attribute_value_index]->bv_len = ZSTR_LEN(value); + attribute_value_index++; } ZEND_HASH_FOREACH_END(); ldap_mods[attribute_index]->mod_bvalues[num_values] = NULL; } @@ -2594,8 +2611,8 @@ PHP_FUNCTION(ldap_modify_batch) zend_argument_must_not_be_empty_error(3); RETURN_THROWS(); } - if (!zend_array_is_list(modifications)) { - zend_argument_value_error(3, "must be a list"); + if (!php_ldap_is_numerically_indexed_array(modifications)) { + zend_argument_value_error(3, "must be an array with numeric keys"); RETURN_THROWS(); } @@ -2689,8 +2706,8 @@ PHP_FUNCTION(ldap_modify_batch) zend_argument_value_error(3, "the value for option \"" LDAP_MODIFY_BATCH_VALUES "\" must not be empty"); RETURN_THROWS(); } - if (!zend_array_is_list(modification_values)) { - zend_argument_value_error(3, "the value for option \"" LDAP_MODIFY_BATCH_VALUES "\" must be a list"); + if (!php_ldap_is_numerically_indexed_array(modification_values)) { + zend_argument_value_error(3, "the value for option \"" LDAP_MODIFY_BATCH_VALUES "\" must be an array with numeric keys"); RETURN_THROWS(); } } ZEND_HASH_FOREACH_END(); @@ -2752,7 +2769,7 @@ PHP_FUNCTION(ldap_modify_batch) /* for each value */ zend_ulong value_index = 0; zval *modification_value_zv = NULL; - ZEND_HASH_FOREACH_NUM_KEY_VAL(Z_ARRVAL_P(modification_values), value_index, modification_value_zv) { + ZEND_HASH_FOREACH_VAL(Z_ARRVAL_P(modification_values), modification_value_zv) { zend_string *modval = zval_get_string(modification_value_zv); if (EG(exception)) { RETVAL_FALSE; @@ -2768,6 +2785,7 @@ PHP_FUNCTION(ldap_modify_batch) ldap_mods[modification_index]->mod_bvalues[value_index]->bv_len = ZSTR_LEN(modval); ldap_mods[modification_index]->mod_bvalues[value_index]->bv_val = estrndup(ZSTR_VAL(modval), ZSTR_LEN(modval)); zend_string_release(modval); + value_index++; } ZEND_HASH_FOREACH_END(); /* NULL-terminate values */ diff --git a/ext/ldap/tests/gh17280.phpt b/ext/ldap/tests/gh17280.phpt new file mode 100644 index 0000000000000..8c75f1276ab58 --- /dev/null +++ b/ext/ldap/tests/gh17280.phpt @@ -0,0 +1,18 @@ +--TEST-- +GH-17280 (ldap_search() fails when $attributes array has holes) +--EXTENSIONS-- +ldap +--FILE-- + +--EXPECTF-- +Warning: ldap_search(): Search: Can't contact LDAP server in %s on line %d +bool(false) diff --git a/ext/ldap/tests/ldap_add_modify_delete_programming_errors.phpt b/ext/ldap/tests/ldap_add_modify_delete_programming_errors.phpt index 14fa50312e7d1..879b465dfeae4 100644 --- a/ext/ldap/tests/ldap_add_modify_delete_programming_errors.phpt +++ b/ext/ldap/tests/ldap_add_modify_delete_programming_errors.phpt @@ -138,6 +138,6 @@ ValueError: ldap_add(): Argument #3 ($entry) key must not be empty ValueError: ldap_add(): Argument #3 ($entry) key must not contain any null bytes Error: Object of class stdClass could not be converted to string ValueError: ldap_add(): Argument #3 ($entry) list of attribute values must not be empty -ValueError: ldap_add(): Argument #3 ($entry) must be a list of attribute values +ValueError: ldap_add(): Argument #3 ($entry) must be an array of attribute values with numeric keys TypeError: LDAP value must be of type string|int|bool, array given Error: Object of class stdClass could not be converted to string diff --git a/ext/ldap/tests/ldap_list_read_search_programming_errors.phpt b/ext/ldap/tests/ldap_list_read_search_programming_errors.phpt index 59e227d818ecf..e3e3b2c91ce6f 100644 --- a/ext/ldap/tests/ldap_list_read_search_programming_errors.phpt +++ b/ext/ldap/tests/ldap_list_read_search_programming_errors.phpt @@ -80,7 +80,7 @@ try { TypeError: ldap_list(): Argument #1 ($ldap) must be of type LDAP\Connection|array, int given TypeError: ldap_list(): Argument #2 ($base) must be of type string when argument #1 ($ldap) is an LDAP\Connection instance TypeError: ldap_list(): Argument #3 ($filter) must be of type string when argument #1 ($ldap) is an LDAP\Connection instance -ValueError: ldap_list(): Argument #4 ($attributes) must be a list +ValueError: ldap_list(): Argument #4 ($attributes) must be an array with numeric keys TypeError: ldap_list(): Argument #4 ($attributes) must be a list of strings, int given ValueError: ldap_list(): Argument #4 ($attributes) must not contain strings with any null bytes ValueError: ldap_list(): Argument #4 ($attributes) must not contain strings with any null bytes diff --git a/ext/ldap/tests/ldap_modify_batch_programming_error.phpt b/ext/ldap/tests/ldap_modify_batch_programming_error.phpt index e391a403101f5..b123f437aa3ff 100644 --- a/ext/ldap/tests/ldap_modify_batch_programming_error.phpt +++ b/ext/ldap/tests/ldap_modify_batch_programming_error.phpt @@ -216,19 +216,6 @@ try { echo $e::class, ': ', $e->getMessage(), PHP_EOL; } -$modification_values_not_list2 = [ - [ - "attrib" => "attrib1", - "modtype" => LDAP_MODIFY_BATCH_ADD, - "values" => $not_list2, - ], -]; -try { - var_dump(ldap_modify_batch($ldap, $valid_dn, $modification_values_not_list2)); -} catch (Throwable $e) { - echo $e::class, ': ', $e->getMessage(), PHP_EOL; -} - $modification_missing_attrib_key = [ [ "modtype" => LDAP_MODIFY_BATCH_ADD, @@ -257,8 +244,8 @@ try { --EXPECT-- ValueError: ldap_modify_batch(): Argument #2 ($dn) must not contain any null bytes ValueError: ldap_modify_batch(): Argument #3 ($modifications_info) must not be empty -ValueError: ldap_modify_batch(): Argument #3 ($modifications_info) must be a list -ValueError: ldap_modify_batch(): Argument #3 ($modifications_info) must be a list +ValueError: ldap_modify_batch(): Argument #3 ($modifications_info) must be an array with numeric keys +ValueError: ldap_modify_batch(): Argument #3 ($modifications_info) a modification entry must only contain the keys "attrib", "modtype", and "values" TypeError: ldap_modify_batch(): Argument #3 ($modifications_info) must only contain arrays ValueError: ldap_modify_batch(): Argument #3 ($modifications_info) a modification entry must not contain the "values" option when option "modtype" is LDAP_MODIFY_BATCH_REMOVE_ALL ValueError: ldap_modify_batch(): Argument #3 ($modifications_info) a modification entry must only contain the keys "attrib", "modtype", and "values" @@ -270,7 +257,6 @@ ValueError: ldap_modify_batch(): Argument #3 ($modifications_info) a modificatio ValueError: ldap_modify_batch(): Argument #3 ($modifications_info) a modification entry must contain the "values" option when the "modtype" option is not LDAP_MODIFY_BATCH_REMOVE_ALL TypeError: ldap_modify_batch(): Argument #3 ($modifications_info) the value for option "values" must be of type array, string given ValueError: ldap_modify_batch(): Argument #3 ($modifications_info) the value for option "values" must not be empty -ValueError: ldap_modify_batch(): Argument #3 ($modifications_info) the value for option "values" must be a list -ValueError: ldap_modify_batch(): Argument #3 ($modifications_info) the value for option "values" must be a list +ValueError: ldap_modify_batch(): Argument #3 ($modifications_info) the value for option "values" must be an array with numeric keys ValueError: ldap_modify_batch(): Argument #3 ($modifications_info) a modification entry must contain the "attrib" option ValueError: ldap_modify_batch(): Argument #3 ($modifications_info) a modification entry must contain the "modtype" option diff --git a/ext/ldap/tests/ldap_search_error.phpt b/ext/ldap/tests/ldap_search_error.phpt index 04d8af46f5982..10906a44afc00 100644 --- a/ext/ldap/tests/ldap_search_error.phpt +++ b/ext/ldap/tests/ldap_search_error.phpt @@ -20,7 +20,7 @@ $result = ldap_search($link, $dn, $filter); var_dump($result); try { - $result = ldap_search($link, $dn, $filter, array(1 => 'top')); + $result = ldap_search($link, $dn, $filter, array('foo' => 'top')); var_dump($result); } catch (ValueError $exception) { echo $exception->getMessage() . "\n"; @@ -60,7 +60,7 @@ try { --EXPECTF-- Warning: ldap_search(): Search: No such object in %s on line %d bool(false) -ldap_search(): Argument #4 ($attributes) must be a list +ldap_search(): Argument #4 ($attributes) must be an array with numeric keys ldap_search(): Argument #1 ($ldap) must not be empty ldap_search(): Argument #2 ($base) must be the same size as argument #1 ldap_search(): Argument #3 ($filter) must be the same size as argument #1