CODEWITHSUNDEEP
phpsecuritymysqligetsql-injection
lonerunner
lonerunner

Reputation: 1322

Is validating $_GET id in database match secure enough?

I have 2 pages on the website, one is index.php and index page list all posts that exist in database, and other page is post.php and post page display single post when clicked on specific post on index page.

Now the code that i used to list all posts on index.php is:

$postslist = mysqli_query($db, "SELECT * FROM posts");

while ($post = mysqli_fetch_array($postlist)) {
    echo '<a href="' .SITEURL.'/post.php?p='.$post['postid'].'>'.$post['title'].'</a>';
}

And this works and i have all posts displayed on my index.php page and links link to post on post.php page.

And on post.php page i have used code like this:

if(!isset($_GET['p'])){
    echo 'Dont load this page directly';
} 
else {
    $id = $_GET['p'];
    $querypost = mysqli_query($conn,
                    "SELECT * 
                    FROM posts 
                    WHERE postid='$id'");
            $data = mysqli_fetch_array($querypost);

        echo '<h3>' . $data['title'] . '</h3>';
}

And this works fine and retrieve post with that id but i have reading some tutorials and posts here on stackoverflow and this might be little insecure and it was suggested to use code like this just to make sure to make it safe for database use

if(!isset($_GET['p'])){
    echo 'Dont load this page directly';
} 
else {
    $id = $_GET['p'];
            $id = mysqli_real_escape_string($id);
    $querypost = mysqli_query($conn,
                    "SELECT * 
                    FROM posts 
                    WHERE postid='$id'");
            $data = mysqli_fetch_array($querypost);

        echo '<h3>' . $data['title'] . '</h3>';
}

But this throws an error, so is it secure enough just check against database if postid exists and how do i make it secure if this isn't secure enough?

Part 2 of the question

Edit: Well i have taken in to search about methods posted from you guys and after few hours i made it work with mysqli_prepare but using it into post.php is fairly easy as it only connects to posts table and pull all data from one table based on post id.

But when i tried out same method on different page this became rather big solution.

On second page i have to pull data from 5 different tables which are joined using LEFT JOIN to all match of specific id from specific column in table, and this is what it came out only using 3 tables.

$stmt = mysqli_prepare($conn,
            "SELECT * 
            FROM giveaways 
            INNER JOIN members
            ON giveaways.creator = members.steamID
            INNER JOIN sc_steamgames
            ON giveaways.gameid = sc_steamgames.appid
            WHERE giveawayID=?");
mysqli_stmt_bind_param($stmt, "i", $id);

mysqli_stmt_execute($stmt);

mysqli_stmt_bind_result($stmt, $id, $creator, $comment, $tcreated, $tstarting, $tfinish, $provider, $type, $gameid, $memberid, $steamid, $username, $profileurl, $avatar, $avatarmed, $avatarbig, $steamgames, $regdate, $verified, $coins, $gold, $points, $appid, $title, $storeprice, $valuedprice, $pointsworth);

mysqli_stmt_fetch($stmt);

echo $creator .' - '. $comment . ' - '. $gameid . ' - ' .$title.' - '.($storeprice /100) ;
mysqli_stmt_close($stmt);

And this works fine, but you can see how massive it become with 3 tables and i need to pull info from 2 more tables so i was wondering if this is really solution that you would use ?

And another question, if user have to browse a page with static value like

index.php?go=upcoming

Do i need to use also some more security or using it like now

if(isset($_GET['go']) && $_GET['go'] == 'upcoming')

is secure enough? Since there is known value of go and what to expect.

Upvotes: 3

Views: 789

Answers (2)

Bill Karwin
Bill Karwin

Reputation: 562651

If your id column is an integer, use this:

$id = (int) $_GET['p'];

That will be safe, because the (int) type coercion strips out any illicit characters.

Making SQL safe can also be as simple as: always use parameters for dynamic values. This works for integer values and strings and dates. Then you don't need to worry about quoting or escaping or filtering. @ExplosionPills shows an example.

Upvotes: 3

Explosion Pills
Explosion Pills

Reputation: 191779

It's throwing an error because mysqli_real_escape_string expects two arguments, the first of which is the connection $conn.

If you do this it should be secure enough, but it would be better to use a parameterized query. For example:

$stmt = mysqli_prepare($conn, "SELECT cols FROM posts WHERE postid = ?");
mysqli_stmt_bind_param($stmt, 'i', $id);

Checking that the id exists in the database is not secure against injection at all since you have to use a potentially malicious id in the query to do the check in the first place.

Upvotes: 5

Related Questions