1274764 - Increasing memcached performance is possible because Cache::Memcached::Fast does not need detainting

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement, P1)

Description

[Dylan Hardison [:dylan] (he/him)]

https://metacpan.org/pod/Cache::Memcached::Fast#Tainted-data
"In current implementation tainted flag is neither tested nor preserved, storing tainted data and retrieving it back would clear tainted flag. See perlsec."

This means we can delete this code:

https://github.com/bugzilla/bugzilla/blob/master/Bugzilla/Memcached.pm#L249-L271

which means things will be faster.

On BMO at least this is quite slow:

http://people.mozilla.org/~dhardison/1243775/nytprof.before/Bugzilla-Memcached-pm-11-line.html

of course, we can't do this on BMO because we use Cache::Memcached still (we'll get it when upstream merge is done!)

Comment 1

[Frédéric Buclin]

The fact that this module doesn't preserve the tainted flag is a nice security weakness. This means that it could be possible to store unsanitized data and get it back detained, making everybody think the data has already been validated. Sad!

Comment 2

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1274764_1.patch

Comment 3

[David Lawrence [:dkl]]

Comment on attachment 8755869 [details] [diff] [review]
1274764_1.patch

Review of attachment 8755869 [details] [diff] [review]:
-----------------------------------------------------------------

You would also need to remove use 'Bugzilla::Util qw(trick_taint);' from top of file.

> The fact that this module doesn't preserve the tainted flag is a nice security weakness. This means that it could be possible to store unsanitized data and get it back > detained, making everybody think the data has already been validated. Sad!

dylan, is this specific to Cache::Memcached::Fast or does Cache::Memcached also not preserve taintedness? Also since we are blindly just detainting whatever data we get back from memcached now with the old code, we are no worse off with the new change. Ideally we should be checking for taintedness with the data before storing it in memcached.

r=dkl with comment fixed.

Comment 4

[Dylan Hardison [:dylan] (he/him)]

Yeah, Cache::Memcached's data is tainted but not intentionally. If you read in a string from a socket that string happens to be tainted. ::Fast is not using perl-level IO functions, so there is no taintedness going on.

Comment 5

[Dylan Hardison [:dylan] (he/him)]

To ssh://[e-mail]/bugzilla/bugzilla.git
   7ccda08..a0a047d  master -> master