From 886bfd7cac69496e3f73d4bb536f0eec3cba0e4d Mon Sep 17 00:00:00 2001 From: Manuel <5673677+mtrezza@users.noreply.github.com> Date: Sat, 12 Mar 2022 00:19:31 +0100 Subject: [PATCH] fix: security vulnerability that allows remote code execution (ghsa p6h4 93qp jhcm) (#7841) --- resources/buildConfigDefinitions.js | 14 ++ spec/vulnerabilities.spec.js | 283 ++++++++++++++++++++++++++ src/Config.js | 16 +- src/Controllers/DatabaseController.js | 98 +++++---- src/Controllers/index.js | 3 +- src/Options/Definitions.js | 18 ++ src/Options/docs.js | 1 + src/Options/index.js | 7 + src/RestWrite.js | 14 ++ src/Utils.js | 38 ++++ 10 files changed, 452 insertions(+), 40 deletions(-) create mode 100644 spec/vulnerabilities.spec.js create mode 100644 src/Utils.js diff --git a/resources/buildConfigDefinitions.js b/resources/buildConfigDefinitions.js index 99b57b1379..b95f9d027e 100644 --- a/resources/buildConfigDefinitions.js +++ b/resources/buildConfigDefinitions.js @@ -150,6 +150,20 @@ function parseDefaultValue(elt, value, t) { literalValue = t.arrayExpression(array.map((value) => { if (typeof value == 'string') { return t.stringLiteral(value); + } else if (typeof value == 'number') { + return t.numericLiteral(value); + } else if (typeof value == 'object') { + const object = parsers.objectParser(value); + const props = Object.entries(object).map(([k, v]) => { + if (typeof v == 'string') { + return t.objectProperty(t.identifier(k), t.stringLiteral(v)); + } else if (typeof v == 'number') { + return t.objectProperty(t.identifier(k), t.numericLiteral(v)); + } else if (typeof v == 'boolean') { + return t.objectProperty(t.identifier(k), t.booleanLiteral(v)); + } + }); + return t.objectExpression(props); } else { throw new Error('Unable to parse array'); } diff --git a/spec/vulnerabilities.spec.js b/spec/vulnerabilities.spec.js new file mode 100644 index 0000000000..1255d64398 --- /dev/null +++ b/spec/vulnerabilities.spec.js @@ -0,0 +1,283 @@ +const request = require('../lib/request'); + +describe('Vulnerabilities', () => { + describe('Object prototype pollution', () => { + it('denies object prototype to be polluted with keyword "constructor"', async () => { + const headers = { + 'Content-Type': 'application/json', + 'X-Parse-Application-Id': 'test', + 'X-Parse-REST-API-Key': 'rest', + }; + const response = await request({ + headers: headers, + method: 'POST', + url: 'http://localhost:8378/1/classes/PP', + body: JSON.stringify({ + obj: { + constructor: { + prototype: { + dummy: 0, + }, + }, + }, + }), + }).catch(e => e); + expect(response.status).toBe(400); + const text = JSON.parse(response.text); + expect(text.code).toBe(Parse.Error.INVALID_KEY_NAME); + expect(text.error).toBe('Prohibited keyword in request data: {"key":"constructor"}.'); + expect(Object.prototype.dummy).toBeUndefined(); + }); + + it('denies object prototype to be polluted with keypath string "constructor"', async () => { + const headers = { + 'Content-Type': 'application/json', + 'X-Parse-Application-Id': 'test', + 'X-Parse-REST-API-Key': 'rest', + }; + const objResponse = await request({ + headers: headers, + method: 'POST', + url: 'http://localhost:8378/1/classes/PP', + body: JSON.stringify({ + obj: {}, + }), + }).catch(e => e); + const pollResponse = await request({ + headers: headers, + method: 'PUT', + url: `http://localhost:8378/1/classes/PP/${objResponse.data.objectId}`, + body: JSON.stringify({ + 'obj.constructor.prototype.dummy': { + __op: 'Increment', + amount: 1, + }, + }), + }).catch(e => e); + expect(Object.prototype.dummy).toBeUndefined(); + expect(pollResponse.status).toBe(400); + const text = JSON.parse(pollResponse.text); + expect(text.code).toBe(Parse.Error.INVALID_KEY_NAME); + expect(text.error).toBe('Prohibited keyword in request data: {"key":"constructor"}.'); + expect(Object.prototype.dummy).toBeUndefined(); + }); + + it('denies object prototype to be polluted with keyword "__proto__"', async () => { + const headers = { + 'Content-Type': 'application/json', + 'X-Parse-Application-Id': 'test', + 'X-Parse-REST-API-Key': 'rest', + }; + const response = await request({ + headers: headers, + method: 'POST', + url: 'http://localhost:8378/1/classes/PP', + body: JSON.stringify({ 'obj.__proto__.dummy': 0 }), + }).catch(e => e); + expect(response.status).toBe(400); + const text = JSON.parse(response.text); + expect(text.code).toBe(Parse.Error.INVALID_KEY_NAME); + expect(text.error).toBe('Prohibited keyword in request data: {"key":"__proto__"}.'); + expect(Object.prototype.dummy).toBeUndefined(); + }); + }); + + describe('Request denylist', () => { + it('denies BSON type code data in write request by default', async () => { + const headers = { + 'Content-Type': 'application/json', + 'X-Parse-Application-Id': 'test', + 'X-Parse-REST-API-Key': 'rest', + }; + const params = { + headers: headers, + method: 'POST', + url: 'http://localhost:8378/1/classes/RCE', + body: JSON.stringify({ + obj: { + _bsontype: 'Code', + code: 'delete Object.prototype.evalFunctions', + }, + }), + }; + const response = await request(params).catch(e => e); + expect(response.status).toBe(400); + const text = JSON.parse(response.text); + expect(text.code).toBe(Parse.Error.INVALID_KEY_NAME); + expect(text.error).toBe( + 'Prohibited keyword in request data: {"key":"_bsontype","value":"Code"}.' + ); + }); + + it('allows BSON type code data in write request with custom denylist', async () => { + await reconfigureServer({ + requestKeywordDenylist: [], + }); + const headers = { + 'Content-Type': 'application/json', + 'X-Parse-Application-Id': 'test', + 'X-Parse-REST-API-Key': 'rest', + }; + const params = { + headers: headers, + method: 'POST', + url: 'http://localhost:8378/1/classes/RCE', + body: JSON.stringify({ + obj: { + _bsontype: 'Code', + code: 'delete Object.prototype.evalFunctions', + }, + }), + }; + const response = await request(params).catch(e => e); + expect(response.status).toBe(201); + const text = JSON.parse(response.text); + expect(text.objectId).toBeDefined(); + }); + + it('denies write request with custom denylist of key/value', async () => { + await reconfigureServer({ + requestKeywordDenylist: [{ key: 'a[K]ey', value: 'aValue[123]*' }], + }); + const headers = { + 'Content-Type': 'application/json', + 'X-Parse-Application-Id': 'test', + 'X-Parse-REST-API-Key': 'rest', + }; + const params = { + headers: headers, + method: 'POST', + url: 'http://localhost:8378/1/classes/RCE', + body: JSON.stringify({ + obj: { + aKey: 'aValue321', + code: 'delete Object.prototype.evalFunctions', + }, + }), + }; + const response = await request(params).catch(e => e); + expect(response.status).toBe(400); + const text = JSON.parse(response.text); + expect(text.code).toBe(Parse.Error.INVALID_KEY_NAME); + expect(text.error).toBe( + 'Prohibited keyword in request data: {"key":"a[K]ey","value":"aValue[123]*"}.' + ); + }); + + it('denies write request with custom denylist of nested key/value', async () => { + await reconfigureServer({ + requestKeywordDenylist: [{ key: 'a[K]ey', value: 'aValue[123]*' }], + }); + const headers = { + 'Content-Type': 'application/json', + 'X-Parse-Application-Id': 'test', + 'X-Parse-REST-API-Key': 'rest', + }; + const params = { + headers: headers, + method: 'POST', + url: 'http://localhost:8378/1/classes/RCE', + body: JSON.stringify({ + obj: { + nested: { + aKey: 'aValue321', + code: 'delete Object.prototype.evalFunctions', + }, + }, + }), + }; + const response = await request(params).catch(e => e); + expect(response.status).toBe(400); + const text = JSON.parse(response.text); + expect(text.code).toBe(Parse.Error.INVALID_KEY_NAME); + expect(text.error).toBe( + 'Prohibited keyword in request data: {"key":"a[K]ey","value":"aValue[123]*"}.' + ); + }); + + it('denies write request with custom denylist of key/value in array', async () => { + await reconfigureServer({ + requestKeywordDenylist: [{ key: 'a[K]ey', value: 'aValue[123]*' }], + }); + const headers = { + 'Content-Type': 'application/json', + 'X-Parse-Application-Id': 'test', + 'X-Parse-REST-API-Key': 'rest', + }; + const params = { + headers: headers, + method: 'POST', + url: 'http://localhost:8378/1/classes/RCE', + body: JSON.stringify({ + obj: [ + { + aKey: 'aValue321', + code: 'delete Object.prototype.evalFunctions', + }, + ], + }), + }; + const response = await request(params).catch(e => e); + expect(response.status).toBe(400); + const text = JSON.parse(response.text); + expect(text.code).toBe(Parse.Error.INVALID_KEY_NAME); + expect(text.error).toBe( + 'Prohibited keyword in request data: {"key":"a[K]ey","value":"aValue[123]*"}.' + ); + }); + + it('denies write request with custom denylist of key', async () => { + await reconfigureServer({ + requestKeywordDenylist: [{ key: 'a[K]ey' }], + }); + const headers = { + 'Content-Type': 'application/json', + 'X-Parse-Application-Id': 'test', + 'X-Parse-REST-API-Key': 'rest', + }; + const params = { + headers: headers, + method: 'POST', + url: 'http://localhost:8378/1/classes/RCE', + body: JSON.stringify({ + obj: { + aKey: 'aValue321', + code: 'delete Object.prototype.evalFunctions', + }, + }), + }; + const response = await request(params).catch(e => e); + expect(response.status).toBe(400); + const text = JSON.parse(response.text); + expect(text.code).toBe(Parse.Error.INVALID_KEY_NAME); + expect(text.error).toBe('Prohibited keyword in request data: {"key":"a[K]ey"}.'); + }); + + it('denies write request with custom denylist of value', async () => { + await reconfigureServer({ + requestKeywordDenylist: [{ value: 'aValue[123]*' }], + }); + const headers = { + 'Content-Type': 'application/json', + 'X-Parse-Application-Id': 'test', + 'X-Parse-REST-API-Key': 'rest', + }; + const params = { + headers: headers, + method: 'POST', + url: 'http://localhost:8378/1/classes/RCE', + body: JSON.stringify({ + obj: { + aKey: 'aValue321', + code: 'delete Object.prototype.evalFunctions', + }, + }), + }; + const response = await request(params).catch(e => e); + expect(response.status).toBe(400); + const text = JSON.parse(response.text); + expect(text.code).toBe(Parse.Error.INVALID_KEY_NAME); + expect(text.error).toBe('Prohibited keyword in request data: {"value":"aValue[123]*"}.'); + }); + }); +}); diff --git a/src/Config.js b/src/Config.js index 5c64df180a..dd6a44c1da 100644 --- a/src/Config.js +++ b/src/Config.js @@ -33,7 +33,11 @@ export class Config { cacheInfo.schemaCacheTTL, cacheInfo.enableSingleSchemaCache ); - config.database = new DatabaseController(cacheInfo.databaseController.adapter, schemaCache); + config.database = new DatabaseController( + cacheInfo.databaseController.adapter, + schemaCache, + config + ); } else { config[key] = cacheInfo[key]; } @@ -71,6 +75,7 @@ export class Config { allowHeaders, idempotencyOptions, emailVerifyTokenReuseIfValid, + requestKeywordDenylist, }) { if (masterKey === readOnlyMasterKey) { throw new Error('masterKey and readOnlyMasterKey should be different'); @@ -105,6 +110,15 @@ export class Config { this.validateMaxLimit(maxLimit); this.validateAllowHeaders(allowHeaders); this.validateIdempotencyOptions(idempotencyOptions); + this.validateRequestKeywordDenylist(requestKeywordDenylist); + } + + static validateRequestKeywordDenylist(requestKeywordDenylist) { + if (requestKeywordDenylist === undefined) { + requestKeywordDenylist = requestKeywordDenylist.default; + } else if (!Array.isArray(requestKeywordDenylist)) { + throw 'Parse Server option requestKeywordDenylist must be an array.'; + } } static validateIdempotencyOptions(idempotencyOptions) { diff --git a/src/Controllers/DatabaseController.js b/src/Controllers/DatabaseController.js index 21ba2e9477..5c8a18f6e1 100644 --- a/src/Controllers/DatabaseController.js +++ b/src/Controllers/DatabaseController.js @@ -13,6 +13,7 @@ import deepcopy from 'deepcopy'; import logger from '../logger'; import * as SchemaController from './SchemaController'; import { StorageAdapter } from '../Adapters/Storage/StorageAdapter'; +import type { ParseServerOptions } from '../Options'; import type { QueryOptions, FullQueryOptions } from '../Adapters/Storage/StorageAdapter'; function addWriteACL(query, acl) { @@ -257,41 +258,6 @@ const isSpecialUpdateKey = key => { return specialKeysForUpdate.indexOf(key) >= 0; }; -function expandResultOnKeyPath(object, key, value) { - if (key.indexOf('.') < 0) { - object[key] = value[key]; - return object; - } - const path = key.split('.'); - const firstKey = path[0]; - const nextPath = path.slice(1).join('.'); - object[firstKey] = expandResultOnKeyPath(object[firstKey] || {}, nextPath, value[firstKey]); - delete object[key]; - return object; -} - -function sanitizeDatabaseResult(originalObject, result): Promise { - const response = {}; - if (!result) { - return Promise.resolve(response); - } - Object.keys(originalObject).forEach(key => { - const keyUpdate = originalObject[key]; - // determine if that was an op - if ( - keyUpdate && - typeof keyUpdate === 'object' && - keyUpdate.__op && - ['Add', 'AddUnique', 'Remove', 'Increment'].indexOf(keyUpdate.__op) > -1 - ) { - // only valid ops that produce an actionable result - // the op may have happend on a keypath - expandResultOnKeyPath(response, key, result); - } - }); - return Promise.resolve(response); -} - function joinTableName(className, key) { return `_Join:${key}:${className}`; } @@ -397,8 +363,9 @@ class DatabaseController { schemaCache: any; schemaPromise: ?Promise; _transactionalSession: ?any; + options: ParseServerOptions; - constructor(adapter: StorageAdapter, schemaCache: any) { + constructor(adapter: StorageAdapter, schemaCache: any, options: ParseServerOptions) { this.adapter = adapter; this.schemaCache = schemaCache; // We don't want a mutable this.schema, because then you could have @@ -406,6 +373,7 @@ class DatabaseController { // it. Instead, use loadSchema to get a schema. this.schemaPromise = null; this._transactionalSession = null; + this.options = options; } collectionExists(className: string): Promise { @@ -644,7 +612,7 @@ class DatabaseController { if (skipSanitization) { return Promise.resolve(result); } - return sanitizeDatabaseResult(originalUpdate, result); + return this._sanitizeDatabaseResult(originalUpdate, result); }); }); } @@ -871,7 +839,7 @@ class DatabaseController { object, relationUpdates ).then(() => { - return sanitizeDatabaseResult(originalObject, result.ops[0]); + return this._sanitizeDatabaseResult(originalObject, result.ops[0]); }); }); }); @@ -1717,6 +1685,60 @@ class DatabaseController { ]); } + _expandResultOnKeyPath(object: any, key: string, value: any): any { + if (key.indexOf('.') < 0) { + object[key] = value[key]; + return object; + } + const path = key.split('.'); + const firstKey = path[0]; + const nextPath = path.slice(1).join('.'); + + // Scan request data for denied keywords + if (this.options && this.options.requestKeywordDenylist) { + // Scan request data for denied keywords + for (const keyword of this.options.requestKeywordDenylist) { + const isMatch = (a, b) => (typeof a === 'string' && new RegExp(a).test(b)) || a === b; + if (isMatch(firstKey, keyword.key)) { + throw new Parse.Error( + Parse.Error.INVALID_KEY_NAME, + `Prohibited keyword in request data: ${JSON.stringify(keyword)}.` + ); + } + } + } + + object[firstKey] = this._expandResultOnKeyPath( + object[firstKey] || {}, + nextPath, + value[firstKey] + ); + delete object[key]; + return object; + } + + _sanitizeDatabaseResult(originalObject: any, result: any): Promise { + const response = {}; + if (!result) { + return Promise.resolve(response); + } + Object.keys(originalObject).forEach(key => { + const keyUpdate = originalObject[key]; + // determine if that was an op + if ( + keyUpdate && + typeof keyUpdate === 'object' && + keyUpdate.__op && + ['Add', 'AddUnique', 'Remove', 'Increment'].indexOf(keyUpdate.__op) > -1 + ) { + // only valid ops that produce an actionable result + // the op may have happened on a keypath + this._expandResultOnKeyPath(response, key, result); + } + }); + return Promise.resolve(response); + } + static _validateQuery: any => void; } diff --git a/src/Controllers/index.js b/src/Controllers/index.js index 1e4765b666..1ca53bce1f 100644 --- a/src/Controllers/index.js +++ b/src/Controllers/index.js @@ -167,7 +167,8 @@ export function getDatabaseController( } return new DatabaseController( databaseAdapter, - new SchemaCache(cacheController, schemaCacheTTL, enableSingleSchemaCache) + new SchemaCache(cacheController, schemaCacheTTL, enableSingleSchemaCache), + options ); } diff --git a/src/Options/Definitions.js b/src/Options/Definitions.js index bce3756360..5965f4d9b0 100644 --- a/src/Options/Definitions.js +++ b/src/Options/Definitions.js @@ -336,6 +336,24 @@ module.exports.ParseServerOptions = { env: 'PARSE_SERVER_READ_ONLY_MASTER_KEY', help: 'Read-only key, which has the same capabilities as MasterKey without writes', }, + requestKeywordDenylist: { + env: 'PARSE_SERVER_REQUEST_KEYWORD_DENYLIST', + help: + 'An array of keys and values that are prohibited in database read and write requests to prevent potential security vulnerabilities. It is possible to specify only a key (`{"key":"..."}`), only a value (`{"value":"..."}`) or a key-value pair (`{"key":"...","value":"..."}`). The specification can use the following types: `boolean`, `numeric` or `string`, where `string` will be interpreted as a regex notation. Request data is deep-scanned for matching definitions to detect also any nested occurrences. Defaults are patterns that are likely to be used in malicious requests. Setting this option will override the default patterns.', + action: parsers.arrayParser, + default: [ + { + key: '_bsontype', + value: 'Code', + }, + { + key: 'constructor', + }, + { + key: '__proto__', + }, + ], + }, restAPIKey: { env: 'PARSE_SERVER_REST_API_KEY', help: 'Key for REST calls', diff --git a/src/Options/docs.js b/src/Options/docs.js index 576ff60a14..d1b9b8728d 100644 --- a/src/Options/docs.js +++ b/src/Options/docs.js @@ -62,6 +62,7 @@ * @property {String} publicServerURL Public URL to your parse server with http:// or https://. * @property {Any} push Configuration for push, as stringified JSON. See http://docs.parseplatform.org/parse-server/guide/#push-notifications * @property {String} readOnlyMasterKey Read-only key, which has the same capabilities as MasterKey without writes + * @property {RequestKeywordDenylist[]} requestKeywordDenylist An array of keys and values that are prohibited in database read and write requests to prevent potential security vulnerabilities. It is possible to specify only a key (`{"key":"..."}`), only a value (`{"value":"..."}`) or a key-value pair (`{"key":"...","value":"..."}`). The specification can use the following types: `boolean`, `numeric` or `string`, where `string` will be interpreted as a regex notation. Request data is deep-scanned for matching definitions to detect also any nested occurrences. Defaults are patterns that are likely to be used in malicious requests. Setting this option will override the default patterns. * @property {String} restAPIKey Key for REST calls * @property {Boolean} revokeSessionOnPasswordReset When a user changes their password, either through the reset password email or while logged in, all sessions are revoked if this is true. Set to false if you don't want to revoke sessions. * @property {Boolean} scheduledPush Configuration for push scheduling, defaults to false. diff --git a/src/Options/index.js b/src/Options/index.js index d2237e08a8..418d342d15 100644 --- a/src/Options/index.js +++ b/src/Options/index.js @@ -12,6 +12,10 @@ type Adapter = string | any | T; type NumberOrBoolean = number | boolean; type NumberOrString = number | string; type ProtectedFields = any; +type RequestKeywordDenylist = { + key: string | any, + value: any, +}; export interface ParseServerOptions { /* Your Parse Application ID @@ -220,6 +224,9 @@ export interface ParseServerOptions { serverStartComplete: ?(error: ?Error) => void; /* Callback when server has closed */ serverCloseComplete: ?() => void; + /* An array of keys and values that are prohibited in database read and write requests to prevent potential security vulnerabilities. It is possible to specify only a key (`{"key":"..."}`), only a value (`{"value":"..."}`) or a key-value pair (`{"key":"...","value":"..."}`). The specification can use the following types: `boolean`, `numeric` or `string`, where `string` will be interpreted as a regex notation. Request data is deep-scanned for matching definitions to detect also any nested occurrences. Defaults are patterns that are likely to be used in malicious requests. Setting this option will override the default patterns. + :DEFAULT: [{"key":"_bsontype","value":"Code"},{"key":"constructor"},{"key":"__proto__"}] */ + requestKeywordDenylist: ?(RequestKeywordDenylist[]); } export interface CustomPagesOptions { diff --git a/src/RestWrite.js b/src/RestWrite.js index af72fd5294..79c8dc6ba1 100644 --- a/src/RestWrite.js +++ b/src/RestWrite.js @@ -6,6 +6,7 @@ var SchemaController = require('./Controllers/SchemaController'); var deepcopy = require('deepcopy'); const Auth = require('./Auth'); +const Utils = require('./Utils'); var cryptoUtils = require('./cryptoUtils'); var passwordCrypto = require('./password'); var Parse = require('parse/node'); @@ -61,6 +62,19 @@ function RestWrite(config, auth, className, query, data, originalData, clientSDK } } + if (this.config.requestKeywordDenylist) { + // Scan request data for denied keywords + for (const keyword of this.config.requestKeywordDenylist) { + const match = Utils.objectContainsKeyValue(data, keyword.key, keyword.value); + if (match) { + throw new Parse.Error( + Parse.Error.INVALID_KEY_NAME, + `Prohibited keyword in request data: ${JSON.stringify(keyword)}.` + ); + } + } + } + // When the operation is complete, this.response may have several // fields. // response: the actual data to be returned diff --git a/src/Utils.js b/src/Utils.js new file mode 100644 index 0000000000..a447996ec2 --- /dev/null +++ b/src/Utils.js @@ -0,0 +1,38 @@ +/** + * utils.js + * @file General purpose utilities + * @description General purpose utilities. + */ + +/** + * The general purpose utilities. + */ +class Utils { + /** + * Deep-scans an object for a matching key/value definition. + * @param {Object} obj The object to scan. + * @param {String | undefined} key The key to match, or undefined if only the value should be matched. + * @param {any | undefined} value The value to match, or undefined if only the key should be matched. + * @returns {Boolean} True if a match was found, false otherwise. + */ + static objectContainsKeyValue(obj, key, value) { + const isMatch = (a, b) => (typeof a === 'string' && new RegExp(a).test(b)) || a === b; + const isKeyMatch = k => isMatch(key, k); + const isValueMatch = v => isMatch(value, v); + for (const [k, v] of Object.entries(obj)) { + if (key !== undefined && value === undefined && isKeyMatch(k)) { + return true; + } else if (key === undefined && value !== undefined && isValueMatch(v)) { + return true; + } else if (key !== undefined && value !== undefined && isKeyMatch(k) && isValueMatch(v)) { + return true; + } + if (['[object Object]', '[object Array]'].includes(Object.prototype.toString.call(v))) { + return Utils.objectContainsKeyValue(v, key, value); + } + } + return false; + } +} + +module.exports = Utils;