Hey! I'm making a game called Terra Mango. You should check it out.

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 &lt;&gt; 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 &lt;&gt; using either site, you can then log in via the live site with both the strings &lt;&gt; 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&lt; 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:

And not where it's not:

Of course, treat these as guidelines, not rules.


* Too Long, Read Anyway

Comments for this blog entry