Problem with D::P::A::E::Provider::Base.pm
Hi everyone! I really like what I see in Dancer::Plugin::Auth::Extensible, however, there is a real problem in the Provider::Base.pm match_password() sub, and the solution would probably break some current (poorly implemented) sites. First, it is hard coded to use "Crypt::SaltedHash->validate($correct, $given)" for validation, (not to mention It does not even let you set the salt length so it defaults to 4) then to my horror, if that fails, it tries to match it in plain text! Yikes! The reason this is really bad is that at the back end where user validation is concerned, there should be NO guessing and helping from the scrip! This sub should not try to help out at all. I would argue there should be no option for plain text period. You should also have to specify the hashing module you want (i.e. Dancer::Plugin::Passphrase; Crypt::SaltedHash; etc... (in case one is compromised, or just outdated you could switch to another at will) And you should have to specify the algorithm to use. There should never be defaults here. It should just fail if this stuff is not specified period. One thing I like to be able to do is set the password in a user's account to '', or '1' or '0' or anything as a really quick way to force the user to reset their password because they would NEVER be able to log in with that. The way it is now, you have to set a random string and hash it or something crazy... I'm willing to help out however I can because I'm re-writing a fairly large site (from Interchange) and I don't want to write lots of patch code on top when things should be done at the modules. My time is short, and I'm really hurting for cash, but let me know if there is anything I can do to help. Again, I really like what I'm seeing in D::P::A::E and in Dancer overall!! Thanks! Rick Bragg
On Mon, 10 Dec 2012 20:57:15 -0500 "Rick Bragg" <rbragg@gmnet.net> wrote:
Hi everyone!
I really like what I see in Dancer::Plugin::Auth::Extensible, however, there is a real problem in the Provider::Base.pm match_password() sub, and the solution would probably break some current (poorly implemented) sites.
First, it is hard coded to use "Crypt::SaltedHash->validate($correct, $given)" for validation, (not to mention It does not even let you set the salt length so it defaults to 4) then to my horror, if that fails, it tries to match it in plain text! Yikes!
Yeah, it should at least make the salt length configurable, good point, and attempting to match as plain text should be a configurable option too (probably defaulted to off).
The reason this is really bad is that at the back end where user validation is concerned, there should be NO guessing and helping from the scrip! This sub should not try to help out at all. I would argue there should be no option for plain text period. You should also have to specify the hashing module you want (i.e. Dancer::Plugin::Passphrase; Crypt::SaltedHash; etc... (in case one is compromised, or just outdated you could switch to another at will) And you should have to specify the algorithm to use. There should never be defaults here. It should just fail if this stuff is not specified period.
The reason I used Crypt::SaltedHash there is it's good at working out what hashing scheme is in use and just doing the right thing. If it's at all difficult to configure or understand, users might decide not to use it and just use plain text passwords instead; whilst I think they should have that choice, I think it should be seriously discouraged :)
One thing I like to be able to do is set the password in a user's account to '', or '1' or '0' or anything as a really quick way to force the user to reset their password because they would NEVER be able to log in with that. The way it is now, you have to set a random string and hash it or something crazy...
Yeah, that's a good point. A check for an empty password and returning an immediate failure if that's the case would be sensible.
I'm willing to help out however I can because I'm re-writing a fairly large site (from Interchange) and I don't want to write lots of patch code on top when things should be done at the modules. My time is short, and I'm really hurting for cash, but let me know if there is anything I can do to help.
I know the feeling, but if you get time, pull requests would be welcome. I should be able to implement some of the above reasonably soon anyway I should think.
Again, I really like what I'm seeing in D::P::A::E and in Dancer overall!!
Glad to hear you're liking it! -- David Precious ("bigpresh") <davidp@preshweb.co.uk> http://www.preshweb.co.uk/ www.preshweb.co.uk/twitter www.preshweb.co.uk/linkedin www.preshweb.co.uk/facebook www.preshweb.co.uk/cpan www.preshweb.co.uk/github
On Mon, 10 Dec 2012 20:57:15 -0500 "Rick Bragg" <rbragg@gmnet.net> wrote:
Hi everyone!
I really like what I see in Dancer::Plugin::Auth::Extensible, however, there is a real problem in the Provider::Base.pm match_password() sub, and the solution would probably break some current (poorly implemented) sites.
First, it is hard coded to use "Crypt::SaltedHash->validate($correct, $given)" for validation, (not to mention It does not even let you set the salt length so it defaults to 4) then to my horror, if that fails, it tries to match it in plain text! Yikes!
Yeah, it should at least make the salt length configurable, good point, and attempting to match as plain text should be a configurable option too (probably defaulted to off).
The reason this is really bad is that at the back end where user validation is concerned, there should be NO guessing and helping from the scrip! This sub should not try to help out at all. I would argue there should be no option for plain text period. You should also have to specify the hashing module you want (i.e. Dancer::Plugin::Passphrase; Crypt::SaltedHash; etc... (in case one is compromised, or just outdated you could switch to another at will) And you should have to specify the algorithm to use. There should never be defaults here. It should just fail if this stuff is not specified period.
The reason I used Crypt::SaltedHash there is it's good at working out what hashing scheme is in use and just doing the right thing. If it's at all difficult to configure or understand, users might decide not to use it and just use plain text passwords instead; whilst I think they should have that choice, I think it should be seriously discouraged :)
One thing I like to be able to do is set the password in a user's account to '', or '1' or '0' or anything as a really quick way to force the user to reset their password because they would NEVER be able to log in with that. The way it is now, you have to set a random string and hash it or something crazy...
Yeah, that's a good point. A check for an empty password and returning an immediate failure if that's the case would be sensible.
I'm willing to help out however I can because I'm re-writing a fairly large site (from Interchange) and I don't want to write lots of patch code on top when things should be done at the modules. My time is short, and I'm really hurting for cash, but let me know if there is anything I can do to help.
I know the feeling, but if you get time, pull requests would be welcome. I should be able to implement some of the above reasonably soon anyway I should think.
Again, I really like what I'm seeing in D::P::A::E and in Dancer overall!!
Glad to hear you're liking it!
-- David Precious ("bigpresh") <davidp@preshweb.co.uk> http://www.preshweb.co.uk/ www.preshweb.co.uk/twitter www.preshweb.co.uk/linkedin www.preshweb.co.uk/facebook www.preshweb.co.uk/cpan www.preshweb.co.uk/github
_______________________________________________ dancer-users mailing list dancer-users@dancer.pm http://lists.preshweb.co.uk/mailman/listinfo/dancer-users
Thanks David, Yea, it's good stuff! You're right that sticking to one require like "Crypt::SaltedHash" is probably best instead of making that interchangeable... It doesn't make sense to add that much complication. After all, if a problem comes up with a hash algorithm, or new ones are invented, then all that should be dealt with inside Crypt::SaltedHash anyway... I just don't like it fishing to figure out what the alg is though. I think it should be deliberately specified and fail if not in the right form. For example, if SHA256 is used on a site, and it is all of a sudden compromised 0-day, and you simply change the alg to something new, that alone should force all the old passwords to automatically fail making everyone reset there passwords. (then again, all those passwords should also be just deleted ASAP as well! also causing the login fail...) I am working on a dev site right now (not touching the DPAE, just implementing it... Once I have it in reasonable shape, I will see if it's worth putting up for review as well. Thanks again! Rick
On Tue, Dec 11, 2012 at 6:10 AM, David Precious <davidp@preshweb.co.uk> wrote:
The reason I used Crypt::SaltedHash there is it's good at working out what hashing scheme is in use and just doing the right thing. If it's at all difficult to configure or understand, users might decide not to use it and just use plain text passwords instead; whilst I think they should have that choice, I think it should be seriously discouraged :)
At the risk of inflicting dependencies on people, I suggest looking at Authen::Passphrase for dealing with various ways to hash passwords. In particular, using Authen::Passphrase::BlowfishCrypt would be a sensible default scheme as long as the work factor is decently high (12+). David -- David Golden <xdg@xdg.me> Take back your inbox! → http://www.bunchmail.com/ Twitter/IRC: @xdg
On Thu, 13 Dec 2012, David Golden wrote:
On Tue, Dec 11, 2012 at 6:10 AM, David Precious <davidp@preshweb.co.uk> wrote:
The reason I used Crypt::SaltedHash there is it's good at working out what hashing scheme is in use and just doing the right thing. If it's at all difficult to configure or understand, users might decide not to use it and just use plain text passwords instead; whilst I think they should have that choice, I think it should be seriously discouraged :)
At the risk of inflicting dependencies on people, I suggest looking at Authen::Passphrase for dealing with various ways to hash passwords.
In particular, using Authen::Passphrase::BlowfishCrypt would be a sensible default scheme as long as the work factor is decently high (12+).
Or maybe Crypt::Eksblowfish like Dancer::Plugin::Passphrase ? -- Henk
On Thu, Dec 13, 2012 at 11:47 AM, Henk van Oers <hvo.pm@xs4all.nl> wrote:
In particular, using Authen::Passphrase::BlowfishCrypt would be a sensible default scheme as long as the work factor is decently high (12+).
Or maybe Crypt::Eksblowfish like Dancer::Plugin::Passphrase ?
Authen::Passphrase uses Crypt::Eksblowfish and they are both by written by zefram. The advantage of Authen::Passphrass is that it handles multiple schemes in one API (and manages entropy generation). I wasn't debating bcrypt vs sha in general (you can google for that :-), merely saying that Authen::Passphrase would be a way to give people a lot of choice without needing a lot of work if David wanted to open it up to something other than Crypt::SaltedHash. David -- David Golden <xdg@xdg.me> Take back your inbox! → http://www.bunchmail.com/ Twitter/IRC: @xdg
participants (4)
-
David Golden -
David Precious -
Henk van Oers -
Rick Bragg