Skip to content

Commit

Permalink
fix(scram): verify server digest, ensuring mutual authentication
Browse files Browse the repository at this point in the history
  • Loading branch information
mbroadst committed Dec 6, 2019
1 parent cb107a8 commit 806cd62
Show file tree
Hide file tree
Showing 2 changed files with 89 additions and 6 deletions.
48 changes: 42 additions & 6 deletions lib/core/auth/scram.js
Expand Up @@ -19,7 +19,6 @@ try {
var parsePayload = function(payload) {
var dict = {};
var parts = payload.split(',');

for (var i = 0; i < parts.length; i++) {
var valueParts = parts[i].split('=');
dict[valueParts[0]] = valueParts[1];
Expand Down Expand Up @@ -105,6 +104,23 @@ function HI(data, salt, iterations, cryptoMethod) {
return saltedData;
}

function compareDigest(lhs, rhs) {
if (lhs.length !== rhs.length) {
return false;
}

if (typeof crypto.timingSafeEqual === 'function') {
return crypto.timingSafeEqual(lhs, rhs);
}

let result = 0;
for (let i = 0; i < lhs.length; i++) {
result |= lhs[i] ^ rhs[i];
}

return result === 0;
}

/**
* Creates a new ScramSHA authentication mechanism
* @class
Expand Down Expand Up @@ -179,9 +195,19 @@ class ScramSHA extends AuthProvider {

const payload = Buffer.isBuffer(r.payload) ? new Binary(r.payload) : r.payload;
const dict = parsePayload(payload.value());

const iterations = parseInt(dict.i, 10);
if (iterations && iterations < 4096) {
callback(new MongoError(`Server returned an invalid iteration count ${iterations}`), false);
return;
}

const salt = dict.s;
const rnonce = dict.r;
if (rnonce.startsWith('nonce')) {
callback(new MongoError(`Server returned an invalid nonce: ${rnonce}`), false);
return;
}

// Set up start of proof
const withoutProof = `c=biws,r=${rnonce}`;
Expand All @@ -192,25 +218,35 @@ class ScramSHA extends AuthProvider {
cryptoMethod
);

if (iterations && iterations < 4096) {
const error = new MongoError(`Server returned an invalid iteration count ${iterations}`);
return callback(error, false);
}

const clientKey = HMAC(cryptoMethod, saltedPassword, 'Client Key');
const serverKey = HMAC(cryptoMethod, saltedPassword, 'Server Key');
const storedKey = H(cryptoMethod, clientKey);
const authMessage = [firstBare, payload.value().toString('base64'), withoutProof].join(',');

const clientSignature = HMAC(cryptoMethod, storedKey, authMessage);
const clientProof = `p=${xor(clientKey, clientSignature)}`;
const clientFinal = [withoutProof, clientProof].join(',');

const serverSignature = HMAC(cryptoMethod, serverKey, authMessage);

const saslContinueCmd = {
saslContinue: 1,
conversationId: r.conversationId,
payload: new Binary(Buffer.from(clientFinal))
};

sendAuthCommand(connection, `${db}.$cmd`, saslContinueCmd, (err, r) => {
if (r && typeof r.ok === 'number' && r.ok === 0) {
callback(err, r);
return;
}

const parsedResponse = parsePayload(r.payload.value());

This comment has been minimized.

Copy link
@jsvegborn

jsvegborn Dec 19, 2019

TypeError: Cannot read property 'payload' of null. We are getting these errors every now and then in our app, so it would be great to get that fixed 🙂

This comment has been minimized.

Copy link
@mbroadst

mbroadst Dec 19, 2019

Author Member

This looks like you're running into NODE-2390. It's in progress, and we'll have a patch release this week.

This comment has been minimized.

Copy link
@jsvegborn

jsvegborn Dec 19, 2019

Perfect, thanks! 🤩

if (!compareDigest(Buffer.from(parsedResponse.v, 'base64'), serverSignature)) {
callback(new MongoError('Server returned an invalid signature'));
return;
}

if (!r || r.done !== false) {
return callback(err, r);
}
Expand Down
47 changes: 47 additions & 0 deletions test/unit/core/scram_iterations.test.js
Expand Up @@ -65,4 +65,51 @@ describe('SCRAM Iterations Tests', function() {

client.connect();
});

it('should error if server digest is invalid', function(_done) {
const credentials = new MongoCredentials({
mechanism: 'default',
source: 'db',
username: 'user',
password: 'pencil'
});

let done = e => {
done = () => {};
return _done(e);
};

test.server.setMessageHandler(request => {
const doc = request.document;
if (doc.ismaster) {
return request.reply(Object.assign({}, mock.DEFAULT_ISMASTER));
} else if (doc.saslStart) {
return request.reply({
ok: 1,
done: false,
payload: Buffer.from(
'r=VNnXkRqKflB5+rmfnFiisCWzgDLzez02iRpbvE5mQjMvizb+VkSPRZZ/pDmFzLxq,s=dZTyOb+KZqoeTFdsULiqow==,i=10000'
)
});
} else if (doc.saslContinue) {
return request.reply({
ok: 1,
done: false,
payload: Buffer.from('v=bWFsaWNpb3VzbWFsaWNpb3VzVzV')
});
}
});

const client = new Server(Object.assign({}, test.server.address(), { credentials }));
client.on('error', err => {
expect(err).to.not.be.null;
expect(err)
.to.have.property('message')
.that.matches(/Server returned an invalid signature/);

client.destroy(done);
});

client.connect();
});
});

0 comments on commit 806cd62

Please sign in to comment.