[MERGED] Security Enhancements (Session, Cookie, CSRF) & Encryption.

[OmarElgabry]

It seems there are many changes, so I'll explain everything here:

1 . CSRF Tokens
Referring to #416, CSRF tokens are implemented per session, means there is a unique token for the current user. This token will be expired after certain duration(1 day).

Another solution is per-form, but generating a new token for each form could report a false CSRF attack. That's likely to happen when you open more than one form at the same time, because one of them will have a token that's not same as the session.

I added Instruction on how to use this class in Csrf.php. Also I added two samples in login form LoginController::login(), and in change user name form LoginController::editUsername_action().

2 . Encryption & Decryption
It's likely that you will need encryption and decryption in your application. This is needed in case you want to expose something like user_id in cookie. Instead of using plain user_id, it's better to encrypt it.

3 . Session Settings
Referring to #186, Securing session could be done through different approaches.

• Setting session settings manually instead. This allows you to set session cookie settings like Expiry, Secure, HTTP Only.
• Re-generate session id on sensitive actions. When user login in, change password, ...etc it's better to regenerate the session to avoid fixated/hijacked session.
• (Not Implemented) Store session time generation, and check if expired on every request. If so, then clear session.
• (Not Implemented) Validate user's IP Address and User agent(initially will be stored in session). Although they can be faked, It's better to keep them as part of validation methods.
• And more ...

4 . Session Concurrency
Referring to #282. It's a perfectly valid situation if you have two users tries to login using the same user credentials. Allowing access your application with the same user credentials(maybe hacked account) from different devices could be also vulnerable to attacks like DDOS attacks.

So, a good solution is to allow users to login from different devices with the same credentials, but also both of them can't be logged in at the same time.

UserA logs in with his session id('123') and it will be stored in the database.
Then, UserB logs in also using the same email and password of UserA from another PC,
and also store the session id('456') in the database.

Now, Whenever UserA performs any action,
You then check the session_id() against the last one stored in the database('456'),
If they don't match then log both of them out.

1. Some other modifications:

• In deleteCookie(), Cookie token needs to be cleared in database as well as in user browser.
• In loginWithCookie(), explode() may return be split into 2 strings instead of 3, So, as i know this snippet list ($user_id, $token, $hash) = explode(':', $cookie); may trigger an error.
• In LoginController::logout() I added exit() at the end instead of typing exit() every time you call this method.

NOTE Everything is documented, please check the comments in the code. I hope everything was clear.

[panique]

This is excellent! Thank you so much, please allow some time for proper testing.
As this is so good, may I ask you if you have a background in IT security ? There are not many people in the PHP world who have a clean understanding of these things.

Have a wonderful day!

[josh-bridge]

If I'm not mistaken there is no change to the SQL installation file which should have a session_idcolumn added to the users table

Some of the functions use this field when it doesn't seem to exist, such as:

$sql = "SELECT session_id FROM users WHERE user_id = :user_id LIMIT 1";

But yes the rest seems excellent!

[OmarElgabry]

@panique I do have some, but, I am not an Expert though. Just some knowledge about Authentication, Authorization, and some famous attacks and how you can defect yourself against these attacks in PHP. I've digging into frameworks like CakePHP, Laravel, Symphony & Yii to see how those developers take care of these security issues and figure out a suitable solution for your application.

@josh-bridge Yes. I did change it while i was testing the app, I don't know how i forgot to commit it :)

[josh-bridge]

@OmarElgabry as kind of a novice at PHP and web security in general, for my project I've added a changePassword page, would this also need to be protected against CSRF?

If so, would I make a new csrf_token on the password form?
(The form consisting of old_password, new_password and new_password_repeat)

Here is the changePassword fork if you wish to view it :)

[josh-bridge]

Also this error is shown on the login page

Fatal error: Class 'Filter' not found in C:\xampp\htdocs\application\core\Session.php on line 44

Is this because Filter is not a valid class in windows? Or was this file just not committed or something?

[OmarElgabry]

CSRF attack is when a hacker tricks the user to make a request to a website. If the hacker knows the user's old password, then the hacker can login, and change it by himself.

But as your part of defensive security mechanism, It don't see any problems if you do add it too to change password forms.

[OmarElgabry]

Use mater branch because it always contain the stable version.

[josh-bridge]

@OmarElgabry

This was an error I received after committing your code, not from the HUGE branch itself (I am using the master branch)

// filter the value for XSS vulnerabilities
Filter::XSSFilter($_SESSION[$key]);

This is the line that is causing the issue, XSSFilter doesn't appear to exist? At least it doesn't come up when I google the function

[OmarElgabry]

I did nothing to get() method in Session. Please revise my commits carefully, I sent pull request from develop branch, not master.

Here is line 44 (Master) & line 44 (Develop) in Session.php. Doesn't seem to be using Filter in Master branch.

[josh-bridge]

I apologise, I see you didn't write it yourself, but it seems to be a by-product of having to commit your code to the develop branch, as when I go onto your version of the branch the code is still there: https://github.com/OmarElGabry/huge/blob/develop/application/core/Session.php#L44, and the master branch of your version doesn't seem to contain your updated code!

Anyway, all is well now! Thanks!

Edit: I see that the Filter class is defined in the panique develop branch, but since I was using the Master Branch with files replaced with the ones you edited, it still had parts of the develop code that I was unaware was different!

[panique]

@OmarElgabry Thank you very much, this is a very very good pull request, and it's extremely useful for the project! I've just merged it into develop branch and everything runs fine from first try. You code is beautiful and well documented, you've added something very great to this project!

I've for sure put your Github name into the changelog and the Readme!

By the way, the feature that the user can only be logged in from one location/browser might not be optimal for some users, so I'll try to build a switch there, allowing the developer to set in configs that users can indeed be logged in from multiple locations. In the time of always-on-applications and typical smartphones + desktop + tablet-use-cases it might be a security risk we'll have to accept to make some things run fine for the end-user. Very interesting topic!

Have a great time and thank you again!

[OmarElgabry]

Thank you for the appreciation. Just one note, When I added CSRF token in login, and editUsername form, It was just for demonstration, but definitely you(or whoever is going to use HUGE) needs to add this token to every critical actions like edit email, passwords, register, delete/edit user, ..etc.(Although I would recommend to send it with every POST, and even critical GET requests).

For the session concurrency, Yes, Maybe you can give them the option to enable/disable this feature. This line of code checks for session concurrency for every request.

Thank you!

[panique]

(personal notice)

TODO add CSRF token logic into every relevant POST request
TODO session concurrency switch
TODO documentation on CSRF logic