diff --git a/spec/vulnerabilities.spec.js b/spec/vulnerabilities.spec.js index e41e4f9120..c3f9296af0 100644 --- a/spec/vulnerabilities.spec.js +++ b/spec/vulnerabilities.spec.js @@ -4995,6 +4995,115 @@ describe('Vulnerabilities', () => { }); }); + describe('(GHSA-jpq4-7fmq-q5fj) SMS MFA single-use token reuse via concurrent requests', () => { + const mfaHeaders = { + 'X-Parse-Application-Id': 'test', + 'X-Parse-REST-API-Key': 'rest', + 'Content-Type': 'application/json', + }; + + let sentToken; + + beforeEach(async () => { + sentToken = null; + await reconfigureServer({ + auth: { + mfa: { + enabled: true, + options: ['SMS'], + algorithm: 'SHA1', + digits: 6, + period: 30, + sendSMS: token => { + sentToken = token; + }, + }, + }, + }); + }); + + async function setupSmsMfaUser() { + const user = await Parse.User.signUp('smsmfauser', 'password123'); + // Enroll SMS MFA + await request({ + method: 'PUT', + url: `http://localhost:8378/1/users/${user.id}`, + headers: { + ...mfaHeaders, + 'X-Parse-Session-Token': user.getSessionToken(), + }, + body: JSON.stringify({ + authData: { mfa: { mobile: '+15551234567' } }, + }), + }); + const enrollToken = sentToken; + // Confirm enrollment with the received OTP + await request({ + method: 'PUT', + url: `http://localhost:8378/1/users/${user.id}`, + headers: { + ...mfaHeaders, + 'X-Parse-Session-Token': user.getSessionToken(), + }, + body: JSON.stringify({ + authData: { mfa: { mobile: '+15551234567', token: enrollToken } }, + }), + }); + sentToken = null; + return user; + } + + async function requestLoginOtp(username, password) { + try { + await request({ + method: 'POST', + url: 'http://localhost:8378/1/login', + headers: mfaHeaders, + body: JSON.stringify({ + username, + password, + authData: { mfa: { token: 'request' } }, + }), + }); + } catch (_err) { + // Expected: adapter throws "Please enter the token" + } + return sentToken; + } + + it('rejects concurrent logins using the same SMS MFA OTP', async () => { + const user = await setupSmsMfaUser(); + const otp = await requestLoginOtp('smsmfauser', 'password123'); + expect(otp).toBeDefined(); + + const loginWithOtp = () => + request({ + method: 'POST', + url: 'http://localhost:8378/1/login', + headers: mfaHeaders, + body: JSON.stringify({ + username: 'smsmfauser', + password: 'password123', + authData: { mfa: { token: otp } }, + }), + }); + + const results = await Promise.allSettled(Array(10).fill().map(() => loginWithOtp())); + + const succeeded = results.filter(r => r.status === 'fulfilled'); + const failed = results.filter(r => r.status === 'rejected'); + + // Exactly one request should succeed; all others should fail + expect(succeeded.length).toBe(1); + expect(failed.length).toBe(9); + + // Verify the OTP has been consumed + await user.fetch({ useMasterKey: true }); + const mfa = user.get('authData').mfa; + expect(mfa.token).toBeUndefined(); + }); + }); + describe('(GHSA-p2w6-rmh7-w8q3) SQL Injection via aggregate and distinct field names in PostgreSQL adapter', () => { const headers = { 'Content-Type': 'application/json', diff --git a/src/AuthDataLock.js b/src/AuthDataLock.js new file mode 100644 index 0000000000..c809a8b3d8 --- /dev/null +++ b/src/AuthDataLock.js @@ -0,0 +1,47 @@ +// Apply optimistic locking for authData provider field changes. For each lockable +// top-level field in the original authData whose value differs from the incoming +// value, add an equality constraint for the original value to the update WHERE +// clause. Concurrent requests racing the same single-use token will only allow the +// first update to match; subsequent updates miss and surface as OBJECT_NOT_FOUND. +// +// Only fields whose values round-trip cleanly through both storage adapters are +// locked: primitives (string, number, boolean) and arrays. Date values and nested +// objects are skipped because their JSON representation differs between the +// MongoDB and Postgres adapters, and because Parse Server's query-key validator +// rejects deeper paths containing characters like `+` (e.g. phone-number keys). +// Locking the consumed single-use credential (the MFA token string or the +// recovery-code array) is sufficient — its removal invalidates the WHERE clause +// for concurrent writers. +export function applyAuthDataOptimisticLock(query, originalAuthData, newAuthData) { + if (!originalAuthData) { + return; + } + for (const provider of Object.keys(newAuthData)) { + const original = originalAuthData[provider]; + if (!original || typeof original !== 'object') { + continue; + } + for (const [field, value] of Object.entries(original)) { + if (!isLockableAuthDataValue(value)) { + continue; + } + if (JSON.stringify(value) !== JSON.stringify(newAuthData[provider]?.[field])) { + query[`authData.${provider}.${field}`] = value; + } + } + } +} + +function isLockableAuthDataValue(value) { + if (value === null || value === undefined) { + return false; + } + const t = typeof value; + if (t === 'string' || t === 'number' || t === 'boolean') { + return true; + } + if (Array.isArray(value)) { + return true; + } + return false; +} diff --git a/src/RestWrite.js b/src/RestWrite.js index c6c8db969c..6fd6bff94b 100644 --- a/src/RestWrite.js +++ b/src/RestWrite.js @@ -17,6 +17,7 @@ import _ from 'lodash'; import logger from './logger'; import { requiredColumns } from './Controllers/SchemaController'; import { createSanitizedError } from './Error'; +import { applyAuthDataOptimisticLock } from './AuthDataLock'; // query and data are both provided in REST API format. So data // types are encoded by plain old objects. @@ -658,20 +659,20 @@ RestWrite.prototype.handleAuthData = async function (authData) { this.authDataResponse = res.authDataResponse; } + // Capture original authData before mutating userResult via the response reference + const originalAuthData = userResult?.authData + ? Object.fromEntries( + Object.entries(userResult.authData).map(([k, v]) => + [k, v && typeof v === 'object' ? { ...v } : v] + ) + ) + : undefined; + // IF we are in login we'll skip the database operation / beforeSave / afterSave etc... // we need to set it up there. // We are supposed to have a response only on LOGIN with authData, so we skip those // If we're not logging in, but just updating the current user, we can safely skip that part if (this.response) { - // Capture original authData before mutating userResult via the response reference - const originalAuthData = userResult?.authData - ? Object.fromEntries( - Object.entries(userResult.authData).map(([k, v]) => - [k, v && typeof v === 'object' ? { ...v } : v] - ) - ) - : undefined; - // Assign the new authData in the response Object.keys(mutatedAuthData).forEach(provider => { this.response.response.authData[provider] = mutatedAuthData[provider]; @@ -683,24 +684,11 @@ RestWrite.prototype.handleAuthData = async function (authData) { // Then we're good for the user, early exit of sorts if (Object.keys(this.data.authData).length) { const query = { objectId: this.data.objectId }; - // Optimistic locking: include the original array fields in the WHERE clause + // Optimistic locking: include each changed original field in the WHERE clause // for providers whose data is being updated. This prevents concurrent requests - // from both succeeding when consuming single-use tokens (e.g. MFA recovery codes). - if (originalAuthData) { - for (const provider of Object.keys(this.data.authData)) { - const original = originalAuthData[provider]; - if (original && typeof original === 'object') { - for (const [field, value] of Object.entries(original)) { - if ( - Array.isArray(value) && - JSON.stringify(value) !== JSON.stringify(this.data.authData[provider]?.[field]) - ) { - query[`authData.${provider}.${field}`] = value; - } - } - } - } - } + // from both succeeding when consuming single-use tokens (e.g. MFA recovery codes + // as arrays, or MFA SMS OTP tokens as strings). + applyAuthDataOptimisticLock(query, originalAuthData, this.data.authData); try { await this.config.database.update( this.className, @@ -716,6 +704,11 @@ RestWrite.prototype.handleAuthData = async function (authData) { throw error; } } + } else if (this.query && this.data.authData && Object.keys(this.data.authData).length) { + // UPDATE path (e.g. PUT /users/:id during linked-provider re-auth): apply + // the same optimistic lock to the subsequent runDatabaseOperation update so + // concurrent single-use token consumers cannot both succeed. + applyAuthDataOptimisticLock(this.query, originalAuthData, this.data.authData); } } } diff --git a/src/Routers/UsersRouter.js b/src/Routers/UsersRouter.js index 3ac099f0b0..34271a7dd7 100644 --- a/src/Routers/UsersRouter.js +++ b/src/Routers/UsersRouter.js @@ -18,6 +18,7 @@ import { promiseEnsureIdempotency } from '../middlewares'; import RestWrite from '../RestWrite'; import { logger } from '../logger'; import { createSanitizedError } from '../Error'; +import { applyAuthDataOptimisticLock } from '../AuthDataLock'; export class UsersRouter extends ClassesRouter { className() { @@ -314,26 +315,10 @@ export class UsersRouter extends ClassesRouter { // If we have some new validated authData update directly if (validatedAuthData && Object.keys(validatedAuthData).length) { const query = { objectId: user.objectId }; - // Optimistic locking: include the original array fields in the WHERE clause - // for providers whose data is being updated. This prevents concurrent requests - // from both succeeding when consuming single-use tokens (e.g. MFA recovery codes). - // Only array fields need locking — element removal is vulnerable to TOCTOU; - // scalar fields are simply overwritten and don't have concurrency issues. - if (user.authData) { - for (const provider of Object.keys(validatedAuthData)) { - const original = user.authData[provider]; - if (original && typeof original === 'object') { - for (const [field, value] of Object.entries(original)) { - if ( - Array.isArray(value) && - JSON.stringify(value) !== JSON.stringify(validatedAuthData[provider]?.[field]) - ) { - query[`authData.${provider}.${field}`] = value; - } - } - } - } - } + // Prevent concurrent requests from both succeeding when consuming single-use + // tokens (e.g. MFA recovery codes or SMS OTP tokens) by extending the update + // WHERE clause with the original values of changed primitive/array fields. + applyAuthDataOptimisticLock(query, user.authData, validatedAuthData); try { await req.config.database.update('_User', query, { authData: validatedAuthData }, {}); } catch (error) {