I was shocked to find a weakness in my authentication library today. It was possible to get around the password check if you knew the username. All you had to do was to enter 00 as the password. Or you just entered 00 as the username too and you got authenticated as the first user.
The bug was a combination of these three things:
- A password (or/and username) that doesn’t contain any numbers
- MySQL type conversion in conditions – when comparing a VARCHAR column to an INTEGER value it converts VARCHAR to INTEGER, not the other way around.
- My own database wrapper was trying to be smart and didn’t wrap numeric values with apostrophes
Consider this template:
and something along these PHP lines:
-
span class=”st0″>’username’‘password’]) {
-
// will catch 0 but not 00
-
‘username’‘password’‘welcome’‘wrong password’;
-
}
-
}
I won’t go into details about mysqlValue(). The important part is that it checks for special cases of NULL, true, false and is_numeric() and runs the rest through mysql_real_escape_string(). Then it inserts all parameters into the template using vprintf().
The problem is with is_numeric(). After being evaluated as a numeric value, ’00’ is entered into the query without apostrophes.
-
span class=”st0″>’my_username’
And because any varchar that doesn’t contain a number compared to an integer is evaluated as true the result is a catastrophe.
'abc' = 00is evaluated as true.'abc1' = 00would be false however, unfortunately only few users append a number to their password. If you use 00 for both the username and the password you get authenticated as the first user who didn’t do so.Check out this false bug report or the related documentation page.
Why you could want to pass numeric values without apostrophes
I wonder myself :-) Here are some reasons I remembered:
- I need to pass NULL in some cases and for that I can’t use %d
- Using apostrophes for integer comparison (1 = ‘1’) means unnecessary type conversion that takes time. A small amount of time but still.
- An argument for abandoning the is_numeric() code: Using %d and %s in SQL templates provides a visual clue about what values can be expected there. But then I couldn’t insert NULL values.
So how to fix this mess?
What do you think? Should I remove the is_numeric() test and replace all %d with %s in all SQL templates? Remember that
printf("%d", "'123'")will return 0.
Or should I keep the is_numeric() test and rewrite vulnerable queries to use for example CAST:Update 2010-01-18: I added the real reason for wanting to preserve %s in the template.
Update 2010-01-20:
Unfortunately, what I thought to be a solution (CAST(%s AS CHAR)) has another flaw – strips leading zeroes from the username / password which results into authentication failure.
So dear reader, any ideas?