|
| 1 | +'use strict'; |
| 2 | + |
| 3 | +// Guard: GroupLinkBleed (GHSA-v544-rmr5-38gx). Boards.allow lets a board admin |
| 4 | +// update any field of their board, and two checks lived only inside Meteor |
| 5 | +// methods: "a board admin may add only the organizations and teams they belong |
| 6 | +// to" (#5141, setBoardOrgs/setBoardTeams) and inviteUserToBoard's checks (who |
| 7 | +// may invite, disabled accounts, the #6116 same organization/team policy). A |
| 8 | +// client calling Boards.update / Boards.insert directly - /boards/update over |
| 9 | +// DDP - skipped both: it could attach any team or organization to its board, |
| 10 | +// so the whole group saw it and became members, and push any user id into |
| 11 | +// members. |
| 12 | +// |
| 13 | +// This runs the REAL deny rules of server/permissions/boards.js in a vm, with |
| 14 | +// stubbed imports, against the reporter's writes and against every write the |
| 15 | +// cli
10BC8
ent legitimately makes. A write is refused when any deny rule says so, |
| 16 | +// exactly as Meteor's allow/deny decides. |
| 17 | +// Run: node tests/groupLinkBleed.test.cjs |
| 18 | + |
| 19 | +const { test } = require('node:test'); |
| 20 | +const assert = require('node:assert/strict'); |
| 21 | +const fs = require('node:fs'); |
| 22 | +const path = require('node:path'); |
| 23 | +const vm = require('node:vm'); |
| 24 | + |
| 25 | +const ROOT = path.join(__dirname, '..'); |
| 26 | +const read = file => fs.readFileSync(path.join(ROOT, file), 'utf8'); |
| 27 | +const guards = require('../models/lib/boardWriteGuards'); |
| 28 | + |
| 29 | +function loadRules() { |
| 30 | + const denies = []; |
| 31 | + const recorded = []; |
| 32 | + const Boards = { deny: rules => denies.push(rules), allow: () => {} }; |
| 33 | + const resolve = spec => { |
| 34 | + if (spec === '/server/lib/securityLog') return { record: evt => recorded.push(evt) }; |
| 35 | + if (spec === '/models/attachments') return { default: { findOneAsync: async () => null } }; |
| 36 | + return require(path.join(ROOT, spec)); |
| 37 | + }; |
| 38 | + const src = read('server/permissions/boards.js') |
| 39 | + .replace(/^import Boards from .*$/m, '') |
| 40 | + .replace(/^import TableVisibilityModeSettings from .*$/m, |
| 41 | + 'const TableVisibilityModeSettings = { findOneAsync: async () => null };') |
| 42 | + .replace(/^import \{ findWhere, where \} from .*$/m, |
| 43 | + 'const where = (a, q) => (a || []).filter(x => Object.keys(q).every(k => x[k] === q[k]));' |
| 44 | + + 'const findWhere = (a, q) => where(a, q)[0];') |
| 45 | + .replace(/^import \{[^}]*\} from '\/server\/lib\/utils';$/m, |
| 46 | + 'const allowIsBoardAdminOrSiteAdmin = () => true; const canUpdateBoardSort = () => false;' |
| 47 | + + 'const canUpdateBoardSameWidthValue = () => false;') |
| 48 | + .replace(/^import \{[^}]*\} from '\/server\/lib\/subtaskDepositAccess';$/m, |
| 49 | + 'const canWriteSubtaskDeposit = async () => true; const recordSubtaskDepositDenial = () => {};'); |
| 50 | + assert.ok(!/^import /m.test(src), 'every import of server/permissions/boards.js is stubbed'); |
| 51 | + vm.runInNewContext(src, { Boards, require: resolve, console }); |
| 52 | + return { denies, recorded }; |
| 53 | +} |
| 54 | + |
| 55 | +const ADMIN = 'admin'; |
| 56 | +const board = () => ({ |
| 57 | + _id: 'b1', |
| 58 | + members: [ |
| 59 | + { userId: ADMIN, isAdmin: true, isActive: true }, |
| 60 | + { userId: 'bob', isAdmin: false, isActive: true }, |
| 61 | + { userId: 'gone', isAdmin: false, isActive: false }, |
| 62 | + ], |
| 63 | + teams: [{ teamId: 'mine', teamDisplayName: 'Mine', isActive: true }], |
| 64 | +}); |
| 65 | +const fieldsOf = modifier => [...new Set(Object.values(modifier).flatMap(a => Object.keys(a).map(k => k.split('.')[0])))]; |
| 66 | + |
| 67 | +async function refusedUpdate(modifier, doc = board()) { |
| 68 | + const { denies, recorded } = loadRules(); |
| 69 | + for (const rules of denies) { |
| 70 | + if (rules.update && await rules.update(ADMIN, doc, fieldsOf(modifier), modifier)) return { refused: true, recorded }; |
| 71 | + } |
| 72 | + return { refused: false, recorded }; |
| 73 | +} |
| 74 | +async function refusedInsert(doc) { |
| 75 | + const { denies, recorded } = loadRules(); |
| 76 | + for (const rules of denies) { |
| 77 | + if (rules.insert && await rules.insert(ADMIN, doc)) return { refused: true, recorded }; |
| 78 | + } |
| 79 | + return { refused: false, recorded }; |
| 80 | +} |
| 81 | + |
| 82 | +const newMember = userId => ({ userId, isAdmin: false, isActive: true, isNoComments: false, isCommentOnly: false, |
| 83 | + isWorker: false, isNormalAssignedOnly: false, isCommentAssignedOnly: false, isReadOnly: false, isReadAssignedOnly: false }); |
| 84 | + |
| 85 | +test('the reported attack: a foreign team, organization or domain written straight to the board is refused and recorded', async () => { |
| 86 | + for (const modifier of [ |
| 87 | + { $set: { teams: [{ teamId: 'foreign', teamDisplayName: 'x', isActive: true }] } }, |
| 88 | + { $push: { teams: { teamId: 'foreign', teamDisplayName: 'x', isActive: true } } }, |
| 89 | + { $addToSet: { orgs: { orgId: 'foreign', orgDisplayName: 'x', isActive: true } } }, |
| 90 | + { $set: { 'teams.0.teamId': 'foreign' } }, |
| 91 | + { $set: { domains: [{ domain: 'example.com', isActive: true }] } }, |
| 92 | + { $set: { title: 'harmless', orgs: [{ orgId: 'foreign', orgDisplayName: 'x', isActive: true }] } }, |
| 93 | + ]) { |
| 94 | + const { refused, recorded } = await refusedUpdate(modifier); |
| 95 | + assert.equal(refused, true, JSON.stringify(modifier)); |
| 96 | + assert.equal(recorded[0].key, 'authz.group-link', JSON.stringify(modifier)); |
| 97 | + assert.equal(recorded[0].action, 'blocked'); |
| 98 | + } |
| 99 | + for (const field of ['teams', 'orgs', 'domains']) { |
| 100 | + const { refused, recorded } = await refusedInsert({ title: 'x', [field]: [{ isActive: true }] }); |
| 101 | + assert.equal(refused, true, field); |
| 102 | + assert.equal(recorded[0].source, 'ddp:boards.insert'); |
| 103 | + } |
| 104 | +}); |
| 105 | + |
| 106 | +test('the member variant: adding or bringing back a member from the client is refused and recorded', async () => { |
| 107 | + for (const modifier of [ |
| 108 | + { $push: { members: newMember('stranger') } }, |
| 109 | + { $addToSet: { members: newMember('stranger') } }, |
| 110 | + { $set: { members: [...board().members, newMember('stranger')] } }, |
| 111 | + { $set: { 'members.3': newMember('stranger') } }, |
| 112 | + { $set: { 'members.1': newMember('stranger') } }, |
| 113 | + { $set: { 'members.1.userId': 'stranger' } }, |
| 114 | + { $set: { 'members.$.isActive': true } }, |
| 115 | + { $set: { 'members.2.isActive': true } }, |
| 116 | + { $set: { 'members.2': { ...board().members[2], isActive: true } } }, |
| 117 | + { $set: { members: board().members.map(m => ({ ...m, isActive: true })) } }, |
| 118 | + ]) { |
| 119 | + const { refused, recorded } = await refusedUpdate(modifier); |
| 120 | + assert.equal(refused, true, JSON.stringify(modifier)); |
| 121 | + assert.equal(recorded.at(-1).key, 'authz.member-add', JSON.stringify(modifier)); |
| 122 | + } |
| 123 | +}); |
| 124 | + |
| 125 | +test('negative: what the client legitimately writes still passes and is not logged', async () => { |
| 126 | + for (const modifier of [ |
| 127 | + // setMemberPermission (models/boards.js), dotted $set + $unset. |
| 128 | + { $set: { 'members.1.isAdmin': true, 'members.1.isCommentOnly': false }, $unset: { 'members.1.customRoleId': '' } }, |
| 129 | + // removeMember: deactivate, keep the last admin, orphan cleanup. |
| 130 | + { $set: { 'members.1.isActive': false, 'members.1.isAdmin': false }, $unset: { 'members.1.grantedBy': '' } }, |
| 131 | + { $set: { 'members.0.isActive': true } }, |
| 132 | + { $set: { members: board().members.filter(m => m.userId !== 'bob') } }, |
| 133 | + { $pull: { members: { userId: 'bob' } } }, |
| 134 | + // Ordinary board settings. |
| 135 | + { $set: { title: 'Renamed', color: 'belize' } }, |
| 136 | + { $set: { allowsSubtasks: true } }, |
| 137 | + ]) { |
| 138 | + const { refused, recorded } = await refusedUpdate(modifier); |
| 139 | + assert.equal(refused, false, JSON.stringify(modifier)); |
| 140 | + assert.deepEqual(recorded, [], JSON.stringify(modifier)); |
| 141 | + } |
| 142 | + // The UI creates a board without groups or other members. |
| 143 | + const { refused } = await refusedInsert({ title: 'New', members: [newMember(ADMIN)], teams: [], orgs: [] }); |
| 144 | + assert.equal(refused, false); |
| 145 | +}); |
| 146 | + |
| 147 | +test('the pure helpers', () => { |
| 148 | + assert.equal(guards.isGroupLinkField('teams'), true); |
| 149 | + assert.equal(guards.isGroupLinkField('orgs.0.orgId'), true); |
| 150 | + assert.equal(guards.isGroupLinkField('domainsX'), false); |
| 151 | + assert.equal(guards.isGroupLinkField('title'), false); |
| 152 | + assert.equal(guards.addsBoardMember(board(), { $rename: { 'members': 'x' } }), true); |
| 153 | + assert.equal(guards.addsBoardMember(board(), { $set: { 'members.1.isWorker': true } }), false); |
| 154 | + assert.equal(guards.addsBoardMember({}, { $set: { 'members.0.isActive': false } }), true, 'no members: no index exists'); |
| 155 | +}); |
| 156 | + |
| 157 | +test('the methods keep their checks, the catalog has the keys', () => { |
| 158 | + const methods = read('server/models/boards.js'); |
| 159 | + for (const name of ['setBoardOrgs', 'setBoardTeams']) { |
| 160 | + const start = methods.indexOf(`async ${name}(`); |
| 161 | + const guard = methods.indexOf('forbiddenNewGroupIds(', start); |
| 162 | + const write = methods.indexOf('Boards.updateAsync(', start); |
| 163 | + assert.ok(start > 0 && guard > start && guard < write, `${name} checks before writing`); |
| 164 | + } |
| 165 | + const catalog = read('models/lib/securityCategories.js'); |
| 166 | + assert.match(catalog, /'authz\.group-link':\s*\{[^}]*bleed: 'GroupLinkBleed'[^}]*cwe: 'CWE-863'/); |
| 167 | + assert.match(catalog, /'authz\.member-add':\s*\{[^}]*bleed: 'GroupLinkBleed'[^}]*cwe: 'CWE-863'/); |
| 168 | +}); |
| 169 | + |
| 170 | +// The shape of the fault: a field whose only check sits inside a Meteor method |
| 171 | +// is still writable by a client through the collection, unless a deny rule |
| 172 | +// names it. Every board field a method guards with an ownership/membership |
| 173 | +// check must be named by a Boards.deny rule - or, for members, by the add |
| 174 | +// guard. |
| 175 | +test('negative: no board field guarded only by a method is left client-writable', () => { |
| 176 | + const methods = read('server/models/boards.js'); |
| 177 | + const perms = read('server/permissions/boards.js') + read('models/boards.js'); |
| 178 | + const guarded = new Set(); |
| 179 | + for (const m of methods.matchAll(/forbiddenNewGroupIds\(\{\s*previous: board\.(\w+)/g)) guarded.add(m[1]); |
| 180 | + assert.deepEqual([...guarded].sort(), ['orgs', 'teams']); |
| 181 | + for (const field of [...guarded, 'domains']) { |
| 182 | + assert.ok(guards.GROUP_LINK_FIELDS.includes(field), `${field} is denied to client writes`); |
| 183 | + } |
| 184 | + assert.match(perms, /addsBoardMember\(doc, modifier\)/); |
| 185 | + // No client code writes these fields directly: the UI goes through the methods. |
| 186 | + const walk = dir => fs.readdirSync(path.join(ROOT, dir), { withFileTypes: true }).flatMap(e => { |
| 187 | + if (e.name === 'tests' || e.name === 'node_modules' || e.name.startsWith('_build')) return []; |
| 188 | + const rel = `${dir}/${e.name}`; |
| 189 | + return e.isDirectory() ? walk(rel) : (rel.endsWith('.js') ? [rel] : []); |
| 190 | + }); |
| 191 | + const offenders = walk('client').filter(file => |
| 192 | + /Boards\.update(Async)?\([^;]*\b(teams|orgs|domains)\s*:/.test(read(file)) |
| 193 | + || /Boards\.update(Async)?\([^;]*\$(push|addToSet)\s*:\s*\{\s*members\b/.test(read(file))); |
| 194 | + assert.deepEqual(offenders, []); |
| 195 | +}); |
0 commit comments