robrichards / xmlseclibs

A PHP library for XML Security
BSD 3-Clause "New" or "Revised" License
388 stars 181 forks source link

Filter input for its use in XPath expressions #156

Closed jaimeperez closed 6 years ago

jaimeperez commented 6 years ago

In order to avoid XPath injection, user input must be filtered before it ends up in the query. Unfortunately, there's no way to do this with a standard method in PHP, so we need our own filtering function. Current best practice recommends using white lists instead of black lists to allow only a subset of characters. In this case, we allow only letters, numericals, spaces, dashes and underscores.

This fixes a bug also inside a loop, where $identifier is used instead of $idKey (the element in the current loop iteration).

jaimeperez commented 6 years ago

I stand corrected. There was not such a bug, I just didn't see the $idKey variable used as it was hidden by syntax highlighting inside the double-quote-delimited string:

   $iDlist .= " or @$idKey=" ...

It might be that $idKey never comes from user input. Impossible to know as $this->idKeys is never set in the codebase. So to be on the safe side (someone using the library, setting it to user input), I've just filtered it.

The rest (XPath input filtering) still applies.

thijskh commented 6 years ago

So do you agree with the new approach @robrichards?

robrichards commented 6 years ago

@thijskh Yes I do. Just been hesitant to make sure the regex isn't going to inadvertently break BC somewhere. I am going to merge this in at this point and not hold it up anymore.