Is validating $_GET id in database match secure enough?

Question

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.

You should use PDO database class to prevent security issues. — Erdem Ece, Oct 23 '13 at 20:23
The error was mysqli_real_escape_string() expects exactly 2 parameters, i was reading manual about mysqli_real_escape_string on php.net but i didn't understood it right i guess. Since i set the $conn parameter $id = mysqli_real_escape_string($conn, $id); the error disappeared — lonerunner, Oct 23 '13 at 20:23
The best you can do is to [use prepared statements and parameterized queries](http://stackoverflow.com/a/60496/639282) — timwebdev, Oct 23 '13 at 20:47
As well as the SQL problems you also have HTML-injection bugs, leading to cross-site-scripting attacks. Remember to call `htmlspecialchars()` over any variables you output into an HTML page. — bobince, Oct 25 '13 at 11:16

score 5 · Answer 1 · answered Oct 23 '13 at 20:18

5

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.

answered Oct 23 '13 at 20:18

Explosion Pills

188,624
52
326
405

So there is no basically completely safe way to protect against injection, only precautions. This means that even big CMS like WordPress who use same url parameters to retrieve post are vulnerable to that. $conn parameter is connection to database, i added it to myeqli_real_escape_string and now error is gone. Ill google more on your example suggestion a bit later. – lonerunner Oct 23 '13 at 20:39
1

@AleksandarĐorđević I think that words like *completely* simply do not exist in the world of software. However, *properly* parameterized queries are at least in the realm of *safe enough*. WordPress is a bit too abstract to ensure that all queries used by the developer will be properly parameterized. There is no question in my mind that there are vulnerable WordPress sites out there, but it's not because of WordPress itself necessarily. – Explosion Pills Oct 23 '13 at 20:41
I have tried to use mysqli_prepare as you suggested but when i use it with a bit more tables it becomes a bit complex and big is it supposed to be like that or i should rather use other solution, please see the post i edited. – lonerunner Oct 24 '13 at 23:25

score 3 · Answer 2 · answered Oct 23 '13 at 21:22

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.

Is validating $_GET id in database match secure enough?

Part 2 of the question

2 Answers2

Linked