-2

I have an URL like this:

mywebsite.com?id=12345678

I retrieve id and pass it to a function like this:

$id = $_GET["id"];
if(isset($id)){
  dosomething($id);
}

I'm new to php and really don't understand if in some way the $id param could be exploited to run some code. I see it only as a string.

  • 6
    it all depends on what dosomething() does with the raw input from the user - can I craft a string that will be executed as code at any point in the function chain? – schroeder Jan 3 '18 at 11:23
2

It completely depends on what dosomething does.

One of many potential attacks is sql injection if you do not sanitize the parameters being passed to sql and don't use prepared statements.

For example, if dosomething() does something along the lines of: sql.query("SELECT * FROM users WHERE userid=$id") without checking the validity of $id first, I could simply hit your website with: ID 1 OR id!=1. Depending how what you do with the result, you may have just exposed your entire users table. Or I could be nasty and just do: ID = 1; DROP TABLE users.

So yes: you should validate all user and client input before doing anything with it.

  • Not only SQL injection, but a shell execution is possible if dosomething() does something unlikely but possible like system("cat users.txt | grep $id"). – ThoriumBR Jan 3 '18 at 12:43
  • 1
    Indeed, there are many horrible things that can happen when you don't sanitize input, I only mentioned one. – Marc Jan 3 '18 at 12:43
0

This depends on what something() is doing. In general, you don't want users to be able to pass arbitrary data into your scripts. Your code does this, as you don't check, if the user input is what you expected it to be.

Saying this in an abstract way: Never make assumptions about what you get!

Example: You generate a SQL query like this:

if(isset($_GET["id"])) {
    $sql = "SELECT * FROM table WHERE id = ".$_GET["id"].";"
    // execute SQL here
    // process results here
}

Let's assume you get this input from a form, which states, that the "id" field should only be filled with a number (or even use the HTML5 attribute).

Unaware user

This guy inserts a non-numeric value, like ab"fe; and submits the form. Your request will throw an SQL exception/error as the resulting query is:

SELECT * FROM table WHERE id = "ab"fe;"; invalid SQL

This happened because you assumed, you'd only get numeric values, but nothing prevents a user from sending something else. Even if you use the HTML5 attribute, a users browser may ignore it or the user himself crafts a specific request and sends it to your endpoint. Let's look at a worse scenario.

Malicious user

This guy tries to mess with you and sends owned"; DROP DATABASE * where the resulting SQL statement is:

SELECT * FROM table WHERE id = "owned"; DROP DATABASE *; poof, you database is gone...

Generally you always want to make absolutely sure, you know what kind of data you are processing. Use methods like is_numeric() to make sure, that the data contains what you think is does. This process of eliminating unwanted data and preparing it for being used is called sanitizing.

In addition, you should always escape strings before you use them in an SQL statement, to avoid somebody injecting special characters, like "" or ; in SQL.

Bad or the complete lack of data sanitizing is a huge vulnerability, so take care, whenever you use anything a user send to you. Check everything server-side, don't trust JavaScript forms which disable a "Submit" button, as people can still send crafted requests to your server.

0

Contrary to most answers here, for me the need does not depend on what dosomething() does. Applying defense in depth, my line of defense is to always sanitize the value arriving from outside the environment before doing anything else with it. Maybe it will be overkill, but it is better to test a little more for more security at the cost of small performance probably negligible anyway, instead of the contrary.

So, dosomething() has an API and since your language is not doing by default type and content validation, I would check the $id variable before injecting it into dosomething(). Based on its API, you know that $id should be something specific, like an integer, or a string, or whatever. So test for that, including its length as necessary, and abort prematurely and before calling doSomething() is things are not what you expected.

The only other option would be to do the same tests as the very first lines inside dosomething(). But this depends if you control that piece of code or not. For example, if you were using some libraries, the first route of action (to test before calling it) would be better, because even if the library does test things, doing it yourself too should cover more cases and create less weaknesses.

Not the answer you're looking for? Browse other questions tagged or ask your own question.