Skip to content
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

Broken Authentication and session management OWASP A2 #2314

Closed
Songohan22 opened this issue May 11, 2020 · 10 comments
Closed

Broken Authentication and session management OWASP A2 #2314

Songohan22 opened this issue May 11, 2020 · 10 comments

Comments

@Songohan22
Copy link

Songohan22 commented May 11, 2020

Describe the bug
Hello every one, I have found a small vulnerbility your PHP Fusion.

Here is the error details.(Broken Authentication and session management PHP Fusionon)
Cookies are used to maintain session of the particular user and they should expire once the user logs out of his PHP Fusion account.In secure web application,Cookies immediately expire once the user logs out of his account.
But this is not happening in the case of HP Fusion same cookies can be used again and again to open the session of the victim.
In this Loop Hole The Application does not destroy session after logout.. means the cookies are working to login to user account & change account Information, The Cookies are usable i'm able to access the account & edit info.

If anything is wrong please reponse to me. Thank you.

To Reproduce
Steps to reproduce the behavior:

  1. Login to your account.......
  2. Get the cookies using " Brub Suite" or "EditThisCookie"
  3. Logout from the account...
  4. Clear all the cookies related to PHP Fusion
  5. Save the cookies you copied in a text file...
  6. Now Injact/Import Old Cookies to the PHP Fusion by "EditThisCookie"...
  7. As u can see.. you will be again logged In to PHP Fusion .. using old session cookies..

For More Information about This Vulnerability You can check OWASP Guide
https://www.owasp.org/index.php?title=Broken_Authentication_and_Session_Management&setlang=en

Here is a Video POC: https://drive.google.com/file/d/14IIIn0HcVyBk_kfcUJsZdpYgx72xdBZa/view

@Songohan22
Copy link
Author

I look forward to listening your reply.

@Songohan22
Copy link
Author

More information POC link: https://drive.google.com/open?id=1c_wN77CkS5nEK_tAUaRDwMo5g-Zx55K4
Impact: Means the cookies are working to login to user account & change account Information, The Cookies are usable i'm able to access the account & edit info, use all function user.

@FrederickChan
Copy link
Member

FrederickChan commented May 13, 2020

The current cookie is built using the user salt which was probably never updated during logout. I will make the amends tomorrow. If we generate a new one, it will fix the issue.

Thank you for the report.

FrederickChan added a commit that referenced this issue May 14, 2020
@FrederickChan
Copy link
Member

While we need to have a raw password input to create a new hash, it is quite absurd to ask for one during logout. My solution is to use $_SESSION storage to store future data which we will use during Logout. Please verify that you cannot access $_SESSION storage throughout the whole login session. The $_SESSION is "logout_hash" before we close this issue. If your tool can access this session, the storage medium must be SQL, and we will need to impose on the upgrade of this fix as a major instead of minor.

So please advice if anyone can tell me "Yes, I can access these variables with my superman tool", with proof, then we will fix this as a major with 3 new table columns in DB_USERS.

@FrederickChan
Copy link
Member

@JoakimFalk @RobiNN1 , please do not let this issue hinder the new release timeline since it is quite critical that one whether this patch is good enough or not. See my comments above.

Regardless, the patch is done, and IF the patch is not good enough, it could be a major here. We will probably be looking at an upgrade release.

@Songohan22
Copy link
Author

Hello @FrederickChan
Solutions fix issue:

  • User session should be destroyed when logged out.
  • Also add a csrf token into the cookie.
    Thank you.

@FrederickChan
Copy link
Member

FrederickChan commented May 14, 2020

User session should be destroyed when logged out. Also add a csrf token into the cookie.
We did that both over a century ago. 👍 During a point of time, OWASP techniques were not founded yet. Credits to previous lead developers on this.

What we didn't to do was to invalidate it by creating a new one. Implement Round Robin method on the CSRF Token itself. My patch did that, but the method employed is utilizing server $_SESSION as medium for storage.

The current patch is fit for minor release updates as changes are made on codes and file based only. User can just download and patch file, and get it done.

Another method will be database structure adjustments. This will require user to run install/upgrade script, and many will be reluctant to do it and therefore, equals less effective measure in a major patch.

I'm thinking a minor is fast, and more effective, even though it uses $_SESSION.

@Songohan22
Copy link
Author

I just want to give your suggestions for the best service to your customers.

A system can have many or one admin, not necessarily just one, so your idea of saying it doesn't matter is not very true.
Ideally, you should build a SESSION management driver, it will automatically check the value of the SESSION generated in your system has been disabled or not and etc. I think that the way you generate a new session value is not optimal, the more sessions that are created, the harder it will be to manage and be easy exploited, and we don't need to generate a new SESSION code when logging out?

@FrederickChan
Copy link
Member

Maybe we are on the wrong page. I did not say anything regarding Admin or Admin Panel.
From your reply, I assume that you aren't reading my commit. php-fusion/php-fusion@0f80a68

Well, I have redid the test, and this can be simply be overridden if the user closes his browser. It's not effective solution.

@FrederickChan FrederickChan reopened this May 15, 2020
@Songohan22
Copy link
Author

It is only my opinion to comment on this matter.
Thank you for reply.

FrederickChan added a commit that referenced this issue May 15, 2020
FrederickChan added a commit that referenced this issue May 15, 2020
Took 2 hours 28 minutes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants