OWASP · ckarande · Feb 22, 2017 · Sep 12, 2016 · Sep 12, 2016 · Sep 12, 2016
diff --git a/README.md b/README.md
@@ -90,7 +90,7 @@ docker-compose up
 
 
 #### Customizing the Default Application Configuration
-The default application settings (database url, http port, etc.) can be changed by updating the [config file] (https://github.com/OWASP/NodejsGoat/blob/master/config/env/all.js).
+The default application settings (database url, http port, etc.) can be changed by updating the [config file] (https://github.com/OWASP/NodeGoat/blob/master/config/env/all.js).
 
 ## Report bugs, Feedback, Comments
 *  Open a new [issue](https://github.com/OWASP/NodeGoat/issues) or contact team by joining chat at [Slack](https://owasp.slack.com/messages/project-nodegoat/) or [![Join the chat at https://gitter.im/OWASP/NodeGoat](https://badges.gitter.im/Join%20Chat.svg)](https://gitter.im/OWASP/NodeGoat?utm_source=badge&utm_medium=badge&utm_campaign=pr-badge&utm_content=badge)

diff --git a/app/data/allocations-dao.js b/app/data/allocations-dao.js
@@ -17,7 +17,7 @@ function AllocationsDAO(db) {
 
 
     this.update = function(userId, stocks, funds, bonds, callback) {
-        var finalId = parseInt(userId);
+        var parsedUserId = parseInt(userId);
 
         // Create allocations document
         var allocations = {
@@ -28,7 +28,7 @@ function AllocationsDAO(db) {
         };
 
         allocationsCol.update({
-            userId: finalId
+            userId: parsedUserId
         }, allocations, {
             upsert: true
         }, function(err, result) {
@@ -55,51 +55,42 @@ function AllocationsDAO(db) {
         });
     };
 
-    // This is the good implementation, respect the last one
-    this.getByUserId = function(userId, callback) {
-        var finalId = parseInt(userId);
-
-        allocationsCol.findOne({
-            userId: finalId
-        }, function(err, allocations) {
-            if (err) return callback(err, null);
-            if (!allocations) return callback("ERROR: No allocations found for the user", null);
-
-            userDAO.getUserById(finalId, function(err, user) {
-                if (err) return callback(err, null);
-
-                // add user details
-                allocations.userId = userId;
-                allocations.userName = user.userName;
-                allocations.firstName = user.firstName;
-                allocations.lastName = user.lastName;
-
-                callback(null, allocations);
-            });
-
-        });
-    };
-
-    this.getThresholdByUserId = function(userId, threshold, callback) {
-        var finalId = parseInt(userId);
+    this.getByUserIdAndThreshold = function(userId, threshold, callback) {
+        var parsedUserId = parseInt(userId);
+
+        function searchCriteria() {
+
+            if (threshold) {
+                /*
+                // Fix for A1 - 2 NoSQL Injection - escape the threshold parameter properly
+                // Fix this NoSQL Injection which doesn't sanitze the input parameter 'threshold' and allows attackers
+                // to inject arbitrary javascript code into the NoSQL query:
+                // 1. 0';while(true){}'
+                // 2. 1'; return 1 == 1
+                // Also implement fix in allocations.html for UX.                             
+                const parsedThreshold = parseInt(threshold, 10);
+
+                if (parsedThreshold >= 0 && parsedThreshold <= 99) {
+                    return {$where: `this.userId == ${parsedUserId} && this.stocks > ${threshold}`};
+                }
+                throw `The user supplied threshold: ${parsedThreshold} was not valid.`;
+                */
+                return {
+                    $where: `this.userId == ${parsedUserId} && this.stocks > '${threshold}'`
+                };
+            }
+            return {
+                userId: parsedUserId
+            };
+        }
 
-        /*
-        // Fix for A1 - 2 NoSQL Injection - escape the threshold parameter properly
-        Fix this SQL Injection which doesn't sanitze the input parameter 'threshold' and allows attackers
-        to inject arbitrary javascript code into the NoSQL query:
-        1. 0';while(true){}'
-        2. 1'; return 1 == 1
-        */
-        var whereClause = "this.userId == '" + finalId + "' && " + "this.stocks > " + "'" + threshold + "'";
 
-        allocationsCol.findOne({
-            $where: whereClause
-        }, function(err, allocations) {
+        allocationsCol.findOne(searchCriteria(), function(err, allocations) {
 
             if (err) return callback(err, null);
             if (!allocations) return callback("ERROR: No allocations found for the user", null);
 
-            userDAO.getUserById(finalId, function(err, user) {
+            userDAO.getUserById(parsedUserId, function(err, user) {
                 if (err) return callback(err, null);
 
                 // add user details
@@ -113,6 +104,7 @@ function AllocationsDAO(db) {
 
         });
     };
+
 }
 
 module.exports.AllocationsDAO = AllocationsDAO;
diff --git a/app/data/contributions-dao.js b/app/data/contributions-dao.js
@@ -15,11 +15,11 @@ function ContributionsDAO(db) {
     var userDAO = new UserDAO(db);
 
     this.update = function(userId, preTax, afterTax, roth, callback) {
-        var finalId = parseInt(userId);
+        var parsedUserId = parseInt(userId);
 
         // Create contributions document
         var contributions = {
-            userId: finalId,
+            userId: parsedUserId,
             preTax: preTax,
             afterTax: afterTax,
             roth: roth
@@ -35,7 +35,7 @@ function ContributionsDAO(db) {
                 if (!err) {
                     console.log("Updated contributions");
                     // add user details
-                    userDAO.getUserById(finalId, function(err, user) {
+                    userDAO.getUserById(parsedUserId, function(err, user) {
 
                         if (err) return callback(err, null);
 

diff --git a/app/data/user-dao.js b/app/data/user-dao.js
@@ -23,11 +23,11 @@ function UserDAO(db) {
             lastName: lastName,
             benefitStartDate: this.getRandomFutureDate(),
             password: password //received from request param
-                /*
-                // Fix for A2-1 - Broken Auth
-                // Stores password  in a safer way using one way encryption and salt hashing
-                password: bcrypt.hashSync(password, bcrypt.genSaltSync())
-                */
+            /*
+            // Fix for A2-1 - Broken Auth
+            // Stores password  in a safer way using one way encryption and salt hashing
+            password: bcrypt.hashSync(password, bcrypt.genSaltSync())
+            */
         };
 
         // Add email if set
@@ -64,6 +64,16 @@ function UserDAO(db) {
 
     this.validateLogin = function(userName, password, callback) {
 
+        // Helper function to compare passwords
+        function comparePassword(fromDB, fromUser) {
+            return fromDB === fromUser;
+            /*
+            // Fix for A2-Broken Auth
+            // compares decrypted password stored in this.addUser()
+            return bcrypt.compareSync(fromDB, fromUser);
+            */
+        }
+
         // Callback to pass to MongoDB that validates a user document
         function validateUserDoc(err, user) {
 
@@ -86,16 +96,6 @@ function UserDAO(db) {
             }
         }
 
-        // Helper function to compare passwords
-        function comparePassword(fromDB, fromUser) {
-            return fromDB === fromUser;
-            /*
-            // Fix for A2-Broken Auth
-            // compares decrypted password stored in this.addUser()
-            return bcrypt.compareSync(fromDB, fromUser);
-            */
-        }
-
         usersCol.findOne({
             userName: userName
         }, validateUserDoc);

diff --git a/app/routes/allocations.js b/app/routes/allocations.js
@@ -13,22 +13,7 @@ function AllocationsHandler(db) {
         */
         var userId = req.params.userId;
 
-        allocationsDAO.getByUserId(userId, function(err, docs) {
-            if (err) return next(err);
-
-            docs.userId = userId; //set for nav menu items
-
-            return res.render("allocations", docs);
-        });
-    };
-
-    this.displayAllocationsThreshold = function(req, res, next) {
-
-        var userId = req.session.userId;
-        var threshold = req.query.threshold || 0;
-
-        allocationsDAO.getThresholdByUserId(userId, threshold, function(err, docs) {
-
+        allocationsDAO.getByUserIdAndThreshold(userId, req.query.threshold, function(err, docs) {
             if (err) return next(err);
 
             docs.userId = userId; //set for nav menu items

diff --git a/app/routes/index.js b/app/routes/index.js
@@ -56,7 +56,6 @@ var exports = function(app, db) {
 
     // Allocations Page
     app.get("/allocations/:userId", isLoggedIn, allocationsHandler.displayAllocations);
-    app.get("/allocations-threshold", isLoggedIn, allocationsHandler.displayAllocationsThreshold);
 
     // Handle redirect for learning resources link
     app.get("/learn", isLoggedIn, function(req, res, next) {

diff --git a/app/routes/session.js b/app/routes/session.js
@@ -65,16 +65,16 @@ function SessionHandler(db) {
                         userName: userName,
                         password: "",
                         loginError: invalidUserNameErrorMessage
-                            //Fix for A2-2 Broken Auth - Uses identical error for both username, password error
-                            // loginError: errorMessage
+                        //Fix for A2-2 Broken Auth - Uses identical error for both username, password error
+                        // loginError: errorMessage
                     });
                 } else if (err.invalidPassword) {
                     return res.render("login", {
                         userName: userName,
                         password: "",
                         loginError: invalidPasswordErrorMessage
-                            //Fix for A2-2 Broken Auth - Uses identical error for both username, password error
-                            // loginError: errorMessage
+                        //Fix for A2-2 Broken Auth - Uses identical error for both username, password error
+                        // loginError: errorMessage
 
                     });
                 } else {

diff --git a/app/views/allocations.html b/app/views/allocations.html
@@ -3,27 +3,31 @@
 <div class="row">
     <div class="col-lg-12">
 
-      <div class="panel panel-default">
-          <div class="panel-heading">
-              <h3 class="panel-title">
-                  Filter Assets based on Stock Performance
-              </h3>
-          </div>
-
-          <div class="panel-body">
-
-          <form action="/allocations-threshold" method="get" role="search">
-            <div class="form-group">
-              <input type="text" class="form-control" placeholder="Stocks Threshold" name="threshold" />
-              <p class="help-block">Using above threshold value, it will return all assets allocation above
-              the specified stocks percentage number.</p>
+        <div class="panel panel-default">
+            <div class="panel-heading">
+                <h3 class="panel-title">
+                    Filter Assets based on Stock Performance
+                </h3>
             </div>
 
-            <button type="submit" class="btn btn-primary">Submit</button>
-          </form>
+            <div class="panel-body">
+
+                <form action="/allocations/{{userId}}" method="get" role="search">
+                    <div class="form-group">
+                        <!--Fix for A1 - 2 NoSQL Injection - Provide validation for input.
+                        Adhering to defence in depth, on the front-end mostly for UX.
+                        The attacker, or user should not be able to enter anything other than 0-99.              
+                        Also implement fix in allocations-dao.js-->
+                        <!--<input type="number" min="0" max="99" class="form-control" placeholder="Stocks Threshold" name="threshold" />-->
+                        <input type="text" class="form-control" placeholder="Stocks Threshold" name="threshold" />
+                        <p class="help-block">Using above threshold value, it will return all assets allocation above the specified stocks percentage number.</p>
+                    </div>
 
+                    <button type="submit" class="btn btn-primary">Submit</button>
+                </form>
+
+            </div>
         </div>
-      </div>
 
         <div class="panel panel-info">
             <div class="panel-heading">
@@ -44,4 +48,4 @@ <h3 class="panel-title">
 
     </div>
 </div>
-{% endblock %}
+{% endblock %}
diff --git a/app/views/tutorial/a1.html b/app/views/tutorial/a1.html
@@ -251,26 +251,37 @@ <h3 class="panel-title">SSJS Attack Mechanics</h3>
 
                         <h5>$where operator</h5>
                         <p>
-                            MongoDB's <code>$where</code> operator performs JavaScript expression evaluation on the MongoDB server. If the user is able to inject direct code into such queries then such an attack can take place
+                            MongoDB's
+                            <code>$where</code> operator performs JavaScript expression evaluation on the MongoDB server. If the user is able to inject direct code into such queries then such an attack can take place
                         </p>
 
                         <p>
                             Lets consider an example query:
                         </p>
-                        <pre> db.usersCollection.find({ $where: "this.country == '" + req.body.country + '" }); </pre>
+                        <pre> db.allocationsCollection.find({ $where: "this.userId == '" + parsedUserId + "' && " + "this.stocks > " + "'" + threshold + "'" }); </pre>
 
                         <p>
-                            The code will match all documents which have a country field as specified by the user. The problem is that this code is un-sanitized and vulnerable to an SSJS Injection.
+                            The code will match all documents which have a
+                            <code>userId</code> field as specified by
+                            <code>parsedUserId</code> and a
+                            <code>stocks</code> field as specified by
+                            <code>threshold</code>. The problem is that these parameters are not validated, filtered, or sanitised, and vulnerable to SSJS Injection.
                         </p>
 
 
                         <br/>
                         <h5>2. NoSQL SSJS Injection</h5>
                         <p>
-                            An attacker can send the following input for the country field in the request's body which will create a valid JavaScript expression and satisfy the <code> $where</code> query as well, resulting in a DoS attack on the MongoDB server:
+                            An attacker can send the following input for the
+                            <code>threshold</code> field in the requests query, which will create a valid JavaScript expression and satisfy the
+                            <code> $where</code> query as well, resulting in a DoS attack on the MongoDB server:
                         </p>
 
-                        <pre> curl http://localhost/users?country=IL';while(true){};' </pre>
+                        <pre>http://localhost:4000/allocations/2?threshold=5';while(true){};' </pre>
+                        <p>
+                            You can also just drop the following into the Stocks Threshold input box:
+                        </p>
+                        <pre>';while(true){};'</pre>
 
                     </div>
                 </div>
@@ -287,6 +298,9 @@ <h3 class="panel-title">How Do I Prevent It?</h3>
                             <li>Input Validation: Validate inputs to detect malicious values. For NoSQL databases, also validate input types against expected types</li>
                             <li>Least Privilege: To minimize the potential damage of a successful injection attack, do not assign DBA or admin type access rights to your application accounts. Similarly minimize the privileges of the operating system account that the database process runs under.</li>
                         </ul>
+                        For the above NoSQL vulnerability, bare minimum fixes can be found in
+                        <code>allocations.html</code> and
+                        <code>allocations-dao.js</code>
                     </div>
                 </div>
             </div>

diff --git a/server.js b/server.js
@@ -85,11 +85,11 @@ MongoClient.connect(config.db, function(err, db) {
         // Both mandatory in Express v4
         saveUninitialized: true,
         resave: true
-            /*
-            // Fix for A5 - Security MisConfig
-            // Use generic cookie name
-            key: "sessionId",
-            */
+        /*
+        // Fix for A5 - Security MisConfig
+        // Use generic cookie name
+        key: "sessionId",
+        */
 
         /*
         // Fix for A3 - XSS
@@ -127,10 +127,10 @@ MongoClient.connect(config.db, function(err, db) {
     swig.setDefaults({
         // Autoescape disabled
         autoescape: false
-            /*
-            // Fix for A3 - XSS, enable auto escaping
-            autoescape: true // default value
-            */
+        /*
+        // Fix for A3 - XSS, enable auto escaping
+        autoescape: true // default value
+        */
     });
 
     // Insecure HTTP connection

diff --git a/test/security/profile-test.js b/test/security/profile-test.js
@@ -84,7 +84,7 @@ test.after(function() {
     var overWrite = true;
     this.timeout(10000);
     webDriver.quit();
-    zaproxy.core.newSession("new NodeGoat session", overWrite, zapApiKey, function () {});
+    zaproxy.core.newSession("new NodeGoat session", overWrite, zapApiKey, function() {});
     //zaproxy.core.shutdown(zapApiKey, function () {});
 });