1

This is a snippet from a --very old-- php program (used by my school) to search events in a database. I barely know php, so I'm sorry if this question is very obvious, but this part of the program seems very insecure to me. Am I missing something that is preventing SQL injection here? Are the strtolower or the "%" around the search text a security measure or do they have another function?

      $strSuchtext='%' . strtolower($strSuchbegriff) . '%';
      $strSQL="SELECT DISTINCT tuv.uv_id,tuv.fach_id,
                               tuv.stufe_id,tuv.kursart_id,tuv.zug_id
             FROM $strGtabEinUnterrichtsvorhaben AS tuv
            WHERE (tuv.schulform_id=$numSF_ID)
              AND (LOWER(tuv.uv_titel) LIKE '$strSuchtext' )
                  $strCond";
      $stmtSQL = db_exec($strSQL,$boolDEBUG);

"$strSuchtext" is so far unchanged, unfiltered user input. here is the whole code, in case i didn't copy something important: https://pastebin.com/Fjy8b50V

hundertfuenfzigmeterpeter
  • 11
  • 2
  • I don't see any measures here – Your Common Sense Apr 21 '22 at 17:32
  • 2
    No, you're not missing anything, this is very vulnerable – ADyson Apr 21 '22 at 17:38
  • The % is a wildcard character - google how the SQL LIKE operator works to understand more. – ADyson Apr 21 '22 at 17:39
  • And if you look in the php documentation to see what strtolower does you'll soon see that it doesn't have anything to do with sql or security – ADyson Apr 21 '22 at 17:40
  • 1
    There's also a possible attack vector via whatever gets put into the `$strCond` variable. – Alex Howansky Apr 21 '22 at 17:41
  • The truth is, you should never modify any input to prevent SQL injection – Your Common Sense Apr 21 '22 at 17:48
  • I expect that if you're wondering if _any_ legacy code is vulnerable to SQL injection, just assume it is. There's a more than even chance you'll be right. – Bill Karwin Apr 21 '22 at 17:58
  • 2
    The only "modification" to user input that would prevent SQL injection is casting it to integer. That's obviously not usable if you need to query non-integer columns, as often happens. Even [mysqli_real_escape_string](https://stackoverflow.com/questions/5741187/sql-injection-that-gets-around-mysql-real-escape-string) has its weaknesses. Just use prepared statements across the board. – Markus AO Apr 21 '22 at 18:12
  • @AlexHowansky and another vector in $numSF_ID (column name in the join). Since table/column names can't be bound into a prepared statement, they need to be protected either by matching a white-list of known columns (gold standard), or sanitizing to / validating against safe characters only (basically `[a-z0-9_]`). Some purists will frown at the second approach, but it's often the practical thing to do for a quick/fluid task. (A non-existent table/column name will make the query fail, but it won't alter the syntax.) – Markus AO Apr 21 '22 at 18:28
  • @MarkusAO I just cloned the repository to my own server to try to exploit it. Unfortunately I noticed only now that I had probably looked at an older branch. The function "ut_get_webpar" (the function that reads the parameters from the browser) was later changed to filter the input with `htmlspecialchars($_REQUEST[$strParamName],ENT_QUOTES);`. Since the parameter is only used once here, this would mean that a null byte injection would not work, right? – hundertfuenfzigmeterpeter Apr 21 '22 at 19:08
  • htmlspecialchars isn't a filter, it's an encoding function. This might accidentally prevent a certain subset of attacks but it's not what it's designed for and it's not any protection against most types of sql injection – ADyson Apr 21 '22 at 19:19
  • @ADyson But in this particular context it prevents SQL injection, because you can't escape the quotes, right? – hundertfuenfzigmeterpeter Apr 21 '22 at 19:25
  • 1
    _possibly_ ... I haven't tried it. But I wouldn't rely on it because it's the coding equivalent of stuffing a sock into a pipe to prevent leaks...completely the wrong tool for the job, so there are no guarantees. Using prepared statements and parameters is the only foolproof, 100% way to be sure you're not vulnerable – ADyson Apr 21 '22 at 19:34
  • [Is htmlspecialchars enough to prevent an SQL injection](https://stackoverflow.com/questions/22116934/is-htmlspecialchars-enough-to-prevent-an-sql-injection-on-a-variable-enclosed-in) -- the answer is no, but you need multiple injection points. In other notes. When a function located somewhere else delivers user input that's then "assumed safe": Does it specifically ensure the data is SQL injection safe? And if so, does it _also_ ensure that the data is XSS safe? (Output of user input can be triggered with form errors, as seen in your full code.) _Is there a sock on the sock in the pipe?_ – Markus AO Apr 21 '22 at 19:52

0 Answers0