Flyspray - The bug killer!

  • Status New
  • Percent Complete
    0%
  • Task Type Feature Request
  • Category Backend/Core
  • Assigned To No-one
  • Operating System All
  • Severity Medium
  • Priority Low
  • Reported Version 1.0 devel (github master)
  • Due in Version Undecided
  • Due Date Undecided
  • Votes 1
  • Private
Attached to Project: Flyspray - The bug killer!
Opened by James Smith - 14.10.2015
Last edited by peterdd - 18.04.2016

FS#2069 - Improve password security by adding PBKDF2/bcrypt support

For password hashing, PBKDF2 & bcrypt are both more secure than md5 and sha1 (even salted).

My suggestion is to replace the default md5 hash method with bcrypt (set the default work factor to 6).

I am a PHP developer and I’d like to help with this improvement.

Project Manager
peterdd commented on 14.10.2015 18:00

It is still on the wishlist, but not top priority, because it first requires the database or Flyspray is compromised/leaked or auth cookies are stolen.

If you want you can simply fork on github to your own branch and work on that.

Please consider that Flyspray is in use for long time and user base exists, so the existing password hashes must be kept as they are to be back compatible.

If a new user registers or user reset his password, then the better password hashing for the new password can be used.

Project Manager
peterdd commented on 18.04.2016 23:19

Implementation should use a popular accepted and portable method (Joomla, Wordpress, Drupal, ..) to keep Flyspray easy to integrate with other software instead inventing: http://www.openwall.com/phpass/

Edit: Did some research ..
The original Software on openwall.com is not updated for a few years, but was base for different following developments:

Part of Symfony: https://github.com/symfony/security

Joomla CMS: https://github.com/joomla/joomla-cms/blob/staging/libraries/phpass/PasswordHash.php

Different forks of phpass on github:
https://github.com/rchouinard/phpass Need some https://github.com/rchouinard/phpass/network

Wordpress: https://github.com/WordPress/WordPress/blob/master/wp-includes/class-phpass.php

Since PHP5.5: http://php.net/manual/en/function.password-hash.php

and for PHP < 5.5: https://github.com/ircmaxell/password_compat

brent s. commented on 25.06.2016 18:36

Hi- security-centric sysadmin/engineer here.

I'd really have to +1 PBKDF2/bcrypt support- even salted sha256/sha512 would be WORLDS better than plain MD5, and bcrypt especially is already seen as a rather good method of storing passwords. Between the collision and just plain ease of cracking/bruteforcing MD5 (on that note, it seems the login maxes out on 30 chars for a password), MD5 is no longer a viable option, unfortunately.

Is there a reason you'd want to use the same scheme as those CMSes? If you're looking for interoperability, most of those listed (all, I think) support authentication APIs instead, which I'd imagine would be ideal as it cuts down on backend maintenance overhead (if you're going to use unified logins, the backend should only change a password in one place and all places should look to that for authentication). However, I'd say worry about that bridge when it comes to it- when you have the code ready to integrate into outside services, THEN worry about interoperability.

If you wish to not break old passwords, a simple solution can be found via the way dovecot handles passwords in SQL- there is a default scheme in the configuration for dovecot, which it will try to use if no specifier is found. However, a scheme can be applied via a specifier.
Let me provide an example. Let's say your dovecot uses SHA512-CRYPT as the default in a new update (and previously used MD5-CRYPT), but you still have some users that have not yet updated their password (and thus switched over to the new default password scheme), and are still using MD5-CRYPT.

The password fields would look thusly:

user1 | {MD5-CRYPT}$1$suTMY.38$B6ImKxOGFn1tF8Ci2rgzO0 |
user2 | $6$rS9wRqndpYVhjWdC$aal2yWujfhnlz5ZV7gKgf1JaJQWGV1ZJrJAjstDSeMjRvYBgSlnrEnu53Zav0wuHIA6XssKZGuUfipApKSxcH. |

user2 is using the new SHA512 scheme, whereas user1 is still using MD5-CRYPT (old passwords can be prefixed via something like "UPDATE mailbox SET password = CONCAT("{MD5-CRYPT}", password) WHERE password LIKE '$1$%';").
(Alternatively, the inverse could be done- the new passwords could be prefixed as {SHA512-CRYPT} and the old passwords non-prefixed, and the default in the configuration kept as MD5-CRYPT.) (By the way, both of those strings evaluate from "password" if you're curious.)

I imagine a similar method could be done here, as it easily allows multiple hashing schemes in the same table without breaking functionality for older hashes.

Lastly, but most importantly, I want to address this, as it struck a chord with me:
"It is still on the wishlist, but not top priority, because it first requires the database or Flyspray is compromised/leaked or auth cookies are stolen."

I'm not sure if you're aware, but the vast majority of these leaks occur through SQL injection attacks. Would you be willing to vouch that Flyspray is 100% invulnerable to e.g. MySQL injection attacks, and will remain so in the future? If so, can I get that in writing? Because that'd be remarkably impressive. Strong salted hashes for passwords aren't put in place because the surrounding security is weak; they're there as a contingency plan in the event that an unknown vulnerability exists (and they always do, in varying degrees). At the *very* least it gives users enough time to change their passwords following news of a compromise were one to happen. (Plus let us not forget the "disgruntled employee" situation, etc.- not even administrators should be able to know what a user's password is. In fact, PCI compliance requires this.)

I apologize for the diatribe, but I really think this feature request is immensely important and I don't want to see it downplayed or delayed in lieu of other factors. It's a dangerous world out there, and if you can make it a little safer for your users, I highly suggest doing so. I do hope this helps.

Project Manager
peterdd commented on 27.06.2016 04:24

I'm always open for constructive critic. :-)

I have two other applications that need to be upgraded too (currently md5 to a secure hash). One does singlesignon from a php page and a perl page too reading the hashes from the same usertable.

So this issue will not be forgotten by me. I just want avoid a quickshot solution we later maybe regret.

Project Manager
peterdd commented on 27.06.2016 07:15

Most of the stuff exists in Flyspray yet. :-)

  • setup/ needs to remove the md5 as default of passwdcrypt config param for flyspray.conf.php
  • add password.php from https://github.com/ircmaxell/password_compat (update licences directory (MIT))
  • replace the crypt() without salt param call by password_hash() and password_verify()
  • some logical cleanup in class.flyspray.php. (function checkLogin() )
  • It raises minimal required php from php5.3.3 to php5.3.7 if people wants stronger passhashes. (check at setup screen)

Test and if all fine, add to github and it will be in the next rc-release ..

brent s. commented on 27.06.2016 07:34

peterdd-

Thanks so much!

Using a 6-8 char salt with SHA256 or SHA512 should be plenty, if you're worried about known/proven/tested. However, bcrypt is in use in many places already (and has been around for 11 years). I'd highly recommend it (as it is ever so slightly stronger than PBKDF2); https://security.stackexchange.com/questions/4781/do-any-security-experts-recommend-bcrypt-for-password-storage goes into the gritty details as to why- basically, "it's very expensive in time to crack unless you have an FPGA, which are still rather expensive in cost"; it tends to be recommended over PBKDF2 because of its resilience against GPU-centric attacks (which are *much* more cheap in cost and prevalent). Wordpress (for instance) is currently working on adding support for it as well: https://core.trac.wordpress.org/ticket/21022 (Most of *their* deliberation is how to continue, if at all, to support users installing newer versions of Wordpress and then downgrading from PHP 5.3.7+ to PHP 5.2, which is itself ancient- so I wouldn't worry about it terribly much.)

I hope this helps! If you have any questions or have ways you think I might help, do feel free to let me know!

From a personal project of mine, I implemented it this way (though keep in mind I'm far from a web developer, so I bet there is a more elegant way of doing this). Note that $salt is generated outside (if one is specified- I recommend it), and it only does 10 rounds so it isn't ideal. See https://secure.php.net/manual/en/function.password-hash.php for more information (notably, PASSWORD_BCRYPT).

if (isset($salt)) {
        $bcryptopts = array("salt" => $salt,"input_string" => $input_string);
} else {
        $bcryptopts = array("input_string" => $input_string);
}
function bcryptHashResult($bcryptopts) {
        global $algo;
        global $input_string;
        if (isset($bcryptopts['salt'])) {
                $options = ['salt' => $bcryptopts['salt']];
                return password_hash($bcryptopts['input_string'],$options);
        } else {
                return password_hash($input_string,1);
        }
}

and then $salt could be generated by... anything.

The DB's varchar for password WILL need to be extended to (at least) 60, however (I believe you currently have it set as 40), which can be handled via the setup script for upgrades. Since it's varchar, though, it should still be able to hold MD5 checksums as well (especially with the {MD5} etc. method I specified earlier).

Loading...

Available keyboard shortcuts

Tasklist

Task Details

Task Editing