Skip to content

If client-side verify_key is not enabled, don't check it automatically #547

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

Merged
merged 4 commits into from
Sep 26, 2024
Merged

If client-side verify_key is not enabled, don't check it automatically #547

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cf31229
If client-side verify_key is not enabled, don't check it automatically
rlerdorf Oct 20, 2023
e429fc9
fix tests
rlerdorf Oct 20, 2023
78ba261
Fix more tests with a slight refactor
rlerdorf Oct 20, 2023
1527217
Check for spaces in keys when using the non-binary protocol even if k…
rlerdorf Oct 20, 2023
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
19 changes: 13 additions & 6 deletions php_memcached.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,14 +231,21 @@ zend_bool s_memc_valid_key_binary(zend_string *key)
}

static
zend_bool s_memc_valid_key_ascii(zend_string *key)
zend_bool s_memc_valid_key_ascii(zend_string *key, uint64_t verify_key)
Copy link
Contributor

Choose a reason for hiding this comment

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

I was going to make this suggestion, but then I realized the setting is defined as 64-bits. So it goes!

php-memcached/php_memcached.c

Line 4229 in 7fefcb7

REGISTER_MEMC_CLASS_CONST_LONG(OPT_VERIFY_KEY, MEMCACHED_BEHAVIOR_VERIFY_KEY);

Suggested change
zend_bool s_memc_valid_key_ascii(zend_string *key, uint64_t verify_key)
zend_bool s_memc_valid_key_ascii(zend_string *key, zend_bool verify_key)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that is why I made it a uint64_t, but perhaps we should flip that one to a REGISTER_MEMC_CLASS_CONST_BOOL ?

{
const char *str = ZSTR_VAL(key);
size_t i, len = ZSTR_LEN(key);

for (i = 0; i < len; i++) {
if (!isgraph(str[i]) || isspace(str[i]))
return 0;
if (verify_key) {
for (i = 0; i < len; i++) {
if (!isgraph(str[i]) || isspace(str[i]))
return 0;
}
} else { /* if key verification is disabled, only check for spaces to avoid injection issues */
for (i = 0; i < len; i++) {
if (isspace(str[i]))
return 0;
}
}
return 1;
}
Expand All @@ -248,7 +255,7 @@ zend_bool s_memc_valid_key_ascii(zend_string *key)
ZSTR_LEN(key) > MEMC_OBJECT_KEY_MAX_LENGTH || \
(memcached_behavior_get(intern->memc, MEMCACHED_BEHAVIOR_BINARY_PROTOCOL) \
? !s_memc_valid_key_binary(key) \
: !s_memc_valid_key_ascii(key) \
: !s_memc_valid_key_ascii(key, memcached_behavior_get(intern->memc, MEMCACHED_BEHAVIOR_VERIFY_KEY)) \
))) { \
intern->rescode = MEMCACHED_BAD_KEY_PROVIDED; \
RETURN_FALSE; \
Expand Down Expand Up @@ -342,7 +349,7 @@ PHP_INI_MH(OnUpdateSessionPrefixString)
php_error_docref(NULL, E_WARNING, "memcached.sess_prefix too long (max: %d)", MEMCACHED_MAX_KEY - 1);
return FAILURE;
}
if (!s_memc_valid_key_ascii(new_value)) {
if (!s_memc_valid_key_ascii(new_value, 1)) {
php_error_docref(NULL, E_WARNING, "memcached.sess_prefix cannot contain whitespace or control characters");
return FAILURE;
}
Expand Down
5 changes: 4 additions & 1 deletion tests/cas_invalid_key.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,10 @@ Memcached::cas() with strange key
--FILE--
<?php
include dirname(__FILE__) . '/config.inc';
$m = memc_get_instance ();
$m = memc_get_instance (array (
Memcached::OPT_BINARY_PROTOCOL => false,
Memcached::OPT_VERIFY_KEY => true
));

error_reporting(0);
var_dump($m->cas(0, '', true, 10));
Expand Down
5 changes: 4 additions & 1 deletion tests/delete_bykey.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,10 @@ Memcached::deleteByKey()
--FILE--
<?php
include dirname(__FILE__) . '/config.inc';
$m = memc_get_instance ();
$m = memc_get_instance (array (
Memcached::OPT_BINARY_PROTOCOL => false,
Memcached::OPT_VERIFY_KEY => true
));

$m->setByKey('keffe', 'eisaleeoo', "foo");
var_dump($m->getByKey('keffe', 'eisaleeoo'));
Expand Down
5 changes: 4 additions & 1 deletion tests/get.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,10 @@ Memcached::get()
--FILE--
<?php
include dirname(__FILE__) . '/config.inc';
$m = memc_get_instance ();
$m = memc_get_instance (array (
Memcached::OPT_BINARY_PROTOCOL => false,
Memcached::OPT_VERIFY_KEY => true
));

$m->delete('foo');

Expand Down
5 changes: 1 addition & 4 deletions tests/keys_ascii.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,8 @@ Test valid and invalid keys - ascii
include dirname (__FILE__) . '/config.inc';
$ascii = memc_get_instance (array (
Memcached::OPT_BINARY_PROTOCOL => false,
Memcached::OPT_VERIFY_KEY => false
Memcached::OPT_VERIFY_KEY => true
));
// libmemcached can verify keys, but these are tests are for our own
// function s_memc_valid_key_ascii, so explicitly disable the checks
// that libmemcached can perform.

echo 'ASCII: SPACES' . PHP_EOL;
var_dump ($ascii->set ('ascii key with spaces', 'this is a test'));
Expand Down