Hash root user pass upon installation.

antidote · February 2013

Yesterday while going over some code in a plugin I'm creating I noticed something pretty odd: The installer is not hashing the root accounts password upon creation. gm112 and I have tracked down the problem and have created a patch which is waiting in your pull request queue.

It would be great if you patched this quickly and rolled out a security release.
The Pull request in question is: https://github.com/vanillaforums/Garden/pull/1536

antidote · March 2013

Unfortunately due to your apparent lack of interest in this critical issue, our team is going to have to use another software which is properly maintained.

businessdad · March 2013

For the record, I made a couple of tests with Vanilla 2.0.8.14, but, when I installed it (every time from scratch), the Admin password was always hashed correctly. Perhaps I'm missing something, but I can't reproduce the issue.

antidote · March 2013

How is that even possible when the code to hash the password was missing in it's entirety? Please look at the pull request and use the latest version, the bug is still present.

businessdad · March 2013

This is what I'm wondering. I downloaded Vanilla 2.0.8.14 from this website, created a blank DB and ran the setup, configuring the "Super Admin" User. Then I checked the Users table and the password was a hash.

What steps have you followed to reproduce the issue?

businessdad · March 2013

I figured out what's happening: the password is passed as clear text to the UserModel, which converts it into a hash and saves it to the database. It makes sense, actually.

The code that does it is in /applications/dashboard/models/class.usermodel.php, method UserModel::Save () and UserModel::_Insert ().

No oversight, everything seems to be fine.

422 · March 2013

hey @antidote no need to be aggressive, nor to imply people are calling you names.

We that is 99% of us on here are purely voluntary, and so get no satisfaction in discarding the very valid points you have made, including @businessdad, who has stated "everything seems fine"

This does not imply, that he disbelieves you. However if you can demonstrate the issue, we would all be keen to see it, images videos or otherwise.

But calling out, flaming or insulting members isn't tolerated. Please keep it nice.

note not all of us are sql / db gurus !

x00 · March 2013

no one called you a liar.

422 · March 2013

We are MEMBERS just like you !!!!!

The actual staff don't visit the forums often, and based on timezones etc, allow them the courtesy of at least seeing your GitHub repo and responding ! But if you are gonna spit the dummy, at members, ... seems quite silly, as we have nothing to gain from disagreeing with you, or alternatively changing the software !

x00 · March 2013

note that _Insertinternal method used by SaveAdminUser

contain the lines

  if (array_key_exists('Password', $Fields)) {
     $PasswordHash = new Gdn_PasswordHash();
     $Fields['Password'] = $PasswordHash->HashPassword($Fields['Password']);
     $Fields['HashMethod'] = 'Vanilla';
  }

so there might be a edge case for some reason it isn't hashing

I wonder if there is a plugin that is making use of BeforeInsertUser hook.

You solution while appreciated might cause problem with double hashing. I think it needs more investigating what is going on.

I wish people wouldn't throw the baby out with the bath water, when stuff like this happens.

x00 · March 2013

well like I said the hash is there it is using the same method as in your patch so it would double hash.

what version of vanilla/garden are you using?

x00 · March 2013

belligerent

please look this word up. War like. The only person giving lip is you. Do you want help or what?

There is nothing remotely belligerent in the replies to you.

businessdad · March 2013

@antidote said:
Are you calling me a liar?
I'm sorry but we can record the behavior described and supply a video, because clearly you're either making it up to cover this up, or are truly ignorant to what i'm talking about.

I never said you are a liar, simply that I can't reproduce the issue and I asked you what steps do you follow to reproduce it.

Then I added that, from an inspection, the hash code seems to be called correctly when a new User is added to the database. I didn't follow the code step by step, but from what I can see method UserModel::_Insert(), which is called by UserModel::Save() and, as @x00 pointed out, by UserModel::SaveAdminUser(), creates a password hash before saving it.

If this mechanism doesn't work in some circumstances, it would definitely make sense to review it, but first we must figure out what are the conditions that cause the malfunctioning. Therefore, I renew my request to provide some more information that can help reproducing the issue (environment information, versions, settings, steps to reproduce it, etc), so that we can get to the bottom of this.

