Security in my PHP script

Question

I found this source code in a website I just purchased. Just wondering if this script is secure? Can anyone explain this to me?

<?php

if($_GET['map_loc']) {
    $code = $_GET['map_loc'];
    $result= mysql_query("SELECT ttc.continent_id, ttc.continent_id, c.name FROM territories_to_continents ttc
        INNER JOIN continents c
        ON ttc.continent_id = c.continent_id
        WHERE ttc.code = '$code'
        LIMIT 1;
        ");
    $row = mysql_fetch_array($result);
    $mapLoc = $row['name'];
}

?>

It's insecure: http://bobby-tables.com Plus it's just bad code. You simply assume your query can never fail and blindly execute/fetch data from the results. Any failure at any stage will trash later stages. — Marc B, Apr 22 '14 at 18:46
http://stackoverflow.com/questions/723195/should-i-use-or-for-not-equal-in-tsql — John Conde, Apr 22 '14 at 18:46
I'm glad you ask this question because the code is totally insecure! Don't use the old MySQL extension. Use MySQLi or PDO, and regardless of which extension you choose, use Prepared Statements! — ComFreek, Apr 22 '14 at 18:46
use [PDO](http://wiki.hashphp.org/PDO_Tutorial_for_MySQL_Developers) or Mysqli. If you are too lazy, use [pdo wrapper](https://github.com/simon-eQ/PdoWrapper) — samayo, Apr 22 '14 at 18:47
The stock answer is here: http://stackoverflow.com/questions/60174/how-can-i-prevent-sql-injection-in-php?rq=1 - kudos for actually asking! — Joel Hinz, Apr 22 '14 at 18:47

score 1 · Accepted Answer · edited May 23 '17 at 10:26

1

It absolutely not secure.... mysql_* is old and shouldn't be used. You should use PDO or mysqli instead, In the following example, I showed how it could be done with mysqli.

<?php

if(isset($_GET['map_loc'])) {
    $code = $_GET['map_loc'];

    $query = "SELECT ttc.continent_id, ttc.continent_id, c.name FROM territories_to_continents ttc
                INNER JOIN continents c
                ON ttc.continent_id = c.continent_id
                WHERE ttc.code = ?
                LIMIT 1";

    if($stmt = $mysqli->prepare($query)){
        $stmt->bind_param('s', $code);
        $stmt->execute();
        $stmt->bind_result($ttc.continent_id1, $ttc.continent_id2, $mapLoc);
        $stmt->fetch();
        $stmt->free_result();
        $stmt->close();
    }
}
?>

You should definitely check out this famous question for help: How can I prevent SQL injection in PHP?

edited May 23 '17 at 10:26

Community

1
1

answered Apr 22 '14 at 19:06

Arian Faurtosh

17,987
21
77
115

This is secure, but the API is messy. Look at all those calls. – Scott Arciszewski Apr 22 '14 at 19:17
You would use `isset` to avoid notice of unknown key. – Gumbo Apr 22 '14 at 19:22
@Scott I mean you could remove `$stmt->free_result();` and `$stmt->close();` but I think the code looks more complete if you include the calls for closes for memory management and good practice purposes – Arian Faurtosh Apr 22 '14 at 19:56
Heh. I prefer write my own class as a sanity API for PDO. Then I just say $result = $db->query($sql, $param_array); – Scott Arciszewski Apr 22 '14 at 20:11
@Scott yeah i agree! But mysqli is a bit easier for new php programmers to understand. – Arian Faurtosh Apr 22 '14 at 20:13

Security in my PHP script

1 Answers1