Skip to content

Commit 634c44a

Browse files
mtrezzamtrezza
authored
fix: brute force guessing of user sensitive data via search patterns; this fixes a security vulnerability in which internal and protected fields may be used as query constraints to guess the value of these fields and obtain sensitive data (GHSA-2m6g-crv8-p3c6) (#8143)
1 parent 4748e9b commit 634c44a

File tree

5 files changed

+139
-40
lines changed

5 files changed

+139
-40
lines changed

‎.github/workflows/ci.yml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ env:
1313
jobs:
1414
check-mongo:
1515
strategy:
16+
fail-fast: false
1617
matrix:
1718
include:
1819
- name: Mongo 4.0.4, ReplicaSet, WiredTiger

‎spec/RedisCacheAdapter.spec.js‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -378,7 +378,7 @@ describe_only(() => {
378378

379379
const query = new Parse.Query(TestObject);
380380
await query.get(object.id);
381-
expect(getSpy.calls.count()).toBe(3);
381+
expect(getSpy.calls.count()).toBe(4);
382382
expect(putSpy.calls.count()).toBe(1);
383383
expect(delSpy.calls.count()).toBe(2);
384384

@@ -397,7 +397,7 @@ describe_only(() => {
397397

398398
const query = new Parse.Query(TestObject);
399399
await query.get(object.id);
400-
expect(getSpy.calls.count()).toBe(2);
400+
expect(getSpy.calls.count()).toBe(3);
401401
expect(putSpy.calls.count()).toBe(1);
402402
expect(delSpy.calls.count()).toBe(1);
403403

@@ -420,7 +420,7 @@ describe_only(() => {
420420
query.include('child');
421421
await query.get(object.id);
422422

423-
expect(getSpy.calls.count()).toBe(4);
423+
expect(getSpy.calls.count()).toBe(6);
424424
expect(putSpy.calls.count()).toBe(1);
425425
expect(delSpy.calls.count()).toBe(3);
426426

@@ -444,7 +444,7 @@ describe_only(() => {
444444
expect(objects.length).toBe(1);
445445
expect(objects[0].id).toBe(child.id);
446446

447-
expect(getSpy.calls.count()).toBe(2);
447+
expect(getSpy.calls.count()).toBe(3);
448448
expect(putSpy.calls.count()).toBe(1);
449449
expect(delSpy.calls.count()).toBe(3);
450450

‎spec/RestQuery.spec.js‎

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -191,6 +191,79 @@ describe('rest query', () => {
191191
expect(result.results.length).toEqual(0);
192192
});
193193

194+
it('query internal field', async () => {
195+
const internalFields = [
196+
'_email_verify_token',
197+
'_perishable_token',
198+
'_tombstone',
199+
'_email_verify_token_expires_at',
200+
'_failed_login_count',
201+
'_account_lockout_expires_at',
202+
'_password_changed_at',
203+
'_password_history',
204+
];
205+
await Promise.all([
206+
...internalFields.map(field =>
207+
expectAsync(new Parse.Query(Parse.User).exists(field).find()).toBeRejectedWith(
208+
new Parse.Error(Parse.Error.INVALID_KEY_NAME, `Invalid key name: ${field}`)
209+
)
210+
),
211+
...internalFields.map(field =>
212+
new Parse.Query(Parse.User).exists(field).find({ useMasterKey: true })
213+
),
214+
]);
215+
});
216+
217+
it('query protected field', async () => {
218+
const user = new Parse.User();
219+
user.setUsername('username1');
220+
user.setPassword('password');
221+
await user.signUp();
222+
const config = Config.get(Parse.applicationId);
223+
const obj = new Parse.Object('Test');
224+
225+
obj.set('owner', user);
226+
obj.set('test', 'test');
227+
obj.set('zip', 1234);
228+
await obj.save();
229+
230+
const schema = await config.database.loadSchema();
231+
await schema.updateClass(
232+
'Test',
233+
{},
234+
{
235+
get: { '*': true },
236+
find: { '*': true },
237+
protectedFields: { [user.id]: ['zip'] },
238+
}
239+
);
240+
await Promise.all([
241+
new Parse.Query('Test').exists('test').find(),
242+
expectAsync(new Parse.Query('Test').exists('zip').find()).toBeRejectedWith(
243+
new Parse.Error(
244+
Parse.Error.OPERATION_FORBIDDEN,
245+
'This user is not allowed to query zip on class Test'
246+
)
247+
),
248+
]);
249+
});
250+
251+
it('query protected field with matchesQuery', async () => {
252+
const user = new Parse.User();
253+
user.setUsername('username1');
254+
user.setPassword('password');
255+
await user.signUp();
256+
const test = new Parse.Object('TestObject', { user });
257+
await test.save();
258+
const subQuery = new Parse.Query(Parse.User);
259+
subQuery.exists('_perishable_token');
260+
await expectAsync(
261+
new Parse.Query('TestObject').matchesQuery('user', subQuery).find()
262+
).toBeRejectedWith(
263+
new Parse.Error(Parse.Error.INVALID_KEY_NAME, 'Invalid key name: _perishable_token')
264+
);
265+
});
266+
194267
it('query with wrongly encoded parameter', done => {
195268
rest
196269
.create(config, nobody, 'TestParameterEncode', { foo: 'bar' })

‎src/Controllers/DatabaseController.js‎

Lines changed: 34 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -51,47 +51,43 @@ const transformObjectACL = ({ ACL, ...result }) => {
5151
return result;
5252
};
5353

54-
const specialQuerykeys = [
55-
'$and',
56-
'$or',
57-
'$nor',
58-
'_rperm',
59-
'_wperm',
60-
'_perishable_token',
54+
const specialQueryKeys = ['$and', '$or', '$nor', '_rperm', '_wperm'];
55+
const specialMasterQueryKeys = [
56+
...specialQueryKeys,
6157
'_email_verify_token',
58+
'_perishable_token',
59+
'_tombstone',
6260
'_email_verify_token_expires_at',
63-
'_account_lockout_expires_at',
6461
'_failed_login_count',
62+
'_account_lockout_expires_at',
63+
'_password_changed_at',
64+
'_password_history',
6565
];
6666

67-
const isSpecialQueryKey = key => {
68-
return specialQuerykeys.indexOf(key) >= 0;
69-
};
70-
71-
const validateQuery = (query: any): void => {
67+
const validateQuery = (query: any, isMaster: boolean, update: boolean): void => {
7268
if (query.ACL) {
7369
throw new Parse.Error(Parse.Error.INVALID_QUERY, 'Cannot query on ACL.');
7470
}
7571

7672
if (query.$or) {
7773
if (query.$or instanceof Array) {
78-
query.$or.forEach(validateQuery);
74+
query.$or.forEach(value => validateQuery(value, isMaster, update));
7975
} else {
8076
throw new Parse.Error(Parse.Error.INVALID_QUERY, 'Bad $or format - use an array value.');
8177
}
8278
}
8379

8480
if (query.$and) {
8581
if (query.$and instanceof Array) {
86-
query.$and.forEach(validateQuery);
82+
query.$and.forEach(value => validateQuery(value, isMaster, update));
8783
} else {
8884
throw new Parse.Error(Parse.Error.INVALID_QUERY, 'Bad $and format - use an array value.');
8985
}
9086
}
9187

9288
if (query.$nor) {
9389
if (query.$nor instanceof Array && query.$nor.length > 0) {
94-
query.$nor.forEach(validateQuery);
90+
query.$nor.forEach(value => validateQuery(value, isMaster, update));
9591
} else {
9692
throw new Parse.Error(
9793
Parse.Error.INVALID_QUERY,
@@ -111,7 +107,11 @@ const validateQuery = (query: any): void => {
111107
}
112108
}
113109
}
114-
if (!isSpecialQueryKey(key) && !key.match(/^[a-zA-Z][a-zA-Z0-9_\.]*$/)) {
110+
if (
111+
!key.match(/^[a-zA-Z][a-zA-Z0-9_\.]*$/) &&
112+
((!specialQueryKeys.includes(key) && !isMaster && !update) ||
113+
(update && isMaster && !specialMasterQueryKeys.includes(key)))
114+
) {
115115
throw new Parse.Error(Parse.Error.INVALID_KEY_NAME, `Invalid key name: ${key}`);
116116
}
117117
});
@@ -204,27 +204,25 @@ const filterSensitiveData = (
204204
perms.protectedFields.temporaryKeys.forEach(k => delete object[k]);
205205
}
206206

207-
if (!isUserClass) {
208-
return object;
207+
if (isUserClass) {
208+
object.password = object._hashed_password;
209+
delete object._hashed_password;
210+
delete object.sessionToken;
209211
}
210212

211-
object.password = object._hashed_password;
212-
delete object._hashed_password;
213+
if (isMaster) {
214+
return object;
215+
}
213216

214-
delete object.sessionToken;
217+
for (const key in object) {
218+
if (key.charAt(0) === '_') {
219+
delete object[key];
220+
}
221+
}
215222

216-
if (isMaster) {
223+
if (!isUserClass) {
217224
return object;
218225
}
219-
delete object._email_verify_token;
220-
delete object._perishable_token;
221-
delete object._perishable_token_expires_at;
222-
delete object._tombstone;
223-
delete object._email_verify_token_expires_at;
224-
delete object._failed_login_count;
225-
delete object._account_lockout_expires_at;
226-
delete object._password_changed_at;
227-
delete object._password_history;
228226

229227
if (aclGroup.indexOf(object.objectId) > -1) {
230228
return object;
@@ -513,7 +511,7 @@ class DatabaseController {
513511
if (acl) {
514512
query = addWriteACL(query, acl);
515513
}
516-
validateQuery(query);
514+
validateQuery(query, isMaster, true);
517515
return schemaController
518516
.getOneSchema(className, true)
519517
.catch(error => {
@@ -759,7 +757,7 @@ class DatabaseController {
759757
if (acl) {
760758
query = addWriteACL(query, acl);
761759
}
762-
validateQuery(query);
760+
validateQuery(query, isMaster, false);
763761
return schemaController
764762
.getOneSchema(className)
765763
.catch(error => {
@@ -1232,7 +1230,7 @@ class DatabaseController {
12321230
query = addReadACL(query, aclGroup);
12331231
}
12341232
}
1235-
validateQuery(query);
1233+
validateQuery(query, isMaster, false);
12361234
if (count) {
12371235
if (!classExists) {
12381236
return 0;
@@ -1744,7 +1742,7 @@ class DatabaseController {
17441742
return Promise.resolve(response);
17451743
}
17461744

1747-
static _validateQuery: any => void;
1745+
static _validateQuery: (any, boolean, boolean) => void;
17481746
static filterSensitiveData: (boolean, any[], any, any, any, string, any[], any) => void;
17491747
}
17501748

‎src/RestQuery.js‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -187,6 +187,9 @@ RestQuery.prototype.execute = function (executeOptions) {
187187
.then(() => {
188188
return this.buildRestWhere();
189189
})
190+
.then(() => {
191+
return this.denyProtectedFields();
192+
})
190193
.then(() => {
191194
return this.handleIncludeAll();
192195
})
@@ -664,6 +667,30 @@ RestQuery.prototype.runCount = function () {
664667
});
665668
};
666669

670+
RestQuery.prototype.denyProtectedFields = async function () {
671+
if (this.auth.isMaster) {
672+
return;
673+
}
674+
const schemaController = await this.config.database.loadSchema();
675+
const protectedFields =
676+
this.config.database.addProtectedFields(
677+
schemaController,
678+
this.className,
679+
this.restWhere,
680+
this.findOptions.acl,
681+
this.auth,
682+
this.findOptions
683+
) || [];
684+
for (const key of protectedFields) {
685+
if (this.restWhere[key]) {
686+
throw new Parse.Error(
687+
Parse.Error.OPERATION_FORBIDDEN,
688+
`This user is not allowed to query ${key} on class ${this.className}`
689+
);
690+
}
691+
}
692+
};
693+
667694
// Augments this.response with all pointers on an object
668695
RestQuery.prototype.handleIncludeAll = function () {
669696
if (!this.includeAll) {

0 commit comments

Comments
 (0)