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

Defending against query selector injections for v6.0 #10430

Merged
merged 6 commits into from
Jul 29, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,8 @@ declare module 'mongoose' {
*/
runValidators?: boolean;

sanitizeFilter?: boolean;

sanitizeProjection?: boolean;

/**
Expand Down Expand Up @@ -1061,7 +1063,13 @@ declare module 'mongoose' {
*/
returnDocument?: string;
runValidators?: boolean;
/* Set to `true` to automatically sanitize potentially unsafe user-generated query projections */
sanitizeProjection?: boolean;
/**
* Set to `true` to automatically sanitize potentially unsafe query filters by stripping out query selectors that
* aren't explicitly allowed using `mongoose.trusted()`.
*/
sanitizeFilter?: boolean;
/** The session associated with this query. */
session?: mongodb.ClientSession;
setDefaultsOnInsert?: boolean;
Expand All @@ -1078,11 +1086,10 @@ declare module 'mongoose' {
*/
timestamps?: boolean;
upsert?: boolean;
useFindAndModify?: boolean;
writeConcern?: any;
}

type MongooseQueryOptions = Pick<QueryOptions, 'populate' | 'lean' | 'omitUndefined' | 'strict' | 'useFindAndModify' | 'sanitizeProjection'>;
type MongooseQueryOptions = Pick<QueryOptions, 'populate' | 'lean' | 'omitUndefined' | 'strict' | 'useFindAndModify' | 'sanitizeProjection' | 'sanitizeFilter'>;

interface SaveOptions {
checkKeys?: boolean;
Expand Down
5 changes: 5 additions & 0 deletions lib/helpers/clone.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ const getFunctionName = require('./getFunctionName');
const isBsonType = require('./isBsonType');
const isObject = require('./isObject');
const symbols = require('./symbols');
const trustedSymbol = require('./query/trusted').trustedSymbol;
const utils = require('../utils');


Expand Down Expand Up @@ -111,6 +112,10 @@ function cloneObject(obj, options, isArrayChild) {
const ret = {};
let hasKeys;

if (obj[trustedSymbol]) {
ret[trustedSymbol] = obj[trustedSymbol];
}

for (const k of Object.keys(obj)) {
if (specialProperties.has(k)) {
continue;
Expand Down
13 changes: 0 additions & 13 deletions lib/helpers/query/querySelector.js

This file was deleted.

10 changes: 6 additions & 4 deletions lib/helpers/query/sanitizeFilter.js
Original file line number Diff line number Diff line change
@@ -1,23 +1,23 @@
'use strict';

const hasDollarKeys = require('./hasDollarKeys');
const { querySelectorSymbol } = require('./querySelector');
const { trustedSymbol } = require('./trusted');

module.exports = function sanitizeFilter(filter) {
if (filter == null || typeof filter !== 'object') {
return;
return filter;
}
if (Array.isArray(filter)) {
for (const subfilter of filter) {
sanitizeFilter(subfilter);
}
return;
return filter;
}

const filterKeys = Object.keys(filter);
for (const key of filterKeys) {
const value = filter[key];
if (value != null && value[querySelectorSymbol]) {
if (value != null && value[trustedSymbol]) {
continue;
}
if (key === '$and' || key === '$or') {
Expand All @@ -33,4 +33,6 @@ module.exports = function sanitizeFilter(filter) {
filter[key] = { $eq: filter[key] };
}
}

return filter;
};
13 changes: 13 additions & 0 deletions lib/helpers/query/trusted.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
'use strict';

const trustedSymbol = Symbol('mongoose#trustedSymbol');

exports.trustedSymbol = trustedSymbol;

exports.trusted = function trusted(obj) {
if (obj == null || typeof obj !== 'object') {
return obj;
}
obj[trustedSymbol] = true;
return obj;
};
40 changes: 40 additions & 0 deletions lib/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ const validateBeforeSave = require('./plugins/validateBeforeSave');
const Aggregate = require('./aggregate');
const PromiseProvider = require('./promise_provider');
const shardingPlugin = require('./plugins/sharding');
const trusted = require('./helpers/query/trusted').trusted;
const sanitizeFilter = require('./helpers/query/sanitizeFilter');

const defaultMongooseSymbol = Symbol.for('mongoose:default');

Expand Down Expand Up @@ -1107,6 +1109,44 @@ Mongoose.prototype.mongo = require('mongodb');

Mongoose.prototype.mquery = require('mquery');

/**
* Sanitizes query filters against [query selector injection attacks](https://thecodebarbarian.com/2014/09/04/defending-against-query-selector-injection-attacks.html)
* by wrapping any nested objects that have a property whose name starts with `$` in a `$eq`.
*
* ```javascript
* const obj = { username: 'val', pwd: { $ne: null } };
* sanitizeFilter(obj);
* obj; // { username: 'val', pwd: { $eq: { $ne: null } } });
* ```
*
* @method sanitizeFilter
* @param {Object} filter
* @returns Object the sanitized object
* @api public
*/

Mongoose.prototype.sanitizeFilter = sanitizeFilter;

/**
* Tells `sanitizeFilter()` to skip the given object when filtering out potential [query selector injection attacks](https://thecodebarbarian.com/2014/09/04/defending-against-query-selector-injection-attacks.html).
* Use this method when you have a known query selector that you want to use.
*
* ```javascript
* const obj = { username: 'val', pwd: trusted({ $type: 'string', $eq: 'my secret' }) };
* sanitizeFilter(obj);
*
* // Note that `sanitizeFilter()` did not add `$eq` around `$type`.
* obj; // { username: 'val', pwd: { $type: 'string', $eq: 'my secret' } });
* ```
*
* @method trusted
* @param {Object} obj
* @returns Object the passed in object
* @api public
*/

Mongoose.prototype.trusted = trusted;

/*!
* ignore
*/
Expand Down
18 changes: 18 additions & 0 deletions lib/query.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ const isInclusive = require('./helpers/projection/isInclusive');
const mquery = require('mquery');
const parseProjection = require('./helpers/projection/parseProjection');
const removeUnusedArrayFilters = require('./helpers/update/removeUnusedArrayFilters');
const sanitizeFilter = require('./helpers/query/sanitizeFilter');
const sanitizeProjection = require('./helpers/query/sanitizeProjection');
const selectPopulatedFields = require('./helpers/query/selectPopulatedFields');
const setDefaultsOnInsert = require('./helpers/setDefaultsOnInsert');
Expand Down Expand Up @@ -1537,6 +1538,10 @@ Query.prototype.setOptions = function(options, overwrite) {
this._mongooseOptions.sanitizeProjection = options.sanitizeProjection;
delete options.sanitizeProjection;
}
if ('sanitizeFilter' in options) {
this._mongooseOptions.sanitizeFilter = options.sanitizeFilter;
delete options.sanitizeFilter;
}

if ('defaults' in options) {
this._mongooseOptions.defaults = options.defaults;
Expand Down Expand Up @@ -2051,6 +2056,19 @@ Query.prototype.mongooseOptions = function(v) {
*/

Query.prototype._castConditions = function() {
let sanitizeFilterOpt = undefined;
if (this.model != null && utils.hasUserDefinedProperty(this.model.db.options, 'sanitizeFilter')) {
sanitizeFilterOpt = this.model.db.options.sanitizeFilter;
} else if (this.model != null && utils.hasUserDefinedProperty(this.model.base.options, 'sanitizeFilter')) {
sanitizeFilterOpt = this.model.base.options.sanitizeFilter;
} else {
sanitizeFilterOpt = this._mongooseOptions.sanitizeFilter;
}

if (sanitizeFilterOpt) {
sanitizeFilter(this._conditions);
}

try {
this.cast(this.model);
this._unsetCastError();
Expand Down
9 changes: 9 additions & 0 deletions lib/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ const isMongooseObject = require('./helpers/isMongooseObject');
const promiseOrCallback = require('./helpers/promiseOrCallback');
const schemaMerge = require('./helpers/schema/merge');
const specialProperties = require('./helpers/specialProperties');
const { trustedSymbol } = require('./helpers/query/trusted');

let Document;

Expand Down Expand Up @@ -249,6 +250,10 @@ exports.merge = function merge(to, from, options, path) {
const len = keys.length;
let key;

if (from[trustedSymbol]) {
to[trustedSymbol] = from[trustedSymbol];
}

path = path || '';
const omitNested = options.omitNested || {};

Expand Down Expand Up @@ -329,6 +334,10 @@ exports.toObject = function toObject(obj) {
if (exports.isPOJO(obj)) {
ret = {};

if (obj[trustedSymbol]) {
ret[trustedSymbol] = obj[trustedSymbol];
}

for (const k of Object.keys(obj)) {
if (specialProperties.has(k)) {
continue;
Expand Down
1 change: 1 addition & 0 deletions lib/validoptions.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ const VALID_OPTIONS = Object.freeze([
'overwriteModels',
'returnOriginal',
'runValidators',
'sanitizeFilter',
'sanitizeProjection',
'selectPopulatedPaths',
'setDefaultsOnInsert',
Expand Down
4 changes: 2 additions & 2 deletions test/helpers/query.sanitizeFilter.test.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
'use strict';

const assert = require('assert');
const { querySelector } = require('../../lib/helpers/query/querySelector');
const { trusted } = require('../../lib/helpers/query/trusted');
const sanitizeFilter = require('../../lib/helpers/query/sanitizeFilter');

describe('sanitizeFilter', function() {
Expand All @@ -20,7 +20,7 @@ describe('sanitizeFilter', function() {
sanitizeFilter(obj);
assert.deepEqual(obj, { username: 'val', pwd: 'my secret' });

obj = { username: 'val', pwd: querySelector({ $type: 'string', $eq: 'my secret' }) };
obj = { username: 'val', pwd: trusted({ $type: 'string', $eq: 'my secret' }) };
sanitizeFilter(obj);
assert.deepEqual(obj, { username: 'val', pwd: { $type: 'string', $eq: 'my secret' } });
});
Expand Down
21 changes: 21 additions & 0 deletions test/query.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -3884,4 +3884,25 @@ describe('Query', function() {
q = Test.find().select({ email: '$name' }).setOptions({ sanitizeProjection: true });
assert.deepEqual(q._fields, { email: 1 });
});

it('sanitizeFilter option (gh-3944)', function() {
const MySchema = Schema({ username: String, pwd: String });
const Test = db.model('Test', MySchema);

let q = Test.find({ username: 'val', pwd: 'my secret' }).setOptions({ sanitizeFilter: true });
q._castConditions();
assert.ifError(q.error());
assert.deepEqual(q._conditions, { username: 'val', pwd: 'my secret' });

q = Test.find({ username: 'val', pwd: { $ne: null } }).setOptions({ sanitizeFilter: true });
q._castConditions();
assert.ok(q.error());
assert.equal(q.error().name, 'CastError');

q = Test.find({ username: 'val', pwd: mongoose.trusted({ $gt: null }) }).
setOptions({ sanitizeFilter: true });
q._castConditions();
assert.ifError(q.error());
assert.deepEqual(q._conditions, { username: 'val', pwd: { $gt: null } });
});
});