Escaping Data Structures

As we all know, to protect against SQL injection and snafus caused by single quotes in your data, always escape your inputs before running queries. However, what happens when your input is a complex data structure, such as an object, an array of objects, or an array of objects that in turn contain arrays of objects? Running foreach statements and escaping each individual value can result in very long functions, which accomplish basically the same thing (escape all values) but with different named variables and data types. Running mysqli::real_escape_string on the occasional single-dimension function parameter is fine, but for complicated data structures, you should farm out the work to a function.

I have written just such a function as part of my Database class, and it works like a charm:

   /**
    * Function to run real_escape_string recursively on an object or array.
    *
    * @param mixed &$source The object or array to escape.
    *
    * @access private
    * @return null
    */
   private function _escape(&$source)
   {
       foreach ($source as &$var) {
           if (is_array($var) || is_object($var)) {
               $this->_escape($var);
           } else {
               $var = $this->_mysqli->real_escape_string($var);
           }
       }
   }

To salt or not to salt?

I'm writing this as a clarification to my last post, where I defend dropping the `salt` column from the database.

Salt values are useful in hashes in two circumstances:

1) When there is a means by which a pre-hashed password can be passed directly to the system for verification (which is a flaw in many Microsoft products);
2) When a list of hashed passwords is obtained by a malicious user to be brute-force cracked offline.

Because of the way the application is designed, #1 is not a concern. SQL injection is protected against and at no point are passwords handed to the database without being hashed (which would mean a hash of the hash).

#2 is a potential concern, if a security hole were found that permitted getting a dump of the database or the `users` table. However, in this case, having a `salt` column in the users table would be like handing over the second launch key. Even embedding the `salt` column in a separate table in the same database would not present much of a hurdle for a malicious attacker.

The best way to handle `salt` values is to store them in a table in a separate database with a different username and password, so multiple sets of information would be needed to gain access to both the hashed passwords and the salt values.

However, this application doesn't need to be über secure, so instead of creating a separate database, I'm simplifying the codebase and database design by dropping the `salt` column altogether.

For more information: http://en.wikipedia.org/wiki/Salt_(cryptography)

Sometimes, you have to redesign the database

So, as I was moving through the code, I became increasingly dissatisfied with the data types and organization of some of the fields in the database - especially with regard to how logins and sessions work. I redesigned the database schema, which now looks like this (in INSERT syntax):

CREATE TABLE IF NOT EXISTS `users` (
`id` INT(5) NOT NULL AUTO_INCREMENT,
`password` VARCHAR(100) NOT NULL,
`salt` VARCHAR(50),
`email` VARCHAR(320) NOT NULL,
`zip` VARCHAR(10) NOT NULL,
`role` INT(1) NOT NULL,
`isactive` BOOL DEFAULT 1,
`registeredon` DATETIME NOT NULL,
`lastlogon` DATETIME NOT NULL,
PRIMARY KEY (`id`)
) ENGINE=INNODB;

CREATE TABLE IF NOT EXISTS `userphones` (
`userid` INT(5) NOT NULL,
`phone` VARCHAR(15) NOT NULL,
KEY `fk_userphones_userid` (`userid`),
CONSTRAINT `fk_userphones_userid`
FOREIGN KEY (`userid`)
REFERENCES `users` (`id`)
) ENGINE=INNODB;

CREATE TABLE IF NOT EXISTS `usernames` (
`userid` INT(5) NOT NULL,
`name` VARCHAR(50) NOT NULL,
KEY `fk_usernames_userid` (`userid`),
CONSTRAINT `fk_usernames_userid`
FOREIGN KEY (`userid`)
REFERENCES `users` (`id`)
) ENGINE=INNODB;

CREATE TABLE IF NOT EXISTS `useractivation` (
`userid` INT(5) NOT NULL,
`code` VARCHAR(10) NOT NULL,
KEY `fk_useractivation_userid` (`userid`),
CONSTRAINT `fk_useractivation_userid`
FOREIGN KEY (`userid`)
REFERENCES `users` (`id`)
) ENGINE=INNODB;

