fix(942160): check REQUEST_FILENAME

[mat1010]

This change addresses #3779

[theseion]

Looks good. I'd like to get a second opinion before I merge though.

[dune73]

It's a nice bypass and a decent fix. Thanks @mat1010.

I wonder whether we still want to do REQUEST_BASENAME since we now cover REQUEST_FILENAME? It's redundant in most of the cases. But maybe REQUEST_FILENAME allows for a different type of bypass ...

[azurit]

While i was reviewing, before reading @dune73's comment, i come to the same conclusion - REQUEST_BASENAME is not needed.

[mat1010]

You are right. REQUEST_BASENAME is redundant. I removed it in f563dca.

But maybe REQUEST_FILENAME allows for a different type of bypass ...

Not sure if it allows some kind of bypass that wouldn't have worked before. In worst case it could catch more requests as the initial rule since it is now checking the full URI instead of just the name of the script.

While thinking about bypassing this rule I came to an additional easy bypass we have to consider.

Using comments like sleep(/*comment*/5) or pg_sleep(/*comment*/5)- this syntax is not blocked, but would work on a mysql and posgresql server. For benchmark this wont work since the regex is just matching for everything before and after the ,

mysql> SELECT sleep(/*comment*/5);
+-----------+
| sleep( 5) |
+-----------+
|         0 |
+-----------+
1 row in set (5.01 sec)

postgres=# select pg_sleep(/*comment*/5);
 pg_sleep
----------

(1 row)

I will also work on a fix to specifically address comments in the sleep command. Not sure if something like this would be to broad: (?i:sleep\(.*\d+.*\)|benchmark\(.*?\,.*?\))

[theseion]

Try using removeCommentsChar or replaceComments before making the regular expression unnecessarily complex.

[mat1010]

Sorry for the long delay. I added the the comment handling and a test for it.

[fzipi]

@theseion @dune73 @azurit Can we merge this one?