Regarding the calling names, nobody did it, apart from you. As @422 pointed out, we are not paid by Vanilla Ltd and Vanilla is not "our software". We are Users like you who are providing support if and when we have the time for it. I don't see any "belligerent" attitude, just a misunderstanding.

x00 · March 2013

I think this might be the issue

     if ($this->GetID($UserID) !== FALSE) {
        $this->SQL->Put($this->Name, $Fields);
     } else {

It is doing a Put without sanitising. That is the case where there is already a root user in the table.

It is the else that does the _Insert

businessdad · March 2013

@x00 said:
It is doing a Put without sanitising. That is the case where there is already a root user in the table.

Out of curiosity, how do I add another root User? I thought the issue might have occurred after the installation, but I can't figure out how to add a new "root".

x00 · March 2013

By root I actually meant UserID = 1 first user.

You can set Admin on any user. it is the first user who is also the site admin.

x00 · March 2013

I think GND_User table contains the root user, when the setup Configure method is called, which employs the UserModel->SaveAdminUser()

See the code above for why.

businessdad · March 2013

@x00 said:
I think this might be the issue
     if ($this->GetID($UserID) !== FALSE) {
        $this->SQL->Put($this->Name, $Fields);
     } else {
It is doing a Put without sanitising. That is the case where there is already a root user in the table.

Tested and confirmed, that is most likely the bug.

How to reproduce

Create a blank database (e.g. vanillatest).
Install Vanilla (I used version 2.0.18.4), using vanillatest as the database. SuperAdmin password is hashed correctly.
Remove file /conf/config.php.
Run the installation script again, using the already existing and populated vanillatest database. This time, the SuperAdmin password is stored in plain text.

It's curious that the SuperAdmin can still log in, with the plain text password. In theory, that should not be possible, as the password should be hashed again upon login, then compared with the plain text one, and they clearly wouldn't match.

If this is the only case when such issue occurs (we don't know for sure, yet), I would consider it moderately critical. After all, I don't see why anybody should reinstall Vanilla against an already existing database, instead of creating a brand new installation and importing the data from the old one.

x00 · March 2013

@businessdad said:
It's curious that the SuperAdmin can still log in, with the plain text password. In theory, that should not be possible, as the password should be hashed again upon login, then compared with the plain text one, and they clearly wouldn't match.

    function CheckVanilla($Password, $StoredHash) {
        $this->Weak = FALSE;
      if (!isset($StoredHash[0]))
         return FALSE;
      
      if ($StoredHash[0] === '_' || $StoredHash[0] === '$') {
         return parent::CheckPassword($Password, $StoredHash);
      } else if ($Password && $StoredHash !== '*'
         && ($Password === $StoredHash || md5($Password) === $StoredHash)
      ) {
         $this->Weak = TRUE;
         return TRUE;
      }
      return FALSE;
    }

businessdad · March 2013

@antidote said:
Moderately critical? Are you insane? That's a Highly critical, it exposes the whole forum.

If you read carefully what I wrote, I explained why I think it's moderately critical. Based on my tests, an installation made against a blank database will always hash the password. The issue occurs when one runs the installation script again against an already populated database.

While the bug itself can be considered critical, the risk of this happening is, in my opinion, moderate, as I don't see a reason why anyone would reinstall Vanilla against an existing (and already populated) database. I work with databases all the time, and I wouldn't do something like that anyway. If needed, a re-installation should always be performed on a blank database, and then the relevant data imported into the new DB.

Also, please notice that my original statement included another note. Here's the full sentence:

If this is the only case when such issue occurs (we don't know for sure, yet), I would consider it moderately critical.

The meaning of it is far from "it's a minor issue".

By the way, it would be nice if you could stop insulting. It's not by treating everybody like an idiot that you will get people to listen to you.

Todd · March 2013

Also note that any weak passwords get strongly hashed the next time anyone signs in. That include the weak hashes from pretty much every single imported forum (yes, they pretty much all use some variation on md5).

Hash root user pass upon installation.

Comments