fix(auth): block privilege escalation via role/user management
The dynamic-RBAC management routes are themselves grantable (role:* and user:*), so a non-admin holding them could self-escalate: edit their own role to add a permission they lack, mint a privileged role, assign someone the admin role, or reset/delete a more-privileged account. Found by the commit security review (2× HIGH). Fix — enforce the RBAC invariant "you cannot grant beyond yourself": - roles.ts: role:create/update reject any permission not held by the caller (escalates()). An admin holds the full set, so it stays unrestricted. - users.ts: user:create/update reject assigning a role whose permissions exceed the caller's; update/password-reset/delete reject acting on a user whose current role exceeds the caller's (exceedsCaller()). The existing no-lockout + builtin-admin protections are unchanged. Verified: 10-assertion inject test — manager (role:* + user:* but no tariff:update, not admin) gets 403 on self-grant, minting a privileged role, assigning/resetting/deleting an admin; admin stays unrestricted; the manager can still create peers + in-scope roles (not over-blocked). Full build green. Claude-Session: https://claude.ai/code/session_01Xcm6ikLgGoCxxHrxtjkk5V
This commit is contained in:
@@ -2,7 +2,7 @@ import { randomUUID } from "node:crypto";
|
|||||||
import type { FastifyInstance } from "fastify";
|
import type { FastifyInstance } from "fastify";
|
||||||
import { eq, rolePermissions, roles, users, type Db } from "@parking/db";
|
import { eq, rolePermissions, roles, users, type Db } from "@parking/db";
|
||||||
import { ADMIN_ROLE_ID, PERMISSIONS, type Permission } from "@parking/shared";
|
import { ADMIN_ROLE_ID, PERMISSIONS, type Permission } from "@parking/shared";
|
||||||
import { bumpPermsCache, requirePermission } from "../auth.js";
|
import { bumpPermsCache, permissionsFor, requirePermission } from "../auth.js";
|
||||||
|
|
||||||
// Role management (admin). Roles are DATA: an admin composes a role from the
|
// Role management (admin). Roles are DATA: an admin composes a role from the
|
||||||
// code-defined PERMISSIONS grid (resource:action), and users are assigned one
|
// code-defined PERMISSIONS grid (resource:action), and users are assigned one
|
||||||
@@ -10,6 +10,13 @@ import { bumpPermsCache, requirePermission } from "../auth.js";
|
|||||||
// edited or deleted and always resolves to every permission in code. Every write
|
// edited or deleted and always resolves to every permission in code. Every write
|
||||||
// here bumps the in-memory permission cache so changes take effect on the next
|
// here bumps the in-memory permission cache so changes take effect on the next
|
||||||
// request. See @parking/shared PERMISSIONS and ../auth.ts.
|
// request. See @parking/shared PERMISSIONS and ../auth.ts.
|
||||||
|
//
|
||||||
|
// PRIVILEGE-ESCALATION GUARD: `role:update`/`role:create` must NOT let a caller
|
||||||
|
// grant a permission they don't themselves hold — otherwise a non-admin with
|
||||||
|
// `role:*` could edit their own role to add (say) `tariff:update`, or mint a role
|
||||||
|
// that grants admin-equivalent powers, and escalate. So a non-admin caller may
|
||||||
|
// only put permissions they ALREADY hold onto a role. An admin (full set) is
|
||||||
|
// unrestricted, which is the intended behaviour.
|
||||||
|
|
||||||
interface RoleBody {
|
interface RoleBody {
|
||||||
name: string;
|
name: string;
|
||||||
@@ -77,6 +84,15 @@ export async function roleRoutes(app: FastifyInstance, db: Db): Promise<void> {
|
|||||||
};
|
};
|
||||||
});
|
});
|
||||||
|
|
||||||
|
/** Reject any permission in `perms` the caller does not themselves hold — so a
|
||||||
|
* non-admin can't grant privileges beyond their own. Returns the offending
|
||||||
|
* permission, or null if all are within the caller's set. (Admin holds the full
|
||||||
|
* set, so it never trips.) */
|
||||||
|
function escalates(callerRoleId: string, perms: Permission[]): Permission | null {
|
||||||
|
const held = permissionsFor(callerRoleId);
|
||||||
|
return perms.find((p) => !held.has(p)) ?? null;
|
||||||
|
}
|
||||||
|
|
||||||
// Create a composable role from a name + a permission set.
|
// Create a composable role from a name + a permission set.
|
||||||
app.post<{ Body: RoleBody }>("/api/roles", { preHandler: createGuard }, async (req, reply) => {
|
app.post<{ Body: RoleBody }>("/api/roles", { preHandler: createGuard }, async (req, reply) => {
|
||||||
const name = (req.body?.name ?? "").trim();
|
const name = (req.body?.name ?? "").trim();
|
||||||
@@ -86,6 +102,8 @@ export async function roleRoutes(app: FastifyInstance, db: Db): Promise<void> {
|
|||||||
}
|
}
|
||||||
const cleaned = cleanPermissions(req.body?.permissions ?? []);
|
const cleaned = cleanPermissions(req.body?.permissions ?? []);
|
||||||
if (!cleaned.ok) return reply.code(400).send({ error: cleaned.bad });
|
if (!cleaned.ok) return reply.code(400).send({ error: cleaned.bad });
|
||||||
|
const over = escalates(req.user.roleId, cleaned.perms);
|
||||||
|
if (over) return reply.code(403).send({ error: `cannot grant a permission you do not hold: ${over}` });
|
||||||
|
|
||||||
const id = randomUUID();
|
const id = randomUUID();
|
||||||
db.insert(roles).values({ id, name, builtin: 0 }).run();
|
db.insert(roles).values({ id, name, builtin: 0 }).run();
|
||||||
@@ -116,6 +134,8 @@ export async function roleRoutes(app: FastifyInstance, db: Db): Promise<void> {
|
|||||||
if (req.body?.permissions != null) {
|
if (req.body?.permissions != null) {
|
||||||
const cleaned = cleanPermissions(req.body.permissions);
|
const cleaned = cleanPermissions(req.body.permissions);
|
||||||
if (!cleaned.ok) return reply.code(400).send({ error: cleaned.bad });
|
if (!cleaned.ok) return reply.code(400).send({ error: cleaned.bad });
|
||||||
|
const over = escalates(req.user.roleId, cleaned.perms);
|
||||||
|
if (over) return reply.code(403).send({ error: `cannot grant a permission you do not hold: ${over}` });
|
||||||
setPermissions(id, cleaned.perms);
|
setPermissions(id, cleaned.perms);
|
||||||
}
|
}
|
||||||
bumpPermsCache();
|
bumpPermsCache();
|
||||||
|
|||||||
@@ -3,7 +3,7 @@ import bcrypt from "bcrypt";
|
|||||||
import type { FastifyInstance } from "fastify";
|
import type { FastifyInstance } from "fastify";
|
||||||
import { eq, roles, users, type Db } from "@parking/db";
|
import { eq, roles, users, type Db } from "@parking/db";
|
||||||
import { ADMIN_ROLE_ID } from "@parking/shared";
|
import { ADMIN_ROLE_ID } from "@parking/shared";
|
||||||
import { requirePermission } from "../auth.js";
|
import { permissionsFor, requirePermission } from "../auth.js";
|
||||||
|
|
||||||
// User management (admin). Users are created/edited at runtime here — the
|
// User management (admin). Users are created/edited at runtime here — the
|
||||||
// install-time seed-admin.mjs only bootstraps the FIRST admin. Each user has one
|
// install-time seed-admin.mjs only bootstraps the FIRST admin. Each user has one
|
||||||
@@ -13,6 +13,13 @@ import { requirePermission } from "../auth.js";
|
|||||||
// NO-LOCKOUT INVARIANT: the app refuses to delete, or move off the `admin` role,
|
// NO-LOCKOUT INVARIANT: the app refuses to delete, or move off the `admin` role,
|
||||||
// the LAST user still holding `admin`. Administration can therefore never be
|
// the LAST user still holding `admin`. Administration can therefore never be
|
||||||
// locked out of the appliance. See wiki/entities/local-jwt-auth.md.
|
// locked out of the appliance. See wiki/entities/local-jwt-auth.md.
|
||||||
|
//
|
||||||
|
// PRIVILEGE-ESCALATION GUARD: a non-admin caller with `user:*` must NOT be able to
|
||||||
|
// (a) ASSIGN a role whose permissions exceed their own (e.g. hand themselves or a
|
||||||
|
// peer the admin role, or any role broader than theirs), nor (b) MODIFY a user who
|
||||||
|
// already holds a role broader than the caller's (resetting an admin's password is
|
||||||
|
// account takeover; deleting an admin is sabotage). Both are blocked below by
|
||||||
|
// comparing permission SETS. An admin holds the full set, so it is unrestricted.
|
||||||
|
|
||||||
interface CreateBody {
|
interface CreateBody {
|
||||||
username: string;
|
username: string;
|
||||||
@@ -51,6 +58,18 @@ export async function userRoutes(app: FastifyInstance, db: Db): Promise<void> {
|
|||||||
return { id: u.id, username: u.username, roleId: u.roleId, language: u.language, createdAt: u.createdAt };
|
return { id: u.id, username: u.username, roleId: u.roleId, language: u.language, createdAt: u.createdAt };
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/** True if `targetRoleId` grants any permission the caller's role does NOT hold,
|
||||||
|
* i.e. assigning or touching it would let the caller act beyond their own
|
||||||
|
* privileges. (Admin holds the full set, so it never trips.) */
|
||||||
|
function exceedsCaller(callerRoleId: string, targetRoleId: string): boolean {
|
||||||
|
if (callerRoleId === targetRoleId) return false;
|
||||||
|
const held = permissionsFor(callerRoleId);
|
||||||
|
for (const p of permissionsFor(targetRoleId)) {
|
||||||
|
if (!held.has(p)) return true;
|
||||||
|
}
|
||||||
|
return false;
|
||||||
|
}
|
||||||
|
|
||||||
// List all users (no password hashes) + their role names for display.
|
// List all users (no password hashes) + their role names for display.
|
||||||
app.get("/api/users", { preHandler: readGuard }, async () => {
|
app.get("/api/users", { preHandler: readGuard }, async () => {
|
||||||
const rows = db.select().from(users).all();
|
const rows = db.select().from(users).all();
|
||||||
@@ -75,6 +94,10 @@ export async function userRoutes(app: FastifyInstance, db: Db): Promise<void> {
|
|||||||
if (!db.select().from(roles).where(eq(roles.id, roleId)).get()) {
|
if (!db.select().from(roles).where(eq(roles.id, roleId)).get()) {
|
||||||
return reply.code(400).send({ error: "unknown roleId" });
|
return reply.code(400).send({ error: "unknown roleId" });
|
||||||
}
|
}
|
||||||
|
// No-escalation: can't create a user with a role broader than your own.
|
||||||
|
if (exceedsCaller(req.user.roleId, roleId)) {
|
||||||
|
return reply.code(403).send({ error: "cannot assign a role with permissions beyond your own" });
|
||||||
|
}
|
||||||
if (db.select().from(users).where(eq(users.username, username)).get()) {
|
if (db.select().from(users).where(eq(users.username, username)).get()) {
|
||||||
return reply.code(409).send({ error: "username already exists" });
|
return reply.code(409).send({ error: "username already exists" });
|
||||||
}
|
}
|
||||||
@@ -93,6 +116,10 @@ export async function userRoutes(app: FastifyInstance, db: Db): Promise<void> {
|
|||||||
const id = req.params.id;
|
const id = req.params.id;
|
||||||
const existing = db.select().from(users).where(eq(users.id, id)).get();
|
const existing = db.select().from(users).where(eq(users.id, id)).get();
|
||||||
if (!existing) return reply.code(404).send({ error: "user not found" });
|
if (!existing) return reply.code(404).send({ error: "user not found" });
|
||||||
|
// No-escalation: can't modify a user who already outranks you.
|
||||||
|
if (exceedsCaller(req.user.roleId, existing.roleId)) {
|
||||||
|
return reply.code(403).send({ error: "cannot modify a user whose role exceeds your own" });
|
||||||
|
}
|
||||||
|
|
||||||
const next: { username?: string; roleId?: string } = {};
|
const next: { username?: string; roleId?: string } = {};
|
||||||
if (req.body?.username != null) {
|
if (req.body?.username != null) {
|
||||||
@@ -107,6 +134,10 @@ export async function userRoutes(app: FastifyInstance, db: Db): Promise<void> {
|
|||||||
if (!db.select().from(roles).where(eq(roles.id, roleId)).get()) {
|
if (!db.select().from(roles).where(eq(roles.id, roleId)).get()) {
|
||||||
return reply.code(400).send({ error: "unknown roleId" });
|
return reply.code(400).send({ error: "unknown roleId" });
|
||||||
}
|
}
|
||||||
|
// No-escalation: can't promote a user into a role broader than your own.
|
||||||
|
if (exceedsCaller(req.user.roleId, roleId)) {
|
||||||
|
return reply.code(403).send({ error: "cannot assign a role with permissions beyond your own" });
|
||||||
|
}
|
||||||
// No-lockout: don't move the last admin off the admin role.
|
// No-lockout: don't move the last admin off the admin role.
|
||||||
if (roleId !== ADMIN_ROLE_ID && isLastAdmin(id)) {
|
if (roleId !== ADMIN_ROLE_ID && isLastAdmin(id)) {
|
||||||
return reply.code(409).send({ error: "cannot change the role of the last admin" });
|
return reply.code(409).send({ error: "cannot change the role of the last admin" });
|
||||||
@@ -127,9 +158,15 @@ export async function userRoutes(app: FastifyInstance, db: Db): Promise<void> {
|
|||||||
{ preHandler: updateGuard },
|
{ preHandler: updateGuard },
|
||||||
async (req, reply) => {
|
async (req, reply) => {
|
||||||
const id = req.params.id;
|
const id = req.params.id;
|
||||||
if (!db.select().from(users).where(eq(users.id, id)).get()) {
|
const target = db.select().from(users).where(eq(users.id, id)).get();
|
||||||
|
if (!target) {
|
||||||
return reply.code(404).send({ error: "user not found" });
|
return reply.code(404).send({ error: "user not found" });
|
||||||
}
|
}
|
||||||
|
// No-escalation: can't reset the password of a user who outranks you
|
||||||
|
// (that would be account takeover of a more-privileged account).
|
||||||
|
if (exceedsCaller(req.user.roleId, target.roleId)) {
|
||||||
|
return reply.code(403).send({ error: "cannot reset the password of a user whose role exceeds your own" });
|
||||||
|
}
|
||||||
const password = req.body?.password ?? "";
|
const password = req.body?.password ?? "";
|
||||||
if (password.length < MIN_PASSWORD) {
|
if (password.length < MIN_PASSWORD) {
|
||||||
return reply.code(400).send({ error: `password must be at least ${MIN_PASSWORD} characters` });
|
return reply.code(400).send({ error: `password must be at least ${MIN_PASSWORD} characters` });
|
||||||
@@ -146,9 +183,14 @@ export async function userRoutes(app: FastifyInstance, db: Db): Promise<void> {
|
|||||||
{ preHandler: deleteGuard },
|
{ preHandler: deleteGuard },
|
||||||
async (req, reply) => {
|
async (req, reply) => {
|
||||||
const id = req.params.id;
|
const id = req.params.id;
|
||||||
if (!db.select().from(users).where(eq(users.id, id)).get()) {
|
const target = db.select().from(users).where(eq(users.id, id)).get();
|
||||||
|
if (!target) {
|
||||||
return reply.code(404).send({ error: "user not found" });
|
return reply.code(404).send({ error: "user not found" });
|
||||||
}
|
}
|
||||||
|
// No-escalation: can't delete a user who outranks you.
|
||||||
|
if (exceedsCaller(req.user.roleId, target.roleId)) {
|
||||||
|
return reply.code(403).send({ error: "cannot delete a user whose role exceeds your own" });
|
||||||
|
}
|
||||||
if (isLastAdmin(id)) {
|
if (isLastAdmin(id)) {
|
||||||
return reply.code(409).send({ error: "cannot delete the last admin" });
|
return reply.code(409).send({ error: "cannot delete the last admin" });
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -37,6 +37,12 @@ Authentication and authorization, kept **fully local** — a direct consequence
|
|||||||
**last user holding admin** — administration can never be locked out of the appliance.
|
**last user holding admin** — administration can never be locked out of the appliance.
|
||||||
- `event:void` is a permission, NOT a ledger delete: the append-only signed chain is untouched; the
|
- `event:void` is a permission, NOT a ledger delete: the append-only signed chain is untouched; the
|
||||||
permission only gates who may APPEND a void event (there is no void API route yet — forward seam).
|
permission only gates who may APPEND a void event (there is no void API route yet — forward seam).
|
||||||
|
- **No privilege escalation through the RBAC system itself.** `role:create`/`role:update` and
|
||||||
|
`user:create`/`user:update` are themselves grantable, so a non-admin could otherwise self-escalate.
|
||||||
|
Guards (`routes/roles.ts`, `routes/users.ts`): a caller may only put permissions on a role that
|
||||||
|
they *already hold*, and may only assign/modify users whose role is a SUBSET of the caller's own
|
||||||
|
(so no minting a privileged role, handing out the admin role, or resetting/deleting a more-
|
||||||
|
privileged account). An admin holds the full set, so it is unrestricted — the intended behaviour.
|
||||||
|
|
||||||
## Cookie session (browser auth)
|
## Cookie session (browser auth)
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user