diff --git a/spec/PostgresStorageAdapter.spec.js b/spec/PostgresStorageAdapter.spec.js index aa5e692fe4..9c0c5c49ce 100644 --- a/spec/PostgresStorageAdapter.spec.js +++ b/spec/PostgresStorageAdapter.spec.js @@ -589,3 +589,213 @@ describe_only_db('postgres')('PostgresStorageAdapter shutdown', () => { expect(adapter._client.$pool.ending).toEqual(true); }); }); + +describe_only_db('postgres')('PostgresStorageAdapter Increment JSON key escaping', () => { + const request = require('../lib/request'); + const headers = { + 'Content-Type': 'application/json', + 'X-Parse-Application-Id': 'test', + 'X-Parse-REST-API-Key': 'rest', + }; + + it('does not inject additional JSONB keys via double-quote in sub-key name', async () => { + const obj = new Parse.Object('IncrementTest'); + obj.set('metadata', { score: 100, isAdmin: 0 }); + await obj.save(); + + // Advisory payload: sub-key `":0,"isAdmin` produces JSON `{"":0,"isAdmin":amount}` + // which would inject/overwrite the `isAdmin` key via JSONB `||` merge + await request({ + method: 'PUT', + url: `http://localhost:8378/1/classes/IncrementTest/${obj.id}`, + headers, + body: JSON.stringify({ + 'metadata.":0,"isAdmin': { __op: 'Increment', amount: 1 }, + }), + }).catch(() => {}); + + const verify = await new Parse.Query('IncrementTest').get(obj.id); + // isAdmin must NOT have been changed by the injection + expect(verify.get('metadata').isAdmin).toBe(0); + // score must remain unchanged + expect(verify.get('metadata').score).toBe(100); + // No spurious empty-string key should exist + expect(verify.get('metadata')['']).toBeUndefined(); + }); + + it('does not overwrite existing JSONB keys via crafted sub-key injection', async () => { + const obj = new Parse.Object('IncrementTest'); + obj.set('metadata', { balance: 500 }); + await obj.save(); + + // Attempt to overwrite `balance` with 0 via injection, then set injected key to amount + await request({ + method: 'PUT', + url: `http://localhost:8378/1/classes/IncrementTest/${obj.id}`, + headers, + body: JSON.stringify({ + 'metadata.":0,"balance': { __op: 'Increment', amount: 0 }, + }), + }).catch(() => {}); + + const verify = await new Parse.Query('IncrementTest').get(obj.id); + // balance must NOT have been overwritten + expect(verify.get('metadata').balance).toBe(500); + }); + + it('does not escalate write access beyond what CLP already grants', async () => { + // A user with write CLP can already overwrite any sub-key of an Object field + // directly, so the JSON key injection does not grant additional capabilities. + const schema = new Parse.Schema('IncrementCLPTest'); + schema.addObject('metadata'); + schema.setCLP({ + find: { '*': true }, + get: { '*': true }, + create: { '*': true }, + update: { '*': true }, + addField: {}, + }); + await schema.save(); + + const obj = new Parse.Object('IncrementCLPTest'); + obj.set('metadata', { score: 100, isAdmin: 0 }); + await obj.save(); + + // A user with write CLP can already directly overwrite any sub-key + const directResponse = await request({ + method: 'PUT', + url: `http://localhost:8378/1/classes/IncrementCLPTest/${obj.id}`, + headers, + body: JSON.stringify({ + 'metadata.isAdmin': { __op: 'Increment', amount: 1 }, + }), + }); + expect(directResponse.status).toBe(200); + + const afterDirect = await new Parse.Query('IncrementCLPTest').get(obj.id); + // Direct Increment already overwrites the key — no injection needed + expect(afterDirect.get('metadata').isAdmin).toBe(1); + }); + + it('does not bypass protectedFields — injection has same access as direct write', async () => { + const user = await Parse.User.signUp('protuser', 'password123'); + + const schema = new Parse.Schema('IncrementProtectedTest'); + schema.addObject('metadata'); + schema.setCLP({ + find: { '*': true }, + get: { '*': true }, + create: { '*': true }, + update: { '*': true }, + addField: {}, + protectedFields: { '*': ['metadata'] }, + }); + await schema.save(); + + const obj = new Parse.Object('IncrementProtectedTest'); + obj.set('metadata', { score: 100, isAdmin: 0 }); + await obj.save(null, { useMasterKey: true }); + + // Injection attempt on a protected field + await request({ + method: 'PUT', + url: `http://localhost:8378/1/classes/IncrementProtectedTest/${obj.id}`, + headers: { + ...headers, + 'X-Parse-Session-Token': user.getSessionToken(), + }, + body: JSON.stringify({ + 'metadata.":0,"isAdmin': { __op: 'Increment', amount: 1 }, + }), + }).catch(() => {}); + + // Direct write to same protected field + await request({ + method: 'PUT', + url: `http://localhost:8378/1/classes/IncrementProtectedTest/${obj.id}`, + headers: { + ...headers, + 'X-Parse-Session-Token': user.getSessionToken(), + }, + body: JSON.stringify({ + 'metadata.isAdmin': { __op: 'Increment', amount: 1 }, + }), + }); + + // Both succeed — protectedFields controls read access, not write access. + // The injection has the same access as a direct write. + const verify = await new Parse.Query('IncrementProtectedTest').get(obj.id, { useMasterKey: true }); + + // Direct write succeeded (protectedFields doesn't block writes) + expect(verify.get('metadata').isAdmin).toBeGreaterThanOrEqual(1); + + // Verify the field is indeed read-protected for the user + const userResult = await new Parse.Query('IncrementProtectedTest').get(obj.id, { sessionToken: user.getSessionToken() }); + expect(userResult.get('metadata')).toBeUndefined(); + }); + + it('rejects injection when user lacks write CLP', async () => { + const user = await Parse.User.signUp('testuser', 'password123'); + + const schema = new Parse.Schema('IncrementNoCLPTest'); + schema.addObject('metadata'); + schema.setCLP({ + find: { '*': true }, + get: { '*': true }, + create: { '*': true }, + update: {}, + addField: {}, + }); + await schema.save(); + + const obj = new Parse.Object('IncrementNoCLPTest'); + obj.set('metadata', { score: 100, isAdmin: 0 }); + await obj.save(null, { useMasterKey: true }); + + // Without write CLP, the injection attempt is rejected + await request({ + method: 'PUT', + url: `http://localhost:8378/1/classes/IncrementNoCLPTest/${obj.id}`, + headers: { + ...headers, + 'X-Parse-Session-Token': user.getSessionToken(), + }, + body: JSON.stringify({ + 'metadata.":0,"isAdmin': { __op: 'Increment', amount: 1 }, + }), + }).catch(() => {}); + + const verify = await new Parse.Query('IncrementNoCLPTest').get(obj.id); + // isAdmin unchanged — CLP blocked the write + expect(verify.get('metadata').isAdmin).toBe(0); + }); + + it('rejects injection when user lacks write access via ACL', async () => { + const owner = await Parse.User.signUp('owner', 'password123'); + const attacker = await Parse.User.signUp('attacker', 'password456'); + + const obj = new Parse.Object('IncrementACLTest'); + obj.set('metadata', { score: 100, isAdmin: 0 }); + const acl = new Parse.ACL(owner); + acl.setPublicReadAccess(true); + obj.setACL(acl); + await obj.save(null, { useMasterKey: true }); + + // Attacker has public read but not write — injection attempt should fail + await request({ + method: 'PUT', + url: `http://localhost:8378/1/classes/IncrementACLTest/${obj.id}`, + headers: { + ...headers, + 'X-Parse-Session-Token': attacker.getSessionToken(), + }, + body: JSON.stringify({ + 'metadata.":0,"isAdmin': { __op: 'Increment', amount: 1 }, + }), + }).catch(() => {}); + + const verify = await new Parse.Query('IncrementACLTest').get(obj.id); + // isAdmin unchanged — ACL blocked the write + expect(verify.get('metadata').isAdmin).toBe(0); + }); +}); diff --git a/spec/vulnerabilities.spec.js b/spec/vulnerabilities.spec.js index dc73888a67..6c734f308b 100644 --- a/spec/vulnerabilities.spec.js +++ b/spec/vulnerabilities.spec.js @@ -1615,25 +1615,23 @@ describe('(GHSA-gqpp-xgvh-9h7h) SQL Injection via dot-notation sub-key name in I }).catch(() => {}); const elapsed = Date.now() - start; - // Double quotes break JSON structure inside the CONCAT, producing invalid JSONB. - // This causes a database error, NOT SQL injection. If injection succeeded, - // the query would take >= 3 seconds due to pg_sleep. + // Double quotes are escaped in the JSON context, producing a harmless literal key + // name. No SQL injection occurs. If injection succeeded, the query would take + // >= 3 seconds due to pg_sleep. expect(elapsed).toBeLessThan(3000); - // Invalid JSONB cast fails the UPDATE, so the row is not modified const verify = await new Parse.Query('SubKeyTest').get(obj.id); - expect(verify.get('stats')).toEqual({ counter: 0 }); + // Original counter is untouched + expect(verify.get('stats').counter).toBe(0); }); - it_only_db('postgres')('does not execute injected SQL via double quote crafted as valid JSONB in sub-key name', async () => { + it_only_db('postgres')('does not inject additional JSONB keys via double quote crafted as valid JSONB in sub-key name', async () => { const obj = new Parse.Object('SubKeyTest'); obj.set('stats', { counter: 0 }); await obj.save(); - // This payload uses double quotes to craft a sub-key that produces valid JSONB - // (e.g. '{"x":0,"evil":1}') instead of breaking JSON structure. Even so, both - // interpolation sites are inside single-quoted SQL strings, so double quotes - // cannot escape the SQL context — no arbitrary SQL execution is possible. - const start = Date.now(); + // This payload attempts to craft a sub-key that produces valid JSONB with + // injected keys (e.g. '{"x":0,"evil":1}'). Double quotes are escaped in the + // JSON context, so the payload becomes a harmless literal key name instead. await request({ method: 'PUT', url: `http://localhost:8378/1/classes/SubKeyTest/${obj.id}`, @@ -1642,13 +1640,12 @@ describe('(GHSA-gqpp-xgvh-9h7h) SQL Injection via dot-notation sub-key name in I 'stats.x":0,"pg_sleep(3)': { __op: 'Increment', amount: 1 }, }), }).catch(() => {}); - const elapsed = Date.now() - start; - expect(elapsed).toBeLessThan(3000); - // Double quotes craft valid JSONB with extra keys, but no SQL injection occurs; - // original counter is untouched const verify = await new Parse.Query('SubKeyTest').get(obj.id); + // Original counter is untouched expect(verify.get('stats').counter).toBe(0); + // No injected key exists — the payload is treated as a single literal key name + expect(verify.get('stats')['pg_sleep(3)']).toBeUndefined(); }); it_only_db('postgres')('allows valid Increment on nested object field with normal sub-key', async () => { diff --git a/src/Adapters/Storage/Postgres/PostgresStorageAdapter.js b/src/Adapters/Storage/Postgres/PostgresStorageAdapter.js index cd9e51fde3..b65e7d7ed8 100644 --- a/src/Adapters/Storage/Postgres/PostgresStorageAdapter.js +++ b/src/Adapters/Storage/Postgres/PostgresStorageAdapter.js @@ -209,6 +209,7 @@ const handleDotFields = object => { }; const escapeSqlString = value => value.replace(/'/g, "''"); +const escapeJsonString = value => JSON.stringify(value).slice(1, -1); const transformDotFieldToComponents = fieldName => { return fieldName.split('.').map((cmpt, index) => { @@ -1766,8 +1767,9 @@ export class PostgresStorageAdapter implements StorageAdapter { } incrementValues.push(amount); const amountIndex = index + incrementValues.length; - const safeName = escapeSqlString(c); - return `CONCAT('{"${safeName}":', COALESCE($${index}:name->>'${safeName}','0')::int + $${amountIndex}, '}')::jsonb`; + const jsonSafeName = escapeSqlString(escapeJsonString(c)); + const sqlSafeName = escapeSqlString(c); + return `CONCAT('{"${jsonSafeName}":', COALESCE($${index}:name->>'${sqlSafeName}','0')::int + $${amountIndex}, '}')::jsonb`; }) .join(' || '); // Strip the keys