Is this script safe enough from sql injections?

Question

Is this script safe enough from sql injections? Or is it possible to improve it more efficiently? Because i am going to use it in public and don't know about this line "mysql_real_escape_string($_GET['user_id']);" Perhaps its possible to improve it more.

<?
    $id = mysql_real_escape_string($_GET['id']);

    if ($id == 1)
      {
        $userinfo['user_id'] = mysql_real_escape_string($_GET['user_id']);
        $info = $db->fetchArray("SELECT points FROM ". PREFIX ."list WHERE user_id = '{$userinfo['user_id']}'");

        if (!empty($info))
        {
            $user_rank = UserRank($userinfo['user_id']);

            header('Content-type: image/png');
            $points = $info['server_points'];
            $line = "empty";
            $nr = "Number";
            $font = 3;
            $font2 = 2;
            $width = ImageFontWidth($font)* strlen($nr) ;
            $width2 = ImageFontWidth($font)* strlen($points);
            $height = ImageFontHeight($font);

            $im = ImageCreateFrompng(SYS_USER .'/banner.png');
            $points_text_color = imagecolorallocate($im, 225, 100, 112);
            $nr_text_color = imagecolorallocate ($im, 217, 153, 101);
            $line_color = imagecolorallocate ($im, 100, 123, 134);
            imagestring ($im, $font, 40, 18, $points, $points_text_color);
            imagestring ($im, $font2, 40, 11, $line, $line_color);
            imagestring ($im, $font2, 40, 4, $nr, $nr_text_color);
            imagestring ($im, $font, 60, 4, $user_rank, $nr_text_color);

            imagepng($im); 
        }
    }

Well, if you used is_numeric() you could be sure only number will get through — Bojan Kogoj, Sep 25 '11 at 12:09
also, how can $id == 1? mysql_real_escape_string returns a string or false... — chacham15, Sep 25 '11 at 13:49

score 2 · Answer 1 · answered Sep 25 '11 at 12:21

2

for $id use function is_numeric():

if(is_numeric($id)) { // if id not numeric -> false else -> true
   ...
}

answered Sep 25 '11 at 12:21

Kroko

29
4

chacham15 · Answer 2 · 2011-09-25T12:43:16.723

I would seriously consider using bind_param when interacting with sql variables. Your code is such a good example of strings coming from many different places which could possibly have been jeopardized. bind_param enforces that there are no injection attacks in the strings you pass in. If for anything, itll at least give you enough peace of mind not to worry so much to ask this question.

Example:


$name = "Robert ') DROP TABLE Students;"; //see: http://xkcd.com/327/
$conn = new mysqli(DB_SERVER, DB_USER, DB_PASSWORD, DB_NAME) or 
    die('There was a problem connecting to the database.');
$query = "SELECT id FROM Users WHERE name=?";
if ($stmt = $conn->prepare($query)) {
    $stmt->bind_param('s', $name);
    $stmt->execute();
    $stmt->bind_result($result);
    while ($stmt->fetch()) {
        echo $result;
    }
    $stmt->close();
}

Is this script safe enough from sql injections?

2 Answers2