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, "&lt;" ).replace( />/g, "&gt;" )
}

/**
 * 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)

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