Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Password verification does not work on different machines. #172

Closed
walkerandco opened this issue Apr 14, 2019 · 8 comments
Closed

Password verification does not work on different machines. #172

walkerandco opened this issue Apr 14, 2019 · 8 comments

Comments

@walkerandco
Copy link

Welcome to the issues section if it's your first time!

Before creating an issue, please be sure to:

  • [ x] Checkout to the latest version, including submodules
  • [ x] Try to find an isolated way to reproduce the behavior
  • [ x] Fill in all the blanks in the most specific way you can

Steps to reproduce

  1. Deploy a docker image with a node.js application using Argon2.
  2. Hash any password of your choice.
  3. From another IP address (in order to access a different node), verify that password.

Expected behaviour

It should verify the password successfully.

Actual behaviour

argon2.verify return false, indicating the password does not match. This is incorrect, the passwords do match. Creating a new hash and verifiying it on the same Kubernetes node works, however if you try to verify that on another node (which obtains the hash from the same MongoDB database) it fails.

Environment

Operating system:
Nodes: CoreOS latest on GKE
Docker: node:latest

Node version:
v10.15.0

Compiler version:

Additional Information:
This is a docker image for a node application which is built in a Gitlab CI pipeline, a docker image is built and then deployed as part of a helm chart on GKE. Argon2 is a module within that application, used to verify password for user login.

The scheme is that the user enters a password which is immediately hashed with sha-256 when they press a key down (the unhashed version is never retained in the browser), this is then emitted over a https websocket connection to the backend where argon2 is available. The password is injected into a function which calls argon2.verify(). The hash is obtained from a MongoDB database, the password comparator is computed by combining the sha-256 hash, the objectID of the user (constant) and the creation timestamp of that user (constant).

When a user is created, the argon2 hash is stored in MongoDB and is accessible from all nodes. The objectID and creation timestamp are constants (because they are immutable). I have verified these are the same on all nodes for every call.

What appears to be happening is that argon2 is computing the verification differently on each machine. It is presently using this configuration: $argon2i$v=19$m=4096,t=3,p=1

Any ideas why the computation would not be the same on every node, given that they are by nature exactly the same on GKE?

@ranisalt
Copy link
Owner

ranisalt commented Apr 15, 2019

The only thing I can think of is the salt is somehow mismatched, which would generate a different password hash. It could be an issue with @phc/format de/serialization. Can you share a little bit of the code around when you first hash and then where you compare/verify the hashes?

immediately hashed with sha-256 when they press a key down

I am curious as to why that.

@walkerandco
Copy link
Author

Sure so this is when the hash is created:

  /**
   * Updates the password for the specified user.
   * @param  {object}   user     [User object containing search parameters and updated password.]
   * @param  {Function} callback [Callback function.]
   * @return {boolean}           [Success status.]
   */
  async setPassword(user, callback){
    try{ //open try
      let password = await this.inject.parent.argon2.hash(user.system.passwordHash + user._id + user.system.creationDate)
      let result = await this.inject.User.findOneAndUpdate({"_id": user._id, "personal.contact.email": user.personal.contact.email}, { $set: { "system.password": password, "system.passwordHash": null }}, {new: true}).exec()
      //TODO add salting step using secret credde
      if (process.env.LOGLEVEL == 5) console.log(new Date().toISOString().replace(/[TZ]/g, ' ').bgGreen.bold +  "[INFO] " + "Password set for user: " + user.personal.contact.email )
      if (process.env.DEBUG) console.log(user.system.password)
      // callback function
      if (callback) callback(null, 1)
      return 1
    } catch(e) { //end of try
      console.error(new Date().toISOString() + e )
      //callback function
      if (callback) callback(e)
      return 0
    } //end of catch
  }

And this is the verification:


  /**
   * Verifies the input password against hashed password value in MongoDB.
   * @param  {Users}  user [A Users object against which verification of SHA-256 hash is to be performed.]
   * @param  {function}  callback [Callback function.]
   * @return {boolean}      [Success status.]
   */
  async verifyPassword(user, callback) {
    try {
      //query users database by contact.email so that contact.email is matched to password
      let retrieved = await this.inject.User.findOne({ "personal.contact.email": user.personal.contact.email }).exec()
      //run argon2 against the provided sha-256 hash with comparison to the database hash
      if (process.env.DEBUG) console.log(user)
      if (process.env.DEBUG) console.log(retrieved)
      if (await this.inject.parent.argon2.verify(retrieved.system.password, user.system.passwordHash + retrieved._id + retrieved.system.creationDate)) {
        let token = await this.inject.parent.crypto.randomBytes(256).toString('hex')
        let result = await this.inject.User.findOneAndUpdate({"_id": retrieved._id, "personal.contact.email": retrieved.personal.contact.email}, { $push: { "system.sessions": token }}, {new: true})
        if (process.env.DEBUG) console.log(retrieved)
        return {user: retrieved, token: token}
       }
       else return 0
    } catch (err) { //end of try
      //throw an error on runtime exception
      console.log(new Date().toISOString().replace(/[TZ]/g, ' ').bgRed.bold +  "[ERROR] " + err )
      throw new Error("Failed to verify password.")
      //callback function
      if (callback) callback(e)
      return 0
    } //end of catch
  } //end of verifyPassword function

The _id and creationDate are constant strings FYI. I have tested these and they are always correctly passed and the values match the database across all nodes.

