New BuyLocal Developers

With the start of a new semester comes a change in BuyLocal developers. I am passing off control of development to a group in Jeff Miner's Managing IT Resources (MITR) class at RPI, consisting of Alex Adami, Jeremy Walker, and Spencer Hakim. They will be taking over development in the coming week, and will direct the project through its next phase.

Welcome aboard, guys!

Ongoing salt discussion

John, one of the guys working on the AllForLocal project, shot me an email recently regarding my decision to drop the salt out of the password hashing process. Here is his email:

Hey,

I'm e-mailing you in response to your blogger post about password salts in the Buy Local project. I tried commenting on the post, but for one reason or another it didn't seem to work (unless it's waiting on you to okay the comment before it shows up). Anyway, since it was something security related I figured it was important enough to contact you about. Here is the post:


Hey there! I just started following this blog (I'm a developer of allforlocal.com and find it interesting to see what you're up to) and I think there might be an oversight here:

It's true that to keep the salt and password hash separate would be more secure, but that isn't the point of the salt (as far as I'm aware). The real value of the salt comes in when you have an attacker who has somehow obtained the password hashes and is trying to work back to the passwords. Once they've gotten to that point they have two choices: Either brute force the hashes (very time- and computation-expensive) or perform a lookup in a hash directory/rainbow table (http://en.wikipedia.org/wiki/Rainbow_table) which can be nearly instant. It's the second attack that salts protect against, even if the attacker has the salt as well.

The reason for this is because hash directories/rainbow tables store passwords and their resulting hashes to do quick lookups, but even a small change to the password (courtesy of salts) totally changes the resulting hash, making these sorts of lookups useless. Plus there's a bit more entropy since the salts are random. The end product is that attackers _must_ use brute force and even that route is made a bit more difficult for them.

So in short: I encourage you to keep the salts! You'll be sweating a lot less if you ever do run into security troubles down the road.


-John

My reply:

John --

I worked with securing web and database applications and servers for the US Department of Labor for three years, and out of that came the following rules I use when evaluating when and how to secure applications:

1) Confidentiality - Upholding privacy of data and preventing unauthorized access;
2) Integrity - Guarding against unauthorized or malformed inserts/updates/deletes;
3) Availability - Preventing crashes and slow performance when accessing data.

The degree to which each of these categories are important depends on the sensitivity level of the application in question. An application that runs the back-end for a credit card company, say, would require higher levels of CIA than a community website such as BuyLocal.

That said, there should be a certain base layer of security in any application. The question is then one of degree, and how much of a benefit is achieved by whatever measure is taken.

When passwords are entered into the authentication system, they are passed directly to the server without being pre-hashed. In order to pre-hash with a salt, the salt value would need to be passed to the authentication system before it could hash the password, and that isn't being done. In most implementations, the login will not be protected with SSL/TLS, which means passwords transmitted in the clear over the Internet, which is a larger threat than obtaining password hashes.

Since hashes are not distributed, and are only used at authentication time, there are very limited paths to the hash data. The hash data could be obtained through a database vulnerability, or a web vulnerability allowing access to database data, such as SQL injection. A backup copy of the database could be intercepted if it were on, say, my laptop, and my laptop were stolen. The problem here is that if the database is vulnerable, the salt column in the users table is just as vulnerable as the hashed password column, and that extra step wouldn't constitute a serious hurdle for any malicious attacker. Same thing with a database backup - the salt values would be right there with the passwords. Security like this is akin to changing the port that your database server runs on or obfuscating the engine for your server-side scripts - a few seconds of scanning with any of many readily available tools gets past these 'protections,' and implementation of the protections proves to be more hassle than it is worth.

Let me give you another 'for instance' - say someone got the password hash and looked it up in a rainbow table. Say the user's password was 'abc123' and the salt is 'f0293fec'. Based on the current implementation of salt ($hashedPass = makeHash($salt . $password)), the rainbow table would return:

f0293fecabc123

If more than one hashed value was obtained, the attacker could quickly figure out that all of the salt values are 8 character hexidecimal predicates to the actual password. A simple perl script that trims off the first 8 characters is enough to turn the rainbow table results into a password list.

