preg_replace untaints a string

[staabm]

per docs on http://php.net/manual/de/taint.detail.untaint.php a preg_replace should not untained a string.

this PR adds a failing test, which I expect to succeed.

[craigfrancis]

Could we a check for str_ireplace as well... there may be others, but these are the 2 I've seen so far :-)

[staabm]

so do you say that this untaints the string by mistake or is it intended?

[craigfrancis]

Yes, both untaint the string by mistake.

[craigfrancis]

You could do exactly the same test, but with line 15 changed to:

$query = str_ireplace('SELECT', 'SELECT', $query);

In comparison, str_replace does what I'd expect.

[staabm]

str_replace is already covered by a test in https://github.com/laruence/taint/blob/master/tests/008.phpt str_ireplace isnt though

[craigfrancis]

Thanks @staabm :-)

[staabm]

@laruence any hint on what the problem could be?

does any function untaint a string (when not explicitly part of the whitelist in the taint.c source?)

[staabm]

After I used the extension for some time I changed my mind.

preg_replace is used in a lot of places to santize a string. therefore we should assume the string is no longer tainted when it run thru it.

changed the unit test accordingly.

[craigfrancis]

Do you have any examples where preg_replace is used to sanitise a string?

[staabm]

E.g. https://github.com/filp/whoops/blob/master/src/Whoops/Handler/XmlResponseHandler.php#L79 https://github.com/symfony/symfony/blob/5129c4cf7e294b1a5ea30d6fec6e89b75396dcd2/src/Symfony/Component/Filesystem/LockHandler.php#L53

[craigfrancis]

Good point, it can be used for filtering characters (and I use that in a few places as well).

I'm just wondering, on a different tangent, could we have a mode (or function) of taint checking which is extra strict - one that identifies variables that have only been built from strings found within the PHP scripts.

For example, I could have a wrapper for the mysqli functions that checks to make sure that the SQL is made from strings (and maybe some constants), where variables must be passed as parameters.

I suspect that might be too strict for existing code bases, but if you could enable that, then there would be no guessing or risks, like we do when we whitelist preg_replace :-)

[staabm]

I dont think taint can auto detect this case and the user should use untaint() in cases where preg-replace actually sanitizes

[laruence]

and it's better to use

assert(untaint($str));

in product dev, simply set zend.assertions=-1