Security flaw should be fixed, scope for improvement elsewhere,

The login class is well structured code, but has at least one critical security flaw and scope for improvement in other areas.

The critical flaw is that you don't regenerate the session id when authenticated.

This is described here:
owasp.org/index.php/Session_Managem ...

While I get that the separation of concerns principle means that this shouldn't be a requirement for a class that is only implementing authentication:

1) many people using this class will not be aware of the need to change the session id
2) given that session handling is a built-in capability in PHP there is no requirement even for dependency injection.
3) You are already using the session to record the status of authentication

Both login and logout methods should call session_regenerate_id() (regardless of the outcome of authentication in the case of login()).

-----------

After that, the next biggest issue is the way passwords are represented. Your passwords are salted with an effective salt which is good, however md5 is generally considered too weak for protecting passwords. Sha1 is signficantly more difficult to break than md5 (and is part of the base installation along with md5, but PHP's implementation actually executes faster than md5 last time I checked) but an 8 character password can be forced in aroud 17 minutes using a GPU.

security.stackexchange.com/question ...

-------------

A more minor issue is that the code does not lend itself to migrating account data to a more secure algorithm. You have implemented your own structure for managing the salt and hashed password. This adds some complications for anyone with an existing database of logins attempting to migrate to a more effective password hash. Crypt(), provides a standard notation for managing salts, hashed passwords and their associated alogorithms – but I don't think it supports BPKDF2. But I suspect anyone needing beter than bcrypt would not expect to be using your class as is.

unlimitednovelty.com/2012/03/dont-u ...

-----------

A further change I would be making before using this code (but one which would be of more benefit to less experienced developers) is to check the session is working before attempting to carry out any operation which affects the session or is dependent on it. Although the only method which does not require this check is the constructir, adding a check on session_status() here provides coverage for the rest of the methods.

Related to this is checking headers_sent() before regenerate_id().

Handling the undesired outcome of headers_sent() and session_status() will require some sort of dependency injection (overriding a default handler?).

Good day,Colin.

Thank you for your reply.

1. Issues "check the session" (4) and "regenerate the session id" (1):

Yeah, your are right. I'll fix it today.


2. Issue "md5() > sha1()" (3).

I'll fix it too, but I'm not agree that sha1() is much more sequre.

There are several main technics of breaking hashs:

- brute force attack

- dictionary attack (lookup tabels, reverse lookup tables and rainbow tables)

So it doesn't matter which hash function you use - it easy to break (we talking about GPU calculation).

And in most cases dictionary attack save a lot of time for hackers. You could easly find dictionaries for md5(), for sha1() and other hash functions, even for different variation of hash function as md5(md5()) or sha1(md5())... As usual dictionaries are very huge (I have seen one for 8Tb), and contains a lot of variation on passwords, even something like "pa$$w0rd".

So, to prevent the dictionary attack we should use salt. And there are several rules to use salts:

- salt should be as long as a result of hash function

- salt should be generated by random for each new password (including for the same user)

Both rules are provided by my class.

As alternative or addition to salt, you can use secret key technic.

But brute force attack still exists. So your hash will be breaked - it's just question of the time. Of course, you can create slow hash function (key stretching), but brute force is brute force (sha1 is longer just on 6 chars, than md5).

So, salty hash is ok for the most of cases. Also you can force your users to change passwords time to time. (My bank account force me to change the password several times a year).

Of course, if you are going to store some finacial data of your customers or something like that, you should not use md5(), sha1() or other "hash functions form a box".

It's better to use PBKDF2, bcrypt, scrypt or something like that. And, as you notice (issue 3), users wouldn't (and shouldn't) use my class as is.

* Comment "(sha1 is longer just on 6 chars, than md5)" is not actual - I have remembered that sha1() is not length constant function...

I would suggest using password_hash() since it does bcrypt and is part of the PHP core - but only since 5.5 and your code should run on anything from 5.3 up - which is still a current RHEL version. Or at least use a crypt compatible password representation - see also https://en.wikipedia.org/wiki/Crypt_(C)

PHP's crypt is part of the core and has supported md5 for some time.

hashed passwords are encoded by crypt as:

"\$${scheme}\$${salt}$x${hashedpassword}"

Where $scheme is '1' for md5, while $x is '$' or '/' depending on which documentation you read. And the salt abd hashed password are encoded using a crypt-specific mechanism (base 62?).

e.g.

$1$etNnh7FA$OlM7eljE/B7F1J4XYNnk81

This allows the same repository to hold passwords hashed with different algorithms. Hence allowing an easier migration path to more secure algorithms. And the data can also (potentially) be used by other apps.

SHA-1 output is always 160-bits raw?

I have update session routine.

And I have check Sha1. You are right about 160 bit.

Also I have found that theoretically sha1() works 25% slower than md5().

I'm going to check password_hash() on this weekend. But it's required PHP 5.5 or above.

So I should think a while.