2

I'm developing a simple PHP database application for internal use but would like to code it to best practices. Some of my pages are receiving integer values from GET requests and I'm just wondering how much validation and sanitation is really required.

Currently I'm using $num = filter_input(INPUT_GET, 'num', FILTER_VALIDATE_INT, $num_options); with specified min and max values. From here I'm exiting with an error message if $num == false

Is it necessary to also use $mysqli->real_escape_string($num);

Currently I am not bothering because I think it's quite hard to do SQL injection using an integer...

Thanks,

Kevin

UPDATE: To clarify the query I'm doing looks like this

$sql = "SELECT employeeID, concat(FirstName, ' ', LastName) as Name FROM employee WHERE employeeID='$num'";
Kevin Morse
  • 244
  • 2
  • 13

2 Answers2

4

I see your using mysqli, your best option for security is to look into Prepared Statements.

PHP mysqli Prepared Statements

It's a bit involved for an example, but the above link has indepth examples.
Once you get the hang of it though, and build your class. It's really only a normal sql query but instead of including your values you use ?

"SELECT * FROM account WHERE username = ? AND password = ?"

and you bind your values to the statement:

array("bradley", "Passw0rd");

The security comes from, as a short answer, is the fact you don't concat the values into the query string yourself. Making it less prone to sql injection.

Bradmage
  • 1,233
  • 1
  • 15
  • 41
  • Oh whoops, I think I went off topic, sorry. Your mainly concerned about integers. But I guess with prepared statments you have to specify string/int, so that would solve your injection problem with ints, I believe. – Bradmage Mar 11 '12 at 07:25
  • I just posted the exact query I'm using. I'm familiar with prepared statements I just figured they were a little overkill for this situation. – Kevin Morse Mar 11 '12 at 07:26
  • 1
    @KevinMorse prepared statements cannot be an overkill – Your Common Sense Mar 11 '12 at 07:30
  • I agree, I have all my database code locked away in it's class, I go in and tweak it every so often. But I basically use it for everything, unless it's a quick debugging statement I just chuck in a quick raw $db->query("") :D – Bradmage Mar 11 '12 at 07:48
  • @KevinMorse to let you know, prepared statements can be reliable only if used throughout the whole site SQL operations with no exceptions. Otherwise it will do no good. – Your Common Sense Mar 11 '12 at 07:59
  • Yeah sorry about that. Shouldn't go talking about my bad habits. – Bradmage Mar 11 '12 at 08:12
  • I've read the performance overhead is not worth it for prepared statements that will only be executed once. In my case that SQL query just selects a single row so I don't really see why I would bother with the prepared statement. – Kevin Morse Mar 11 '12 at 08:19
  • That would have to be a matter of opinion. The overhead is not worth it to who? What data are you protecting? Do you can if there's vulnerabilities? When it comes down to it only the developer can decide if the overhead is worth it. It's not like comparing two different string functions that have the same outcome. – Bradmage Mar 11 '12 at 08:23
  • Fair enough. In this case there's no data being protected (other than employee names in a 20 person company) but I will look into prepared statements for the future! – Kevin Morse Mar 11 '12 at 08:28
  • Best thing to do is try and use the same library. Make a class, and keep building on it. Mine started out like this, [link](http://stackoverflow.com/a/9651249/1246494) as long as you use the same one, it will get stronger :) – Bradmage Mar 11 '12 at 08:35
0

Like many other PHP users you are taking escaping wrong. You taking it as a some sort of magic wand which makes some "evil characters" "safe".
This is wrong idea.
though prepared statements can be taken as a sort of such a magic wand, escaping is not a synonym for "SQL injection protection". It is a merely string syntax rule - no more, no less.

Is it necessary to also use $mysqli->real_escape_string($num);

It is irrelevant question.
To escape or not to escape decision have to be bound to SQL, not to the data source or any validations:

  • real_escape_string() have to be used for the sql strings, i.e. parts of the query enclosed in quotes. Have to be used unconditionally, despite of whatever previous manipulations.
  • For the any other part of the query real_escape_string() being completely useless.

An explanation:
Data validation rules can be changed.
While SQL building rules have to be explicit and unconditional. To make a developer never ask himself a question like this.
In fact, it's completely different matters: data validation and query building. Why keep in mind such details and build the query accordingly? Why not to build the query based on some set of general purpose rules, irrelevant of the data nature at all?

So, to your question again:

  • if you are adding your data to the query as is, without quotes, real_escape_string() going to be completely useless in this case, but casting/validation become essential.
  • if you are adding your data to the query using prepared statement, real_escape_string() going to be completely useless and even harmful.
  • if you are adding your data to the query in quotes - you ought to do real_escape_string() in this case.
  • it is also worth to mention that if you are adding your data to the query as a part of SQL language - as an identifier or an SQL keyword - real_escape_string() is completely useless too, as well as prepared statement. Whitelisting is your only friend here
Your Common Sense
  • 156,878
  • 40
  • 214
  • 345
  • If I understand correctly you're saying that if I'm not inserting this into a quoted part of my SQL query it doesn't need to be escaped? – Kevin Morse Mar 11 '12 at 07:20
  • If you are not inserting this into a quoted part of your SQL, escaping become completely, absolutely and utterly **useless.** – Your Common Sense Mar 11 '12 at 07:30
  • What does a quoted part of SQL mean? Please see the exact query that I've now posted in my original post. – Kevin Morse Mar 11 '12 at 07:31
  • You have your $num quoted in apostrophes. thus you are treating it as a string. Strings in SQL **have** to be escaped. – Your Common Sense Mar 11 '12 at 07:35
  • yes, in a way. At least when your identifier will be changed to string, you'll be notified with SQL error. But the way PHP users do their code makes me sick. – Your Common Sense Mar 11 '12 at 07:46
  • Well the filter_input function only returns an int or else false in which case it couldn't get into the statement so I should be fine! – Kevin Morse Mar 11 '12 at 07:55