New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Authorization bypass in Authorization.php #11

Open
raymontag opened this Issue Apr 23, 2018 · 26 comments

Comments

Projects
None yet
7 participants
@raymontag

raymontag commented Apr 23, 2018

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

This comment has been minimized.

Dygear commented Apr 23, 2018

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

@raymontag

This comment has been minimized.

raymontag commented Apr 23, 2018

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

@fgeek

This comment has been minimized.

fgeek commented Apr 23, 2018

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

@raymontag

This comment has been minimized.

raymontag commented Apr 23, 2018

@fgeek Not yet, will do this evening.

@fgeek

This comment has been minimized.

fgeek commented Apr 23, 2018

@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

This comment has been minimized.

Contributor

crazy4chrissi commented Apr 23, 2018

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

This comment has been minimized.

fgeek commented Apr 23, 2018

@crazy4chrissi nice. much appreciated :)

@raymontag

This comment has been minimized.

raymontag commented Apr 24, 2018

@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

This comment has been minimized.

raymontag commented Apr 24, 2018

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

@carnil

This comment has been minimized.

carnil commented Apr 25, 2018

@wbowling

This comment has been minimized.

wbowling commented Apr 25, 2018

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;

@raymontag

This comment has been minimized.

raymontag commented Apr 25, 2018

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

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

@raymontag

This comment has been minimized.

raymontag commented Apr 25, 2018

@wbowling You are right, thanks for the addition!

@MichaelGooden

This comment has been minimized.

MichaelGooden commented Apr 25, 2018

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

This comment has been minimized.

Contributor

crazy4chrissi commented Apr 25, 2018

@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 crazy4chrissi reopened this Apr 25, 2018

@raymontag

This comment has been minimized.

raymontag commented Apr 25, 2018

@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

This comment has been minimized.

MichaelGooden commented Apr 25, 2018

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

crazy4chrissi pushed a commit that referenced this issue Apr 25, 2018

@crazy4chrissi

This comment has been minimized.

Contributor

crazy4chrissi commented Apr 25, 2018

Please comment on the latest commit.

@wbowling

This comment has been minimized.

wbowling commented Apr 25, 2018

@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

crazy4chrissi pushed a commit that referenced this issue Apr 25, 2018

@raymontag

This comment has been minimized.

raymontag commented Apr 25, 2018

@MichaelGooden Thx for the link :)

@crazy4chrissi

This comment has been minimized.

Contributor

crazy4chrissi commented Apr 26, 2018

@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

This comment has been minimized.

MichaelGooden commented Apr 26, 2018

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

This comment has been minimized.

raymontag commented Apr 26, 2018

@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

This comment has been minimized.

wbowling commented Apr 27, 2018

Maybe @wbowling sees something I missed?

Nothing else from me

@crazy4chrissi

This comment has been minimized.

Contributor

crazy4chrissi commented Apr 27, 2018

@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

This comment has been minimized.

MichaelGooden commented Apr 27, 2018

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);
        }
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment