Skip to content

Commit 661f160

Browse files
authored
fix: MFA single-use token bypass via concurrent authData login requests ([GHSA-w73w-g5xw-rwhf](GHSA-w73w-g5xw-rwhf)) (#10327)
1 parent a536a85 commit 661f160

File tree

2 files changed

+117
-1
lines changed

2 files changed

+117
-1
lines changed

spec/vulnerabilities.spec.js

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4040,6 +4040,91 @@ describe('(GHSA-2299-ghjr-6vjp) MFA recovery code reuse via concurrent requests'
40404040
});
40414041
});
40424042

4043+
describe('(GHSA-w73w-g5xw-rwhf) MFA recovery code reuse via concurrent authData-only login', () => {
4044+
const mfaHeaders = {
4045+
'X-Parse-Application-Id': 'test',
4046+
'X-Parse-REST-API-Key': 'rest',
4047+
'Content-Type': 'application/json',
4048+
};
4049+
4050+
let fakeProvider;
4051+
4052+
beforeEach(async () => {
4053+
fakeProvider = {
4054+
validateAppId: () => Promise.resolve(),
4055+
validateAuthData: () => Promise.resolve(),
4056+
};
4057+
await reconfigureServer({
4058+
auth: {
4059+
fakeProvider,
4060+
mfa: {
4061+
enabled: true,
4062+
options: ['TOTP'],
4063+
algorithm: 'SHA1',
4064+
digits: 6,
4065+
period: 30,
4066+
},
4067+
},
4068+
});
4069+
});
4070+
4071+
it('rejects concurrent authData-only logins using the same MFA recovery code', async () => {
4072+
const OTPAuth = require('otpauth');
4073+
4074+
// Create user via authData login with fake provider
4075+
const user = await Parse.User.logInWith('fakeProvider', {
4076+
authData: { id: 'user1', token: 'fakeToken' },
4077+
});
4078+
4079+
// Enable MFA for this user
4080+
const secret = new OTPAuth.Secret();
4081+
const totp = new OTPAuth.TOTP({
4082+
algorithm: 'SHA1',
4083+
digits: 6,
4084+
period: 30,
4085+
secret,
4086+
});
4087+
const token = totp.generate();
4088+
await user.save(
4089+
{ authData: { mfa: { secret: secret.base32, token } } },
4090+
{ sessionToken: user.getSessionToken() }
4091+
);
4092+
4093+
// Get recovery codes from stored auth data
4094+
await user.fetch({ useMasterKey: true });
4095+
const recoveryCode = user.get('authData').mfa.recovery[0];
4096+
expect(recoveryCode).toBeDefined();
4097+
4098+
// Send concurrent authData-only login requests with the same recovery code
4099+
const loginWithRecovery = () =>
4100+
request({
4101+
method: 'POST',
4102+
url: 'http://localhost:8378/1/users',
4103+
headers: mfaHeaders,
4104+
body: JSON.stringify({
4105+
authData: {
4106+
fakeProvider: { id: 'user1', token: 'fakeToken' },
4107+
mfa: { token: recoveryCode },
4108+
},
4109+
}),
4110+
});
4111+
4112+
const results = await Promise.allSettled(Array(10).fill().map(() => loginWithRecovery()));
4113+
4114+
const succeeded = results.filter(r => r.status === 'fulfilled');
4115+
const failed = results.filter(r => r.status === 'rejected');
4116+
4117+
// Exactly one request should succeed; all others should fail
4118+
expect(succeeded.length).toBe(1);
4119+
expect(failed.length).toBe(9);
4120+
4121+
// Verify the recovery code has been consumed
4122+
await user.fetch({ useMasterKey: true });
4123+
const remainingRecovery = user.get('authData').mfa.recovery;
4124+
expect(remainingRecovery).not.toContain(recoveryCode);
4125+
});
4126+
});
4127+
40434128
describe('(GHSA-37mj-c2wf-cx96) /users/me leaks raw authData via master context', () => {
40444129
const headers = {
40454130
'X-Parse-Application-Id': 'test',

src/RestWrite.js

Lines changed: 32 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -650,6 +650,15 @@ RestWrite.prototype.handleAuthData = async function (authData) {
650650
// We are supposed to have a response only on LOGIN with authData, so we skip those
651651
// If we're not logging in, but just updating the current user, we can safely skip that part
652652
if (this.response) {
653+
// Capture original authData before mutating userResult via the response reference
654+
const originalAuthData = userResult?.authData
655+
? Object.fromEntries(
656+
Object.entries(userResult.authData).map(([k, v]) =>
657+
[k, v && typeof v === 'object' ? { ...v } : v]
658+
)
659+
)
660+
: undefined;
661+
653662
// Assign the new authData in the response
654663
Object.keys(mutatedAuthData).forEach(provider => {
655664
this.response.response.authData[provider] = mutatedAuthData[provider];
@@ -660,14 +669,36 @@ RestWrite.prototype.handleAuthData = async function (authData) {
660669
// uses the `doNotSave` option. Just update the authData part
661670
// Then we're good for the user, early exit of sorts
662671
if (Object.keys(this.data.authData).length) {
672+
const query = { objectId: this.data.objectId };
673+
// Optimistic locking: include the original array fields in the WHERE clause
674+
// for providers whose data is being updated. This prevents concurrent requests
675+
// from both succeeding when consuming single-use tokens (e.g. MFA recovery codes).
676+
if (originalAuthData) {
677+
for (const provider of Object.keys(this.data.authData)) {
678+
const original = originalAuthData[provider];
679+
if (original && typeof original === 'object') {
680+
for (const [field, value] of Object.entries(original)) {
681+
if (
682+
Array.isArray(value) &&
683+
JSON.stringify(value) !== JSON.stringify(this.data.authData[provider]?.[field])
684+
) {
685+
query[`authData.${provider}.${field}`] = value;
686+
}
687+
}
688+
}
689+
}
690+
}
663691
try {
664692
await this.config.database.update(
665693
this.className,
666-
{ objectId: this.data.objectId },
694+
query,
667695
{ authData: this.data.authData },
668696
{}
669697
);
670698
} catch (error) {
699+
if (error.code === Parse.Error.OBJECT_NOT_FOUND) {
700+
throw new Parse.Error(Parse.Error.SCRIPT_FAILED, 'Invalid auth data');
701+
}
671702
this._throwIfAuthDataDuplicate(error);
672703
throw error;
673704
}

0 commit comments

Comments
 (0)