Same password, same options, different verify result

[djbingham]

Hi,

I'm running identical container Docker Swarm Mode stacks on two identical servers. On one of the servers, password verification always works fine. On the other, it keeps failing regardless of whether the input password is correct or not. When it fails, it seems to fail for all passwords.

To make sure it was a problem with Secure Password and not with my (and others) typing, I copied a stored password hash directly from one database to the other, checked that the stored values were identical and then attempted to log in to each instance of my software by copying and pasting the same password into each log in form. One passed verification, the other failed.

This also happened a couple of days ago. At that time, I found https://github.com/emilbayes/secure-password/issues/10 and so changed the way I stored hashes to match:

const savedHash = Buffer.alloc(securePassword.HASH_BYTES);
savedHash.write(dbhashedvalue);

After deploying that change, everything worked fine for a couple of days. However, today I've noticed that password verification is failing again on the same server as before.

I don't understand how this issue can arise on one server but not the other. Neither has significant usage at the moment and they're running identical container stacks. One of the containers is a Node app which handles password hashing and verification via Secure Password. That container is currently limited to 150MB memory in each stack. I limit Secure Password to 64MB and have checked that the memory utilisation when not hashing passwords generally sits at 62MB, which leaves a little room for growth (not much, but this project is in early stages and for a small, fixed-size team of users, so trying to avoid increasing the cost of hosting).

I generate password hashes with the following function and save the resulting hash string to MongoDB (running in a separate container on the same stack):

const SecurePassword = require('secure-password');

const securePassword = SecurePassword({
    memlimit: 67108864 # 64Mb
});

function hashPassword(userPassword) {
    return new Promise((resolve, reject) => {
        console.debug('Hashing password');

        securePassword.hash(
            Buffer.from(userPassword),
            (error, hashBuffer) => {
                if (error) {
                    reject(error);
                }

                resolve(hashBuffer.toString());
            }
        );
    });
}

I verify the passwords with this function:

function verifyPassword(userPassword, correctHash) {
    const passwordBuffer = Buffer.from(userPassword);
    const hashBuffer = Buffer.alloc(SecurePassword.HASH_BYTES);
    hashBuffer.write(correctHash);

    return new Promise((resolve, reject) => {
        securePassword.verify(passwordBuffer, hashBuffer, async (error, result) => {
            if (error) {
                reject(error);
            }

            switch (result) {
                case SecurePassword.INVALID_UNRECOGNIZED_HASH:
                    reject({message: 'Unexpected error'});
                    break;
                case SecurePassword.INVALID:
                    reject({invalid: true, message: 'Invalid password'});
                    break;
                case SecurePassword.VALID:
                    resolve();
                    break;
                case SecurePassword.VALID_NEEDS_REHASH:
                    try {
                        newHash = await hashPassword(userPassword);
                        resolve(newHash);
                    } catch (error) {
                        resolve(); // Resolve because password verified correctly although failed to create new hash
                    }
                    break
                default:
                    reject({message: 'Unexpected error'});
            }
        });
    });
}

On the failing server, it hits the SecurePassword.INVALID case every time.

Any ideas why this could be failing on one server and not the other?

[emilbayes]

It sounds like the functions are in separate projects? Can you check that they have matching major versions of node and secure-password? Also, can you post a sample password / hash pair?

The above samples look correct, so I can't immediately think of what would be wrong

[djbingham]

Hi @emilbayes, thanks for the quick reply!

Those functions are in the same project. I'm running two copies of the project on two different Docker swarms.

Node version: 9.11.1 Secure Password version: 3.0.0

Password: changeme Hash: $argon2id$v=19$m=65536,t=2,p=1$49cWoO3+fsK3HOFu3cRc/g$BwhPqR5BtORXs3E0PU0S+uf9SSyRjMSr0AkA9VNM5yY\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000

I've restarted the server that was having issues and it's now working fine again. No change to code or data, just a restart of the host VM which automatically started a new instance of each container in my Docker swarm. I guess that's why my previous update seemed to fix the issue - that would have replaced the container too.

Prior to restarting the server, I tried to just restart the container that runs Secure Password but that sent Docker into a frenzy, restarting every container over and over again with the error starting container failed: Address already in use.

[emilbayes]

I think this must be an issue with docker or maybe the permissions that your container has regarding memory. Under the hood libsodium may mark some memory with special flags, that I don't know if docker allows, eg. https://github.com/jedisct1/libsodium/blob/10207d5aa6abff3ac211a41b0a71910e30f69711/src/libsodium/crypto_pwhash/argon2/argon2-core.c#L96-L98

I will close the issue for now, but please reopen if you find more information!

[djbingham]

