Skip to content

Commit f44e306

Browse files
mtrezzamtrezza
authored
fix: Server crash via deeply nested query condition operators ([GHSA-9xp9-j92r-p88v](GHSA-9xp9-j92r-p88v)) (#10202)
1 parent bc9aa37 commit f44e306

File tree

8 files changed

+179
-8
lines changed

8 files changed

+179
-8
lines changed

‎spec/RequestComplexity.spec.js‎

Lines changed: 128 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,30 @@ describe('request complexity', () => {
3737
return where;
3838
}
3939

40+
function buildNestedOrQuery(depth) {
41+
let where = { username: 'test' };
42+
for (let i = 0; i < depth; i++) {
43+
where = { $or: [where, { username: 'test' }] };
44+
}
45+
return where;
46+
}
47+
48+
function buildNestedAndQuery(depth) {
49+
let where = { username: 'test' };
50+
for (let i = 0; i < depth; i++) {
51+
where = { $and: [where, { username: 'test' }] };
52+
}
53+
return where;
54+
}
55+
56+
function buildNestedNorQuery(depth) {
57+
let where = { username: 'test' };
58+
for (let i = 0; i < depth; i++) {
59+
where = { $nor: [where, { username: 'test' }] };
60+
}
61+
return where;
62+
}
63+
4064
describe('config validation', () => {
4165
it('should accept valid requestComplexity config', async () => {
4266
await expectAsync(
@@ -45,6 +69,7 @@ describe('request complexity', () => {
4569
includeDepth: 10,
4670
includeCount: 100,
4771
subqueryDepth: 5,
72+
queryDepth: 10,
4873
graphQLDepth: 15,
4974
graphQLFields: 300,
5075
},
@@ -59,6 +84,7 @@ describe('request complexity', () => {
5984
includeDepth: -1,
6085
includeCount: -1,
6186
subqueryDepth: -1,
87+
queryDepth: -1,
6288
graphQLDepth: -1,
6389
graphQLFields: -1,
6490
},
@@ -112,6 +138,7 @@ describe('request complexity', () => {
112138
expect(config.requestComplexity.includeDepth).toBe(3);
113139
expect(config.requestComplexity.includeCount).toBe(50);
114140
expect(config.requestComplexity.subqueryDepth).toBe(5);
141+
expect(config.requestComplexity.queryDepth).toBe(-1);
115142
expect(config.requestComplexity.graphQLDepth).toBe(50);
116143
expect(config.requestComplexity.graphQLFields).toBe(200);
117144
});
@@ -123,6 +150,7 @@ describe('request complexity', () => {
123150
includeDepth: 5,
124151
includeCount: 50,
125152
subqueryDepth: 5,
153+
queryDepth: -1,
126154
graphQLDepth: 50,
127155
graphQLFields: 200,
128156
});
@@ -216,6 +244,106 @@ describe('request complexity', () => {
216244
});
217245
});
218246

247+
describe('query depth', () => {
248+
let config;
249+
250+
beforeEach(async () => {
251+
await reconfigureServer({
252+
requestComplexity: { queryDepth: 3 },
253+
});
254+
config = Config.get('test');
255+
});
256+
257+
it('should allow $or within depth limit', async () => {
258+
const where = buildNestedOrQuery(3);
259+
await expectAsync(
260+
rest.find(config, auth.nobody(config), '_User', where)
261+
).toBeResolved();
262+
});
263+
264+
it('should reject $or exceeding depth limit', async () => {
265+
const where = buildNestedOrQuery(4);
266+
await expectAsync(
267+
rest.find(config, auth.nobody(config), '_User', where)
268+
).toBeRejectedWith(
269+
jasmine.objectContaining({
270+
message: jasmine.stringMatching(/Query condition nesting depth exceeds maximum allowed depth of 3/),
271+
})
272+
);
273+
});
274+
275+
it('should reject $and exceeding depth limit', async () => {
276+
const where = buildNestedAndQuery(4);
277+
await expectAsync(
278+
rest.find(config, auth.nobody(config), '_User', where)
279+
).toBeRejectedWith(
280+
jasmine.objectContaining({
281+
message: jasmine.stringMatching(/Query condition nesting depth exceeds maximum allowed depth of 3/),
282+
})
283+
);
284+
});
285+
286+
it('should reject $nor exceeding depth limit', async () => {
287+
const where = buildNestedNorQuery(4);
288+
await expectAsync(
289+
rest.find(config, auth.nobody(config), '_User', where)
290+
).toBeRejectedWith(
291+
jasmine.objectContaining({
292+
message: jasmine.stringMatching(/Query condition nesting depth exceeds maximum allowed depth of 3/),
293+
})
294+
);
295+
});
296+
297+
it('should reject mixed nested operators exceeding depth limit', async () => {
298+
// $or > $and > $nor > $or = depth 4
299+
const where = {
300+
$or: [
301+
{
302+
$and: [
303+
{
304+
$nor: [
305+
{ $or: [{ username: 'a' }, { username: 'b' }] },
306+
],
307+
},
308+
],
309+
},
310+
],
311+
};
312+
await expectAsync(
313+
rest.find(config, auth.nobody(config), '_User', where)
314+
).toBeRejectedWith(
315+
jasmine.objectContaining({
316+
message: jasmine.stringMatching(/Query condition nesting depth exceeds maximum allowed depth of 3/),
317+
})
318+
);
319+
});
320+
321+
it('should allow with master key even when exceeding limit', async () => {
322+
const where = buildNestedOrQuery(4);
323+
await expectAsync(
324+
rest.find(config, auth.master(config), '_User', where)
325+
).toBeResolved();
326+
});
327+
328+
it('should allow with maintenance key even when exceeding limit', async () => {
329+
const where = buildNestedOrQuery(4);
330+
await expectAsync(
331+
rest.find(config, auth.maintenance(config), '_User', where)
332+
).toBeResolved();
333+
});
334+
335+
it('should allow unlimited when queryDepth is -1', async () => {
336+
await reconfigureServer({
337+
requestComplexity: { queryDepth: -1 },
338+
});
339+
config = Config.get('test');
340+
const where = buildNestedOrQuery(15);
341+
await expectAsync(
342+
rest.find(config, auth.nobody(config), '_User', where)
343+
).toBeResolved();
344+
});
345+
});
346+
219347
describe('include limits', () => {
220348
let config;
221349

‎spec/SecurityCheckGroups.spec.js‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ describe('Security Check Groups', () => {
3737
config.mountPlayground = false;
3838
config.readOnlyMasterKey = 'someReadOnlyMasterKey';
3939
config.readOnlyMasterKeyIps = ['127.0.0.1', '::1'];
40+
config.requestComplexity = { queryDepth: 10 };
4041
await reconfigureServer(config);
4142

4243
const group = new CheckGroupServerConfig();
@@ -66,6 +67,7 @@ describe('Security Check Groups', () => {
6667
includeDepth: -1,
6768
includeCount: -1,
6869
subqueryDepth: -1,
70+
queryDepth: -1,
6971
graphQLDepth: -1,
7072
graphQLFields: -1,
7173
};

‎spec/vulnerabilities.spec.js‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2747,3 +2747,25 @@ describe('(GHSA-c442-97qw-j6c6) SQL Injection via $regex query operator field na
27472747
});
27482748
});
27492749
});
2750+
2751+
describe('(GHSA-9xp9-j92r-p88v) Stack overflow process crash via deeply nested query operators', () => {
2752+
it('rejects deeply nested $or query when queryDepth is set', async () => {
2753+
await reconfigureServer({
2754+
requestComplexity: { queryDepth: 10 },
2755+
});
2756+
const auth = require('../lib/Auth');
2757+
const rest = require('../lib/rest');
2758+
const config = Config.get('test');
2759+
let where = { username: 'test' };
2760+
for (let i = 0; i < 15; i++) {
2761+
where = { $or: [where, { username: 'test' }] };
2762+
}
2763+
await expectAsync(
2764+
rest.find(config, auth.nobody(config), '_User', where)
2765+
).toBeRejectedWith(
2766+
jasmine.objectContaining({
2767+
message: jasmine.stringMatching(/Query condition nesting depth exceeds maximum allowed depth/),
2768+
})
2769+
);
2770+
});
2771+
});

‎src/Controllers/DatabaseController.js‎

Lines changed: 16 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -113,34 +113,43 @@ const validateQuery = (
113113
query: any,
114114
isMaster: boolean,
115115
isMaintenance: boolean,
116-
update: boolean
116+
update: boolean,
117+
options: ?ParseServerOptions,
118+
_depth: number = 0
117119
): void => {
118120
if (isMaintenance) {
119121
isMaster = true;
120122
}
123+
const rc = options?.requestComplexity;
124+
if (!isMaster && rc && rc.queryDepth !== -1 && _depth > rc.queryDepth) {
125+
throw new Parse.Error(
126+
Parse.Error.INVALID_QUERY,
127+
`Query condition nesting depth exceeds maximum allowed depth of ${rc.queryDepth}`
128+
);
129+
}
121130
if (query.ACL) {
122131
throw new Parse.Error(Parse.Error.INVALID_QUERY, 'Cannot query on ACL.');
123132
}
124133

125134
if (query.$or) {
126135
if (query.$or instanceof Array) {
127-
query.$or.forEach(value => validateQuery(value, isMaster, isMaintenance, update));
136+
query.$or.forEach(value => validateQuery(value, isMaster, isMaintenance, update, options, _depth + 1));
128137
} else {
129138
throw new Parse.Error(Parse.Error.INVALID_QUERY, 'Bad $or format - use an array value.');
130139
}
131140
}
132141

133142
if (query.$and) {
134143
if (query.$and instanceof Array) {
135-
query.$and.forEach(value => validateQuery(value, isMaster, isMaintenance, update));
144+
query.$and.forEach(value => validateQuery(value, isMaster, isMaintenance, update, options, _depth + 1));
136145
} else {
137146
throw new Parse.Error(Parse.Error.INVALID_QUERY, 'Bad $and format - use an array value.');
138147
}
139148
}
140149

141150
if (query.$nor) {
142151
if (query.$nor instanceof Array && query.$nor.length > 0) {
143-
query.$nor.forEach(value => validateQuery(value, isMaster, isMaintenance, update));
152+
query.$nor.forEach(value => validateQuery(value, isMaster, isMaintenance, update, options, _depth + 1));
144153
} else {
145154
throw new Parse.Error(
146155
Parse.Error.INVALID_QUERY,
@@ -581,7 +590,7 @@ class DatabaseController {
581590
if (acl) {
582591
query = addWriteACL(query, acl);
583592
}
584-
validateQuery(query, isMaster, false, true);
593+
validateQuery(query, isMaster, false, true, this.options);
585594
return schemaController
586595
.getOneSchema(className, true)
587596
.catch(error => {
@@ -829,7 +838,7 @@ class DatabaseController {
829838
if (acl) {
830839
query = addWriteACL(query, acl);
831840
}
832-
validateQuery(query, isMaster, false, false);
841+
validateQuery(query, isMaster, false, false, this.options);
833842
return schemaController
834843
.getOneSchema(className)
835844
.catch(error => {
@@ -1340,7 +1349,7 @@ class DatabaseController {
13401349
query = addReadACL(query, aclGroup);
13411350
}
13421351
}
1343-
validateQuery(query, isMaster, isMaintenance, false);
1352+
validateQuery(query, isMaster, isMaintenance, false, this.options);
13441353
if (count) {
13451354
if (!classExists) {
13461355
return 0;

‎src/Options/Definitions.js‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -692,6 +692,12 @@ module.exports.RequestComplexityOptions = {
692692
action: parsers.numberParser('includeDepth'),
693693
default: 5,
694694
},
695+
queryDepth: {
696+
env: 'PARSE_SERVER_REQUEST_COMPLEXITY_QUERY_DEPTH',
697+
help: 'Maximum nesting depth of `$or`, `$and`, `$nor` query operators. Set to `-1` to disable. Default is `-1`.',
698+
action: parsers.numberParser('queryDepth'),
699+
default: -1,
700+
},
695701
subqueryDepth: {
696702
env: 'PARSE_SERVER_REQUEST_COMPLEXITY_SUBQUERY_DEPTH',
697703
help: 'Maximum nesting depth of `$inQuery`, `$notInQuery`, `$select`, `$dontSelect` subqueries. Set to `-1` to disable. Default is `5`.',

‎src/Options/docs.js‎

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎src/Options/index.js‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -434,6 +434,9 @@ export interface RequestComplexityOptions {
434434
/* Maximum nesting depth of `$inQuery`, `$notInQuery`, `$select`, `$dontSelect` subqueries. Set to `-1` to disable. Default is `5`.
435435
:DEFAULT: 5 */
436436
subqueryDepth: ?number;
437+
/* Maximum nesting depth of `$or`, `$and`, `$nor` query operators. Set to `-1` to disable. Default is `-1`.
438+
:DEFAULT: -1 */
439+
queryDepth: ?number;
437440
/* Maximum depth of GraphQL field selections. Set to `-1` to disable. Default is `50`.
438441
:ENV: PARSE_SERVER_REQUEST_COMPLEXITY_GRAPHQL_DEPTH
439442
:DEFAULT: 50 */

‎src/Security/CheckGroups/CheckGroupServerConfig.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -145,7 +145,7 @@ class CheckGroupServerConfig extends CheckGroup {
145145
if (!rc) {
146146
throw 1;
147147
}
148-
const values = [rc.includeDepth, rc.includeCount, rc.subqueryDepth, rc.graphQLDepth, rc.graphQLFields];
148+
const values = [rc.includeDepth, rc.includeCount, rc.subqueryDepth, rc.queryDepth, rc.graphQLDepth, rc.graphQLFields];
149149
if (values.some(v => v === -1)) {
150150
throw 1;
151151
}

0 commit comments

Comments
 (0)
⚡