FreePBX stores administrator passwords in cleartext in the database

Milestone 2.3 deleted

and MD5sums cant be undone? I think the idea here the old saying " you cant secure a box if you cant secure physical access to it". If the box is physically inacssesable then it wouldn't mater if the password is in plaint text.
On a side note, I havnt seen trixbox 2 yet, but i hope that they moved the password up to the "landing" page (http://yourServerIP/)

MD5SUMs can't easily be reversed - which is why they are a secure way of storing passwords. A good number of applications quite happily say "don't forget your password - we can't get it back" and people accept that. Storing passwords in cleartext simply creates an even bigger security headache for any database manager who needs to grant access to tables to other applications for whatever reason.

moving this to 3.0

To store admin passwords in SHA256 format in the db, I did the following:

1. Increase the password field. I increased to varchar(64), 40 bytes long should be sufficient

2. Modify admin/header_auth.php

# diff -u header_auth.php-orig header_auth.php      
--- header_auth.php-orig        2007-09-28 21:58:59.000000000 +0200
+++ header_auth.php     2007-10-08 10:14:28.000000000 +0200
@@ -22,7 +22,7 @@
                                // not logged in, and have provided a user/pass
                                $_SESSION['AMP_user'] = new ampuser($_SERVER['PHP_AUTH_USER']);
 
-                               if (!$_SESSION['AMP_user']->checkPassword($_SERVER['PHP_AUTH_PW'])) {
+                               if (!$_SESSION['AMP_user']->checkPassword(hash("sha256", $_SERVER['PHP_AUTH_PW']))) {
                                        // failed, one last chance -- fallback to amportal.conf db admin user
                                        if ( (count(getAmpAdminUsers()) == 0) && ($_SERVER['PHP_AUTH_USER'] == $amp_conf['AMPDBUSER']) 
                                          && ($_SERVER['PHP_AUTH_PW'] == $amp_conf['AMPDBPASS'])) {

3. Modify admin/modules/core/functions.inc.php

# diff -u functions.inc.php-orig functions.inc.php
--- functions.inc.php-orig      2007-10-08 10:49:58.000000000 +0200
+++ functions.inc.php   2007-10-08 10:07:16.000000000 +0200
@@ -539,9 +539,10 @@
 /* begin page.ampusers.php functions */
 
 function core_ampusers_add($username, $password, $extension_low, $extension_high, $deptname, $sections) {
+       $password_hash = hash("sha256", $password);
        $sql = "INSERT INTO ampusers (username, password, extension_low, extension_high, deptname, sections) VALUES (";
        $sql .= "'".$username."',";
-       $sql .= "'".$password."',";
+       $sql .= "'".$password_hash."',";
        $sql .= "'".$extension_low."',";
        $sql .= "'".$extension_high."',";
        $sql .= "'".$deptname."',";

Patch file for modifying ampusers table to encrypt password with SHA256

I'm not yet convinced that having these in clear text in the db is that big of a deal. For now I'm deferring this to see if there is more feedback for or against.

$me++. I really dont see how encrypting the admin password is going to stop the DHS from eavesdropping on your calls

That's not the issue. The issue is that if the box, or backups are compromised in any way, it becomes ridiculously trivial to extract passwords and have an unauthorised person access the phone system, particularly if the web UI is exposed on the internet. (not something I'd normally recommend, however there have been occasions where clients have demanded it)

Add to this that many people (myself included) will refuse to use one of my standard passwords on any database where the password isn't encrypted - which in many ways compromises security further since you end up having to write down the password you've chosen to use.

Given the fact that there was a patch submitted not quite a year ago to achieve this, I'm amazed that you've taken this attitude now - especially given that I submitted this bug 18 months ago and it was accepted at the time!

hmm - if the box is compromised, it is trivial to get into the system anyhow. But there is the issue of backup files that may be further exposed that is a good point. As far as exposing FreePBX to the web unprotected, that is just plan a really bad idea - as I am sure if ever put to the test, it would fail from a security standpoint.

Anyhow - I'll re-open the ticket to allow for more discussion on this. There were no opinions discussed since last November which is why mbrevda closed it.

I'm glad it's reopened. I was not aware that discussion was need for this change to be implemented. For me it's common sense not having plain text passwords in a database. With plain text password, db admins can get passwords by accident. With encrypted passwords you must change it to have access, which will be noticed by the password owner. This is just one of the reasons to encrypt passwords in a db. The problem of backup's with plain text passwords is already mentioned. Next to this, it is fairly easy for a system admin to reset the password when forgotten. Just create an encrypted password on the unix prompt and store it in the database (install hashalot, then 'sha256 -x'. Voila access granted again with the possibility for an audit trail. Plain text password shouldn't be in a database. For many people it could be just fine when it is, but for many it can be a problem. In that case the source must be patched on every update of the core.

ok: im starting to be convinced, and this really isn't such a big deal (to do) anyway. pvanstam: would you re post the changes in diff format please? Also there needs to be some sort of mechanism that will upgrade current db entries and change them to sha

philipple: what kind of collateral damage can we expect from such a change? Maybe it should be an option in amportal.conf?

AMPDBPASSSECURE=true/false

Then we leave it false by default an start spreading FUD until 3.0 where we change it to true by default (that's going to be a one way street though, as there will be no way to automatically go back as we cant reverse the hash)

pvanstam: ping. Any update here, or should I close this ticket?

if we change it then we change it across the board. It means an upgrade script that should:

• check if the field 'password' exists
• if so, then make a new field 'password_sha256'
• pull the old passwords, hash them and store them in 'password_scha256'
• delete the 'password' field

This way it knows the right format and knows if it has been converted. Then the only question, does the code look for the hash and if the field does not exist try the old passowrd field without the hash as a safety measure? Anyhow - it's late, if we do this we do need the upgrade script to go with it that goes in the upgrade code and it should probably be done in table.php in the upgrade directory, I don't want to put it into core's install.php

I have been coming back to this ticket over many, many months and can't believe there is still debate.

If you need another reason to go encrypted let me give you a scenario (can think of many more).

FreePBX is configured in a corporate environment (the kind of environment you want it in.. they have money). You get a tech savvy user that finds a _minor_ exploit or sql_injection possibility in _any_ of the existing code, all this query would require is _read_ access which lets the user dump the passwords in plain text and potentially _escalate_ privilege to that of an administrator with little to no effort.

This isn't this far fetched. http://www.freepbx.org/trac/ticket/2773 http://www.freepbx.org/trac/ticket/2776

Think of the consequences of this, the user then uses that access privilege to listen to the voice mail of other staff, CEO, anyone.. without their knowledge because there is no requirement to modify the password to access it (ie. it's not hashed). If it was hashed with sha256, you'd want to hope that there was a rainbow table with the password in it, otherwise you would need _write_ access to the database to gain access (update password and log in).

I believe it is an absolute liability having this information sitting in plain text when it can be changed to a hash with minor impact. Adding to this, it's not just the security of the FreePBX box you need to worry about, the user's privacy.. and the potential that the users would use the same password for other services on the network is a real risk.

Don't get me wrong, I do appreciate the effort that has gone into this project and it has saved me masses of time, my concerns revolve purely around the attitude towards security.

Will be more than happy to test this patch if it is implemented.

SuperJuice?: There are feature requests open here for more than a few years with comments, pros and cons being added all the time. If this where a commercial product, we would gather in the corporate meeting room and debate it. We would have an resolution in a couple of hours vs. a couple of years. Oh, and you would pay threw your nose for the product.

If you read the ticket carefully, you'll notice that it has already passed the debate stage. At this point the dev's seem to be pro implementing it. Now that that stage has been passed, someone has to step up to the plate and actually write the code. Both you, pvanstam and rjch seem to think this is important. One of you has even started to write a patch for this feature. The dev's ultimately end up soldering the responsibility of supporting the project and its good name. Therefor they filter submitted patches to keep the quality as high as they can, and to make sure that the patch fits in with the overall structure of the program.

While they are pro the idea, there obviously not as concerned about security as you are (maybe because past experience hasn't shown a need for concern?). In order to get the patch rolling you can either: submit one, wait for a dev to critic it, revise and submit another, till there happy with. Option b. is to cau$se a dev to be con$eren for your cau$e and get him to implement it right away. :)

My conern was raised after the comment:

07/08/08 21:13:21 changed by mbrevda ¶ pvanstam: ping. Any update here, or should I close this ticket?

I think if nothing else it should stay open.

Is the current plan to implement pvanstam's patch? or are you unsatisfied with this solution? if so, what are the shortfalls?

Option B, how much we talking? ;)

Theoretical this isnt such a difficult thing to do. Personally I hade a couple of questions as to the best way to do it. p_lindheimer was pretty clear in his post as to what the requirements for such a patch would be. (i.e. if we were going to include the current one, we would of done it already :) ) Again: all that's left now is for someone to actually do it.

Philippe: wrt plan b.?

my stance is pretty simple, the patch is not complete. It looks simple and I'm sure it would work once you modify the database, which is easy to do on a new install. So when someone wants to provide the chunk of code that I listed above, in a format that can be dropped into a typical tables.php script used during version upgrades, then we can drop it in to test and probably include. See the upgrades directory for many examples. The code is not rocket science and if it's important enough then someone will do it. (or someone will bribe another dev to do it).

A comment on security. I agree that security is important. However, if in the concerned scenario described, you are best off assuming that there are probably a lot of other issues. FreePBX has not had the staff to harden against security. Also, if worried about the security, do a little research on VoIP in general and don't think that things like VLANs in a corporate environment are really going to do much against a smart person who want to listen in. The strongest thing that was pointed out, by the original poster and that I think has the most merit, is the plane text password present in the backup files as I can see a lot of incidents where this could be an issue. It suggests another thing that someone should consider, which is to provide an encryption key to use to encrypt the the entire database backup set since it includes voicemail.conf and probably other files (pinsets) that might be exposed. (Of course this would then cover the plane test passwords:-) )

pong

it's true the current patch is simple. It's not ment to be included in the upgrade as-is. It's a first step. I'm not yet fully aware of how the upgrade path is working and the possibilities with it. In a few day's I can update the patch, because right now I'm not on the latest FreePBX.

Regard, Pim

pvanstam: ping? At this rate im not sure your going to make it in time for 2.5

this has been stressed as important based on the discussion above but there is no patch that migrates current systems to the new format and we are about to enter feature freeze for 2.5. So we either need that submitted, need someone to offer up some cash to bribe someone to finish the work (as a guess, maybe $300) or we're going to move this out of this release and it won't have a chance to be evaluated until the next milestone. Thanks.

given the lack of interest (despite the strong opinions) moving this out of the current milestone.

bump - the heated discussion implied this was pretty important to some participants but it still lacks the migration code to be implemented. Do one of you in the thread want to provide those last parts so we can get this in for you?

Last chance here... Personally, I really dont see the point

Replying to mbrevda:

Last chance here... Personally, I really dont see the point

Using md5 could be cracked. Btw don't use easy passwords. If you look at the patch you will see I use sha256. You won't crack that within minutes ...

I will try to deliver a patch with migration in it, but I'm not a daily coder. I guess there could be an option in amportal.conf so the system manager can deside to encrypt passwords or not. For me it's a big issue. I now patch systems by hand.

Regards, Pim van Stam

You still have the completely wrong attitude about this. We shouldn't have to convince you that salt+hashing the passwords is a good idea.

Look at the example you provided, it's rainbow tables against passwords constructed with all lower case characters. Here is the table he used, look at the http://www.antsight.com/zsl/rainbowcrack/#MD5%20Table

Characterset [abcdefghijklmnopqrstuvwxyz0123456789]

So a 36G rainbow table, if it had have one character not from the lower case character set, it would have come up with nothing (unless there was an absolutely minute chance of a collision).

Rainbow tables have been around for as long as md5's, this doesn't mean that hashing the password should be overlooked, it's not saying the method is 'broken'. It was even stated in the original ticket "(or similar)", SHA256 is even used in the example attached. Have fun doing SHA* with rainbow tables.

I have totally lost interest in fighting this battle, if you are happy with it how it is by all means leave it, I was just trying to bring focus to the fact that if this is being sold as an enterprise product there are certain expectations enterprise customers have.

I have to agree on both opinions present there.

1) No method is 100% safe

but ...

2) in huge env with 4 or 5 guys who do the daily job on DB and can have a 'view' over the tables and so 'see' the boss password, it should be encrypted.

So, I don't really see a real technical necessity about it, but to have it in production environment in any company (at least in france) it should encrypt password to be allowed in production.

If I can free some time I'll write the needed script.

Script to update the ampusers database

I think this is a good idea. I've applied the code, as supplied by Tensibai and pvanstam (with a couple of minor fixes and additions) to trunk, so this will be part of the 2.6 upgrade

the changes in r7726 are incomoplete, re-opening ticket.

• needs changes in framework to handle an updated framework running an un-updated core
• needs to code in core updated, ampuser tab

Added the missing bits. Don't really know how often someone will be running a new framework without updating core, and it won't break mid-session, as auth is stored in a cookie.. PL needs to convince me that I need to double the amount of code for a very small potential issue.

(In [7740]) changing from sha256 to sha1 because php4 does not support hash() function re #1380

(In [7741]) changing from sha256 to sha1 because php4 does not support hash() function re #1380

I call this done!