Bugs, Security, and Other Tweaks
There were supposed to be only eight parts to this series, but as we started releasing them, Chris and I realized that there was going to need to be a follow-up post to address some of the bug fixes, security patches, and a few other minor changes.
NOTE: All the changes we’re going to cover in this article are already included in the source code.
Bug Fixes
After releasing the live app, a handful of bugs showed up in the comments. We tried to address these as quickly as possible to keep the app from causing unnecessary grief for our users. We’ll go over the major bugs here.
Account Created, List Failed Error
The first thing we saw was that when there were more than just one or two people trying to create accounts, the app started failing to create user lists after an account was created. Upon reviewing the code, I found that the error seemed to be coming from the following line:
$userID = $this->_db->lastInsertId();
$userID seemed to be unreliable, and therefore the query to insert a new list into the database was failing regularly. To fix this, we implemented a complex query that worked around the use of lastInsertId():
/*
* If the UserID was successfully
* retrieved, create a default list.
*/
$sql = "INSERT INTO lists (UserID, ListURL) VALUES
(
(
SELECT UserID
FROM users
WHERE Username=:email
),
(
SELECT MD5(UserID)
FROM users
WHERE Username=:email
)
)";
Performance-wise, this is going to be slower than the original post, but it’s incredibly more reliable (since implementing this fix, we’ve had no reports of this error). Any MySQL supergeeks who may have a better solution, please post it in the comments!
Double-Clicking “Add” Sometimes Added Duplicate Entries
One little user interface quirk that was discovered was that you could click multiple times in succession on the “Add” button. In our original JavaScript, we only cleared the value of the input field upon a successful AJAX result. That is ideal, since when that text disappears that is your visual queue that it’s been added to your list successfully. Plus, generally that happens quickly enough it feels pretty instant. However, if you straight up “double-click” on that add button (which people absolutely still do that), you might get two or more requests off before the success comes back and clears the fields (when the field is clear, the submit button will do nothing).
One method to fix this could have been to clear the field as soon as a click happens, but the problem there is that if the save is unsuccessful you’ll lose your text. Instead, we just add a little more smarts. When the submit button is clicked and there is text ready to add, the AJAX request is made and the button is disabled. Upon success, the field is cleared and the button is re-enabled. This ensures only one submission is possible.
In /js/lists.js, we added the following at line 114:
$('#add-new').submit(function(){
// ... variables and whitelist stuff ...
if (newListItemText.length > 0) {
// Button is DISABLED
$("#add-new-submit").attr("disabled", true);
$.ajax({
// Ajax params stuff
success: function(theResponse){
// list adding stuff
// field is cleared and button is RE-ENABLED
$("#new-list-item-text").val("");
$("#add-new-submit").removeAttr("disabled");
}
NOTE: As you can see, we remove the “disabled” attribute entirely upon a successful response from our query. That is the only way to re-able a submit button. Setting disabled to “false” has no effect.
Changing Email Address with a Blank Email Crippled Account
It was also brought to our attention that clicking the “Change Email” button with a blank field would not only succeed, but would cripple the account and make it unusable. Fixing this was as simple as making sure the email address submitted was valid by inserting the following in the updateEmail() method in /inc/class.users.inc.php:
if( FALSE === $username = $this->validateUsername($_POST['username']) )
{
return FALSE;
}
Then, instead of binding the $_POST value to the query, we bind the new variable $username, which contains the valid email address if the check didn’t fail. Note the use of the new function validateUserEmail()—we’ll go over that in the next section on security.
Security Issues
After we worked the bugs out of our app, we turned to the security holes that were pointed out by commenters. Some of these were simple oversights on our part, and some of the problems were news to us. With the help of our readers, though, we tried to patch everything up.
JavaScript Could Be Inserted Into Edited Items
When creating new items, we checked for any JavaScript in the input using the cleanHREF() function, then stripped out unwanted tags on the server side using strip_tags() and a whitelist of acceptable tags. However, we had missed that JavaScript could be inserted into existing items when they were edited. To correct this issue, we turned to a preexisting input sanitizing function (lines 00326-00384) posted by Zoran in the comments of Part 8.
We wrapped the code in a method called cleanInput() and placed it in /inc/class.users.inc.php:
/**
* Removes dangerous code from the href attribute of a submitted link
*
* @param string $input The string to be cleansed
* @return string The clean string
*/
private function cleanInput($data)
{
// http://svn.bitflux.ch/repos/public/popoon/trunk/classes/externalinput.php
// ----------------------------------------------------------------------
// | Copyright (c) 2001-2006 Bitflux GmbH |
// ----------------------------------------------------------------------
// | Licensed under the Apache License, Version 2.0 (the "License"); |
// | you may not use this file except in compliance with the License. |
// | You may obtain a copy of the License at |
// | http://www.apache.org/licenses/LICENSE-2.0 |
// | Unless required by applicable law or agreed to in writing, software |
// | distributed under the License is distributed on an "AS IS" BASIS, |
// | WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or |
// | implied. See the License for the specific language governing |
// | permissions and limitations under the License. |
// ----------------------------------------------------------------------
// | Author: Christian Stocker <chregu@bitflux.ch> |
// ----------------------------------------------------------------------
//
// Kohana Modifications:
// * Changed double quotes to single quotes, changed indenting and spacing
// * Removed magic_quotes stuff
// * Increased regex readability:
// * Used delimeters that aren't found in the pattern
// * Removed all unneeded escapes
// * Deleted U modifiers and swapped greediness where needed
// * Increased regex speed:
// * Made capturing parentheses non-capturing where possible
// * Removed parentheses where possible
// * Split up alternation alternatives
// * Made some quantifiers possessive
// Fix &entityn;
$data = str_replace(array('&','<','>'), array('&amp;','&lt;','&gt;'), $data);
$data = preg_replace('/(&#*w )[x00-x20] ;/u', '$1;', $data);
$data = preg_replace('/(&#x*[0-9A-F] );*/iu', '$1;', $data);
$data = html_entity_decode($data, ENT_COMPAT, 'UTF-8');
// Remove any attribute starting with "on" or xmlns
$data = preg_replace('#(<[^>] ?[x00-x20"'])(?:on|xmlns)[^>]* >#iu', '$1>', $data);
// Remove javascript: and vbscript: protocols
$data = preg_replace('#([a-z]*)[x00-x20]*=[x00-x20]*([`'"]*)[x00-x20]*j[x00-x20]*a[x00-x20]*v[x00-x20]*a[x00-x20]*s[x00-x20]*c[x00-x20]*r[x00-x20]*i[x00-x20]*p[x00-x20]*t[x00-x20]*:#iu', '$1=$2nojavascript...', $data);
$data = preg_replace('#([a-z]*)[x00-x20]*=(['"]*)[x00-x20]*v[x00-x20]*b[x00-x20]*s[x00-x20]*c[x00-x20]*r[x00-x20]*i[x00-x20]*p[x00-x20]*t[x00-x20]*:#iu', '$1=$2novbscript...', $data);
$data = preg_replace('#([a-z]*)[x00-x20]*=(['"]*)[x00-x20]*-moz-binding[x00-x20]*:#u', '$1=$2nomozbinding...', $data);
// Only works in IE: <span style="width: expression(alert('Ping!'));"></span>
$data = preg_replace('#(<[^>] ?)style[x00-x20]*=[x00-x20]*[`'"]*.*?expression[x00-x20]*([^>]* >#i', '$1>', $data);
$data = preg_replace('#(<[^>] ?)style[x00-x20]*=[x00-x20]*[`'"]*.*?behaviour[x00-x20]*([^>]* >#i', '$1>', $data);
$data = preg_replace('#(<[^>] ?)style[x00-x20]*=[x00-x20]*[`'"]*.*?s[x00-x20]*c[x00-x20]*r[x00-x20]*i[x00-x20]*p[x00-x20]*t[x00-x20]*:*[^>]* >#iu', '$1>', $data);
// Remove namespaced elements (we do not need them)
$data = preg_replace('#</*w :w[^>]* >#i', '', $data);
do
{
// Remove really unwanted tags
$old_data = $data;
$data = preg_replace('#</*(?:applet|b(?:ase|gsound|link)|embed|frame(?:set)?|i(?:frame|layer)|l(?:ayer|ink)|meta|object|s(?:cript|tyle)|title|xml)[^>]* >#i', '', $data);
}
while ($old_data !== $data);
return $data;
}
Then, we modified updateListItem() on line 239 to call the new method:
$newValue = $this->cleanInput(strip_tags(urldecode(trim($_POST["value"])), WHITELIST));
CATCH: This function appears to encode any non-English characters. Foreign language users may see some unexpected behavior.
Cross-Site Request Forgeries
Another risk we hadn’t considered when building this app was the possibility that a malicious user could send bogus requests to our app by piggybacking on a Colored Lists user’s session in a form of attack called Cross-Site Request Forgeries (CSRF). The snippet of JavaScript below, placed on any site, would be executed if a user that was logged in to our app were to visit the page. (Huge thanks to Dan at Sketchpad for pointing this out and providing the above sample attack.)
<script type="text/javascript">
var form = document.createElement("form");
form.setAttribute("method", "post");
form.setAttribute("action", "http://coloredlists.com/db-interaction/users.php");
var fields = new Array();
fields["user-id"] = "158";
fields["action"] = "deleteaccount";
for(var key in fields)
{
var hiddenField = document.createElement("input");
hiddenField.setAttribute("type", "hidden");
hiddenField.setAttribute("name", key);
hiddenField.setAttribute("value", fields[key]);
form.appendChild(hiddenField);
}
document.body.appendChild(form);
form.submit();
</script>
To remedy this, we need to generate a token to include with each form submission that is also stored in the session. That way we can make sure the two match before executing any requests. In doing this, CSRF is virtually eliminated.
In /common/base.php, we added following at line 19:
if ( !isset($_SESSION['token']) )
{
$_SESSION['token'] = md5(uniqid(rand(), TRUE));
}
This creates a unique token for the user’s session. Then, on every form on our site, we added the following hidden input:
<input type="hidden" name="token"
value="<?php echo $_SESSION['token']; ?>" />
And updated both /db-interaction/lists.inc.php and /db-interactions/users.inc.php with the following starting at line 15:
if ( $_POST['token'] == $_SESSION['token']
&& !empty($_POST['action'])
&& isset($_SESSION['LoggedIn'])
&& $_SESSION['LoggedIn']==1 )
Now any request made without a valid token will fail. For more on CSRF, visit Chris Shiflett’s blog.
Some Input Was Improperly Sanitized
Above, we talked about the problem with blank email change requests breaking accounts, and we created a method called validateUsername() that made sure only valid email addresses were allowed to change an existing user email. That method looks like this:
/**
* Verifies that a valid email address was passed
*
* @param string $email The email address to check
* @return mixed The email address on success, FALSE on failure
*/
private function validateUsername($email)
{
$pattern = "/^([ a-zA-Z0-9]) ([ a-zA-Z0-9._-])*@([a-zA-Z0-9_-]) ([a-zA-Z0-9._-] ) $/";
$username = htmlentities(trim($email), ENT_QUOTES);
return preg_match($pattern, $username) ? $username : FALSE;
}
Essentially, it uses a regular expression to match the pattern of a valid email address, and either returns the validated email address of boolean FALSE.
Other Changes
Aside from bugs and security patches, there were a couple parts of the site that we just felt should have been better.
Made Public URLs Tougher to Guess
First, the original public list URLs were determined using dechex(), and they were short and easy to guess. We modified them to use MD5 instead to create longer, much more difficult to guess public URLs. This happens right at the list’s creation when the query calls SELECT MD5(UserID) in createAccount() on line 100.
Allowed “Safe” Links in List Items
Some links are acceptable, and we felt that our app would be much more useful if safe links were allowed in list items. To allow this, we simply removed the call to strip_tags() in formatListItems() (found in /inc/lists.inc.php on line 173):
return "tttt<li id="$row[ListItemID]" rel="$order" "
. "class="$c" color="$row[ListItemColor]">$ss"
. $row['ListText'].$d
. "$se</li>n";
The items are now sanitized on the way in, so we don’t need to worry about them on the way out.
Summary
The steps we took above helped make our app more secure and dependable. However, we know that nothing is ever perfect, so if you’ve got other bugs, security holes, or suggestions, let us know in the comments!
Series Authors
Jason Lengstorf is a software developer based in Missoula, MT. He is the author of PHP for Absolute Beginners and regularly blogs about programming. When not glued to his keyboard, he’s likely standing in line for coffee, brewing his own beer, or daydreaming about being a Mythbuster.Chris Coyier is a designer currently living in Chicago, IL. He is the co-author of Digging Into WordPress, as well as blogger and speaker on all things design. Away from the computer, he is likely to be found yelling at Football coaches on TV or picking a banjo.