8 Replies - 682 Views - Last Post: 16 July 2013 - 06:17 AM Rate Topic: -----

#1 Lieoften  Icon User is offline

  • D.I.C Head

Reputation: 10
  • View blog
  • Posts: 171
  • Joined: 06-January 10

New To PDO, Concerns about security

Posted 15 July 2013 - 03:56 PM

Alrighty, so i was talking to some blokes on another forum (namely:stackoverflow... which is possibly the worst site i've every been to in regards to coding). But anyway, they convinced me that I should switch from Mysqli to PDO. Which I've done, and understand why people like PDO so much. however, seeing as I'm so new to PDO, I'm starting to wonder if my code is secure at all... I was wondering if you guys could just give me some pointers on how to make a code more secure and all that.

Thanks in advanced
J.S
<?php
class user
{
	protected $db;
	protected $uid;
	protected $email;
	protected $username;
	protected $citcode;
	protected $password;
	public $error;
	protected $token;
	function __construct($db)
	{
		$this->db = $db;
	}
	function error($error)
	{
		$this->error = $error;	
	}
	function passwordCrypt($password)
	{
		$password = crypt($password, '$2a$07$ThisIsSparta$');
		$this->password = $password;
		return $this->password;
	}
	function registerUser($username, $password, $email, $citcode, $db)
	{
		$username = htmlspecialchars($username);
		$verifyCC = $this->checkCitCode($citcode);
		$verifyUser = $this->checkUsername($username, $email);
		if($verifyUser == true)
		{
			if($verifyCC == true)
			{
				if(empty($username))
				{
					$this->error('no username');
				}
				else
				{
					if(!empty($password))
					{
						$password = $this->passwordCrypt($password);
						$date = date('Y-m-d');
						$sql = "INSERT INTO users(username, password, email, joindate, citcode, exp) VALUES (:username, :password, :email, NOW(), :citcode, :exp);";
						$sql .= "INSERT INTO bank_accounts(type, balance) VALUES ('personal', '10000');";
						$query = $this->db->prepare($sql);
						$query->bindValue('username', $username);
						$query->bindValue('password', $password);
						$query->bindValue('email', $email);
						$query->bindValue('citcode', $citcode);
						$query->bindValue('exp', '0');
						try{
							$query->execute(); return true;
						}catch(PDOException $e){ $e->getMessage(); }
					}
					else
					{
						return false;
					}
				}	
			}
			else
			{
				return $this->error('Could not Verify Citizen Code');
			}
		}
		else
		{
			return $this->error('Username/Password is already taken');
		}
	}
	function checkCitCode($citcode)
	{
		$sql = "SELECT amount, active FROM citizen_codes WHERE citizen_code = :citcode";
		$query = $this->db->prepare($sql);
		$query->bindValue('citcode', $citcode);
		try{ $query->execute(); 
			while($row = $query->fetch())
			{
				if($row['active'] >= $row['amount'])
				{
					return false;	
				}
				else
				{
					$active = $row['active'] +1;
					$sql = "UPDATE citizen_codes SET active = :active WHERE citizen_code = :citcode";
					$query = $this->db->prepare($sql);
					$query->bindValue('active', $active);
					$query->bindValue('citcode', $citcode);
					try { $query->execute(); return true; }
					catch(PDOException $e) { $e->getMessage(); }
				}
			}
		}
		catch(PDOException $e) { $e->getMessage(); }
	}
	private function updateExp($uid, $exp)
	{
		$sql = "SELECT exp FROM users WHERE uid= :uid";
		$que = $this->db->prepare($sql);
		$que->bindValue('uid', $uid);
		try {
			$que->execute();
			$row = $que->fetch();
			$exp = $row[0];
			$expu = $row[0] + $exp;
			$stmt = "UDPATE users SET exp= :exp WHERE uid= :uid";
			$query = $this->db->prepare($sql);
			$query->bindValue('exp', $expu);
			$query->bindValue('uid', $uid);
			try{$query->execute();}catch(PDOException $e) { $e->getMessage(); }
		}catch(PDOException $e) { $e->getMessage(); }
	}
	function checkUsername($username, $email)
	{
		$sql = "SELECT count(*) FROM users WHERE username = :username OR email = :email";
		$que = $this->db->prepare($sql);
		$que->bindValue('username', $username);
		$que->bindValue('email', $email);
		try { 
			$que->execute(); 
			$row = $que->fetch();
				if($row[0] > 0)
				{
					return false;
				}
				else
				{
					return true;	
				}
			} catch(PDOException $e) { $e->getMessage(); }	
	}
	function checkPassword($uid, $password)
	{
		$sql = "SELECT password FROM users WHERE uid = '{$uid}';";
		$que = $this->db->query($sql);
		if(!$que)
		{
			return false;
		}
		else
		{
			$password = $this->passwordCrypt($password);
			$row = $que->fetch_array();
			if($row[0] == $password)
			{
				return true;	
			}
			else
			{
				return false;	
			}
		}
	}
	function changePassword($uid, $password, $confirm, $current)
	{	
		if($this->checkPassword($uid, $current) == true)
		{
			$password = $this->passwordCrypt($password);
			$sql = "UPDATE users SET password = ? WHERE uid = ?";
			$que = $this->db->prepare($sql);
			$que->bindValue('1', $passoword);
			$que->bindValue('2', $uid);

		}
	}
	function generate_secure_token($length = 16) 
	{ 
		 
	} 
	function setSession($username, $password)
	{
		$password = $this->passwordCrypt($password);
		$username = $username;
		$sql = "SELECT uid FROM users WHERE username = :username AND password = :password";
		$query = $this->db->prepare($sql);
		$query->bindParam('username', $username);
		$query->bindParam('password', $password);
		if(!$query)
		{
			return false;
		}
		else
		{
			$_SESSION['token'] = 'onedayillbearealtoken';
			$query->execute();
			$row = $query->fetch();
			$_SESSION['uid'] = $row[0];

			return true;
		}
	}
}


Is This A Good Question/Topic? 0
  • +

Replies To: New To PDO, Concerns about security

#2 CTphpnwb  Icon User is offline

  • D.I.C Lover
  • member icon

Reputation: 2834
  • View blog
  • Posts: 9,740
  • Joined: 08-August 08

Re: New To PDO, Concerns about security

Posted 15 July 2013 - 04:56 PM

Since you're using prepared statements you shouldn't have a problem with SQL injection. It's good that you're attempting to prevent XSS but I'd use htmlspecialchars() when displaying information, not storing it. It's no danger when stored, and you might want the ability to actually execute some things when they are rendered.
Was This Post Helpful? 1
  • +
  • -

#3 Atli  Icon User is offline

  • D.I.C Lover
  • member icon

Reputation: 3636
  • View blog
  • Posts: 5,759
  • Joined: 08-June 10

Re: New To PDO, Concerns about security

Posted 15 July 2013 - 04:58 PM

Hey.

What reasons did they give you to switch from MySQLi to PDO? I can get behind switching from the old MySQL API to PDO 100%, but I'm not sure switching from MySQLi to PDO is really a major issue. At least not something you'd rewrite bunch of existing code over. - MySQLi is plenty secure, and it supports all the advanced MySQL features the old MySQL API lacked.

Anyways. Regarding the security of your code. The only thing that jumps out at me regarding that is the fact that you seem to be using a short, static salt for all your password hashes. You'd normally want to create a random salt for each password, so the password hashes themselves become unique even if two users choose the same password. - Instead of doing this by yourself, I suggest you take advantage of the new password hashing functions, or if you are not yet on PHP 5.5, try this implementation of them, meant for backwards compatibility with PHP 5.3 and 5.4.

Even if you don't want to use those functions in your code, and want to write it yourself instead, the code in the last link is open-source, so it should give you a good idea as to how you can do that safely. - Generally you should avoid writing security related code yourself when there is official or community verified code available to do the same. Less chance of bugs going unnoticed.


Also, a couple of other things I noticed. In you checkPassword method, you are still using the fetch_array method from the MySQLi library. Also in the changePassword method you seem to have forgotten to call execute.

And finally, you may want to look into the PDOStatement::fetchColumn method. You do a lot of fetching single columns from result sets, to get COUNTs and such. That method simplifies that a bit.
Was This Post Helpful? 1
  • +
  • -

#4 Atli  Icon User is offline

  • D.I.C Lover
  • member icon

Reputation: 3636
  • View blog
  • Posts: 5,759
  • Joined: 08-June 10

Re: New To PDO, Concerns about security

Posted 15 July 2013 - 05:04 PM

P.S.
I just noticed one more thing in there. Your bcrypt hash is starting with $2a. To explain why this is bad, I'll quote the manual for the crypt() function:

php.net/crypt said:

Versions of PHP before 5.3.7 only support "$2a$" as the salt prefix: PHP 5.3.7 introduced the new prefixes to fix a security weakness in the Blowfish implementation. Please refer to this document for full details of the security fix, but to summarise, developers targeting only PHP 5.3.7 and later should use "$2y$" in preference to "$2a$".


So, if you are using PHP 5.3.6 or earlier, upgrade to 5.3.7 or later! Then change the prefix to $2y :)
Was This Post Helpful? 1
  • +
  • -

