-1
$linkedStudies = $this->LinkedStudies->find('all')
->where(array(
'is_citation' => true, 
'protocol_ids REGEXP' => '^([0-9]+)*'.$id.'([0-9]+)*$'
));

(I am using CakePHP and MySQL with the Zend framework.)

Note the $id variable in the where clause.

kig
  • 5
  • 3
  • Another one bites the dust. See https://stackoverflow.com/questions/3653462/is-storing-a-delimited-list-in-a-database-column-really-that-bad – Your Common Sense Mar 16 '21 at 09:34

1 Answers1

0

It is not vulnerable to SQL injection, because the expression will be passed as a proper parameter to the query by CakePHP. (Thanks @ndm for pointing this out below!)

Note though that using user input in regular expressions is a potential denial of service vulnerability, because it's relatively easy to deliberately create a regexp that will be very slow on anything but small inputs.

In case of the above, it would be a good solution to only allow $id from a hardcoded set (validate it in php to be from a given list), because protocol_ids sounds like it has a finite number of possible values.

Gabor Lengyel
  • 14,129
  • 4
  • 32
  • 59
  • "It is technically vulnerable to SQL injection due to string concatenation." - Is there a way to remedy this? – kig Mar 15 '21 at 22:04
  • See the last paragraph for this particular case. – Gabor Lengyel Mar 15 '21 at 22:06
  • 2
    If that snippet uses the CakePHP query builder then it's not vulnerable to SQL injections, as the right hand side of array style key > value conditions will be subject to binding, except if it's an object that implements the expression interface. – ndm Mar 15 '21 at 23:37
  • @ndm Apologies, you are ofc right, thank you for pointing this out! I should have properly read the question. The DoS argument still holds though, so I will fix my answer instead of deleting it. – Gabor Lengyel Mar 16 '21 at 01:47