-1

I've a php code where I get the page number from a GET request and then run a sql query to select records from the database by the page number

$maxPerPage = 20;

$page = $_GET["p"];

$applicants = DB::query('SELECT * FROM registrees ORDER BY id DESC LIMIT 
'.$page*$maxPerPage.','.$maxPerPage);

My question is can someone inject an SQL query in this code ? and if it could happen, I need examples of the sql-injection that can run here.

Your Common Sense
  • 156,878
  • 40
  • 214
  • 345
xTrimy
  • 804
  • 9
  • 23
  • yes ... just add the proper string to the GET p parameter ... 0;delete from a_table_name; – ScaisEdge Nov 23 '19 at 15:27
  • What do you mean by proper string ? – xTrimy Nov 23 '19 at 15:28
  • look above .... – ScaisEdge Nov 23 '19 at 15:28
  • Tried it now... `Notice: A non well formed numeric value encountered` – xTrimy Nov 23 '19 at 15:29
  • mine is just a sample thinked is 2 seconds .. there are more sophisticaed strings .. so don't use php var .. in sql ..or .. sanitize you input before use.. + – ScaisEdge Nov 23 '19 at 15:31
  • The problem here is the `$page` is multiplied with `$maxPerPage` so any string placed in `$page` will throw an error, am I right ? – xTrimy Nov 23 '19 at 15:31
  • Okay, I know that for sure, I asked the question because I need to know an example of the sql-injection in my case.... – xTrimy Nov 23 '19 at 15:32
  • 2
    @xTrimy I think it will be hard to inject something here, however, do you even want to run the risk? Why not code properly, like you should, and prevent sql-injection in the normal way? – KIKO Software Nov 23 '19 at 15:32
  • I asked this question in purpose of learning not because I want to run the risk :D – xTrimy Nov 23 '19 at 15:33
  • @xTrimy Then simply assume it is possible. If you want to learn then look it up. There are plenty of example out there. – KIKO Software Nov 23 '19 at 15:33
  • I thought that stack overflow would help me but I'm disappointed, thanks guys. – xTrimy Nov 23 '19 at 15:38
  • `I need examples of the sql-injection that can run here` I think this what exactly what I need and said it in my question, you helped me but didn't answer my question and know one could, but thank you and thanks to everyone who tried.... – xTrimy Nov 23 '19 at 18:05
  • 1
    Try `0;DELETE FROM registries;--` Often times when injecting you want to follow your injected SQL with a `--` to indicate to the server that anything following the injected code should be treated as a comment. I'm assuming you got the error "A non well formed numeric value encountered" because `maxPerPage` was tacking on some extra code after the `$page` injection. – JNevill Nov 23 '19 at 20:10
  • 1
    @JNevill This won't work. `$page` is multiplied by 20 before it becomes part of the query string. Multiplying anything else but a number by 20 will result in zero. If it is a number the result will be a number, and nothing is 'injected'. – KIKO Software Nov 23 '19 at 20:20
  • Ah! Ok. Totally missed that. Makes sense. I can just imagine the folks reading this in the future thinking "All I have to do to avoid sql injection is multiple all my `$_GET` variables by some number. ;) – JNevill Nov 23 '19 at 21:00
  • Casting a `$_GET` variable to a numeric using `(int)` or `(float)` will accomplish the same thing. But that doesn't work if the input is supposed to be a string. I don't know why so many developers go to such lengths to avoid using query parameters. – Bill Karwin Nov 24 '19 at 00:11

2 Answers2

4

You are being saved in this case by PHP, not by SQL.

https://wiki.php.net/rfc/invalid_strings_in_arithmetic explains the Notice: A non well formed numeric value encountered notice. PHP 7.1 introduced more strict rules about using strings in arithmetic expressions.

Your expression $page*$maxPerPage is an arithmetic expression, but you're trying to multiply a string $page by an integer. This causes the notice.

You could ignore the notice by not enabling error_reporting(E_NOTICE); or by suppressing the notice with the @ operator:

@$numberOfApples = "10 apples" + "5 apples";

Whether you suppress the notice or not, the value will be converted to a numeric, ignoring the extra text after the digits. This happens before the result of the multiplication is interpolated into your SQL string, so it's guaranteed to be an integer, therefore it's safe from SQL injection.

Another workaround would be to coerce $page to be an integer as you fetch it from the $_GET superglobal:

$page = (int) $_GET["p"];

Once you do that, you can use it in your multiplication without causing a notice. But by casting it to an integer, you've already filtered out anything that could cause SQL injection.

So the comments above were unable to provide an example of an SQL injection exploit, because there is none possible in this example.

But it's still a good habit to use query parameters instead of concatenating strings. The reason is that if you use different methods, it requires the programmer to understand deeply how expressions will evaluate in every case. If they see a string-concatenation expression like yours, they will of course notice it as a potential SQL injection vulnerability, and it will take some time to analyze it until they understand that it is safe.

You want to make your code easy to maintain by people who come after you. That means making it clear and consistent. There will undoubtedly be cases where you have to use query parameters because you're interpolating strings, not results of arithmetic expressions. Any programmer who reads your code will wonder, "why use query parameters only sometimes?"

So I agree with the other comments that you should use query parameters, not string concatenation, even when you know because of some nuance of PHP expressions that it is safe in a specific case.

Bill Karwin
  • 538,548
  • 86
  • 673
  • 828
-1

Use mysqli_real_escape_string($connection, $_GET[“p”]); to secure inputs.

Marty1452
  • 430
  • 5
  • 19