#5 Lieoften  Icon User is offline

  • D.I.C Head

Reputation: 10
  • View blog
  • Posts: 171
  • Joined: 06-January 10

Re: New To PDO, Concerns about security

Posted 15 July 2013 - 05:56 PM

The reason i'm using a static Hash right now is because I haven't gotten around to implementing the hash system yet. That's actually what i'm working on as we speak.. (mostly because i'm using JustHost and i just realized i can change the version of PHP i'm using... i've been stuck on php 5.2 for the past week, when in fact i could've been using 5.4... that's fixed now, thank god).

I realized a good hour after i hadn't updated the checkPassword function. It's been converted to PDO now.

anyway; 'the others' over at SO convinced me to upgrade to PDO by telling me how 'out of date mysqli' is and how 'mysqli is prone to sql injections' so on and so forth.
Was This Post Helpful? 0
  • +
  • -

#6 Lieoften  Icon User is offline

  • D.I.C Head

Reputation: 10
  • View blog
  • Posts: 171
  • Joined: 06-January 10

Re: New To PDO, Concerns about security

Posted 15 July 2013 - 06:20 PM

Double posting because I have a PDO question that doesn't warrant its own thread and i cannot find a decent answer anywhere; does PDO support Multiqueries?
Was This Post Helpful? 0
  • +
  • -

#7 Atli  Icon User is offline

  • D.I.C Lover
  • member icon

Reputation: 3636
  • View blog
  • Posts: 5,759
  • Joined: 08-June 10

Re: New To PDO, Concerns about security

Posted 15 July 2013 - 07:00 PM

View PostLieoften, on 16 July 2013 - 12:56 AM, said:

anyway; 'the others' over at SO convinced me to upgrade to PDO by telling me how 'out of date mysqli' is and how 'mysqli is prone to sql injections' so on and so forth.

That's completely wrong.

There are three popular ways to access MySQL:
  • The old MySQL API extension, using functions such as mysql_query(). This one is old, deprecated, vulnerable to SQL Injection, and lacking support for many features introduced to MySQL after version 3. It should not be used any more.

  • The improved MySQL extension (MySQLi for short), using OOP methods such as $mysqli->query(). It also has MySQL API backwards-compatible (for the most part) functions like mysqli_query(), for those who are not ready for OOP but need support for features the old MySQL API doesn't have, like prepared statements. - It's a perfectly viable choice today.

  • PDO, which you know about already. PDO's only real advantage over MySQLi is that it provides a great standard interface to access a bunch of databases, not just MySQL. - There are of course differences between them, that many would probably count as advantages, but none that makes the use of PDO over MySQLi some vital "upgrade" that needs to happen.



So, basically, people who insist MySQLi needs to be replaced by PDO are full of it.
Was This Post Helpful? 0
  • +
  • -

#8 Atli  Icon User is offline

  • D.I.C Lover
  • member icon

Reputation: 3636
  • View blog
  • Posts: 5,759
  • Joined: 08-June 10

Re: New To PDO, Concerns about security

Posted 15 July 2013 - 07:19 PM

View PostLieoften, on 16 July 2013 - 01:20 AM, said:

does PDO support Multiqueries?

Yes, it does. At least for MySQL. Not sure if this is database specific or not.

The PDOStatement::nextRowset method can be used to go through multiple result sets. The documentation states that this is primarily meant for procedure calls that return multiple result sets, but it does work for multiple queries as well.

For example:
$sql = "SELECT * FROM the_table 
            WHERE some_date BETWEEN '2010-01-01' AND '2010-12-31 23:59:59';
        SELECT * FROM the_table 
            WHERE some_date BETWEEN '2011-01-01' AND '2011-12-31 23:59:59';
        SELECT * FROM the_table 
            WHERE some_date BETWEEN '2012-01-01' AND '2012-12-31 23:59:59';";

$result = $dbLink->query($sql);

$year = 2010;
do {
    echo "<p>Year " . ($year++) . "<br>";
    foreach ($result as $row) {
        foreach ($row as $field => $value) {
            echo "{$field}={$value}, ";
        }
        echo "<br>";
    }
    echo "</p><hr>";
}
while ($result->nextRowset());


Not the greatest example I've made (tired as hell :)), but you get the point.
Was This Post Helpful? 0
  • +
  • -

#9 CTphpnwb  Icon User is offline

  • D.I.C Lover
  • member icon

Reputation: 2834
  • View blog
  • Posts: 9,740
  • Joined: 08-August 08

Re: New To PDO, Concerns about security

Posted 16 July 2013 - 06:17 AM

My 2 cents on vulnerability to SQL injection and the three most popular APIs:
It's possible to use the same exact query in all three and be vulnerable in all three. Do something like this
$sql = "INSERT INTO users(username, password, email, joindate, citcode, exp) VALUES ($username, $password, $email, NOW(), $citcode, $exp);";

and it doesn't matter if you prepare the query in PDO or MySQLi, it will be just as vulnerable as if you'd used MySQL because you'll be "preparing" a query that has user supplied data in it. It may be why you were told to avoid MySQLi because it's easy to treat it as if it were MySQL. Although you can make the same mistake with PDO, it seems that people do it more often when using MySQLi.
Was This Post Helpful? 0
  • +
  • -

Page 1 of 1