CODEWITHSUNDEEP
phpmysqlsecurity
user629300
user629300

Reputation: 47

PHP MySQL vulnerability

I use this function for preventing SQL injection

  function cleanQuery($string)
{
  if(get_magic_quotes_gpc())  // prevents duplicate backslashes
  {
    $string = stripslashes($string);
  }
  if (phpversion() >= '4.3.0')
  {
    $string = mysql_real_escape_string($string);
  }
  else
  {
    $string = mysql_escape_string($string);
  }
  return htmlentities($string);
}

and i use it like that 

$sql = "select * from news where id = ".cleanQuery($id)." ";

the query should be safe when calling the page.php?id=1 however when adding ' to the end of URL like that page.php?id=1' it gives error

Warning: mysql_fetch_object(): supplied argument is not a valid MySQL result resource

that means the page is still having a vulnerability i think, any body has a solution ?

Upvotes: 1

Views: 1588

Answers (6)

davogotland
davogotland

Reputation: 2777

prepared statements is always the way to go about sql queries. php has a library called mysqli. the fact that the "i" in mysqli stands for "improved" says alot :)

here's an example! first, i did this to my database:

create database mydatabase default character set = UTF8;
use mydatabase;
create table news(id int auto_increment, title varchar(50), body text, primary key (id));
insert into news(title, body) values('good news','are good');
insert into news(title, body) values('old news','are old');

and then i used this php script (named news.php) to access my table:

<?php
    //my root user doesn't have a password, so third argument is empty string
    $db = new mysqli("localhost", "root", "", "mydatabase");

    if(mysqli_connect_errno()) {
        die("unable to connect to database: " . mysqli_connect_error());
    }

    //change character set to utf8
    if(!$db->set_charset("utf8")) {
        die("Error loading character set utf8:\n{$mysqli->error}");
    }

    //the question marks denote parameters to be bound
    $sql = "SELECT * FROM news WHERE id BETWEEN ? AND ?;";
    $statement = $db->stmt_init();
    $statement->prepare($sql);
    $sqlError = $db->error;

    if($sqlError != "") {
        die("there was a problem with your query<br />\n$sql<br />\nerror reports:<br />\n$sqlError");
    }

    //the "i":s denote both parameters to bind are int
    $statement->bind_param("ii", $min, $max);
    $min = $_GET['min'];
    $max = $_GET['max'];
    $statement->execute();
    $statement->store_result();
    $statement->bind_result($id, $title, $body);

    //everytime fetch is called, a new line is attempted to be read.
    //if a line was read, two things happen:
    //1. true is returned
    //2. the values of the columns in the fetched result row is stored in the
    //      variables bound as results on the line above
    while($statement->fetch()) {
        print "$id, $title, $body";
    }

    $statement->free_result();
    $statement->close();
    $db->close();
?>

i called the script like so:

http://localhost/news.php?min=1&max=2

Upvotes: 1

Kenaniah
Kenaniah

Reputation: 5201

Use PDO's prepared statements. Prepared statements have the nice property of preventing all forms of parameter injection when you parametrize every external variable in your query. That said, be careful of data type conversions.

Upvotes: 4

Emmanuel
Emmanuel

Reputation: 5403

If it's an id, just use is_numeric to validate:

if(ctype_digit($_GET['id'])){
    $id = $_GET['id'];
} else {
    $id = 1; // or whatever your default id is
}

Or inline:

$sql = "select * from news where id = '".(ctype_digit($_GET['id']) ? $_GET['id'] : 1)."'";

Upvotes: 1

dobs
dobs

Reputation: 2764

For integer use (int)

$sql = 'select * from news where id = '.(int) $_GET[id];

Upvotes: 2

Pekka
Pekka

Reputation: 449613

The query will break because your condition is not wrapped in quotes. Thus, only numeric values can work.

To make it not break, validate the number before passing the query, or use

$sql = "select * from news where id = '".cleanQuery($id)."' ";

By the way, htmlentities() is unnecessary and potentially harmful in your sanitation function.

Upvotes: 1

Czechnology
Czechnology

Reputation: 14992

If your id is numeric, the easiest solution is simply

$sql = "select * from news where id = '".intval($_GET['id'])."'";

Upvotes: 1

Related Questions