Authorization bypass in Authorization.php

raymontag commented 6 years ago

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.

Dygear commented 6 years ago

This is the line you are talking about, if ($password == SYSTEMPASSWORD)?

raymontag commented 6 years ago

@Dygear Yes. E.g. just set $password in your config to '0e1'. You should be able to login with just '0' as password.

fgeek commented 6 years ago

@raymontag did you request CVE identifier for this vulnerability (https://cveform.mitre.org/)?

raymontag commented 6 years ago

@fgeek Not yet, will do this evening.

fgeek commented 6 years ago

@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

crazy4chrissi commented 6 years ago

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.

fgeek commented 6 years ago

@crazy4chrissi nice. much appreciated :)

raymontag commented 6 years ago

@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

raymontag commented 6 years ago

@crazy4chrissi One addition: From my research stable versions1.9.5 to 1.9.7.1 are affected.

carnil commented 6 years ago

CVE-2018-10362

wbowling commented 6 years ago

https://github.com/phpLiteAdmin/pla/commit/41545fe058e674a983f557bff13787df53167274#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;

raymontag commented 6 years ago

FTR: CVE-2018-10362 was assigned for this.

Edit: Sorry, didn't saw that @carnil mentioned it.

raymontag commented 6 years ago

@wbowling You are right, thanks for the addition!

MichaelGooden commented 6 years ago

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

crazy4chrissi commented 6 years ago

@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.

raymontag commented 6 years ago

@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...

MichaelGooden commented 6 years ago

Or just with simple statistical analysis: https://blog.ircmaxell.com/2014/11/its-all-about-time.html

crazy4chrissi commented 6 years ago

Please comment on the latest commit.

wbowling commented 6 years ago

@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 https://github.com/phpLiteAdmin/pla/commit/41545fe058e674a983f557bff13787df53167274

raymontag commented 6 years ago

@MichaelGooden Thx for the link :)

crazy4chrissi commented 6 years ago

@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.

MichaelGooden commented 6 years ago

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

raymontag commented 6 years ago

@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.

wbowling commented 6 years ago

Maybe @wbowling sees something I missed?

Nothing else from me

crazy4chrissi commented 6 years ago

@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 ;)

MichaelGooden commented 6 years ago

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);
        }

crazy4chrissi commented 5 years ago

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.

fgeek commented 5 years ago

@ crazy4chrissi Thank you for your work towards more secure open-source software!

Dygear commented 5 years ago

@crazy4chrissi What's your email address?

phpLiteAdmin / pla

Authorization bypass in Authorization.php #11