0

We seem to have located an SQL injection vulnerability on one of our websites. The SQL query they are running is as follows:

select * from jobs where jobs.status='on' and industry_id=''

If the user changes the value of industry (in the URL) to the below value, then it outputs the name of the database on the search results.

-1' UNION SELECT concat(user(),0x3a3a,database()),2,3,4,5,6,7,8,9,10,11,12,13,14-- -

The PHP code that builds this part of the SQL query is:

$extra_sql = "and industry_id='".mysql_real_escape_string($_GET['industry'])."'";

I thought that if a value was escaped using mysql_real_escape_string() then this wouldn't be possible, so therefore I have a few questions:

  1. How can we fix this security problem?
  2. Is there a quick way to fix this other than to go through every single SQL query?

Thanks in advance.

Gumbo
  • 643,351
  • 109
  • 780
  • 844
V4n1ll4
  • 5,973
  • 14
  • 53
  • 92
  • 4
    use pdo or msqli - your problem is you're still using `mysql_*` functions – Novocaine Nov 12 '15 at 11:27
  • Like @Novocaine said: prepared statements ;) – GuyT Nov 12 '15 at 11:28
  • **Binding parameter** is the way to go – Lukasz Szozda Nov 12 '15 at 11:29
  • is industry_id an int or varchar/string/somethingelse? – BeNdErR Nov 12 '15 at 11:29
  • is your id only numerical? – Noob Nov 12 '15 at 11:30
  • Yes industry_id is an `int(11)` – V4n1ll4 Nov 12 '15 at 11:31
  • Then use intval(industry_id) – GuyT Nov 12 '15 at 11:34
  • 1
    See [this post](http://stackoverflow.com/a/12118602/871150) and look for the section **Safe Examples** for a guide to writing a safe SQL Query. You really do need to go through every SQL query you have, as they'll all potentially be unsecure. – Novocaine Nov 12 '15 at 11:38
  • 1
    `mysql_real_escape_string` is a patchy haphazard solution to a patchy haphazard problem, there are various illustrations of ways to sidestep the real escaping, used in the function. The thing is that the `mysqli_real_escape_string` is haphazard but it is an improvement on your version which is the older mysql_ version. As others have said, look into PDO or Mysqli OO with some haste as if people are already probing your site, they'll be grabbing your data soon enough.... – Martin Nov 12 '15 at 11:39

1 Answers1

1

change it into this

$industryID = (int) trim($_GET['industry']);

$extra_sql = "and industry_id='".$industryID."'";
Noob
  • 154
  • 1
  • 1
  • 14
  • 1
    To give some explanation to the above code, the data value required is of an integer type so that means that PHP can force the input value to be of the same time, this is a more basic level of binding but works to sidestep any abuse if the value required is an integer or other non-string. User inputs are always natively string types. – Martin Nov 12 '15 at 11:41