Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: A3 XSS Attack #300

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
9 changes: 4 additions & 5 deletions app/data/allocations-dao.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,6 @@ const AllocationsDAO = function(db){
const 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:
Expand All @@ -73,10 +72,10 @@ const AllocationsDAO = function(db){
return {$where: `this.userId == ${parsedUserId} && this.stocks > ${parsedThreshold}`};
}
throw `The user supplied threshold: ${parsedThreshold} was not valid.`;
*/
return {
$where: `this.userId == ${parsedUserId} && this.stocks > '${threshold}'`
};

// return {
// $where: `this.userId == ${parsedUserId} && this.stocks > '${threshold}'`
// };
}
return {
userId: parsedUserId
Expand Down
8 changes: 2 additions & 6 deletions app/data/user-dao.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,10 @@ function UserDAO(db) {
firstName,
lastName,
benefitStartDate: this.getRandomFutureDate(),
password //received from request param
/*
// 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())
*/
};

// Add email if set
Expand Down Expand Up @@ -58,12 +56,10 @@ function UserDAO(db) {

// Helper function to compare passwords
const comparePassword = (fromDB, fromUser) => {
return 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
Expand Down
9 changes: 4 additions & 5 deletions app/routes/contributions.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,16 +29,15 @@ function ContributionsHandler(db) {

/*jslint evil: true */
// Insecure use of eval() to parse inputs
const preTax = eval(req.body.preTax);
const afterTax = eval(req.body.afterTax);
const roth = eval(req.body.roth);
// const preTax = eval(req.body.preTax);
// const afterTax = eval(req.body.afterTax);
// const roth = eval(req.body.roth);

/*
//Fix for A1 -1 SSJS Injection attacks - uses alternate method to eval
const preTax = parseInt(req.body.preTax);
const afterTax = parseInt(req.body.afterTax);
const roth = parseInt(req.body.roth);
*/

const {
userId
} = req.session;
Expand Down
27 changes: 13 additions & 14 deletions app/routes/session.js
Original file line number Diff line number Diff line change
Expand Up @@ -61,18 +61,18 @@ function SessionHandler(db) {
const invalidPasswordErrorMessage = "Invalid password";
if (err) {
if (err.noSuchUser) {
console.log("Error: attempt to login with invalid user: ", userName);
// console.log("Error: attempt to login with invalid user: ", userName);

// Fix for A1 - 3 Log Injection - encode/sanitize input for CRLF Injection
// that could result in log forging:
// - Step 1: Require a module that supports encoding
// const ESAPI = require('node-esapi');
const ESAPI = require('node-esapi');
// - Step 2: Encode the user input that will be logged in the correct context
// following are a few examples:
// console.log('Error: attempt to login with invalid user: %s',
// ESAPI.encoder().encodeForHTML(userName));
// console.log('Error: attempt to login with invalid user: %s',
// ESAPI.encoder().encodeForJavaScript(userName));
console.log('Error: attempt to login with invalid user: %s',
ESAPI.encoder().encodeForJavaScript(userName));
// console.log('Error: attempt to login with invalid user: %s',
// ESAPI.encoder().encodeForURL(userName));
// or if you know that this is a CRLF vulnerability you can target this specifically as follows:
Expand All @@ -82,18 +82,18 @@ function SessionHandler(db) {
return res.render("login", {
userName: userName,
password: "",
loginError: invalidUserNameErrorMessage,
// loginError: invalidUserNameErrorMessage,
//Fix for A2-2 Broken Auth - Uses identical error for both username, password error
// loginError: errorMessage
loginError: errorMessage,
environmentalScripts
});
} else if (err.invalidPassword) {
return res.render("login", {
userName: userName,
password: "",
loginError: invalidPasswordErrorMessage,
// loginError: invalidPasswordErrorMessage,
//Fix for A2-2 Broken Auth - Uses identical error for both username, password error
// loginError: errorMessage
loginError: errorMessage,
environmentalScripts
});
} else {
Expand All @@ -112,9 +112,10 @@ function SessionHandler(db) {
// Fix the problem by regenerating a session in each login
// by wrapping the below code as a function callback for the method req.session.regenerate()
// i.e:
// `req.session.regenerate(() => {})`
req.session.userId = user._id;
return res.redirect(user.isAdmin ? "/benefits" : "/dashboard");
req.session.regenerate(() => {
req.session.userId = user._id;
return res.redirect(user.isAdmin ? "/benefits" : "/dashboard");
});
});
};

Expand All @@ -141,12 +142,10 @@ function SessionHandler(db) {
const FNAME_RE = /^.{1,100}$/;
const LNAME_RE = /^.{1,100}$/;
const EMAIL_RE = /^[\S]+@[\S]+\.[\S]+$/;
const PASS_RE = /^.{1,20}$/;
/*
// const PASS_RE = /^.{1,20}$/;
//Fix for A2-2 - Broken Authentication - requires stronger password
//(at least 8 characters with numbers and both lowercase and uppercase letters.)
const PASS_RE =/^(?=.*\d)(?=.*[a-z])(?=.*[A-Z]).{8,}$/;
*/

errors.userNameError = "";
errors.firstNameError = "";
Expand Down
4 changes: 2 additions & 2 deletions app/views/allocations.html
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@ <h3 class="panel-title">
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" />
<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>

Expand Down
12 changes: 6 additions & 6 deletions artifacts/db-reset.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,25 +15,25 @@ const USERS_TO_INSERT = [
"userName": "admin",
"firstName": "Node Goat",
"lastName": "Admin",
"password": "Admin_123",
//"password" : "$2a$10$8Zo/1e8KM8QzqOKqbDlYlONBOzukWXrM.IiyzqHRYDXqwB3gzDsba", // Admin_123
// "password": "Admin_123",
"password" : "$2a$10$8Zo/1e8KM8QzqOKqbDlYlONBOzukWXrM.IiyzqHRYDXqwB3gzDsba", // Admin_123
"isAdmin": true
}, {
"_id": 2,
"userName": "user1",
"firstName": "John",
"lastName": "Doe",
"benefitStartDate": "2030-01-10",
"password": "User1_123"
// "password" : "$2a$10$RNFhiNmt2TTpVO9cqZElb.LQM9e1mzDoggEHufLjAnAKImc6FNE86",// User1_123
// "password": "User1_123"
"password" : "$2a$10$RNFhiNmt2TTpVO9cqZElb.LQM9e1mzDoggEHufLjAnAKImc6FNE86",// User1_123
}, {
"_id": 3,
"userName": "user2",
"firstName": "Will",
"lastName": "Smith",
"benefitStartDate": "2025-11-30",
"password": "User2_123"
//"password" : "$2a$10$Tlx2cNv15M0Aia7wyItjsepeA8Y6PyBYaNdQqvpxkIUlcONf1ZHyq", // User2_123
// "password": "User2_123"
"password" : "$2a$10$Tlx2cNv15M0Aia7wyItjsepeA8Y6PyBYaNdQqvpxkIUlcONf1ZHyq", // User2_123
}];

const tryDropCollection = (db, name) => {
Expand Down
25 changes: 9 additions & 16 deletions server.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,13 @@ const consolidate = require("consolidate"); // Templating library adapter for Ex
const swig = require("swig");
// const helmet = require("helmet");
const MongoClient = require("mongodb").MongoClient; // Driver for connecting to MongoDB
const http = require("http");
// const http = require("http");
const marked = require("marked");
//const nosniff = require('dont-sniff-mimetype');
const app = express(); // Web framework to handle routing requests
const routes = require("./app/routes");
const { port, db, cookieSecret } = require("./config/config"); // Application config properties
/*

// Fix for A6-Sensitive Data Exposure
// Load keys for establishing secure HTTPS connection
const fs = require("fs");
Expand All @@ -25,7 +25,6 @@ const httpsOptions = {
key: fs.readFileSync(path.resolve(__dirname, "./artifacts/cert/server.key")),
cert: fs.readFileSync(path.resolve(__dirname, "./artifacts/cert/server.crt"))
};
*/

MongoClient.connect(db, (err, db) => {
if (err) {
Expand Down Expand Up @@ -82,22 +81,20 @@ MongoClient.connect(db, (err, db) => {
secret: cookieSecret,
// Both mandatory in Express v4
saveUninitialized: true,
resave: true
resave: true,
/*
// Fix for A5 - Security MisConfig
// Use generic cookie name
key: "sessionId",
*/

/*
// Fix for A3 - XSS
// TODO: Add "maxAge"
cookie: {
httpOnly: true
httpOnly: true,
// Remember to start an HTTPS server to get this working
// secure: true
secure: true
}
*/

}));

Expand Down Expand Up @@ -134,24 +131,20 @@ MongoClient.connect(db, (err, db) => {
// Template system setup
swig.setDefaults({
// Autoescape disabled
autoescape: false
/*
// autoescape: false
// Fix for A3 - XSS, enable auto escaping
autoescape: true // default value
*/
});

// Insecure HTTP connection
http.createServer(app).listen(port, () => {
console.log(`Express http server listening on port ${port}`);
});
// http.createServer(app).listen(port, () => {
// console.log(`Express http server listening on port ${port}`);
// });

/*
// Fix for A6-Sensitive Data Exposure
// Use secure HTTPS protocol
https.createServer(httpsOptions, app).listen(port, () => {
console.log(`Express http server listening on port ${port}`);
});
*/

});