When Sanitization Kills
Sorry about the dramatic title; I really doubt that input sanitization has killed anything.
TL;DR:
Don't sanitize password inputs.
TL;RA*:
Can you spot the bugs in the following code?
/**
* Sanitizes the given input by removing "<", and ">" characters.
*/
function sanitizeUserInput ( input ) {
return input.replace( /</g, "<" ).replace( />/g, ">" )
}
/**
* For Beta Site: Updates the user's password in the database.
*/
function updatePasswordBeta ( username, plainTextPassword, callback ) {
// validate password
if ( plainTextPassword.length < 6 ) {
process.nextTick( function() {
callback( "Your password sucks." );
});
return;
}
// just assume secureHash properly salts and hashes its input
var hashedPassword = secureHash( plainTextPassword );
// just assume update password behaves correctly...
updatePassword( username, hashedPassword, callback );
}
/**
* For Live Site: Updates the user's password in the database.
*/
function updatePasswordLive ( username, plainTextPassword, callback ) {
// validate password
if ( plainTextPassword.length < 6 ) {
process.nextTick( function() {
callback( "Your password sucks." );
});
return;
}
var sanitizedPassword = sanitizeUserInput( plainTextPassword );
// just assume secureHash properly salts and hashes its input
var hashedPassword = secureHash( sanitizedPassword );
// just assume update password behaves correctly...
updatePassword( username, hashedPassword, callback );
}
/**
* For the Beta Site: Checks if the given username and password are valid.
*
* @param {String} username
* @param {String} plainTextPassword
* @param {Function} callback - Defined as: function(err, isValid)
*/
function verifyEmailAndPasswordBeta ( username, plainTextPassword, callback ) {
var loginHashedPassword = secureHash( plainTextPassword );
getUserHashedPassword( username ).then( function( err, dbHashedPassword ) {
if ( err ) { callback( err ); return; }
callback( null, dbHashedPassword !== null && loginHashedPassword === dbHashedPassword);
});
}
/**
* For the Live Site: Checks if the given username and password are valid.
*
* @param {String} username
* @param {String} plainTextPassword
* @param {Function} callback - Defined as: function(err, isValid)
*/
function verifyEmailAndPasswordLive ( username, plainTextPassword, callback ) {
var loginHashedPassword = secureHash( sanitizeUserInput( plainTextPassword ) );
getUserHashedPassword( username ).then( function( err, dbHashedPassword ) {
if ( err ) { callback( err ); return; }
callback( null, dbHashedPassword !== null && loginHashedPassword === dbHashedPassword);
});
}
If you can't find the bugs, don't worry, the engineers at cloudflare.com, a leader in Internet security, didn't spot them either!
Here are some hints:
- The password
<>
is absolutely not a valid password. - The password
<>
is a valid password. - The validation for logging in and updating passwords differs between the beta and live sites.
This causes some very strange behavior:
If you set your password to <>
using either site, you can then log in via the live site with both the strings <>
and <>
! That's right, your 8 character password now has an entropy of ~2.
If you set your password to 12345<
on the live site, you can't log in with that password on the beta site; you'll have to use 12345<
instead – while both of those passwords will work on the live site!
"But I thought sanitizing user input was a good thing?"
It absolutely is; but only in cases where it is actually needed:
- When outputting the input into an environment where that input may be treated as more than just a string (HTML, CSS, JavaScript, XML, JSON, Command Lines, etc.)
- For HTML sanitization/escaping:
- PHP: htmlspecialchars
- Node.js/io.js: lodash.escape
- C#: System.Web.HttpUtility.HtmlEncode
- For URL query sanitization/escaping:
- PHP: urlencode
- Node.js/io.js: encodeURIComponent
- C#: HttpUtility.UrlEncode(String)
- For HTML sanitization/escaping:
And not where it's not:
- SQL queries. Use parameterized SQL queries instead. Always. Beware of Little Bobby Tables (mobile link)?
- Databases.
- Annnnnd Passwords, obviously.
Of course, treat these as guidelines, not rules.
* Too Long, Read Anyway