CREATE TABLE IF NOT EXISTS `sessions` (
`id` VARCHAR(50) NOT NULL,
`userid` INT(5) NOT NULL,
`ip` VARCHAR(39) NOT NULL,
`lastactive` DATETIME NOT NULL,
KEY `fk_sessions_userid` (`userid`),
CONSTRAINT `fk_sessions_userid`
FOREIGN KEY (`userid`)
REFERENCES `users` (`id`)
) ENGINE=INNODB;

CREATE TABLE IF NOT EXISTS `categories` (
`id` INT(3) NOT NULL AUTO_INCREMENT,
`name` VARCHAR(50) NOT NULL,
`description` TINYTEXT NOT NULL,
`parent` INT(3) NOT NULL,
PRIMARY KEY (`id`),
KEY `fk_categories_parent` (`parent`),
CONSTRAINT `fk_categories_parent`
FOREIGN KEY (`parent`)
REFERENCES `categories` (`id`)
) ENGINE=INNODB;

CREATE TABLE IF NOT EXISTS `stores` (
`id` INT(6) NOT NULL AUTO_INCREMENT,
`name` VARCHAR(100) NOT NULL,
`description` TEXT NOT NULL,
`dateadded` DATETIME NOT NULL,
`address` VARCHAR(100) NOT NULL,
`city` VARCHAR(50) NOT NULL,
`state` VARCHAR(50) NOT NULL,
`zip` VARCHAR(10) NOT NULL,
`country` VARCHAR(50) NOT NULL,
`latitude` FLOAT(10,6) NOT NULL,
`longitude` FLOAT(10,6) NOT NULL,
PRIMARY KEY (`id`)
) ENGINE=INNODB;

CREATE TABLE IF NOT EXISTS `storephones` (
`storeid` INT(6) NOT NULL,
`phone` VARCHAR(15) NOT NULL,
KEY `fk_storephones_storeid` (`storeid`),
CONSTRAINT `fk_storephones_storeid`
FOREIGN KEY (`storeid`)
REFERENCES `stores` (`id`)
) ENGINE=INNODB;

CREATE TABLE IF NOT EXISTS `storefaxes` (
`storeid` INT(6) NOT NULL,
`fax` VARCHAR(15) NOT NULL,
KEY `fk_storefaxes_storeid` (`storeid`),
CONSTRAINT `fk_storefaxes_storeid`
FOREIGN KEY (`storeid`)
REFERENCES `stores` (`id`)
) ENGINE=INNODB;

CREATE TABLE IF NOT EXISTS `storeemails` (
`storeid` INT(6) NOT NULL,
`email` VARCHAR(320) NOT NULL,
KEY `fk_storeemails_storeid` (`storeid`),
CONSTRAINT `fk_storeemails_storeid`
FOREIGN KEY (`storeid`)
REFERENCES `stores` (`id`)
) ENGINE=INNODB;

CREATE TABLE IF NOT EXISTS `storecategories` (
`storeid` INT(6) NOT NULL,
`categoryid` INT(3) NOT NULL,
KEY `fk_storecategories_storeid` (`storeid`),
KEY `fk_storecategories_categoryid` (`categoryid`),
CONSTRAINT `fk_storecategories_storeid`
FOREIGN KEY (`storeid`)
REFERENCES `stores` (`id`),
CONSTRAINT `fk_storecategories_categoryid`
FOREIGN KEY (`categoryid`)
REFERENCES `stores` (`id`)
) ENGINE=INNODB;

CREATE TABLE IF NOT EXISTS `storeowners` (
`storeid` INT(6) NOT NULL,
`userid` INT(5) NOT NULL,
KEY `fk_storeowners_storeid` (`storeid`),
KEY `fk_storeowners_userid` (`userid`),
CONSTRAINT `fk_storeowners_storeid`
FOREIGN KEY (`storeid`)
REFERENCES `stores` (`id`),
CONSTRAINT `fk_storeowners_userid`
FOREIGN KEY (`userid`)
REFERENCES `users` (`id`)
) ENGINE=INNODB;

CREATE TABLE IF NOT EXISTS `manufacturers` (
`id` INT(5) NOT NULL AUTO_INCREMENT,
`name` VARCHAR(50) NOT NULL,
PRIMARY KEY (`id`),
UNIQUE KEY (`name`)
) ENGINE=INNODB;

CREATE TABLE IF NOT EXISTS `models` (
`id` INT(10) NOT NULL AUTO_INCREMENT,
`name` VARCHAR(50) NOT NULL,
PRIMARY KEY (`id`),
UNIQUE KEY (`name`)
) ENGINE=INNODB;

CREATE TABLE IF NOT EXISTS `products` (
`id` INT(11) NOT NULL AUTO_INCREMENT,
`storeid` INT(6) NOT NULL,
`name` VARCHAR(100) NOT NULL,
`description` TEXT NOT NULL,
`price` DOUBLE NOT NULL,
`dateadded` DATETIME NOT NULL,
`views` INT(5) NOT NULL,
`condition` INT(1) NOT NULL,
`issoldonline` BOOL NOT NULL,
PRIMARY KEY (`id`),
KEY `fk_products_storeid` (`storeid`),
CONSTRAINT `fk_products_storeid`
FOREIGN KEY (`storeid`)
REFERENCES `stores` (`id`)
) ENGINE=INNODB;

CREATE TABLE IF NOT EXISTS `productmanufacturers` (
`productid` INT(11) NOT NULL,
`manufacturerid` INT(5) NOT NULL,
KEY `fk_productmanufacturers_productid` (`productid`),
KEY `fk_productmanufacturers_manufacturerid` (`manufacturerid`),
CONSTRAINT `fk_productmanufacturers_productid`
FOREIGN KEY (`productid`)
REFERENCES `products` (`id`),
CONSTRAINT `fk_productmanufacturers_manufacturerid`
FOREIGN KEY (`manufacturerid`)
REFERENCES `manufacturers` (`id`)
) ENGINE=INNODB;

CREATE TABLE IF NOT EXISTS `productmodels` (
`productid` INT(11) NOT NULL,
`modelid` INT(10) NOT NULL,
KEY `fk_productmodels_productid` (`productid`),
KEY `fk_productmodels_modelid` (`modelid`),
CONSTRAINT `fk_productmodels_productid`
FOREIGN KEY (`productid`)
REFERENCES `products` (`id`),
CONSTRAINT `fk_productmodels_modelid`
FOREIGN KEY (`modelid`)
REFERENCES `models` (`id`)
) ENGINE=INNODB;

CREATE TABLE IF NOT EXISTS `tags` (
`id` INT(5) NOT NULL AUTO_INCREMENT,
`name` VARCHAR(50) NOT NULL,
PRIMARY KEY (`id`)
) ENGINE=INNODB;

CREATE TABLE IF NOT EXISTS `producttags` (
`productid` INT(11) NOT NULL,
`tagid` INT(5) NOT NULL,
KEY `fk_producttags_productid` (`productid`),
KEY `fk_producttags_tagid` (`tagid`),
CONSTRAINT `fk_producttags_productid`
FOREIGN KEY (`productid`)
REFERENCES `products` (`id`),
CONSTRAINT `fk_producttags_tagid`
FOREIGN KEY (`tagid`)
REFERENCES `tags` (`id`)
) ENGINE=INNODB;

CREATE TABLE IF NOT EXISTS `messages` (
`id` INT(10) NOT NULL AUTO_INCREMENT,
`to` INT(5) NOT NULL,
`from` INT(5) NOT NULL,
`title` VARCHAR(100) NOT NULL,
`body` TEXT NOT NULL,
`created` DATETIME NOT NULL,
`isread` BOOL NOT NULL,
PRIMARY KEY (`id`),
KEY `fk_messages_to` (`to`),
KEY `fk_messages_from` (`from`),
CONSTRAINT `fk_messages_to`
FOREIGN KEY (`to`)
REFERENCES `users` (`id`),
CONSTRAINT `fk_messages_from`
FOREIGN KEY (`from`)
REFERENCES `users` (`id`)
) ENGINE=INNODB;

CREATE TABLE IF NOT EXISTS `flagtypes` (
`id` INT(2) NOT NULL AUTO_INCREMENT,
`name` VARCHAR(50) NOT NULL,
`description` TINYTEXT NOT NULL,
PRIMARY KEY (`id`)
) ENGINE=INNODB;

CREATE TABLE IF NOT EXISTS `flags` (
`id` INT(10) NOT NULL AUTO_INCREMENT,
`flagtypeid` INT(2) NOT NULL,
`itemid` INT(3) NOT NULL,
`ipaddress` VARCHAR(39) NOT NULL,
`createdate` DATETIME NOT NULL,
`isresolved` BOOL NOT NULL,
PRIMARY KEY (`id`),
KEY `fk_flags_flagtypeid` (`flagtypeid`),
CONSTRAINT `fk_flags_flagtypeid`
FOREIGN KEY (`flagtypeid`)
REFERENCES `flagtypes` (`id`)
) ENGINE=INNODB;

CREATE TABLE IF NOT EXISTS `zipcodes` (
`zip` CHAR(5) NOT NULL,
`state` CHAR(2) NOT NULL,
`city` VARCHAR(100) NOT NULL,
`latitude` FLOAT(10,6) NOT NULL,
`longitude` FLOAT(10,6) NOT NULL,
UNIQUE KEY (`zip`)
) ENGINE=INNODB;

The most important change from the last database schema is that all of the fields are set to NOT NULL and the database is significantly more normalized than the last iteration. The only field that is not set to NOT NULL is the `salt` field in the `users` table, which is a legacy holdover from the past design. The salt field was used thus:

$hashedPassword = sha1($obj->salt . $_POST['password']);

Although the `salt` value is unique to every userid, it never changes, and thus doesn't provide any additional security benefit. The only thing it does do is prevent a malicious attacker from running sha1(random_password) and generating a list of hashed values for a brute-force attack. However, the application will never accept a hashed value and pass it directly to the database without re-hashing it, so having a pre-generated list of hashes is useless. This release will end up using a two-stage check for passwords, and replacing sha1(salt . password) hashed passwords with sha256(password) hashes in the database, and clearing out the `salt` value. Once all `salt` values have been eliminated, the `salt` column can be deleted from the database, and the code can be modified.

A two-stage check would look something like this:

$hashedPassword = sha256($_POST['password']);
if ($this->db->getUser($_POST['username'], $hashedPassword)) {
/* Process login */
} else {
$hashedPassword = sha1(
$this->db->getSalt($_POST['username']) .
$_POST['password']
);
if ($this->db->getUser($_POST['username'], $hashedPassword)) {
/* Process login */
$this->db->updatePassword($_POST['username'], $_POST['password']);
} else {
print_error('Incorrect username and/or password.');
}
}

Presentation Slides & Progress

I have continued to work on standardizing and optimizing the codebase for BuyLocal. At this point, I am behind where I had hoped to be, but am on track for completing the standardization / optimization by the end of the month. Ron and I are meeting sometime this week to discuss additional changes that need to be made to the site, as well as next steps. I would imagine that another student would be picking up the project in the fall, so I will be developing transition documents for that purpose.

My slides for my presentations are available here:

http://www.kevinfodness.com/misc/20090522_update.pdf
http://www.kevinfodness.com/misc/20090626_update.pdf