bug: fix NoSQL operator injection in admin loginHandler

README.md

@@ -128,7 +128,9 @@ echo '{"username": "admin@snyk.io", "password": {"$gt": ""}}' | http --json $GOO
 ```
 
 We know the username, and we pass on what seems to be an object of some sort.
-That object structure is passed as-is to the `password` property and has a specific meaning to MongoDB - it uses the `$gt` operation which stands for `greater than`. So, we in essence tell MongoDB to match that username with any record that has a password that is greater than `empty string` which is bound to hit a record. This introduces the NoSQL Injection vector.
+That object structure would be passed as-is to the `password` property and has a specific meaning to MongoDB - it uses the `$gt` operation which stands for `greater than`. So, we would in essence tell MongoDB to match that username with any record that has a password that is greater than `empty string` which is bound to hit a record. This is the NoSQL Injection vector.
+
+`loginHandler` now rejects any credential that is not a primitive string and wraps both values in an explicit `$eq` comparison, so the request above returns `401` instead of an admin session.
 
 #### Open redirect
 

exploits/nosql-exploits.sh

@@ -12,9 +12,9 @@ alias ns2='echo '"'"'{"username":"admin@snyk.io", "password":"SuperSecretPasswor
 # failed login
 alias ns3='echo '"'"'{"username":"admin@snyk.io", "password":"WrongPassword"}'"'"' | http --json $GOOF_HOST/login -v'
 
-# successful login, NOSQL Injection, knowing the username
+# NOSQL Injection attempt, knowing the username - rejected with 401 since loginHandler requires string credentials
 alias ns4='echo '"'"'{"username": "admin@snyk.io", "password": {"$gt": ""}}'"'"' | http --json $GOOF_HOST/login -v'
 
-# successful login, NOSQL Injection, without knowing the username
+# NOSQL Injection attempt, without knowing the username - rejected with 401 since loginHandler requires string credentials
 alias ns5='echo '"'"'{"username": {"$gt": ""}, "password": {"$gt": ""}}'"'"' | http --json $GOOF_HOST/login -v'
 

routes/index.js

@@ -35,20 +35,24 @@
 };
 
 exports.loginHandler = function (req, res, next) {
-  if (validator.isEmail(req.body.username)) {
-    User.find({ username: req.body.username, password: req.body.password }, function (err, users) {
-      if (users.length > 0) {
-        const redirectPage = req.body.redirectPage
-        const session = req.session
-        const username = req.body.username
-        return adminLoginSuccess(redirectPage, session, username, res)
-      } else {
-        return res.status(401).send()
-      }
-    });
-  } else {
+  const username = req.body.username;
+  const password = req.body.password;
+
+  if (typeof username !== 'string' || typeof password !== 'string' || !validator.isEmail(username)) {
     return res.status(401).send()
   }
+
+  User.find({ username: { $eq: username }, password: { $eq: password } }, function (err, users) {
+    if (err) return next(err);
+
+    if (users && users.length > 0) {
+      const redirectPage = req.body.redirectPage
+      const session = req.session
+      return adminLoginSuccess(redirectPage, session, username, res)
+    } else {
+      return res.status(401).send()
+    }
+  });
 };
 
 function adminLoginSuccess(redirectPage, session, username, res) {

tests/login-nosql-injection.spec.js

@@ -0,0 +1,91 @@
+'use strict';
+
+// Regression tests for NoSQL operator injection in POST /login.
+// mongoose 4.2.4 cannot talk to a modern mongod, so the model is stubbed with an
+// in-memory collection that implements the operator semantics the exploit relies on.
+const path = require('path');
+const tap = require('tap');
+
+const users = [{ username: 'admin@snyk.io', password: 'SuperSecretPassword' }];
+
+function matches(doc, query) {
+  return Object.keys(query).every(function (key) {
+    const condition = query[key];
+    if (condition !== null && typeof condition === 'object') {
+      if ('$eq' in condition) return doc[key] === condition.$eq;
+      if ('$gt' in condition) return doc[key] > condition.$gt;
+      if ('$ne' in condition) return doc[key] !== condition.$ne;
+      throw new Error('unsupported operator: ' + JSON.stringify(condition));
+    }
+    return doc[key] === condition;
+  });
+}
+
+const models = {
+  Todo: { find: function () { return { sort: function () { return { exec: function () {} }; } }; } },
+  User: {
+    find: function (query, cb) {
+      return cb(null, users.filter(function (doc) { return matches(doc, query); }));
+    },
+  },
+};
+
+require.cache[require.resolve('mongoose')] = {
+  id: require.resolve('mongoose'),
+  filename: require.resolve('mongoose'),
+  loaded: true,
+  exports: { model: function (name) { return models[name]; } },
+};
+
+const routes = require(path.join(__dirname, '..', 'routes'));
+
+function login(body) {
+  return new Promise(function (resolve) {
+    const req = { body: body, session: {} };
+    const res = {
+      status: function (code) { this.statusCode = code; return this; },
+      send: function () { resolve({ status: this.statusCode || 200, session: req.session }); },
+      redirect: function (location) { resolve({ status: 302, location: location, session: req.session }); },
+    };
+    routes.loginHandler(req, res, function (err) { resolve({ error: err, session: req.session }); });
+  });
+}
+
+tap.test('operator injection payloads cannot authenticate', function (t) {
+  const payloads = [
+    { $gt: '' },
+    { $ne: null },
+    { $ne: 'nope' },
+    { $regex: '.*' },
+    ['SuperSecretPassword'],
+  ];
+
+  return Promise.all(payloads.map(function (password) {
+    return login({ username: 'admin@snyk.io', password: password }).then(function (result) {
+      t.equal(result.status, 401, 'password ' + JSON.stringify(password) + ' is rejected');
+      t.notOk(result.session.loggedIn, 'no session is established');
+    });
+  }));
+});
+
+tap.test('an object username cannot authenticate', function (t) {
+  return login({ username: { $ne: null }, password: { $ne: null } }).then(function (result) {
+    t.equal(result.status, 401);
+    t.notOk(result.session.loggedIn);
+  });
+});
+
+tap.test('a wrong password is rejected', function (t) {
+  return login({ username: 'admin@snyk.io', password: 'wrong' }).then(function (result) {
+    t.equal(result.status, 401);
+    t.notOk(result.session.loggedIn);
+  });
+});
+
+tap.test('valid credentials still authenticate', function (t) {
+  return login({ username: 'admin@snyk.io', password: 'SuperSecretPassword' }).then(function (result) {
+    t.equal(result.status, 302);
+    t.equal(result.location, '/admin');
+    t.equal(result.session.loggedIn, 1);
+  });
+});