Undocumented 3.2.0 change: non-ascii keys no longer supported.

[SpencerMalone]

In #479 we moved from iscntrl -> !isgraph. The problem there is non-ascii characters are "not graphical", but they are valid keys according to the memcached server.

I understand the libmemcached client does a !isgraph, and thus it may be worth adopting this (even though protocol does technically support non-ascii keys), but at a minimum this should probably be a major release as unexpected loss of keys is a really scary unexpected change.

[rlerdorf]

I don't really understand why you think it is a major change. libmemcached always did an isgraph() check, so these keys were failing at the libmemcached level. It was inconsistent for the check at the extension level not to use the same check. And it caused all sorts of confusion to sometimes have bad keys error at the extension level and other times at the libmemcached level. None of these keys were getting through before, it is solely about where it was caught and it is much cleaner to always catch it at the extension level. if/when libmemcached changes their check, we will follow suit at the extension level.

[SpencerMalone]

Hmmm, is the resulting error code just different? We were not seeing a "negative" response code with some of our keys before upgrading from 3.1.5 -> 3.2.0, and now we get bad key errors. I'll dig up exactly what characters are giving us behavioral changes.

[rlerdorf]

Yes, libmemcached has always done an isgraph() check on the keys so those keys did not get through to memcached. By making the check consistent in the PHP extension you now see those errors at the extension level.
https://github.com/awesomized/libmemcached/blob/v1.x/src/libmemcached/key.cc#L43-L81
And if you look at the code for any of the by_key operations in libmemcached you can see that it calls that memcached_key_test() function and returns an error code.

[SpencerMalone]

Run in a https://psysh.org/ REPL. I'll try to get you reproducing example repository in the next few days:

Memcached 3.1.5 extension:

>>> $client->set(utf8_encode("\x5A\x6F\xEB"), "data");
=> true
>>> $client->get(utf8_encode("\x5A\x6F\xEB"));
=> "data"

Memcached 3.2.0 extension:

>>> $client->set(utf8_encode("\x5A\x6F\xEB"), "data");
=> false
>>> $client->getResultCode();
=> 33

Talking to the same target memcached instance (in this case, technically google memorystore for memcached, but I don't know how relevant that is)

[rlerdorf]

which libmemcached version?

[SpencerMalone]

Good question, you're going to hate me:
3.2.0 we're using https://github.com/awslabs/aws-elasticache-cluster-client-libmemcached which declares itself a mild fork of ol' 1.0.18 (I do see it correctly has isgraph checks on memcached_key_test)
3.1.5, we were using 1.0.18 from the following yum packages:

libmemcached-opt-libs.x86_64       1.0.18-2.el7.remi                @remi-safe  
php-pecl-memcached.x86_64          3.1.5-1.el7.remi.7.4             @remi-php7.4

which is presumably stock 1.0.18.

I'm gonna try to get you a reproduction with 3.2.0 running normal libmemcached 1.0.18

[SpencerMalone]

https://github.com/SpencerMalone/memcached-315-320-key-replication is the replication with standardized libs:

./run-test.sh     
[+] Building 0.0s (0/1)                                                                                                                                                                                                                                
[+] Building 1.2s (9/9) FINISHED                                                                                                                                                                                                                       
 => [internal] load build definition from Dockerfile.315                                                                                                                                                                                          0.0s
 => => transferring dockerfile: 265B                                                                                                                                                                                                              0.0s
 => [internal] load .dockerignore                                                                                                                                                                                                                 0.0s
 => => transferring context: 2B                                                                                                                                                                                                                   0.0s
 => [internal] load metadata for docker.io/library/php:8.2-cli-bullseye                                                                                                                                                                           1.1s
 => [1/4] FROM docker.io/library/php:8.2-cli-bullseye@sha256:12f345879ac894871c7c716a73e38088c7ee5f0fd72545a8623c2f2646686543                                                                                                                     0.0s
 => [internal] load build context                                                                                                                                                                                                                 0.0s
 => => transferring context: 33B                                                                                                                                                                                                                  0.0s
 => CACHED [2/4] RUN apt-get update && apt-get install -y libz-dev libmemcached-dev libmemcached-tools                                                                                                                                            0.0s
 => CACHED [3/4] RUN pecl install memcached-3.1.5     && docker-php-ext-enable memcached                                                                                                                                                          0.0s
 => CACHED [4/4] ADD memtest.php /memtest.php                                                                                                                                                                                                     0.0s
 => exporting to image                                                                                                                                                                                                                            0.0s
 => => exporting layers                                                                                                                                                                                                                           0.0s
 => => writing image sha256:3cc50c36a8b3249875da3815ffc032d21a3a1f0aef3d4a4764f13e5442120e4e                                                                                                                                                      0.0s
 => => naming to docker.io/library/php-memcached-cli:3.1.5-1.0.18                                                                                                                                                                                 0.0s

Use 'docker scan' to run Snyk tests against images to find vulnerabilities and learn how to fix them
[+] Building 0.3s (9/9) FINISHED                                                                                                                                                                                                                       
 => [internal] load build definition from Dockerfile.320                                                                                                                                                                                          0.0s
 => => transferring dockerfile: 265B                                                                                                                                                                                                              0.0s
 => [internal] load .dockerignore                                                                                                                                                                                                                 0.0s
 => => transferring context: 2B                                                                                                                                                                                                                   0.0s
 => [internal] load metadata for docker.io/library/php:8.2-cli-bullseye                                                                                                                                                                           0.2s
 => [internal] load build context                                                                                                                                                                                                                 0.0s
 => => transferring context: 33B                                                                                                                                                                                                                  0.0s
 => [1/4] FROM docker.io/library/php:8.2-cli-bullseye@sha256:12f345879ac894871c7c716a73e38088c7ee5f0fd72545a8623c2f2646686543                                                                                                                     0.0s
 => CACHED [2/4] RUN apt-get update && apt-get install -y libz-dev libmemcached-dev libmemcached-tools                                                                                                                                            0.0s
 => CACHED [3/4] RUN pecl install memcached-3.2.0     && docker-php-ext-enable memcached                                                                                                                                                          0.0s
 => CACHED [4/4] ADD memtest.php /memtest.php                                                                                                                                                                                                     0.0s
 => exporting to image                                                                                                                                                                                                                            0.0s
 => => exporting layers                                                                                                                                                                                                                           0.0s
 => => writing image sha256:08fea393cb33207bb37ac4dfadf755ff13e575b51964600a76ad51c80ee1080c                                                                                                                                                      0.0s
 => => naming to docker.io/library/php-memcached-cli:3.2.0-1.0.18                                                                                                                                                                                 0.0s

Use 'docker scan' to run Snyk tests against images to find vulnerabilities and learn how to fix them
c43b356f00e652c050ed108f628b7f271bca5d0f2b0c2de868c6d670459a1557
######### TESTING 3.1.5 ##########


Deprecated: Function utf8_encode() is deprecated in /memtest.php on line 7
bool(true)
string(7) "SUCCESS"

Deprecated: Function utf8_encode() is deprecated in /memtest.php on line 9
string(4) "data"
######### TESTING 3.2.0 ##########

Deprecated: Function utf8_encode() is deprecated in /memtest.php on line 7
bool(false)
string(46) "A BAD KEY WAS PROVIDED/CHARACTERS OUT OF RANGE"

Deprecated: Function utf8_encode() is deprecated in /memtest.php on line 9
bool(false)

Running the following test PHP script:

<?php

$memcached = new Memcached();

$memcached->addServer('memcached', 11211);

var_dump($memcached->set(utf8_encode("\x5A\x6F\xEB"), "data"));
var_dump($memcached->getResultMessage());
var_dump($memcached->get(utf8_encode("\x5A\x6F\xEB")));

[rlerdorf]

So this is a weird edge-case that happens to work. If that is really your production code it means you are not doing any key verification client-side which is a bit risky.
Try this with 3.1.5:

<?php

$memcached = new Memcached();

$memcached->addServer('memcached', 11211);
$memcached->setOption(Memcached::OPT_VERIFY_KEY, true);
var_dump($memcached->set(utf8_encode("\x5A\x6F\xEB"), "data"));
var_dump($memcached->getResultMessage());
var_dump($memcached->get(utf8_encode("\x5A\x6F\xEB")));

I think you will find that this key gets caught by libmemcached's key check.

But maybe we need to skip that check in the PHP extension when Memcached::OPT_VERIFY_KEY is off. I can work up a patch for that, but you may want to review if you want to continue running with no client-side key verification.

[SpencerMalone]

I definitely agree with that :) (both on "this showed us we need to be more stringent about sanitization of user input into keys" and "enabling that option")

I actually didn't know that flag existed until I was digging in the issues and PRs for this repo today. Not sure how to get https://www.php.net/manual/en/memcached.constants.php updated, but I've noticed there's some other missing options and constants on there over the years.

I'm gonna throw one more wrench into this: The behavior of forcing verification of (in particular) no spaces in keys has been part of the 3.x behavior for years now, I know #519 says this isn't the job of the extension, but defaulting to disabling that may open up a bunch of people to memcached injections through key manipulation. I personally would love to either see OPT_VERIFY_KEY default to on, or keeping "no spaces in keys" validation in place (even though it is superfluous with "only graphical characters" IIRC)

[rlerdorf]

I've updated the PR to keep the check for spaces in keys even if key verification is not enabled