0
\$\begingroup\$

I am using db_dataobject and have provided two examples of an insert and a select query below.

Can you please help me rewrite them in a more secure way using PDO? Is there any other way to improve them?

INSERT

 $user= new DataObjects_user;                
                        $user->username  = mysql_real_escape_string (strip_tags($_REQUEST['username']));
                        $user->password  =encryptpass(mysql_real_escape_string(strip_tags($_REQUEST['password'])));
                        $user->email  =mysql_real_escape_string(strip_tags ($_REQUEST['email']));   
                        $user->dob  = mysql_real_escape_string(strip_tags($_REQUEST['dob']));
                        $user->member_since  = date("Y-m-d H:i:s");
                        $id = $user->insert();

SELECT

$username= mysql_real_escape_string (strip_tags($_REQUEST['username']));
    $password=encryptpass($_REQUEST['password']);
    $user= new DataObjects_user;
    $password=mysql_escape_string(($password));


    $user->query("select activated,userid,email,username from {$user->__table} where (username = '$username' or email='$username') AND password = '$password' ");

    if($user->fetch())
    {
\$\endgroup\$
2
\$\begingroup\$

I come back to this at the end, but if this answer is tltr, basically, I end up suggesting not to write your own DB abstraction layer, It's been done before, way better than you're doing it now.
That doesn't mean the rest of this answer doesn't contain valuable tips on how to improve your code quality in general, so please do read through it:

For a start, when you're using an object to contain data, that object is responsible for that data's integrety, not the user of the object. By that I mean: if I am to use your object, you must be able to ensure that, regardless of how I use your object (I may forget the strip_tags call, for example) the input won't contain any markup.
The only way to do that is to define setter methods that take care of this:

class DataObjects_User
{
    private $name = null;
    public function getName()
    {
        return $this->name;
    }
    public function setName($name = null)
    {//setting null ~= unsetting
        if ($name !== null && !is_string($name))
        {
            throw new InvalidArgumentException('Name must be string');//don't hush-up errors
        }
        $this->name = is_null($name) ? $name : strip_tags($name);
        return $this;
    }
}

Your code appears to require me to set a creation date manualy. Why? Why not create a field in the table that has NOW() as a default value? or an ON UPDATE CURDATE clause? wouldn't that be easier?
even so, you could omit this, by simply adding a constructor to the object above, too:

class DataObjects_User
{
    private $name = null;
    private $created = null;
    public function __construct()
    {
        $this->created = new \DateTime;//I prefer to keep everything OO
    }
    public function getCreated($format = 'Y-m-j H:i:s')
    {//use class constants for the format
        if ($format === false)
        {//allow user to get the object itself?
            return $this->created;
        }
        return $this->created->format($format);
    }
    public function setCreated(DateTime $obj = null)
    {//again, null to unset
        $this->created = $obj;
        return $this;
    }
    public function getName()
    {
        return $this->name;
    }
    public function setName($name = null)
    {//setting null ~= unsetting
        if ($name !== null && !is_string($name))
        {
            throw new InvalidArgumentException('Name must be string');//don't hush-up errors
        }
        $this->name = is_null($name) ? $name : strip_tags($name);
        return $this;
    }
}

You may be wondering why I keep adding the option of setting the properties to null... well, that's perfectly simple: If these objects represent data in the DB, it's not unlikely you'll find yourself using these objects to build queries. If some of these properties have default values, or values that are irrelevant to a query, you can uset them, by passing null (or nothing, since it's the default value) to their setters.

$user = new DataObjects_User();//constructor sets created date!
$user->setName('Bobby');
    ->setCreated();//unset created

Now you can pass $user to a generic createBind() method. Besides, if you have a couple of these objects, just create an interface/abstract class if only for typehinting, and extend all dataobjects from that:

class DataObjects_User extends DataObjects_Abstract{}
//the createBind method then could look like this:
public function createBind(DataObjects_Abstract $model)
{
    $getters = get_class_methods($model);
    $bind = array();
    foreach($getters as $getter)
    {
        if (substr($getter,0,3) === 'get' && method_exists('s'.substr($getter,1), $model))
        {//getter and setter exist, this is a property
            if ($model->{$getter}() !== null)
            {//avoid null properties
                $bind[':'.substr($getter,3)] = $model->{$getter}();
            }
        }
    }
    return $bind;
}
//when applied to $user
var_dump(
    $queryObject->createBind(
         $user->setName('Bobby')
              ->setCreated()
    )
);

That should yield:

array(':name' => 'Bobby');

Which makes life a lot easier when using prepared statements, doesn't it?

since I'm on the subject: When using PDO, you don't have to bother with all those deprecated mysql_real_escape_string calls anymore... just use prepared statements. If you can't, then learn to use them. It's not that hard, and an awful lot safer at any rate than what you have now.

Basically, with the $user object, being passed through the createBind method, all you need to do is:

$bind = $queryObject->createBind(
     $user->setName('Bobby')
          ->setCreated()
);
$fields = array_keys($bind);
$query = 'SELECT * FROM tbl WHERE ';
foreach($fields as $field)
{
    $query .= substr($field,1) .' = '.$field;
}
$statement = $pdoInstance->prepare($query);
$statement->execute($bind);

This is just as secure as your code, if not more secure. The downsides:

  • the properties of your objects have to be the same as the field-names in your db
  • data and logic are too tightly coupled here...

Lastly, I'd like to point that, even though it's not something that'll leave a gaping hole in your app's security, storing passwords the way you are doing is not done. Common practice is to store password-hashes, not encrypted passwords. Encryption, as the word implies, is a two way deal: anything that is encrypted can be decrypted. A hash-function, by its very nature is a one-way algorythm: A hash can't be de-hashed (well, it can be brute-forced, but even then, collisions, salt and time-complexity make that less viable to would-be hackers). Instead of encrypting the pass, try this, inside the user model:

private $passHash = null;
const PASS_SALT = '$0m3_r@Nd0M_5tr1n9';
public function setPassHash($password)
{
    $this->passHash = hash('sha256', self::PASS_SALT.$password);
    return $this;
}

But that's just on the data model. Any abstraction layer you're writing to query the DB isn't going to cut it as your app grows. As I've stated previously, in other answers:

On the whole, I'd suggest you use Doctrine entities, though. ORM's aren't very inviting at first, but once you get the hang of how easy it is to distil tables from objects and objects from tables (yes, it works both ways), you haven't got anything to worry about.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Or, you could simply use PDO. I don't think he neads an entire ORM solution. +1 for the nice answer. \$\endgroup\$ – Pinoniq Sep 4 '13 at 15:31
  • \$\begingroup\$ thanks so much for the detailed answer , I will go through it in detail. quick question will the PDO bit apply to my code ? I am using PEAR DB_DataObject.. not sure if / how they work together. \$\endgroup\$ – ivan Sep 5 '13 at 0:35
  • \$\begingroup\$ @ivan: I don't think you can combine the two, to get the best of both worlds. Personally, I'd not use the PEAR package, simply because there's no guarantee it's installed on your host, and because it essentially does what other abstraction layers do, too: Doctrine allows you to use entities, like a DataObject, to perform queries, but it doesn't require you to actually install new extensions. Yes, it'll be a tad slower than the PEAR package, but it's better known => more documentation and more portable. That and development of PEAR packages have a nasty habit of being discontinued \$\endgroup\$ – Elias Van Ootegem Sep 5 '13 at 6:53
  • \$\begingroup\$ The package you're using is still under development ATM, but who's to say that, a year from now, it'll still be actively maintained? \$\endgroup\$ – Elias Van Ootegem Sep 5 '13 at 6:53

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.