Authorization bypass in Authorization.php
Closed this issue · 29 comments
The attemptGrant function of the Authorization class uses '==' comparison instead of '===' comparison. This can lead to a problem if the password is a number written in scientific notation. E.g.:
php > var_dump('200' == '2e2');
bool(true)
php > var_dump('0' == '0e12345');
You should use === even if this is just a problem with a small impact.
This is the line you are talking about, if ($password == SYSTEMPASSWORD)?
@Dygear Yes. E.g. just set $password in your config to '0e1'. You should be able to login with just '0' as password.
@raymontag did you request CVE identifier for this vulnerability (https://cveform.mitre.org/)?
@raymontag thank you. Site https://www.phpliteadmin.org/ seems to list https://bitbucket.org/phpliteadmin/public/issues as official issue tracker. Can you report this there so that upstream developers are aware? If you need any help just contact me via henri@nerv.fi
I just fixed this. At 2 more places, I exchanged == with === . There, hashs are compared, so it may be a a lot harder to exploit, but let's better be at the safe side, as it does not hurt at all.
The development version download, the bitbucket and github repos should contain the fix. Stable version is affected I guess. I will check if all versions are affected and prepare a security alert post. I guess it is a good time to release a new stable version soon, after such a long time.
Please confirm that this solves your concern.
@crazy4chrissi nice. much appreciated :)
@crazy4chrissi It does fix it, indeed. Thank you for your fast reply :)
And yes, for the hash comparison it's much more unprobable from my view. There are cases where this can be a problem with hashes nevertheless. If you are interested in this, have a quick glance at the presentation by Gregor: http://gregorkopf.de/slides_berlinsides_2010.pdf
@crazy4chrissi One addition: From my research stable versions1.9.5 to 1.9.7.1 are affected.
41545fe#diff-40de8edc3e821c8cb567cbc0b253e6cbL40 is actually exploitable regardless of what the admin password is, by just iterating through random salts until an md5 happens to match 0e[0-9]{14}, eg just using the default install:
curl http://localhost:4444/ -b "pla3412_1_9_7_1=0; pla3412_1_9_7_1_salt=T1YSLHj7R;"will log you in because md5("admin_T1YSLHj7R") is 0e179250003459658275905707244744 and md5("admin_T1YSLHj7R") == "0" is true. You can also get the cookie name by performing a logout and check the headers for Set-Cookie: pla3412_1_9_7_1=deleted;
FTR: CVE-2018-10362 was assigned for this.
Edit: Sorry, didn't saw that @carnil mentioned it.
You should really be using a constant time function to compare strings like this, whether they are raw passwords or hashes:
https://secure.php.net/manual/en/function.hash-equals.php
EDIT: I realise now that you support older versions of PHP. I highly recommend use of this polyfill library to get access to hash-equals and other security enhancing features added in newer PHP versions: https://github.com/symfony/polyfill
@MichaelGooden Yeah, I thought we already did, but now realize this was only for the csrf token check. Agreed we should use hash_equals(). But no, I think we are not going to include the polyfill library. If somebody uses PHP older than 5.6 nowadays, he either doesn't care about security, or has other security measures (e.g. access blocked by firewall). If this server is publicly accessible, he probably has much bigger problems than a timing attack against his phpLiteAdmin. One main goal of our project is to keep phpLiteAdmin small, and I think its just not worth the extra bytes.
@wbowling What you mean is that this is exploitable before the changes made in 41545fe, right? Just to make sure I don't miss something else you found.
@crazy4chrissi @MichaelGooden I don't think that timing attacks are a realistic scenario against '==='. This doesn't even work on localhost without magic. Maybe with some crazy math skills in statistics...
Or just with simple statistical analysis: https://blog.ircmaxell.com/2014/11/its-all-about-time.html
Please comment on the latest commit.
@wbowling What you mean is that this is exploitable before the changes made in 41545fe, right? Just to make sure I don't miss something else you found.
@crazy4chrissi yes before the changes, it's fixed in 41545fe
@MichaelGooden Thx for the link :)
@raymontag @wbowling @MichaelGooden
What do you think about the current version? I'd like to do a new stable release, but I want to make sure that I did not miss anything.
I know password_hash() and password_verify() would be better, but I don't want to add so much code for the old php versions that don't support it.
My personal opinion is to drop older PHP versions. password_hash() and password_verify() were added in PHP 5.5.0. If you really want to support those ancient buggy insecure versions of PHP, the shim for these functions is also not that bad: https://github.com/ircmaxell/password_compat/blob/master/lib/password.php
@crazy4chrissi @MichaelGooden From my point of view timing attacks are still not an issue. Just taking the average is not enough for sure. I talked to some experienced exploit developers and they support my oppinion. However, you can go this way, it will not make it worse probably :)
Regarding your changes in general, it seems fine for me. Maybe @wbowling sees something I missed?
Regarding what function to use, I don't have strong oppinions.
@MichaelGooden For most projects I would agree to drop support for anything that receives no security updates anymore. But
- Don't raise the php requirement in a security update, if possible. This means users of PHP 5.4 could not update and would need to stay with an insecure version of phpLiteAdmin.
- It is not like there are no security updates for PHP 5.4 at all. Imho Debian wheezy still backports security fixes into php 5.4. I am not saying you should run a server with that, but there are people that do, and they have reasons,
- Just have look at the stats. This means 43% of all servers running PHP in wild currently run PHP older than 5.6.
- I agree that scripts should require new php versions to force users to update their php. But phpLiteAdmin is not the script that people will update their PHP for. They might for Wordpress or Typo3. But not for phpLiteAdmin, they would just use an old version.
And we want to keep things small, so I don't want to add too much fallback-code for old php-versions. If possible, let's just do it the old way ;)
All valid points, let me get off my soapbox and comment on the actual code ;)
The only thing I can see is to be aware that on PHP versions prior to 5.3.7 this will be vulnerable to the BCRYPT implementation issue.
EDIT: May be worth mentioning that your hash_equals shim is not multibyte safe. Consider adding the following strlen shim from Anthony Ferrara (ircmaxell)?
/**
* Count the number of bytes in a string
*
* We cannot simply use strlen() for this, because it might be overwritten by the mbstring extension.
* In this case, strlen() will count the number of *characters* based on the internal encoding. A
* sequence of bytes might be regarded as a single multibyte character.
*/
function _strlen($binary_string) {
if (function_exists('mb_strlen')) {
return mb_strlen($binary_string, '8bit');
}
return strlen($binary_string);
}Fixed in phpLiteAdmin 1.9.8 released today.
I know, this should have been released a lot faster.
If anyone wants to help me and join the phpLiteAdmin team, please contact me.
@ crazy4chrissi Thank you for your work towards more secure open-source software!
@crazy4chrissi What's your email address?