I have noticed that you raised an issue (https://github.com/jedisct1/libsodium/issues/533) and had a PR to libsodium accepted to distinguish between crypto_pwhash_str_verify failing due to an invalid passsword and failing due to the OS refusing to allocate memory. You do this by setting errno within libsodium, but I haven't found where that is read or whether it is exposed in a way that Secure Password would be able to access. Is there any way for Secure Password to detect whether a verify failure is due to a memory allocation error?

[emilbayes]

That would make sense! Especially with what you mention about your constrained memory limits. I just remembered this note in sodium-native: https://github.com/sodium-friends/sodium-native/blob/82828b55b0b248fd11019cd529fcfd33a3a3f362/src/crypto_pwhash_str_verify_async.cc#L33-L40

I might revisit that and see if you can tell a memory exhaustion from a mismatch.

I actually have a PR (https://github.com/emilbayes/secure-password/pull/5) open for limiting concurrency. I will merge that and major bump, and then you can test that?

[djbingham]

Sure, I'm happy to test an update :-)

I hadn't thought about concurrency causing the memory usage to go above the limit. I suppose memlimit is per hashing operation? If that's true, with my current container limits I could run out of memory with just two concurrent hashing requests - not beyond the realms of possibility even with just 5 users.

That might go some way to explaining my issue, although it's still odd that the container becomes permanently unable to verify hashes. It's like it ran out of memory once and then decided it couldn't access a big chunk of memory ever again!

I don't think this is likely to be possible, but I have a serious lack of knowledge about memory management so it's worth checking: is it possible that a request can allocate some memory when not enough is available? e.g. Two requests each want 64MB, but with only 88MB available at least one of those requests can't have its full amount so it gets a bit allocated and then fails. From then on, the allocated memory is left in an unusable state because it's allocated to a process that never finished?

[emilbayes]

You are right that I should mention that memlimit and opslimit are on a per password basis. Node runs with 4 worker threads by default, so it will run those concurrently. That is all clearer from the docs in #5, so I will merge that ASAP.

I do not yet understand why the container becomes completely unusable, but have a test setup now so I can check it myself. I'm working on a fix so you will get an error if the password is neither a match nor mismatch :)

[djbingham]

That's great to hear, thanks a lot for looking into this. I've got an update ready for my project to limit concurrency to 2 and increase the memory limit on that container to 200MB as soon as #5 is merged.

[emilbayes]

I will just land a bugfix to sodium-native before I release the next major

[emilbayes]

@djbingham I have a PR pending on sodium-native now that I also think will fix your "deadlock" and not give you a invalid password when it's is actually a error: https://github.com/sodium-friends/sodium-native/pull/63

[andrewfinnell]

I have the set same exact issue I believe. Hashing the same buffer twice produces two very different hashes. It seems like some salt is being inserted into the password and is not available to the user.

const securePassword = require('secure-password');
const pwd = securePassword();
const b1 = Buffer.from('mypassword');
console.log(pwd.hashSync(b1));
console.log(pwd.hashSync(b1));

This produces two very different hashes..

[emilbayes]

@andrewfinnell Hey! Sounds like a different issue to me. The same password given to securePassword.hash always gives two different hashes, exactly as you mentioned, due to salting. However extracting all the parameters, including the salt, is done by securePassword.verify so you don't have to worry about it. Please open a new issue if you need more help :)

EDIT: Just saw your edit with the code sample. This is exactly how it is supposed to work. You need to use the verify function and check the return value for how to proceed.

[andrewfinnell]

node: 8.9.3

Something else I noticed is that doing a Buffer.from() on the same string returns two different Buffer results. I've been banging my head on this all day. I'm wondering if 'select is broken', or rather if my version of Node has a bug in it.

const SecurePassword = require('secure-password');
const pwd = SecurePassword();

const text = 'mypassword';

var b1 = Buffer.from(text);
var b2 = Buffer.from(text);

var h1 = pwd.hashSync(b1);
var h2 = pwd.hashSync(b2);

var c1 = h1.toString('base64');
var c2 = h2.toString('base64');

var r1 = Buffer.from(c1, 'base64');
var r2 = Buffer.from(c2, 'base64');

console.log(pwd.verifySync(h1, h2));
console.log(pwd.verifySync(r1, r2));

Results:

Symbol(INVALID)
Symbol(INVALID)

This fails... which is essentially what happens when you hash the password and store it in a database, to then retrieve it later. It sounded like it might be similar to this issue but I can see why it would be different as they're running into memory issues. I should have read all of the comments better before posting.

This code still fails for me though.

[emilbayes]

@andrewfinnell You're comparing two hashes, but the verify function takes a plaintext password and a hash:

const SecurePassword = require('secure-password');
const pwd = SecurePassword();

const text = 'mypassword';

var b1 = Buffer.from(text, 'utf8');
var b2 = Buffer.from(text, 'utf8');

var h1 = pwd.hashSync(b1);
var h2 = pwd.hashSync(b2);

var c1 = h1.toString('base64');
var c2 = h2.toString('base64');

var r1 = Buffer.from(c1, 'base64');
var r2 = Buffer.from(c2, 'base64');

console.log(pwd.verifySync(b1, r1) === SecurePassword.VALID);
console.log(pwd.verifySync(b2, r2) === SecurePassword.VALID);

Hope this helps! :)

[andrewfinnell]

Let's pretend this conversation didn't happen lol. Thank you. In my original test code I have the two buffers swapped on the verify. sigh you're amazing.

[emilbayes]

No worries! :) For posterity, I will also clear up that if there would be any OOM issues during the hashing, no hash would result, and during the verification there would never be a positive match. What @djbingham is experiencing I'm almost certain is due to a errno not being re-set

[djbingham]

@emilbayes How close are you to the next major release? I notice the Sodium Native PR (https://github.com/sodium-friends/sodium-native/pull/63) has failed a CI test.

[emilbayes]

@djbingham Yes, there's still errors on Windows, since the node bindings don't seem to be able to access errno on Windows. I'm still working on resolving this