I'm a believer in the mantra 'do it right or don't do it at all.' If salts will be re-implemented, they will be implemented after SSL is mandated for the login, and they will be (at a minimum) stored in a separate table, accessible only by using separate credentials (to protect against SQL injection), inserted at a random position, of random length, and preferably, of random quantity - and they will certainly push the password length beyond 64 characters in all circumstances to make rainbow table lookups virtually impossible. However, I don't believe that level of security is required for an application of this type, and I'd rather spend my time working on security measures to protect against more likely vectors of attack - such as SSL/TLS for protecting the login, protecting against SQL injection, protecting against session hijacking, XSS, etc.

Switching to HTML 5

To switch to HTML 5, all you have to do is change your doctype to the following:

HTML 5 is fully backwards compatible with all browsers, so it has all of the advantages of HTML 4.01. Plus, it lets you write strict XML compatible markup without the parser throwing errors - such as being able to close tags with a trailing slash (e.g., ). HTML 5 is basically a convergence of HTML 4 and XHTML 1.1 with a whole load of new elements thrown in, which look to be very promising.

mysql & Foreign Keys

If part of your create database code looks like this:

CREATE TABLE IF NOT EXISTS `users` (
  `id` INT(5) NOT NULL AUTO_INCREMENT,
  `password` VARCHAR(128) 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,
  `description` VARCHAR(25) NOT NULL,
  KEY `fk_userphones_userid` (`userid`),
  CONSTRAINT `fk_userphones_userid`
  FOREIGN KEY (`userid`)
  REFERENCES `users` (`id`)
) ENGINE=INNODB;

And you try to load those statements in via a $mysqli->multi_query() command, mysql will fail with error 150, but it won't fail if you either add them sequentially using a foreach() and $mysqli->real_query or if you run them through phpmyadmin or similar.

Why?

Error 150 denotes a foreign key problem, which is triggered because the field names for the foreign key don't match up. The only way to get this error to go away on a multi_query is to make the `id` field in the `users` table `userid` instead, so the field names between tables match (which is not a requirement of a foreign key). Joy.

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

Back from Honeymoon

My wife and I have returned from our two week honeymoon. I will resume work on the BuyLocal site. I am developing the second version of the code, which will involve re-writing some sections and changing the overall file structure, so it will be some time before the site will be in a workable format that I can post via Subversion for all to see. I will continue to post updates here to reflect progress.

Project Introduction and Status

The BuyLocal project has been active for over a year at this point. Over the past year, two undergraduate students, Aaron Ryden and Mike Pennisi, created the current BuyLocal website (http://www.buylocal.rpi.edu) as an open source project. I (Kevin Fodness) took over the project at the end of the spring semester, and have been working on it under the Rensselaer Center for Open Source Software (http://rcos.cs.rpi.edu, down at the time of writing due to technical difficulties). The project was created to establish a web space whereby locally owned, independent businesses could list their locaion, contact information, hours, and specialty, and optionally inventory, so that members of a community could search for a category of goods or a specific item in their local community to avoid using the mega-retailers. The project has the potential to branch out into other community-oriented activities, such as community-oriented mapping projects, in the future.

Here's where I come in. The website is fully functional as-is, but lacks the polish necessary for a public release as an open source project. I have an extensive background in web development on PHP/mySQL (which is what BuyLocal is written in), and I will be using this expertise and additional research to implement the following changes to the current codebase:

• Changing the doctype to HTML 4.01 Strict, and ensuring all HTML validates properly;
• Ensuring all CSS validates properly;
• Eliminating inline CSS declarations wherever possible;
• Establishing a more secure / best-practices directory structure;
• Fully separating form from function;
• Implementing PEAR PHP coding standards;
• Implementing phpDocumentor documentation tags and generating documentation;
• Fully commenting / documenting all code;
• Cleaning up database design / implementing best-practices & normalization;
• Testing the layout on a wide variety of browsers on a wide variety of platforms;
• Implementing the recommendations in the Web Accessibility Content Guidelines.

Once these changes have been made, I will release an updated version of the code to the Google Code page (http://code.google.com/p/buylocalopensourcerensselaer/).

I have just recently finished rebuilding and rebaselining my browser test environments, setting up a DEV site for BuyLocal for me to work on, and tracking down and reviewing the PEAR standards. I still need to review more information about object oriented PHP best practices, since I am an old-school procedural programmer, and BuyLocal is written using object oriented PHP. I hope to have an updated version of the BuyLocal codebase released at the end of June or beginning of July.