Re sha-256, this was originally an experiment. The actual communication with the backend is via WS with HTTPS so technically the hash does very little, but the idea is to evict the password from memory in plaintext straight away. In practice, an XSS attack or side-channel attack would render it a useless practice and the HTTPS provides the transport security anyway.

@ranisalt
Copy link
Owner

ranisalt commented Apr 16, 2019

While that should not happen, you should just hash the password straight away.

Did you check if the concatenated stuff is exactly the same for both functions? Log user.system.passwordHash + user._id + user.system.creationDate before hashing and log user.system.passwordHash + retrieved._id + retrieved.system.creationDate before verifying. My guess now is that some of them are not being concatenated correctly (e.g. being converted to "[object Object]")

SHAing it does not add any level of security (it effectively makes the hash become the password) and concatenating extra data is of no use. I guess you are trying to avoid rainbow tables there appending id and date, but argon2 is already protected against it by the salt.

If you want to get rid of the password as soon as possible, delete it from objects right after hashing or verifying, there's not much else you can do.

@ranisalt
Copy link
Owner

Hey, if you wish to incorporate additional data into the hash, I could add that to the library, as Argon2 supports it (see 2.)

@walkerandco
Copy link
Author

I agree, this was the idea with the experimentation of SHA-256. This is a built-in to the crypto API standard for browser JavaScript so it was good to play around with as a PoC. There is argon2 for the browser like this https://github.com/antelle/argon2-browser and some WASM versions as well. It would be interesting to use argon2 in the browser to hash the password text itself straight away, and then hash again on the server with additional values.

If you're wondering why I've developed an obsession with client-side hashing its because of a legal change in the UK and Europe generally. GDPR and general legal developments in the UK have meant that if a user has a password which is compromised from your database of passwords and that password is re-used to gain access and cause further damage to another service or a greater loss to the user who is re-using their password, you can be held liable for it. Not many people understand that GDPR requires us to protect all sensitive data using any and all means available, the exact extent of what it means by "secure" hasn't been legally tested yet in the UK, but most commentators, myself included, predict it will probably align with some of the other wording of GDPR and require the best available security measures to be used. Generally data considered sensitive is data which is about a "protected characteristic" such as gender, age, ethnic origin, disability etc. Previously, in the UK the Data Protection Act 1998 just required "appropriate organisational and technical measures" which essentially means HTTPS will do nicely. Now with GDPR it goes a little further and says "'Processed in a manner that ensures appropriate security of the personal data, including protection against unauthorised or unlawful processing and against accidental loss, destruction or damage, using appropriate technical or organisational measures'" and "‘Taking into account the state of the art, the costs of implementation and the nature, scope, context and purposes of processing as well as the risk of varying likelihood and severity for the rights and freedoms of natural persons, the controller and the processor shall implement appropriate technical and organisational measures to ensure a level of security appropriate to the risk’". So in short, its now expected that you would reasonably protect something that could be used as a vector to attack something else such as a password if your server is hacked and the plaintext is stolen and re-used. This of course, is yet to be tested in court. You can read about it here though if you're interested in the scope of it: https://ico.org.uk/for-organisations/guide-to-data-protection/guide-to-the-general-data-protection-regulation-gdpr/security/

In terms of the combined data to be verified, this is what we send it across all servers, the hash is stored in a MongoDB database and remains constant. Given these are test values, I'll just share them to give you an example (in this case the password is the world's favourite: password):

My local dev environment

Argon2 hash

$argon2i$v=19$m=4096,t=3,p=1$Y/gIfNpXYvC7npO7OqLHgw$pGy3TF7ulg1TDvxB1iXcV1Jas+yCljv0sb3NLo/JMh0

Verify comparator

5e884898da28047151d0e56f8dc6292773603d0d6aabbdd62a11ef721d1542d85c6f529c2b5dc01ec03e9a79Fri Feb 22 2019 01:38:36 GMT+0000 (Greenwich Mean Time)

Remote docker instance on kubernetes node:

Argon2 hash

$argon2i$v=19$m=4096,t=3,p=1$Y/gIfNpXYvC7npO7OqLHgw$pGy3TF7ulg1TDvxB1iXcV1Jas+yCljv0sb3NLo/JMh0

Verify comparator

5e884898da28047151d0e56f8dc6292773603d0d6aabbdd62a11ef721d1542d85c6f529c2b5dc01ec03e9a79Fri Feb 22 2019 01:38:36 GMT+0000 (Coordinated Universal Time)

So you were definately right about the additional data being different. It's a bit of a JS quirk I think because in the database the creationDate is: 2019-02-22 01:38:36.063 as a string type. It looks like JS converts this in a string concatenation which I wasn't expecting. By including the TZ of the host, naturally the string is different which explains what we're seeing here as you rightly pointed out in the first place :D

In regards to your suggestion: #172 (comment) . I think it's an awesome idea, it would be very nice to have a way of including additional data formally as part of the argon2 module. I'm more than happy to test as well if you like.

@ranisalt
Copy link
Owner

Hello @walkerandco, branch ad should enable you to use associated data as per the spec I linked above. Just add an associatedData option, note that it should be a Buffer, not a String.

Let me know if I can improve something (maybe call it ad only?)

@walkerandco
Copy link
Author

I think this is a very good idea @ranisalt, I think you would be better keeping it as associatedData purely because ad might be a bit too short.

@ranisalt
Copy link
Owner

v0.22.0 is published with the associated data support.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants