views:

324

answers:

6

I have the following code that is presenting a 'word-of-the-day', As I am relatively new to php coding, I wanted to make sure that there weren't any security issues for how I am selecting from my database from the cookie value. Thanks.

if ($word_of_the_day) {
   $wotd = $wpdb->get_results("SELECT term,definition FROM glossary WHERE term = '{$word_of_the_day}'");
   foreach ($wotd as $term) { }
}
elseif ($_COOKIE['WOTD']) {
   $word_of_the_day = htmlspecialchars(addslashes($_COOKIE['WOTD']));
   $wotd = $wpdb->get_results("SELECT term,definition FROM glossary WHERE term = '{$word_of_the_day}'");
   foreach ($wotd as $term) { }
}
else {
   $wotd = $wpdb->get_results("SELECT term,definition FROM glossary ORDER BY RAND() LIMIT 1");
   foreach ($wotd as $term) {
      setcookie("WOTD", $term->term, time()+86400);
   }
}
+1  A: 

You should check out mysql_real_escape_string: "Escapes special characters in a string for use in a SQL statement". You don't have to do the stuff that you're doing with htmlspecialchars and addslashes manually. Are you familiar with SQL injection security risks? If the variable that you're including in the SELECT statement, $word_of_the_day, comes from the user, then you have a potential SQL injection problem.

Sarah Vessels
A: 

Where does $word_of_the_day come from? If it comes from user input, you are open to SQL injection.

mgroves
+3  A: 

Well if $word_for_the_day comes from user input, there's your first problem. Do this before you use it:

$word_for_the_day = mysql_real_escape_string($word_for_the_day);

Your cookie actually looks OK. The htmlspecialchars() and addslashes() calls, in the context you're using them, don't appear vulnerable to SQL injection or XSS attacks.

cletus
+1  A: 

addslashes is extremely weak. First thing, run everything you query from the db through mysql_escape_string to prevent sql injection. That's just the basics.

if($word_of_the_day){

$word_of_the_day = mysql_escape_string($word_of_the_day); 
 $wotd = $wpdb->get_results ("SELECT term,definition FROM glossary WHERE term = '{$word_of_the_day}'");

Also, cookies in general aren't very secure no matter how secure code you write. For a much more secure solution, I recommend you use PHP sessions ($_SESSION). You can store variables in this superglobal variable and it will stay there between page loads.

http://www.php.net/manual/en/session.examples.basic.php

After that, you may want to protect against session hijacking or poisoning if you're really going for it

Stanislav Palatnik
+1  A: 

Another option you could consider would be to store the id of the word, instead of the word itself in the cookie. That way, it can only ever be an integer. Of course, using the word is fine too, as long as you mysql_real_escape_string it first, I just wanted to offer another option.

notJim
A: 

One of the safest ways is to use the PDO MySQL functions, which implements parameters:

$db = new PDO('mysql:host=hostname;dbname=defaultDbName', 'username', 'password');
$stmt = $db->prepare('SELECT term,definition FROM glossary WHERE term = :wotd');
if ($stmt) {
    if ($stmt->execute(array(':wotd' => $word_of_the_day))) { //This is safe for any input method
        $info = $stmt->fetchAll();
        foreach($info as $row) {
            //Whatever
        }
    }
}

The PDO drivers does the correct escaping / quoting according to the data type in the table.

Jrgns