Merge pull request keystonejs#4478 from keystonejs/molomby/sec-fixes

Security fixes

admin/server/api/download.js

@@ -8,6 +8,7 @@ supports more features at the moment (custom .toCSV method on lists, etc)
 var _ = require('lodash');
 var async = require('async');
 var moment = require('moment');
+var escapeValueForExcel = require('../security/escapeValueForExcel');
 
 var FN_ARGS = /^function\s*[^\(]*\(\s*([^\)]*)\)/m;
 
@@ -64,6 +65,11 @@ module.exports = function (req, res) {
 			}
 		});
 
+		// Prevent CSV macro injection
+		_.forOwn(rowData, (value, prop) => {
+			rowData[prop] = escapeValueForExcel(value);
+		});
+
 		return rowData;
 
 	};

fields/types/markdown/MarkdownField.js

@@ -109,6 +109,15 @@ var renderMarkdown = function (component) {
 	$(component.refs.markdownTextarea).markdown(options);
 };
 
+// Simple escaping of html tags and replacing newlines for displaying the raw markdown string within an html doc
+var escapeHtmlForRender = function (html) {
+	return html
+		.replace(/\&/g, '&')
+		.replace(/\</g, '&lt;')
+		.replace(/\>/g, '&gt;')
+		.replace(/\n/g, '<br />');
+};
+
 module.exports = Field.create({
 	displayName: 'MarkdownField',
 	statics: {
@@ -159,15 +168,11 @@ module.exports = Field.create({
 	},
 
 	renderValue () {
-		// TODO: victoriafrench - is this the correct way to do this? the object
-		// should be creating a default md where one does not exist imo.
-
-		const innerHtml = (
-			this.props.value !== undefined
-			&& this.props.value.md !== undefined
-		)
-		? this.props.value.md.replace(/\n/g, '<br />')
-		: '';
+		// We want to render the raw markdown string, without parsing it to html
+		// The markdown string *itself* may include html though so we need to escape it first
+		const innerHtml = (this.props.value && this.props.value.md)
+			? escapeHtmlForRender(this.props.value.md)
+			: '';
 
 		return (
 			<FormInput

fields/types/markdown/MarkdownType.js

@@ -1,7 +1,8 @@
 var FieldType = require('../Type');
 var marked = require('marked');
-var util = require('util');
+var sanitizeHtml = require('sanitize-html');
 var TextType = require('../text/TextType');
+var util = require('util');
 var utils = require('keystone-utils');
 
 /**
@@ -14,6 +15,11 @@ function markdown (list, path, options) {
 
 	this.toolbarOptions = options.toolbarOptions || {};
 	this.markedOptions = options.markedOptions || {};
+
+	// See sanitize-html docs for defaults
+	// .. https://www.npmjs.com/package/sanitize-html#what-are-the-default-options
+	this.sanitizeOptions = options.sanitizeOptions || {};
+
 	this.height = options.height || 90;
 	this.wysiwyg = ('wysiwyg' in options) ? options.wysiwyg : true;
 
@@ -42,18 +48,29 @@ markdown.prototype.addToSchema = function (schema) {
 	};
 
 	var markedOptions = this.markedOptions;
+	var sanitizeOptions = this.sanitizeOptions;
 
 	var setMarkdown = function (value) {
-		if (value === this.get(paths.md)) {
-			return value;
-		}
-		if (typeof value === 'string') {
-			this.set(paths.html, marked(value, markedOptions));
-			return value;
-		} else {
+		// Clear if saving invalid value
+		if (typeof value !== 'string') {
+			this.set(paths.md, undefined);
 			this.set(paths.html, undefined);
+
 			return undefined;
 		}
+
+		var newMd = sanitizeHtml(value, sanitizeOptions);
+		var newHtml = marked(newMd, markedOptions);
+
+		// Return early if no changes to save
+		if (newMd === this.get(paths.md) && newHtml === this.get(paths.html)) {
+			return newMd;
+		}
+
+		this.set(paths.md, newMd);
+		this.set(paths.html, newHtml);
+
+		return newMd;
 	};
 
 	schema.nested[this.path] = true;

fields/types/markdown/Readme.md

@@ -11,7 +11,9 @@ Stores a nested structure in the model with the properties:
 }
 ```
 
-The `html` path is updated when the `md` path is set by [Marked](https://github.com/chjj/marked)
+When the `md` path is set, the value is first sanitized using [`sanitize-html`](https://github.com/punkave/sanitize-html), then rendered into HTML using [`marked`](https://github.com/chjj/marked).
+(Options for both these packages can be provided in the field definition, see below.)
+The resultant HTML is persisted as `html`.
 
 ## Options
 
@@ -35,10 +37,15 @@ Comma separated list of buttons to hide.
 { type: Types.Markdown, toolbarOptions: { hiddenButtons: 'H1,H6,Code' } }
 ```
 
-`markedOptions` `object`
+`markedOptions` `Object`
 
 markedOptions are an object within options. When generating the html, these options are passed directly in to [Marked](https://github.com/chjj/marked). See the `marked` documentation for details on what options are valid.
 
+`sanitizeOptions` `Object`
+
+Supplied as options to the [`sanitize-html`](https://github.com/punkave/sanitize-html) package.
+If not supplied the field will inherit the [package defaults](https://github.com/punkave/sanitize-html#what-are-the-default-options).
+
 ## Schema
 
 The markdown field will automatically convert markdown to html when the `md` property is changed, via a setter on the `md` path.

fields/types/password/PasswordType.js

@@ -3,6 +3,8 @@ var bcrypt = require('bcrypt-nodejs');
 var FieldType = require('../Type');
 var util = require('util');
 var utils = require('keystone-utils');
+var dumbPasswords = require('dumb-passwords');
+
 
 var regexChunk = {
 	digitChar: /\d/,
@@ -18,20 +20,23 @@ var detailMsg = {
 	lowChar: 'use at least one lower case character',
 	upperChar: 'use at least one upper case character',
 };
+const defaultOptions = { min: 8, max: 72, workFactor: 10, rejectCommon: true };
+
 /**
  * password FieldType Constructor
  * @extends Field
  * @api public
  */
 function password (list, path, options) {
-	this.options = options;
+	// Apply default and enforced options (you can't sort on password fields)
+	options = Object.assign({}, defaultOptions, options, { nosort: false });
+
 	this._nativeType = String;
 	this._underscoreMethods = ['format', 'compare'];
 	this._fixedSize = 'full';
-	// You can't sort on password fields
-	options.nosort = true;
-	this.workFactor = options.workFactor || 10;
+
 	password.super_.call(this, list, path, options);
+
 	for (var key in this.options.complexity) {
 		if ({}.hasOwnProperty.call(this.options.complexity, key)) {
 			if (key in regexChunk !== key in this.options.complexity) {
@@ -42,8 +47,8 @@ function password (list, path, options) {
 			}
 		}
 	}
-	if (this.options.max <= this.options.min) {
-		throw new Error('FieldType.Password: options - min must be set at a lower value than max.');
+	if (this.options.max < this.options.min) {
+		throw new Error('FieldType.Password: options - maximum password length cannot be less than the minimum length.');
 	}
 }
 password.properName = 'Password';
@@ -88,7 +93,7 @@ password.prototype.addToSchema = function (schema) {
 			return next();
 		}
 		var item = this;
-		bcrypt.genSalt(field.workFactor, function (err, salt) {
+		bcrypt.genSalt(field.options.workFactor, function (err, salt) {
 			if (err) {
 				return next(err);
 			}
@@ -162,44 +167,48 @@ password.prototype.compare = function (item, candidate, callback) {
  * Asynchronously confirms that the provided password is valid
  */
 password.prototype.validateInput = function (data, callback) {
-	var min = this.options.min;
-	var max = this.options.max;
-	var complexity = this.options.complexity;
+	var { min, max, complexity, rejectCommon } = this.options;
 	var confirmValue = this.getValueFromData(data, '_confirm');
 	var passwordValue = this.getValueFromData(data);
 
-	var validation = validate(passwordValue, confirmValue, min, max, complexity);
+	var validation = validate(passwordValue, confirmValue, min, max, complexity, rejectCommon);
 
 	utils.defer(callback, validation.result, validation.detail);
 };
 
-var validate = password.validate = function (pass, confirm, min, max, complexity) {
-	var detail = '';
-	var result = true;
-	max = max || 72;
+var validate = password.validate = function (pass, confirm, min, max, complexity, rejectCommon) {
+	var messages = [];
+
 	if (confirm !== undefined
 		&& pass !== confirm) {
-		detail = 'passwords must match\n';
+		messages.push('Passwords must match.');
 	}
 
 	if (min && typeof pass === 'string' && pass.length < min) {
-		detail += 'password must be longer than ' + min + ' characters\n';
+		messages.push('Password must be longer than ' + min + ' characters.');
 	}
 
 	if (max && typeof pass === 'string' && pass.length > max) {
-		detail += 'password must not be longer than ' + max + ' characters\n';
+		messages.push('Password must not be longer than ' + max + ' characters.');
 	}
 
 	for (var prop in complexity) {
 		if (complexity[prop] && typeof pass === 'string') {
 			var complexityCheck = (regexChunk[prop]).test(pass);
 			if (!complexityCheck) {
-				detail += detailMsg[prop] + '\n';
+				messages.push(detailMsg[prop]);
 			}
 		}
 	}
-	result = detail.length === 0;
-	return { result, detail };
+
+	if (rejectCommon && dumbPasswords.check(pass)) {
+		messages.push('Password must not be a common, frequently-used password.');
+	}
+
+	return {
+		result: messages.length === 0,
+		detail: messages.join(' \n'),
+	};
 };
 
 /**

fields/types/password/Readme.md

@@ -4,7 +4,7 @@ Stores a `String` in the model.
 
 Displayed as a password field in the Admin UI, with a 'change' button.
 
-Passwords are automatically encrypted with bcrypt, and expose a method to compare a string to the encrypted hash.
+Passwords are automatically encrypted with `bcrypt`, and expose a method to compare a string to the encrypted hash.
 
 > Note: The encryption happens with a **pre-save hook** added to the **schema**, so passwords set will not be encrypted until an item has been saved to the database.
 
@@ -18,35 +18,62 @@ Passwords are automatically encrypted with bcrypt, and expose a method to compar
 
 `workFactor` `Number`
 
-The bcrypt workfactor to use when generating the hash, higher numbers are slower but more secure (defaults to `10`).
+Supplied as the `bcrypt` cost parameter; controls the computational cost of generating and validating a hash.
+Higher values are slower but, since they take longer to generate, more secure against brute force attacks.
+
+Defaults to `10`.
+At this level, a modern laptop (Late 2016 MacBook Pro, 3.3 GHz Intel Core i7) can produces around ~4 hashes/second.
+
+The `bcrypt` algorithim applies this value as a power of two.
+As such, passwords with a workfactor of `11` will take twice as long to store and validate as those with a workfactor of `10`.
+
+Values lower than `4` are ignored by the underlying implementation (a value `10` is substituted).
+
+`min` `Number`
+
+Defines the minimum allowed password length in characters.
+
+Defaults to `8` in accordance with the [NIST Digital Identity Guidelines](http://nvlpubs.nist.gov/nistpubs/SpecialPublications/NIST.SP.800-63b.pdf).
+
+`max` `Number`
+
+Defines the maximum allowed password length in characters.
+
+The `bcrypt` algorithm, used by this field, operates on a 72 byte value.
+Most implementation (including [the one we use](https://www.npmjs.com/package/bcrypt-nodejs)), silently truncate the string provided if it exceeds this limit.
+The `max` length option defaults to 72 characters in an attempt to align with this limit.
+
+> Note: If multi-byte (ie. non-ASCII) characters are allowed, it will be possible to exceed the 72 byte limit without triggering the 72 character validation limit.
+
+Can be set to `false` to disable the max length check.
+
+> Note: Disabling `max` or setting its value to >72 prevents validation errors but does not address the underlying algorithmic limitation.
+
+`rejectCommon` `Boolean`
+
+Controls whether values should be validated against a list of known-common passwords.
+
+Defaults to `true` in accordance with the [NIST Digital Identity Guidelines](http://nvlpubs.nist.gov/nistpubs/SpecialPublications/NIST.SP.800-63b.pdf).
+
+Implemented with the [`dumb-passwords` package](https://www.npmjs.com/package/dumb-passwords)
+which validates against 10,000 common passwords complied by [security analyst Mark Burnett](https://xato.net/10-000-top-passwords-6d6380716fe0).
 
 `complexity` `Object`
 
 Allows to set complexity requirements:
 
 * `digitChar` `Boolean` - when set to `true`, requires at least one digit
-* `spChar` `Boolean` - when set to `true`, requires at least one from the following special characters: !, @, #, $, %, ^, &, \*, (, ), +
+* `spChar` `Boolean` - when set to `true`, requires at least one from the following special characters: `!`, `@`, `#`, `$`, `%`, `^`, `&`, `*`, `(`, `)`, `+`
 * `asciiChar` `Boolean` - when set to `true`, allows only ASCII characters (from range U+0020--U+007E)
 * `lowChar` `Boolean` - when set to `true`, requires at least one lower case character
 * `upperChar` `Boolean` - when set to `true`, requires at least one upper case character
 
-### Example
+Example:
 
 ```js
 { type: Types.Password, complexity: { digitChar: true, asciiChar: true } }
 ```
 
-`max` `Number`
-
-Sets the maximum password length; defaults to 72, in accordance with [bcrypt](https://www.google.com/search?q=bcrypt+max+length), which truncates the password to the first 72 bytes.
-
-Can be set to `false` to disable the max length.
-
-> Note: Disabling `max` or setting its value to >72 does not override the bcrypt specification.
-
-`min` `Number`
-
-Defines the minimum password length; disabled by default.
 
 ## Underscore methods
 

lib/list/getCSVData.js

@@ -1,5 +1,6 @@
 var _ = require('lodash');
 var listToArray = require('list-to-array');
+var escapeValueForExcel = require('../security/escapeValueForExcel');
 
 /**
  * Applies option field transforms to get the CSV value for a field
@@ -110,6 +111,11 @@ function getCSVData (item, options) {
 		}
 	});
 
+	// Prevent CSV macro injection
+	_.forOwn(rtn, (value, prop) => {
+		rtn[prop] = escapeValueForExcel(value);
+	});
+
 	return rtn;
 }
 

lib/security/csrf.js

@@ -48,9 +48,13 @@ exports.requestToken = function (req) {
 		return req.headers[exports.XSRF_HEADER_KEY];
 	} else if (req.headers && req.headers[exports.CSRF_HEADER_KEY]) {
 		return req.headers[exports.CSRF_HEADER_KEY];
-	} else if (req.cookies && req.cookies[exports.XSRF_COOKIE_KEY]) {
-		return req.cookies[exports.XSRF_COOKIE_KEY];
 	}
+	// JM: If you think we should be checking the req.cookie here you don't understand CSRF.
+	// On pages loaded from this app (on the same origin) JS will have access to the cookie and should add the CSRF value as one of the headers above.
+	// Other pages, like those created by an attacker, can still create requests to this app (to which the browser will add cookie information) but,
+	// since the calling page itself can't access the cookie, it will be unable to add the CSRF header, body or query param to the request.
+	// The fact that we *don't* check the CSRF value that comes in with the cookie is what makes this CSRF implementation work.
+	// See.. https://en.wikipedia.org/wiki/Cross-site_request_forgery#Cookie-to-header_token
 	return '';
 };