phpvms / phpvms_v2

Virtual Airline Management (not maintained)
http://www.phpvms.net
BSD 3-Clause "New" or "Revised" License
41 stars 46 forks source link

Security issue: ResetPassword in Login.php #140

Open Shadolio opened 5 years ago

Shadolio commented 5 years ago

Hello πŸ˜ƒ πŸ›©

I have found an issue that might be a big security hole for the accounts of pilots logging to the virtual airline system. πŸ›„

The following line of code in function ResetPassword() in Login.php generates a new password then sends it to the pilot's email and updates it in the database.

https://github.com/nabeelio/phpvms_v2/blob/d024c3232afc9725a21dd1c18b301740b24acfab/core/modules/Login/Login.php#L86

It uses the current date and time, which makes is unique, but absolutely predictable; I could easily generate the same code on my own machine, then use it to login to another's pilot's account. All what I need is his email address.

In fact, that is what I actually did in C#:

  1. Get the email of the pilot I want to reset his/her password.
  2. Send an HTTP POST request to http://www.somevirtualairline.net/index.php/Login/ResetPassword with "email" = "myemail@hostprovider.com" in the formurl-encoded body.
  3. When the request succeeds, I don't use anything from the response body, but I take the Date from the Headers.
  4. Then, I use this date to generate a string using the same format, get its MD5 hash output, and take the first 6 characters with substring function.

Now, I have the newly generated password without hacking into pilot's email inbox but only by knowing his email address. After that, I can use that password to login to his account, and change the password again, and maybe change the email. That might not seem so dangerous as there is not private data to access, but it might be worse if I knew the email of the CEO, then used that to gain access to the admin features and play with anything like members' status, schedules, ranks, etc...

Moreover, I was just thinking today that the public access to this function also makes it easy to keep changing the password of any pilot, by keeping on sending requests to this endpoint, even if I do not want to use his account, but just to disturb his/her peace.

Therefore, my suggestions for both problems are:

  1. to salt the date with a random number generated at the moment, so that I cannot or it's much harder to generate the same string and its MD5 hash from another machine at the same time.
  2. restrict access to this functionality, maybe by making this function private.
    private function ResetPassword()

To generate the random number, this function is available in PHP and documented here.

int rand(void)

Although it is noted that it is not cryptographically secure, I think it is sufficient for our use. However, if we need and can afford it, there is the following function but is available only in PHP 7:

string random_bytes (int $length)

Nevertheless, there is a userland implementation for PHP 5.2 to 5.6 inclusive at https://github.com/paragonie/random_compat, as documented in the PHP reference.

I am not experienced in PHP syntax, but here is my suggestion for the new password:

$newpw = substr(md5( date('mdYhs' . rand() )), 0, 6)
nabeelio commented 5 years ago

Hey, this repo/project is basically deprecated, but if you can submit a PR to fix, I'll accept it. It can probably also be expanded to 10 chars instead of just 6

nabeelio commented 5 years ago

But yes thanks for the thorough check, it def is a vulnerability

Shadolio commented 5 years ago

I also wonder then if the number of characters (6 or 10 chars) is based on something.