Record's PasswordField keeps its salt close to its chest

[Owen Fraser-Green]

Hi,

<newbie-alert>
I've hit a couple of snags using PasswordField (in conjunction with
squeryl-record):

1. The salt is created randomly within PasswordField but there doesn't
appear to be any means to store the salt making verification of the
password later impossible. An easy workaround seems to be just
override salt with a separate StringField which I store in the record.
Somehow though, this feels like it shouldn't be necessary.
2. When verifying the password, there's isn't any method to do the
match (such as MappedPassword's match_?). Given that in the above
workaround, I've extracted the salt into its own field, I can at least
do this outside PasswordField, but then I need to brake open
PasswordField and copy-and-paste what's going into the hash function
(hash("{"+s+"} salt={"+salt_i.get+"}")). This seems somewhat broken.
</newbie-alert>

Cheers,
Owen

[Tim Nelson]

Hi Owen,

I noticed the same thing regarding the salt. Also, since the set_! method is overwritten, even if the password is already hashed, it gets hashed again when setting the field from the value in the db.

Anyway, I decided to write my own password field using jBCrypt. I posted it in a Gist if you want to use it [1].

A couple of notes:

1. You have to manually call hashIt to actually hash the password.

2. If you want to have 2 fields that get compared (ie a confirm password field), you need to create a second field and pass it to the password field.

3. I'm using this with MongoDB, but it's just a simple string (jBCrypt can check for a match without having to pass it the salt), so it should work with SQL.

4. Don't forget to add the jBCrypt dependency.

This is how I have the fields in my User class:

      object password extends PasswordField(this, 32, Full(confirmPassword)) {

 override def displayName = "Password"

 override def shouldDisplay_? = if (isUnregistered) true else false

}

object confirmPassword extends StringField(this, 32) {

 override def ignoreField_? = true

 override def displayName = "Confirm Password"

 private def elem = S.fmapFunc(S.SFuncHolder(this.setFromAny(_))) {

      funcName =>

      <input type="password" maxlength={maxLength.toString}

        name={funcName}

        value={valueBox openOr ""}

        tabindex={tabIndex toString}/>

    }

    override def toForm: Box[NodeSeq] =

      uniqueFieldId match {

        case Full(id) => Full(elem % ("id" -> (id + "_field")))

        case _ => Full(elem)

      }

}

[1] https://gist.github.com/816439

Tim

[David Whittaker]

Has this been fixed?  I can see that a match_? method has been added to PasswordField and that a salt method exists, but it seems like the private val salt_i is used during hashing, so overriding salt does nothing.  Since salt_i is randomly generated as far as I can tell passwords are impossible to verify.  I'm also seeing the re-hashing behaviour Tim mentioned when the User is retrieved from the DB.  Maybe I'm missing something, I see that record's ProtoUser uses PasswordField.... can anyone verify that they've used that or PasswordField on it's own without issue?

--
You received this message because you are subscribed to the Google Groups "Lift" group.
To post to this group, send email to lif...@googlegroups.com.
To unsubscribe from this group, send email to liftweb+u...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/liftweb?hl=en.

[Ján Raska]

I confirm it doesn't work on it's own. Possibly one can use salt to make it work, as it is defined as "def salt = this.salt_i" which makes it of type FatLazy, that can be manually set by calling salt.set(yourValue). However, I was unable to figure out at which point to insert there a value I obtain from my salt field. It was easy to set up that one with 

override def defaultValue = passwordField.salt.is

So I was able to save generated salt into database, but I was unable to make PasswordField use it when matching passwords. I guess I'd have to make my own PasswordField for now, possibly saving both, hash and salt into one field with some delimiter to make it a bit easier.

[Ján Raska]

I've solved this problem by writing a small trait and mixing it into my password field. It works with Squeryl-record, I don't know about other implementations such as MongoDB. It appends salt to the password hash and stores it together in single field. Hashing is only done in apply, not in set_!() method, because set_!() is also used to populate field with data from database, so hashing in set_!() (as it is in PasswordField) causes that the password is double hashed. As far as I've tested, this is the only limitation. It's only about 30 lines so I paste it here, feel free to use it. Also if anyone from Lift committers thinks that this piece of code could be used to fix original PasswordField, feel free to do so:

trait MyPasswordTypedField[OwnerType <: Record[OwnerType]] extends Field[String, OwnerType] with PasswordTypedField

{

  def mySalt = 

  {

    val myValue = valueBox.map(v => v.toString) openOr ""

    if(myValue.isEmpty || myValue.length <= 28)

      salt.get

    else

      myValue.substring(28)

  }

  override def match_?(toTest: String): Boolean = 

    is == hash("{"+toTest+"} salt={"+mySalt+"}")+mySalt

  override def set_!(in: Box[String]): Box[String] = {

    // to have private validatedValue (see PasswordField) set

    super.set_!(in)

    // return original, non-hashed value so that there is no double hashing when reading from DB

    in

  }

  

  override def apply(in: Box[MyType]): OwnerType = 

  {

  val hashed = in.map(s => hash("{"+s+"} salt={"+mySalt+"}")+mySalt)

    super.apply(hashed)

  }

}

Then in my User entity class I do:

class User private() extends Record[User] with KeyedRecord[Long]

{

def meta = User 

@Column(name="id")

override val idField = new LongField(this)

val email = new EmailField(this,50)

val userName = new StringField(this,50)

val password = new PasswordField(this) with MyPasswordTypedField[User]

val firstName = new StringField(this,50)

val lastName = new StringField(this,50)

val superUser = new BooleanField(this)

}

And it works beautifly now.

[David Whittaker]

Hi Ján,

Looking into this is on my todo list.  You can create an Assembla ticket and assign it to me  if you'd like.

2011/4/27 Ján Raska <ras...@gmail.com>

[Mahmood Ali]

Greetings David,

> Looking into this is on my todo list.  You can create an Assembla ticket and
> assign it to me  if you'd like.

I created a ticket for this awhile ago:
https://www.assembla.com/spaces/liftweb/tickets/937-passwordfield-not-storing-salt
. I resurrected the ticket and assigned it to you.

Thanks a lot.
- Mahmood

[Ján Raska]

Hello Dave,

A ticket is already set up by Mahmood. However, looking at https://groups.google.com/forum/#!topic/liftweb/rjUbydmctCA and ticket #822, it might be a good idea not to bother with SHA any more and go straight with JBcrypt. It seems that PasswordField is completely broken with this implementation, so I doubt anyone is using it. If that assumption is correct and it doesn't work for MongoDB and CouchDB implementations either, you could solve 2 tickets (at least from Record side) with one piece of code. In this case it wouldn't require a backward compatibility for existing databases either, as is mentioned at #822.

[Tim Nelson]

Hi,

I agree with Jan. I already have a working version using jBCrypt (see my earlier post in this thread). We could use that.

Tim

[David Whittaker]

Sounds good to me.  So Tim, you can verify that changing the hash method won't affect the mongo modules?  I tend to agree with Ján that it's too broken as is to be used, but It can't hurt to be sure.  I feel like when I originally looked into this I found that PasswordField is used within ProtoUser.  Does mongo have a proto user implementation?

[Tim Nelson]

On Thursday, April 28, 2011 10:47:28 AM UTC-5, Dave Whittaker wrote:

Sounds good to me.  So Tim, you can verify that changing the hash method won't affect the mongo modules?  I tend to agree with Ján that it's too broken as is to be used, but It can't hurt to be sure.  I feel like when I originally looked into this I found that PasswordField is used within ProtoUser.  Does mongo have a proto user implementation?

I'm not aware of any implementations using ProtoUser directly. There's just this [1], but it's rather old and doesn't use ProtoUser and it's using MongoPasswordFIeld.

Tim

1 - https://github.com/eltimn/lift-mongo-protouser

 

Hi Ján,

2011/4/27 Ján Raska <ras...@gmail.com>

To unsubscribe from this group, send email to liftwe...@googlegroups.com.

For more options, visit this group at http://groups.google.com/group/liftweb?hl=en.

--
You received this message because you are subscribed to the Google Groups "Lift" group.

To post to this group, send email to lif...@googlegroups.com.

To unsubscribe from this group, send email to liftwe...@googlegroups.com.

For more options, visit this group at http://groups.google.com/group/liftweb?hl=en.

--
You received this message because you are subscribed to the Google Groups "Lift" group.

To post to this group, send email to lif...@googlegroups.com.

To unsubscribe from this group, send email to liftwe...@googlegroups.com.

For more options, visit this group at http://groups.google.com/group/liftweb?hl=en.

[Ján Raska]

I have one suggestion for Tim's code - have a default apply to hash the password, so one doesn't have to call hashIt or use two parameter apply. I can't imagine anyone wanting passwordField("newPassword") call to actually set unencrypted value and as far as I know, DB implementation libraries use set() and set_!() methods to set field value when reading from database.

This would make sure, that value is always hashed, otherwise a developer mistake (other Fields usually have just 1 param in apply) could cause plaintext being stored in DB. If anyone wants to insert a value without hashing (eg. already hashed value when migrating), they can still use set() method, I think it's public.

[Tim Nelson]

It's convenient when creating using regular snippets for forms and not using toForm. You can't chain calls when using set because it returns FieldType.

Having said that, I would be fine with this change.

Tim

Hi Ján,

2011/4/27 Ján Raska <ras...@gmail.com>

To unsubscribe from this group, send email to liftwe...@googlegroups.com.

For more options, visit this group at http://groups.google.com/group/liftweb?hl=en.

--
You received this message because you are subscribed to the Google Groups "Lift" group.

To post to this group, send email to lif...@googlegroups.com.

To unsubscribe from this group, send email to liftwe...@googlegroups.com.

For more options, visit this group at http://groups.google.com/group/liftweb?hl=en.

--
You received this message because you are subscribed to the Google Groups "Lift" group.

To post to this group, send email to lif...@googlegroups.com.

To unsubscribe from this group, send email to liftwe...@googlegroups.com.

For more options, visit this group at http://groups.google.com/group/liftweb?hl=en.

--
You received this message because you are subscribed to the Google Groups "Lift" group.
To post to this group, send email to lif...@googlegroups.com.
To unsubscribe from this group, send email to liftweb+u...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/liftweb?hl=en.

[David Pollak]

It'd be ideal if:

• The same code us used in MappedPassword and Record's password used the same mechanism (bcrypt)
• The Mapper code was a seamless transition from the current hash mechanism to a new hash mechanism such that passwords currently stored will still work and passwords stored with the new bcrypt work
  

Lift, the simply functional web framework http://liftweb.net

Simply Lift http://simply.liftweb.net
Beginning Scala http://www.apress.com/book/view/1430219890
Follow me: http://twitter.com/dpp
Blog: http://goodstuff.im

[Ján Raska]

On Apr 28, 2011, at 19:45 , David Pollak wrote:

It'd be ideal if:

• The same code us used in MappedPassword and Record's password used the same mechanism (bcrypt)

I agree, but is it necessary for this to be done at the same time if Record's password doesn't work at all currently? My suggestion was, that if Dave was about to fix it, it'd be wise to already implement bcrypt and not to bother with SHA any more

• The Mapper code was a seamless transition from the current hash mechanism to a new hash mechanism such that passwords currently stored will still work and passwords stored with the new bcrypt work
  

As far as I understand MappedPassword code, it shouldn't be difficult: checking if salt field is empty (bcrypt has salt included in a hash) or checking if hash starts with $2$ or $2a$ will determine that it's bcrypt hash. Adding to that, I'd suggest, that if SHA password is detected, after successful authentication it should be replaced by a bcrypt hash right away. We shouldn't wait for users to actually change the password, they don't do that very often.

[David Whittaker]

2011/4/28 Ján Raska <ras...@gmail.com>

On Apr 28, 2011, at 19:45 , David Pollak wrote:

It'd be ideal if:

• The same code us used in MappedPassword and Record's password used the same mechanism (bcrypt)

I agree, but is it necessary for this to be done at the same time if Record's password doesn't work at all currently? My suggestion was, that if Dave was about to fix it, it'd be wise to already implement bcrypt and not to bother with SHA any more

It is something that I plan to do, but it's not at the top of the list right now.  I think there is time to like into MappedPassword as well and see if there is enough common ground to get a fix together for both.

 

• The Mapper code was a seamless transition from the current hash mechanism to a new hash mechanism such that passwords currently stored will still work and passwords stored with the new bcrypt work
  

As far as I understand MappedPassword code, it shouldn't be difficult: checking if salt field is empty (bcrypt has salt included in a hash) or checking if hash starts with $2$ or $2a$ will determine that it's bcrypt hash. Adding to that, I'd suggest, that if SHA password is detected, after successful authentication it should be replaced by a bcrypt hash right away. We shouldn't wait for users to actually change the password, they don't do that very often.

I don't think that this would be possible.  Since it's a one way hash users would have to enter their password again to rehash it.  The only time you can be sure this will be done is on a password change or during authentication and I don't think firing off SQL updates that the user isn't planning on during authentication would be a good idea. 

[Rohit Rai]

Hi Guys,

I am getting the same issue while using Record's MMPU . . . The user
registers, but is not able to login. In a custom implementation on
MMPU I am using MongoRecord's Password field which works.

I have the application with the code using Record's MMPU failing to
login on github here - https://github.com/rohit-tingendab/lift-mongo-demo

You can try the code here - https://github.com/ghostm/lift-todo-mongodb/
to verify that MongoDB password works as expected.

If an approach to solve this thing is planned I can commit time
implementing it . . . Ofcourse someone will have to guide and review
my code!

PS. - DPP - Can I somehow override the MMPU's password field with an
Object of MongoPassword untill someone can get this issue fixed?

On Apr 29, 12:02 am, David Whittaker <d...@iradix.com> wrote:
> 2011/4/28 Ján Raska <ras...@gmail.com>
>
>
>
> > On Apr 28, 2011, at 19:45 , David Pollak wrote:
>
> > It'd be ideal if:
>

> >    - The same code us used in MappedPassword and Record's password used

> >    the same mechanism (bcrypt)
>
> > I agree, but is it necessary for this to be done at the same time if
> > Record's password doesn't work at all currently? My suggestion was, that if
> > Dave was about to fix it, it'd be wise to already implement bcrypt and not
> > to bother with SHA any more
>
> It is something that I plan to do, but it's not at the top of the list right
> now.  I think there is time to like into MappedPassword as well and see if
> there is enough common ground to get a fix together for both.
>
>
>

> >    - The Mapper code was a seamless transition from the current hash

> >>>>>https://groups.google.com/forum/#!topic/liftweb/rjUbydmctCAand ticket

> >>>>> #822, it might be a good idea not to bother with SHA any more and go
> >>>>> straight with JBcrypt. It seems that PasswordField is completely broken with
> >>>>> this implementation, so I doubt anyone is using it. If that assumption is
> >>>>> correct and it doesn't work for MongoDB and CouchDB implementations either,
> >>>>> you could solve 2 tickets (at least from Record side) with one piece of
> >>>>> code. In this case it wouldn't require a backward compatibility for existing
> >>>>> databases either, as is mentioned at #822.
>
> >>>>> On Apr 28, 2011, at 16:33 , David Whittaker wrote:
>
> >>>>> Hi Ján,
>
> >>>>> Looking into this is on my todo list.  You can create an Assembla
> >>>>> ticket and assign it to me  if you'd like.
>
> >>>>> 2011/4/27 Ján Raska <ras...@gmail.com>
>
> >>>>>> I've solved this problem by writing a small trait and mixing it into
> >>>>>> my password field. It works with Squeryl-record, I don't know about other
> >>>>>> implementations such as MongoDB. It appends salt to the password hash and
> >>>>>> stores it together in single field. Hashing is only done in apply, not in
> >>>>>> set_!() method, because set_!() is also used to populate field with data
> >>>>>> from database, so hashing in set_!() (as it is in PasswordField) causes that
> >>>>>> the password is double hashed. As far as I've tested, this is the only
> >>>>>> limitation. It's only about 30 lines so I paste it here, feel free to use
> >>>>>> it. Also if anyone from Lift committers thinks that this piece of code could
> >>>>>> be used to fix original PasswordField, feel free to do so:
>

> >>>>>> trait MyPasswordTypedField[OwnerType <: Record[OwnerType]] extendsField[String, OwnerType]

> >>>>>>  val password = new PasswordField(this) withMyPasswordTypedField[User]

> >>>>>>> *      object password extends PasswordField(this, 32,
> >>>>>>> Full(confirmPassword)) {*
> >>>>>>> *  override def displayName = "Password"*
> >>>>>>> *  override def shouldDisplay_? = if (isUnregistered) true else
> >>>>>>> false*
> >>>>>>> * }*
> >>>>>>> * object confirmPassword extends StringField(this, 32) {*
> >>>>>>> *  override def ignoreField_? = true*
> >>>>>>> *...
>
> read more »

[David Whittaker]

Rohit,

There is a ticket open (and assigned to me) describing the issue: https://www.assembla.com/spaces/liftweb/tickets/937-passwordfield-not-storing-salt.  Unfortunately I haven't had the time yet to look into doing this the proper way, using DBCrypt and making sure to maintain backwards compatibility.  I appreciate your offer to help but Lift has a strict policy regarding IP rights and who can contribute code.  If you have a work around (and I know that there is at least one in a previous post floating around on this list if you don't) I would suggest that you use that for the time being and watch the ticket for an official fix.

-Dave

[Rohit Rai]

Hi David,

Yes, I agree proper IP rights are management are important to any project success. But there is nothing holding back for a person who has a solution and willing to assign the rights (which I am always willing to). Whenever I use a open source application I am always willing to give back and I plan to give back to lift too over the coming years! 

But the thing on assigning rights will come "if" and "when" I am able to solve the issue and have the code ready and upto acceptable standards for being committed into the project (which should always be the first criteria for getting code in). 

I saw that the issue is blocking any progress on the tasks and is open for a some time so raised a hand for you to shed of your load if you don't have time to work on it... as you have already mentioned in the conversation earlier that it is not your priority.

And just for anyone capable of coding good in scala or contributing to Lift (which I personally am not too confident about), don't be disheartened reading this chain... It's not impossible to contribute to Lift, 

For reference here are instructions on how to contribute -

http://www.assembla.com/spaces/liftweb/wiki/Guide_to_Committing_Code

" you must have first signed an IP assignment form"

Regards,

Rohit

[David Pollak]

On Fri, Jun 3, 2011 at 6:23 AM, Rohit Rai <ro...@tingendab.com> wrote:

Hi David,

Yes, I agree proper IP rights are management are important to any project success. But there is nothing holding back for a person who has a solution and willing to assign the rights (which I am always willing to). Whenever I use a open source application I am always willing to give back and I plan to give back to lift too over the coming years! 

The first and most important part of contributing to Lift is contributing to the community.  Helping people on this list is the first and most important criteria for being a Lift committer.  The next criteria is committing to stick around the community and support any code that you contribute.  The third most important thing is writing quality code.

I'm sorry the PasswordField thing is blocking you right now.  I'm also more globally sorry about the state of the Record APIs... they really are not unified across the drivers.  I'm hoping David has a good 3-4 days over the next development cycle to unify some of the APIs and to address some of the more glaring issues.
 

--

Lift, the simply functional web framework http://liftweb.net

Simply Lift http://simply.liftweb.net

[Rohit Rai]

I agree 200% with your words David... And I plan to do all that ! It was just this one thing that can I think I can contribute to so I offered help.

I am doing this project to smooth the ramp for newbie's to start with!

I have worked with javaee for life... Rugby in past... Working on Grails... Made some very minor contribution to it too...

I strongly feel that if we ease the ramp up for newbies like me we will see much more happening in the community and much better apps coming in the world :)

I am a thick skinned, upfront and very very stubborn guy... So you will be seeing my code coming in the lift repo sooner than later!

I haven't seen any project discouraging contributions so proactively! But that's my opinion that I feel we can improve on. Not criticism!

Cheers,
Rohit

>> "* **you must have first signed an IP assignment form"*
>> *
>> *
>> *
>> *Regards,

[David Whittaker]

On Fri, Jun 3, 2011 at 10:28 AM, David Pollak <feeder.of...@gmail.com> wrote:

On Fri, Jun 3, 2011 at 6:23 AM, Rohit Rai <ro...@tingendab.com> wrote:

Hi David,

Yes, I agree proper IP rights are management are important to any project success. But there is nothing holding back for a person who has a solution and willing to assign the rights (which I am always willing to). Whenever I use a open source application I am always willing to give back and I plan to give back to lift too over the coming years! 

The first and most important part of contributing to Lift is contributing to the community.  Helping people on this list is the first and most important criteria for being a Lift committer.  The next criteria is committing to stick around the community and support any code that you contribute.  The third most important thing is writing quality code.

I'm sorry the PasswordField thing is blocking you right now.  I'm also more globally sorry about the state of the Record APIs... they really are not unified across the drivers.  I'm hoping David has a good 3-4 days over the next development cycle to unify some of the APIs and to address some of the more glaring issues.
 

That makes two of us.  Our first large production Lift site is about a month from release now and once that is out my schedule should free up.

[Kevin]

>From: Tim Nelson <tnell...@gmail.com>
>Date: Tue, 8 Feb 2011 05:45:34 -0800 (PST)

>Anyway, I decided to write my own password field using jBCrypt.

Tim's solution is great but I also like to see other option. Unifying
the API for this issue is important to me.

Thanks David and Lift's committers!