diff --git a/plugins/dbviewer/api/api.js b/plugins/dbviewer/api/api.js index 8ac9d1c731b..edb89723b59 100644 --- a/plugins/dbviewer/api/api.js +++ b/plugins/dbviewer/api/api.js @@ -15,11 +15,15 @@ const FEATURE_NAME = 'dbviewer'; // upper bound on rows returned per find()/aggregation page, to keep a crafted // limit/iDisplayLength from requesting an unbounded result set const MAX_DBVIEWER_LIMIT = 10000; -// Aggregation-stage allow-list and the recursive sanitizer that strips blocked -// stages at every depth (including inside $facet sub-pipelines). Kept in a -// dedicated module so it can be unit-tested in isolation. -const { sanitizeAggregation, ALLOWED_STAGES_USER, ALLOWED_STAGES_GLOBAL_ADMIN } = require('./parts/aggregation_guard.js'); -const { sanitizeProjection, escapeRegExp } = require('./parts/query_guard.js'); +// Allow-list of MongoDB operators and the validator that rejects a pipeline using +// anything outside it, at any depth. Kept in a dedicated module so it can be +// unit-tested in isolation. +const { sanitizeAggregation, ALLOWED_OPERATORS_USER, ALLOWED_OPERATORS_GLOBAL_ADMIN } = require('./parts/aggregation_guard.js'); +const { findDisallowedProjectionValue, escapeRegExp } = require('./parts/query_guard.js'); +// Fields the viewer must never return, and the helpers that remove them. Held in one +// module because the same redaction applies in three places (document read, +// collection read, aggregation) and a per-site list drifts. +const { hasRedactedFields, redactFields, redactionStage } = require('./parts/redaction.js'); var spawn = require('child_process').spawn, child; @@ -139,10 +143,8 @@ var spawn = require('child_process').spawn, } dbs[dbNameOnParam].collection(params.qstring.collection).findOne(docFilter, function(err, results) { if (!err) { - if (params.qstring.collection === 'members' && results) { - delete results.password; - delete results.api_key; - delete results.two_factor_auth; + if (hasRedactedFields(params.qstring.collection)) { + redactFields(params.qstring.collection, results); } else if (params.qstring.collection === 'auth_tokens' && results) { if (results._id) { @@ -238,10 +240,15 @@ var spawn = require('child_process').spawn, common.returnMessage(params, 400, common.unsafeQueryError(badOp)); return false; } - //restrict the projection to plain field include/exclude — drop any - //expression / field-path alias (e.g. {x:"$password"}) that could - //compute or rename fields the viewer otherwise removes - sanitizeProjection(projection); + //the projection must be plain field include/exclude. An expression or + //field-path alias (e.g. {x:"$password"}) could compute or rename fields + //the viewer otherwise removes, so reject rather than quietly dropping + //the field and returning different results than were asked for + var badProjection = findDisallowedProjectionValue(projection); + if (badProjection) { + common.returnMessage(params, 400, 'Projection for "' + badProjection.name + '" must be 0 or 1'); + return; + } var base_filter = {}; if (!params.member.global_admin) { @@ -284,10 +291,8 @@ var spawn = require('child_process').spawn, var stream = cursor.skip(skip).limit(limit).stream({ transform: function(doc) { - if (params.qstring.collection === 'members' && doc) { - delete doc.password; - delete doc.api_key; - delete doc.two_factor_auth; + if (hasRedactedFields(params.qstring.collection)) { + redactFields(params.qstring.collection, doc); } else if (params.qstring.collection === 'auth_tokens' && doc) { if (doc._id) { @@ -418,13 +423,14 @@ var spawn = require('child_process').spawn, aggregation.push({ "$limit": Math.min(iDisplayLength, MAX_DBVIEWER_LIMIT) }); } } - if (collection === 'members') { + var redaction = redactionStage(collection); + if (redaction) { // Insert the redaction as the very first stage so no // user-supplied stage — including a leading $match using // $expr, or a $project/$group that aliases or references the // field — can read the raw credential fields before they are // removed. - aggregation.splice(0, 0, {"$project": {"password": 0, "api_key": 0, "two_factor_auth": 0}}); + aggregation.splice(0, 0, redaction); } else if (collection === 'auth_tokens') { aggregation.splice(0, 0, {"$addFields": {"_id": "***redacted***"}}); @@ -573,26 +579,34 @@ var spawn = require('child_process').spawn, } // handle aggregation request else if (isContainDb && params.qstring.aggregation) { - // Validate the pipeline against the caller's allow-list and run - // it. Global admins get the broader list (may join/union, but - // still no writes and never into a redacted collection); other - // users get the restricted list. Disallowed stages are stripped; - // server-side-JS operators and joins into members/auth_tokens - // reject the request. - var runAggregation = function(allowedStages) { + // Validate the pipeline against the caller's allow-list of MongoDB + // operators and run it unchanged. Global admins get the broader + // list (they may join and union, but still not into a redacted + // collection); other users get the restricted one. Anything the + // caller may not use rejects the request rather than being removed + // from the pipeline, so the result is never quietly different from + // what was asked for. + var runAggregation = function(allowedOperators) { try { let aggregation = EJSON.parse(params.qstring.aggregation); - var guard = sanitizeAggregation(aggregation, allowedStages); + var guard = sanitizeAggregation(aggregation, allowedOperators); if (guard.error) { - var msg = guard.error.type === "join" - ? 'Aggregation may not join the "' + guard.error.name + '" collection' - : 'Aggregation may not use the "' + guard.error.name + '" operator'; + var msg; + if (guard.error.type === "join") { + msg = 'Aggregation may not join the "' + guard.error.name + '" collection'; + } + else if (guard.error.type === "stage") { + msg = 'Aggregation stage at ' + guard.error.where + ' does not name a pipeline stage'; + } + else { + msg = 'Aggregation may not use the "' + guard.error.name + '" operator (at ' + guard.error.where + ')'; + } common.returnMessage(params, 400, msg); return; } - if (guard.changes && Object.keys(guard.changes).length > 0) { - log.d("Removed stages from pipeline: ", JSON.stringify(guard.changes)); - } + //guard.changes is always empty now that nothing is stripped. It is + //still passed so the response keeps its "removed" field and the UI + //contract does not change aggregate(params.qstring.collection, aggregation, guard.changes); } catch (e) { @@ -600,12 +614,12 @@ var spawn = require('child_process').spawn, } }; if (params.member.global_admin) { - runAggregation(ALLOWED_STAGES_GLOBAL_ADMIN); + runAggregation(ALLOWED_OPERATORS_GLOBAL_ADMIN); } else { userHasAccess(params, params.qstring.collection, function(hasAccess) { if (hasAccess || params.qstring.collection === "events_data" || params.qstring.collection === "drill_events") { - runAggregation(ALLOWED_STAGES_USER); + runAggregation(ALLOWED_OPERATORS_USER); } else { common.returnMessage(params, 401, 'User does not have right tot view this colleciton'); diff --git a/plugins/dbviewer/api/parts/aggregation_guard.js b/plugins/dbviewer/api/parts/aggregation_guard.js index 0f83b196f2d..c7ee510a27b 100644 --- a/plugins/dbviewer/api/parts/aggregation_guard.js +++ b/plugins/dbviewer/api/parts/aggregation_guard.js @@ -1,104 +1,179 @@ /** * @module plugins/dbviewer/api/parts/aggregation_guard * @description Validates a DB Viewer aggregation pipeline against a role-specific - * allow-list of stages, and rejects pipelines that use server-side-JavaScript - * operators or join/union into a redacted (credential) collection. + * allow-list of MongoDB operators, and rejects pipelines that join or union into a + * redacted (credential) collection. * - * One routine, two lists: - * - non-global users get ALLOWED_STAGES_USER (no joins, no writes); - * - global admins get ALLOWED_STAGES_GLOBAL_ADMIN (the same plus join/union - * stages). - * Anything not in the applicable list is stripped from the pipeline at every - * depth. Two hard rules apply to everyone, at any depth, and reject the request: - * - no $function / $accumulator / $where (server-side JavaScript); - * - no join/union into members / auth_tokens (their field redaction only - * applies to the top-level source collection, so a join would return raw - * credentials — denied even for global admins). + * HOW THIS WORKS, AND WHY IT IS SHAPED THIS WAY + * + * The pipeline is walked blindly: every object and array, at every depth, with no + * attempt to work out which arrays are sub-pipelines and which are ordinary + * expression arrays. Only keys are examined, and only keys beginning with "$". + * Any such key that is not on the allow-list rejects the whole request. + * + * Two properties make that both safe and complete: + * + * - MongoDB will not let a "$"-prefixed key be anything other than an operator. + * A document may store a field named "$price", but it can only be read through + * $getField, where the name is a VALUE. {$match:{"$price":5}} is an error + * ("unknown top level operator"), as is projecting or sorting on it. So every + * "$"-prefixed key in a valid pipeline is an operator, and there is no + * legitimate query this rejects. + * - A stage only executes if its name appears as a literal key in the submitted + * pipeline. Computed values never become stages: an object built with $setField + * whose key is "$lookup" is data, and joins nothing. So checking literal keys + * catches everything that can run. + * + * KEYS ONLY, NEVER VALUES. {$literal:"$lookup"} is a legitimate expression + * returning the string "$lookup", and "$fieldname" / "$$ROOT" are ordinary value + * references. Inspecting values would reject valid queries. + * + * EXACT MATCH ONLY. $mergeObjects is legitimate and must not be caught by a prefix + * or substring test for $merge. + * + * WHY AN ALLOW-LIST RATHER THAN A LIST OF DANGEROUS OPERATORS + * + * An omission here rejects a valid query, which is a support ticket. An omission + * from a list of dangerous operators permits an exfiltration, which is a + * vulnerability. Those costs are not equal, so this fails closed: an operator + * introduced by a future MongoDB release is rejected until reviewed and added. + * + * The previous two versions of this guard both failed by trying to identify + * sub-pipelines from their contents, which let one unrecognised sibling stage hide + * a $lookup from the stage filter. Nothing here infers structure. If a future + * change reintroduces inference, make it fail closed. + * + * MAINTENANCE. The lists below were verified against a live MongoDB 8.0 by probing + * every entry; verify_operators.js in this directory is the means of re-verifying + * them. Run it on a MongoDB upgrade: operators added by the new release will be + * rejected until added here, and entries that no longer exist should be removed. + * That probe found a real misspelling in the previous version of this file, + * "$sharedDataDistribution" for "$shardedDataDistribution", so do not hand-edit + * these lists without re-running it. + * + * DELIBERATELY ABSENT, and therefore rejected: + * - joins and unions for non-global users: $lookup, $graphLookup, $unionWith + * (they read a second collection, bypassing the per-collection access check) + * - writes: $out, $merge + * - server-side JavaScript: $function, $accumulator, $where + * - server, cluster and internal introspection: $currentOp, $collStats, + * $indexStats, $planCacheStats, $listSessions, $listLocalSessions, + * $listSampledQueries, $listSearchIndexes, $shardedDataDistribution, + * $changeStream, $changeStreamSplitLargeEvents, $mergeCursors, $documents, + * $listClusterCatalog, and every $_internal* stage */ 'use strict'; -// Stages a non-global user may run. No joins/unions (they read a second -// collection, bypassing the per-collection access check) and no writes. -const ALLOWED_STAGES_USER = { - "$addFields": true, - "$bucket": true, - "$bucketAuto": true, - "$count": true, - "$densify": true, - "$facet": true, - "$fill": true, - "$geoNear": true, - "$group": true, - "$limit": true, - "$match": true, - "$project": true, - "$querySettings": true, - "$redact": true, - "$replaceRoot": true, - "$replaceWith": true, - "$sample": true, - "$search": true, - "$searchMeta": true, - "$set": true, - "$setWindowFields": true, - "$skip": true, - "$sort": true, - "$sortByCount": true, - "$unset": true, - "$unwind": true, - "$vectorSearch": true //atlas specific -}; +// Pipeline stages a non-global user may run. Unchanged from the previous version of +// this guard, so this is not a widening of what anyone can do. +const STAGES_USER = [ + "$addFields", "$bucket", "$bucketAuto", "$count", "$densify", "$facet", + "$fill", "$geoNear", "$group", "$limit", "$match", "$project", + "$querySettings", "$redact", "$replaceRoot", "$replaceWith", "$sample", + "$search", "$searchMeta", "$set", "$setWindowFields", "$skip", "$sort", + "$sortByCount", "$unset", "$unwind", "$vectorSearch" +]; -// Global admins may additionally use the join/union stages. Still no write -// stages, and still never into a protected collection (see findHardViolation). -const ALLOWED_STAGES_GLOBAL_ADMIN = Object.assign({}, ALLOWED_STAGES_USER, { - "$lookup": true, - "$graphLookup": true, - "$unionWith": true -}); +// Join and union stages, for global admins only. Still never into a protected +// collection: see findProtectedJoin below. +const STAGES_GLOBAL_ADMIN_ONLY = ["$lookup", "$graphLookup", "$unionWith"]; -// Expression operators that run server-side JavaScript. Never allowed for -// anyone, at any depth — they live inside otherwise-allowed stages. -const DENIED_OPERATORS = { - "$function": true, - "$accumulator": true, - "$where": true -}; +// Expression operators, accumulators and window operators. These live inside +// stages rather than being stages, and a blind walk meets them constantly, so they +// have to be listed or every real query is rejected. +const EXPRESSION_OPERATORS = [ + // arithmetic + "$abs", "$add", "$ceil", "$divide", "$exp", "$floor", "$ln", "$log", + "$log10", "$mod", "$multiply", "$pow", "$round", "$sqrt", "$subtract", + "$trunc", + // array + "$arrayElemAt", "$arrayToObject", "$concatArrays", "$filter", "$first", + "$firstN", "$in", "$indexOfArray", "$isArray", "$last", "$lastN", "$map", + "$maxN", "$minN", "$objectToArray", "$range", "$reduce", "$reverseArray", + "$size", "$slice", "$sortArray", "$zip", + // boolean and comparison + "$and", "$not", "$or", "$cmp", "$eq", "$gt", "$gte", "$lt", "$lte", "$ne", + // conditional + "$cond", "$ifNull", "$switch", + // data size + "$binarySize", "$bsonSize", + // date + "$dateAdd", "$dateDiff", "$dateFromParts", "$dateFromString", + "$dateSubtract", "$dateToParts", "$dateToString", "$dateTrunc", + "$dayOfMonth", "$dayOfWeek", "$dayOfYear", "$hour", "$isoDayOfWeek", + "$isoWeek", "$isoWeekYear", "$millisecond", "$minute", "$month", "$second", + "$week", "$year", "$tsIncrement", "$tsSecond", + // literal, object, variable and misc + "$literal", "$getField", "$rand", "$setField", "$unsetField", + "$mergeObjects", "$let", + // set + "$allElementsTrue", "$anyElementTrue", "$setDifference", "$setEquals", + "$setIntersection", "$setIsSubset", "$setUnion", + // string + "$concat", "$indexOfBytes", "$indexOfCP", "$ltrim", "$regexFind", + "$regexFindAll", "$regexMatch", "$replaceOne", "$replaceAll", "$rtrim", + "$split", "$strLenBytes", "$strLenCP", "$strcasecmp", "$substr", + "$substrBytes", "$substrCP", "$toLower", "$trim", "$toUpper", + // text score + "$meta", + // trigonometry + "$sin", "$cos", "$tan", "$asin", "$acos", "$atan", "$atan2", "$asinh", + "$acosh", "$atanh", "$sinh", "$cosh", "$tanh", "$degreesToRadians", + "$radiansToDegrees", + // type conversion + "$convert", "$isNumber", "$toBool", "$toDate", "$toDecimal", "$toDouble", + "$toInt", "$toLong", "$toObjectId", "$toString", "$type", "$toUUID", + // accumulators, valid in $group and/or $setWindowFields + "$addToSet", "$avg", "$bottom", "$bottomN", "$covariancePop", + "$covarianceSamp", "$denseRank", "$derivative", "$documentNumber", + "$expMovingAvg", "$integral", "$linearFill", "$locf", "$max", "$median", + "$min", "$percentile", "$push", "$rank", "$shift", "$stdDevPop", + "$stdDevSamp", "$sum", "$top", "$topN" +]; -// Collections whose contents DB Viewer redacts. A join/union into them would -// return the raw documents (redaction only applies to the top-level source), -// so such joins are rejected for everyone — including global admins. +// Query operators, reachable through $match. $where is absent on purpose: it runs +// server-side JavaScript. +const QUERY_OPERATORS = [ + "$eq", "$gt", "$gte", "$in", "$lt", "$lte", "$ne", "$nin", + "$and", "$not", "$nor", "$or", + "$exists", "$type", "$expr", "$jsonSchema", "$mod", "$regex", "$options", + "$text", "$geoIntersects", "$geoWithin", "$near", "$nearSphere", + "$all", "$elemMatch", "$size", + "$bitsAllClear", "$bitsAllSet", "$bitsAnyClear", "$bitsAnySet", + "$comment" +]; + +/** + * Build a lookup object from operator name lists. + * @returns {object} map of operator name to true + */ +function toMap() { + var map = {}; + for (var i = 0; i < arguments.length; i++) { + var list = arguments[i]; + for (var j = 0; j < list.length; j++) { + map[list[j]] = true; + } + } + return map; +} + +const ALLOWED_OPERATORS_USER = toMap(STAGES_USER, EXPRESSION_OPERATORS, QUERY_OPERATORS); +const ALLOWED_OPERATORS_GLOBAL_ADMIN = toMap(STAGES_USER, STAGES_GLOBAL_ADMIN_ONLY, EXPRESSION_OPERATORS, QUERY_OPERATORS); + +// Collections whose contents DB Viewer redacts. A join or union into them would +// return the raw documents, because the redaction only applies to the top-level +// source collection, so those are rejected for everyone including global admins. const PROTECTED_JOIN_COLLECTIONS = { "members": true, - "auth_tokens": true + "auth_tokens": true, + "password_reset": true }; -// All recognized aggregation STAGE operators (any role's allow-list plus the -// known blocked stages), used to recognize a nested array as a sub-pipeline -// (array of stage objects) and tell it apart from an ordinary expression array. -const KNOWN_STAGE_OPERATORS = Object.assign({ - "$out": true, - "$merge": true, - "$documents": true, - "$collStats": true, - "$indexStats": true, - "$currentOp": true, - "$listSessions": true, - "$listLocalSessions": true, - "$listSampledQueries": true, - "$listSearchIndexes": true, - "$planCacheStats": true, - "$changeStream": true, - "$changeStreamSplitLargeEvents": true, - "$mergeCursors": true, - "$sharedDataDistribution": true -}, ALLOWED_STAGES_GLOBAL_ADMIN); - /** - * Extract the collection name from a join "from"/"coll" reference, which may be - * a plain string ("members") or the cross-database object form - * ({ db, coll }). Returns [] when no collection name can be determined. + * Extract the collection name from a join "from"/"coll" reference, which may be a + * plain string ("members") or the cross-database object form ({ db, coll }). * @param {*} from - a $lookup.from / $graphLookup.from / $unionWith.coll value * @returns {string[]} the referenced collection name(s) */ @@ -113,7 +188,7 @@ function collectionOf(from) { } /** - * Collection names a single stage joins / unions from. + * Collection names a single stage joins or unions from. * @param {object} stage - one aggregation stage * @returns {string[]} target collection names referenced by join/union operators */ @@ -140,47 +215,16 @@ function joinTargetsOf(stage) { } /** - * Does this array look like an aggregation sub-pipeline — a non-empty array - * whose every element is a stage object (an object carrying a recognized stage - * operator key)? Expression arrays (e.g. $concat operands) do not match. - * @param {*} arr - candidate value - * @returns {boolean} true if it is a sub-pipeline - */ -function isSubPipeline(arr) { - if (!Array.isArray(arr) || arr.length === 0) { - return false; - } - for (var i = 0; i < arr.length; i++) { - var el = arr[i]; - if (!el || typeof el !== "object" || Array.isArray(el)) { - return false; - } - var isStage = false; - for (var k in el) { - if (Object.prototype.hasOwnProperty.call(el, k) && KNOWN_STAGE_OPERATORS[k] === true) { - isStage = true; - break; - } - } - if (!isStage) { - return false; - } - } - return true; -} - -/** - * Deep-walk a node for a hard-rule violation that must reject the whole request, - * regardless of role: a server-side-JS operator, or a join/union into a - * protected (redacted) collection. Detection-only (no mutation), so a blanket - * deep walk is safe and stays correct for any (incl. future) nested shape. - * @param {*} node - pipeline / stage / expression node - * @returns {{type: string, name: string}|null} the violation, or null + * Deep-scan for a join or union into a protected collection, at any depth. This one + * inspects VALUES (the "from"/"coll" names) rather than keys, so it stays a + * separate pass from the operator check. + * @param {*} node - pipeline / stage / expression node (not mutated) + * @returns {object|null} { name } of the protected collection, or null */ -function findHardViolation(node) { +function findProtectedJoin(node) { if (Array.isArray(node)) { for (var i = 0; i < node.length; i++) { - var inArr = findHardViolation(node[i]); + var inArr = findProtectedJoin(node[i]); if (inArr) { return inArr; } @@ -191,17 +235,14 @@ function findHardViolation(node) { var targets = joinTargetsOf(node); for (var t = 0; t < targets.length; t++) { if (PROTECTED_JOIN_COLLECTIONS[targets[t]] === true) { - return { type: "join", name: targets[t] }; + return { name: targets[t] }; } } for (var key in node) { if (!Object.prototype.hasOwnProperty.call(node, key)) { continue; } - if (DENIED_OPERATORS[key] === true) { - return { type: "operator", name: key }; - } - var inVal = findHardViolation(node[key]); + var inVal = findProtectedJoin(node[key]); if (inVal) { return inVal; } @@ -211,112 +252,94 @@ function findHardViolation(node) { } /** - * Walk a kept stage's value and strip disallowed stages from any sub-pipeline - * nested anywhere within it (structural — $facet branches, a .pipeline field, - * or any other nested-pipeline shape). A sub-pipeline emptied by stripping is - * removed from its parent object (Mongo rejects an empty $facet branch). - * @param {*} value - the stage value (or any nested node) - * @param {object} allowedStages - the role's allow-list - * @param {object} changes - accumulator: keys are removed stage names - * @returns {void} + * Deep-scan for a "$"-prefixed KEY that is not on the allow-list. + * + * Blind traversal: no assumption about which arrays are sub-pipelines. Keys only, + * never values. Exact match, so $mergeObjects is not confused with $merge. + * + * @param {*} node - pipeline / stage / expression node (not mutated) + * @param {object} allowedOperators - allow-list for the caller's role + * @param {string} path - position of the current node, for the error message + * @returns {object|null} { name, where } of the first offending key, or null */ -function stripNested(value, allowedStages, changes) { - if (Array.isArray(value)) { - if (isSubPipeline(value)) { - stripStages(value, allowedStages, changes); - } - else { - for (var i = 0; i < value.length; i++) { - stripNested(value[i], allowedStages, changes); +function findDisallowedOperator(node, allowedOperators, path) { + if (Array.isArray(node)) { + for (var i = 0; i < node.length; i++) { + var inArr = findDisallowedOperator(node[i], allowedOperators, path + "[" + i + "]"); + if (inArr) { + return inArr; } } - return; + return null; } - if (value && typeof value === "object") { - for (var k in value) { - if (!Object.prototype.hasOwnProperty.call(value, k)) { + if (node && typeof node === "object") { + for (var key in node) { + if (!Object.prototype.hasOwnProperty.call(node, key)) { continue; } - var child = value[k]; - if (Array.isArray(child) && isSubPipeline(child)) { - stripStages(child, allowedStages, changes); - if (child.length === 0) { - delete value[k]; - } + // require an explicit `true` so inherited Object.prototype keys + // (constructor, __proto__, ...) are never treated as allow-listed + if (key.charAt(0) === "$" && allowedOperators[key] !== true) { + return { name: key, where: path + "." + key }; } - else { - stripNested(child, allowedStages, changes); + var inVal = findDisallowedOperator(node[key], allowedOperators, path + "." + key); + if (inVal) { + return inVal; } } } + return null; } /** - * Recursively remove stages not in `allowedStages` from a pipeline, at every - * depth. Mutates the pipeline in place. - * @param {Array} pipeline - aggregation pipeline - * @param {object} allowedStages - the role's allow-list (values must be === true) - * @param {object} changes - accumulator: keys are removed stage names - * @returns {void} + * Validate an aggregation pipeline. The pipeline is never modified: one that uses + * something it may not use is rejected, so the caller either runs exactly what was + * asked for or gets an error explaining why not. + * + * @param {Array} pipeline - parsed aggregation pipeline (not mutated) + * @param {object} allowedOperators - ALLOWED_OPERATORS_USER or _GLOBAL_ADMIN + * @returns {{changes: object, error: ({type: string, name: string, where: string}|null)}} + * When error is set the caller must reject the request. changes is always + * empty and is kept only so the response shape does not change. */ -function stripStages(pipeline, allowedStages, changes) { - if (!Array.isArray(pipeline)) { - return; +function sanitizeAggregation(pipeline, allowedOperators) { + var join = findProtectedJoin(pipeline); + if (join) { + return { changes: {}, error: { type: "join", name: join.name } }; } - for (var z = 0; z < pipeline.length; z++) { - var stage = pipeline[z]; - if (!stage || typeof stage !== "object") { - continue; - } - for (var key in stage) { - if (!Object.prototype.hasOwnProperty.call(stage, key)) { - continue; - } - // require an explicit `true` so inherited Object.prototype keys - // (constructor, __proto__, …) are never treated as allow-listed - if (allowedStages[key] !== true) { - changes[key] = true; - delete stage[key]; + var operator = findDisallowedOperator(pipeline, allowedOperators, "pipeline"); + if (operator) { + return { changes: {}, error: { type: "operator", name: operator.name, where: operator.where } }; + } + // Top-level elements are stages by position, so each must actually name one. + // This is not structural inference: it only says that a stage object carries a + // stage operator. It catches an element whose only keys are ordinary names + // ("constructor", "__proto__", a stray field) with a clear message, rather than + // handing it to MongoDB to reject. + if (Array.isArray(pipeline)) { + for (var i = 0; i < pipeline.length; i++) { + var stage = pipeline[i]; + if (!stage || typeof stage !== "object" || Array.isArray(stage)) { + return { changes: {}, error: { type: "stage", name: String(stage), where: "pipeline[" + i + "]" } }; } - else { - stripNested(stage[key], allowedStages, changes); - var v = stage[key]; - if (v && typeof v === "object" && !Array.isArray(v) && Object.keys(v).length === 0) { - delete stage[key]; + var named = false; + for (var key in stage) { + if (Object.prototype.hasOwnProperty.call(stage, key) && key.charAt(0) === "$") { + named = true; + break; } } + if (!named) { + return { changes: {}, error: { type: "stage", name: Object.keys(stage).join(","), where: "pipeline[" + i + "]" } }; + } } - if (Object.keys(stage).length === 0) { - pipeline.splice(z, 1); - z--; - } - } -} - -/** - * Validate and sanitize an aggregation pipeline for a given role's allow-list. - * Rejects (without mutating) when a hard rule is violated; otherwise strips any - * stage not in the allow-list and returns what was removed. - * @param {Array} pipeline - aggregation pipeline (mutated in place when valid) - * @param {object} allowedStages - ALLOWED_STAGES_USER or ALLOWED_STAGES_GLOBAL_ADMIN - * @returns {{changes: object, error: ({type: string, name: string}|null)}} - * When error is set the caller must reject the request and not run the - * pipeline. Otherwise changes lists the removed stage names. - */ -function sanitizeAggregation(pipeline, allowedStages) { - var violation = findHardViolation(pipeline); - if (violation) { - return { changes: {}, error: violation }; } - var changes = {}; - stripStages(pipeline, allowedStages, changes); - return { changes: changes, error: null }; + return { changes: {}, error: null }; } module.exports = { - ALLOWED_STAGES_USER, - ALLOWED_STAGES_GLOBAL_ADMIN, - DENIED_OPERATORS, + ALLOWED_OPERATORS_USER, + ALLOWED_OPERATORS_GLOBAL_ADMIN, PROTECTED_JOIN_COLLECTIONS, sanitizeAggregation }; diff --git a/plugins/dbviewer/api/parts/query_guard.js b/plugins/dbviewer/api/parts/query_guard.js index db0bbd85dd8..aa2359cafd3 100644 --- a/plugins/dbviewer/api/parts/query_guard.js +++ b/plugins/dbviewer/api/parts/query_guard.js @@ -7,36 +7,39 @@ 'use strict'; /** - * Restrict a find() projection to plain field inclusion / exclusion. + * Check that a find() projection is plain field inclusion / exclusion. * - * A projection value is only allowed to be 0, 1 or a boolean (strict - * include/exclude). Any other value is dropped: + * A projection value may only be 0, 1 or a boolean (strict include/exclude). + * Anything else rejects the request: * - expressions and field-path aliases — e.g. { leak: "$password" } or - * { x: { $function: ... } } — would compute new fields from, or rename, - * fields the viewer otherwise removes from the response (MongoDB 4.4+ find() + * { x: { $function: ... } } — would compute new fields from, or rename, fields + * the viewer otherwise removes from the response (MongoDB 4.4+ find() * projections accept expressions); - * - other numbers (2, NaN, …) are not valid include/exclude values and can - * make the query throw. - * Keeping projections to strict include/exclude removes that whole avenue. + * - other numbers (2, NaN, …) are not valid include/exclude values and can make + * the query throw. * - * @param {object} projection - parsed projection object (mutated in place) - * @returns {object} changes - keys are the projection fields that were dropped + * The offending field used to be deleted from the projection and the query run + * anyway, which meant a caller asking for something they may not have silently got + * different results instead of being told. The projection is now left untouched and + * the caller is rejected, matching how the aggregation guard behaves. + * + * @param {object} projection - parsed projection object (not mutated) + * @returns {object|null} { name } of the first offending field, or null when the + * projection is acceptable */ -function sanitizeProjection(projection) { - var changes = {}; +function findDisallowedProjectionValue(projection) { if (!projection || typeof projection !== "object" || Array.isArray(projection)) { - return changes; + return null; } for (var key in projection) { if (Object.prototype.hasOwnProperty.call(projection, key)) { var value = projection[key]; if (value !== 0 && value !== 1 && value !== true && value !== false) { - changes[key] = true; - delete projection[key]; + return { name: key }; } } } - return changes; + return null; } /** @@ -52,6 +55,6 @@ function escapeRegExp(str) { } module.exports = { - sanitizeProjection, + findDisallowedProjectionValue, escapeRegExp }; diff --git a/plugins/dbviewer/api/parts/redaction.js b/plugins/dbviewer/api/parts/redaction.js new file mode 100644 index 00000000000..cf96b7a577c --- /dev/null +++ b/plugins/dbviewer/api/parts/redaction.js @@ -0,0 +1,81 @@ +/** + * @module plugins/dbviewer/api/parts/redaction + * @description Fields DB Viewer must never return, and the helpers that remove them. + * + * A field belongs here when holding it is enough on its own to act as somebody else: + * - members.password / api_key / two_factor_auth authenticate a dashboard user; + * - password_reset.prid is the password-reset token. The reset route looks it up + * directly as password_reset.findOne({prid}), so the value is the reset link. + * + * The same redaction has to be applied in three places (single-document read, + * collection read, aggregation). Keeping the list here rather than at each call site + * is deliberate: a per-site copy drifts, and a field missed at one of the three is a + * field that leaks. Adding a collection here covers all three. + * + * auth_tokens is deliberately absent. Its secret is the _id itself, which cannot be + * dropped without breaking the row, so the viewer replaces the value instead. That + * stays at the call sites. + */ + +'use strict'; + +const REDACTED_FIELDS = Object.freeze({ + members: Object.freeze(["password", "api_key", "two_factor_auth"]), + password_reset: Object.freeze(["prid"]) +}); + +/** + * Whether a collection has fields that must be withheld. + * + * @param {string} collection - collection name + * @returns {boolean} true when the collection has redacted fields + */ +function hasRedactedFields(collection) { + return Object.prototype.hasOwnProperty.call(REDACTED_FIELDS, collection); +} + +/** + * Remove a collection's redacted fields from a document, in place. + * + * @param {string} collection - collection the document came from + * @param {object} doc - document to redact (mutated); a falsy doc is returned as is + * @returns {object} the same document + */ +function redactFields(collection, doc) { + if (!doc || !hasRedactedFields(collection)) { + return doc; + } + var fields = REDACTED_FIELDS[collection]; + for (var i = 0; i < fields.length; i++) { + delete doc[fields[i]]; + } + return doc; +} + +/** + * Build a $project stage excluding a collection's redacted fields. + * + * The caller must splice this in as the very first stage, so no user-supplied stage + * can read the raw values before they are removed. + * + * @param {string} collection - collection being aggregated + * @returns {object|null} the stage, or null when the collection has none + */ +function redactionStage(collection) { + if (!hasRedactedFields(collection)) { + return null; + } + var fields = REDACTED_FIELDS[collection]; + var projection = {}; + for (var i = 0; i < fields.length; i++) { + projection[fields[i]] = 0; + } + return { "$project": projection }; +} + +module.exports = { + REDACTED_FIELDS, + hasRedactedFields, + redactFields, + redactionStage +}; diff --git a/plugins/dbviewer/api/parts/verify_operators.js b/plugins/dbviewer/api/parts/verify_operators.js new file mode 100644 index 00000000000..b9e8d46c2b6 --- /dev/null +++ b/plugins/dbviewer/api/parts/verify_operators.js @@ -0,0 +1,166 @@ +/** + * Verifies the operator allow-lists in aggregation_guard.js against a running + * MongoDB, so they are not maintained by hand and by hope. + * + * WHY THIS EXISTS + * The guard rejects any "$"-prefixed key it does not recognise. That fails safe, + * but it means a missing entry rejects a valid query, and a misspelled entry is + * both useless and invisible. The previous version of the guard carried + * "$sharedDataDistribution" where MongoDB spells it "$shardedDataDistribution"; + * this script is what found that. + * + * RUN IT on a MongoDB upgrade, and whenever the lists are edited: + * + * mongosh "" --quiet --file verify_operators.js + * + * It reports entries MongoDB does not recognise (remove or fix those) and + * confirms the deliberately excluded operators still exist (so the exclusions are + * still meaningful). It does not and cannot report operators a new MongoDB release + * has ADDED: those surface as a rejected query, which is the safe direction, and + * are added here after review. + * + * HOW THE PROBE WORKS + * Every probe is bounded with maxTimeMS and never iterates a cursor. Both matter: + * $changeStream opens a tailable cursor, so iterating it blocks forever. An + * unrecognised name is identified by MongoDB's own error code rather than by + * message text - Location40324 for a stage, Location31325 for an expression, + * code 15952 for a $group accumulator - because the wording differs between + * "Unrecognized pipeline stage name" and "Unknown expression" and is not stable. + * + * Each category has a control: a name that must be reported fake and one that must + * be reported real. If a control fails, the discriminator is wrong and the run's + * output means nothing. + */ + +/* eslint-disable no-undef */ +'use strict'; + +var probeDb = db.getSiblingDB("dbviewer_operator_probe"); +probeDb.probe.insertOne({ a: 1, arr: [1, 2], s: "x" }); + +/** + * Run a pipeline, bounded, without iterating a cursor. + * @param {Array} pipeline - pipeline to attempt + * @returns {object} { ok, code } + */ +function attempt(pipeline) { + try { + var res = probeDb.runCommand({ aggregate: "probe", pipeline: pipeline, cursor: {}, maxTimeMS: 400 }); + return { ok: res.ok === 1, code: res.code }; + } + catch (e) { + return { ok: false, code: e.code }; + } +} + +/** + * Whether MongoDB recognises a pipeline stage name. + * @param {string} op - operator name + * @returns {boolean} true when recognised + */ +function stageExists(op) { + var stage = {}; + stage[op] = {}; + var r = attempt([stage]); + return r.ok || r.code !== 40324; +} + +/** + * Whether MongoDB recognises an expression operator, in $project or as a $group + * accumulator (several are only valid in one position). + * @param {string} op - operator name + * @returns {boolean} true when recognised + */ +function expressionExists(op) { + var inner = {}; + inner[op] = []; + var r = attempt([{ $project: { x: inner } }]); + if (r.ok || r.code !== 31325) { + return true; + } + var acc = {}; + acc[op] = "$a"; + var g = attempt([{ $group: { _id: null, v: acc } }]); + return g.ok || g.code !== 15952; +} + +/** + * Whether MongoDB recognises a query operator. + * @param {string} op - operator name + * @returns {boolean} true when recognised + */ +function queryExists(op) { + var candidates = [{ f: {} }, {}]; + candidates[0].f[op] = 1; + candidates[1][op] = 1; + for (var i = 0; i < candidates.length; i++) { + try { + probeDb.runCommand({ find: "probe", filter: candidates[i], limit: 1, maxTimeMS: 400 }); + return true; + } + catch (e) { + if (!/unknown operator|unknown top level operator/i.test(e.message)) { + return true; + } + } + } + return false; +} + +var guard = { ALLOWED_OPERATORS_USER: {}, ALLOWED_OPERATORS_GLOBAL_ADMIN: {} }; +try { + guard = require("./aggregation_guard.js"); +} +catch (e) { + print("Could not require aggregation_guard.js (" + e.message + ")."); + print("Run this from plugins/dbviewer/api/parts, or paste the lists in manually."); +} + +var allowed = Object.keys(guard.ALLOWED_OPERATORS_GLOBAL_ADMIN || {}); + +// Operators the guard deliberately omits. Confirming these still exist keeps the +// exclusions meaningful: an exclusion of something MongoDB no longer has is dead +// weight, and one that was never real (a typo) is a hole. +var EXCLUDED = [ + "$out", "$merge", "$function", "$accumulator", "$where", + "$currentOp", "$collStats", "$indexStats", "$planCacheStats", + "$listSessions", "$listLocalSessions", "$listSampledQueries", + "$listSearchIndexes", "$shardedDataDistribution", "$mergeCursors", + "$documents", "$listClusterCatalog" +]; + +/** + * Whether MongoDB recognises a name in any position. + * @param {string} op - operator name + * @returns {boolean} true when recognised anywhere + */ +function existsAnywhere(op) { + return stageExists(op) || expressionExists(op) || queryExists(op); +} + +var controlsOk = + !existsAnywhere("$notARealOperatorAtAll") + && stageExists("$match") + && expressionExists("$add") + && queryExists("$gt"); + +print("controls " + (controlsOk ? "passed" : "FAILED - the results below are meaningless")); + +var unrecognised = allowed.filter(function(op) { + return !existsAnywhere(op); +}); +print("allow-listed operators checked: " + allowed.length); +print("NOT recognised by this MongoDB (fix or remove): " + (unrecognised.length ? unrecognised.join(" ") : "(none)")); + +var goneExclusions = EXCLUDED.filter(function(op) { + return !existsAnywhere(op); +}); +print("excluded operators checked: " + EXCLUDED.length); +print("excluded but NOT recognised (typo, or gone from this version): " + (goneExclusions.length ? goneExclusions.join(" ") : "(none)")); + +var wronglyAllowed = EXCLUDED.filter(function(op) { + return (guard.ALLOWED_OPERATORS_GLOBAL_ADMIN || {})[op] === true; +}); +print("excluded operators present in the allow-list (must be none): " + (wronglyAllowed.length ? wronglyAllowed.join(" ") : "(none)")); + +probeDb.dropDatabase(); diff --git a/test/unit-tests/plugins.dbviewer.aggregation-guard.js b/test/unit-tests/plugins.dbviewer.aggregation-guard.js index 541ecaaa3f5..f6f58c563ff 100644 --- a/test/unit-tests/plugins.dbviewer.aggregation-guard.js +++ b/test/unit-tests/plugins.dbviewer.aggregation-guard.js @@ -1,78 +1,145 @@ require("should"); var guard = require("../../plugins/dbviewer/api/parts/aggregation_guard.js"); -var USER = guard.ALLOWED_STAGES_USER; -var ADMIN = guard.ALLOWED_STAGES_GLOBAL_ADMIN; +var USER = guard.ALLOWED_OPERATORS_USER; +var ADMIN = guard.ALLOWED_OPERATORS_GLOBAL_ADMIN; -// sanitizeAggregation(pipeline, allowedStages) -> { changes, error } -// - strips stages not in the role's allow-list (recursively, at every depth) -// - rejects (error) server-side-JS operators and joins into redacted -// collections, for any role, at any depth +// sanitizeAggregation(pipeline, allowedOperators) -> { changes, error } +// +// The pipeline is never modified. Every "$"-prefixed KEY, at any depth, must be on +// the role's allow-list, or the request is rejected. Values are never inspected, +// and matching is exact. +// +// The guard used to strip disallowed stages instead, which required guessing which +// nested arrays were sub-pipelines. That guess is what the reported bypass defeated: +// one unrecognised sibling stage made a whole branch invisible to the filter. describe("dbviewer aggregation guard", function() { - describe("stage allow-list (non-global user)", function() { - it("keeps allow-listed stages untouched", function() { - var p = [{$match: {a: 1}}, {$group: {_id: "$x"}}, {$limit: 5}]; + describe("the reported bypass", function() { + it("rejects a $lookup hidden behind an unrecognised sibling stage", function() { + // $_internalInhibitOptimization is a real but undocumented MongoDB + // stage. Because it was not in the old list of known stage names, the + // old guard stopped treating this array as a pipeline and left the + // sibling $lookup in place. + var p = [ + {$limit: 1}, + { + $facet: { + leak: [ + {$lookup: {from: "password_reset", pipeline: [{$project: {prid: 1}}], as: "docs"}}, + {$_internalInhibitOptimization: {}}, + {$unwind: "$docs"}, + {$replaceRoot: {newRoot: "$docs"}} + ] + } + } + ]; + // Two independent checks now catch this. The join check runs first and + // reports the collection, since password_reset is protected for every + // role; the operator check would reject it anyway. var res = guard.sanitizeAggregation(p, USER); - (res.error === null).should.equal(true); - Object.keys(res.changes).length.should.equal(0); - p.length.should.equal(3); - }); - it("strips a non-allowed stage ($lookup) for a normal user", function() { - var p = [{$lookup: {from: "events", as: "e"}}, {$limit: 5}]; + res.error.type.should.equal("join"); + res.error.name.should.equal("password_reset"); + }); + it("rejects the same shape aimed at an unprotected collection, on the operator", function() { + var p = [ + {$limit: 1}, + { + $facet: { + leak: [ + {$lookup: {from: "apps", pipeline: [{$project: {key: 1}}], as: "docs"}}, + {$_internalInhibitOptimization: {}}, + {$unwind: "$docs"}, + {$replaceRoot: {newRoot: "$docs"}} + ] + } + } + ]; var res = guard.sanitizeAggregation(p, USER); - (res.error === null).should.equal(true); - res.changes.should.have.property("$lookup"); - p.length.should.equal(1); - p[0].should.have.property("$limit"); + res.error.type.should.equal("operator"); + res.error.name.should.equal("$lookup"); + res.error.where.should.containEql("$facet"); }); - it("strips write stages ($out) for everyone (in no list)", function() { - var p = [{$match: {a: 1}}, {$out: "stolen"}]; - var res = guard.sanitizeAggregation(p, USER); - (res.error === null).should.equal(true); - res.changes.should.have.property("$out"); - JSON.stringify(p).indexOf("$out").should.equal(-1); + it("rejects the unrecognised stage on its own too", function() { + var res = guard.sanitizeAggregation([{$limit: 1}, {$_internalInhibitOptimization: {}}], USER); + res.error.name.should.equal("$_internalInhibitOptimization"); + }); + it("rejects a $lookup nested arbitrarily deep", function() { + var p = [{$facet: {a: [{$facet: {b: [{$lookup: {from: "apps", as: "d"}}]}}]}}]; + guard.sanitizeAggregation(p, USER).error.name.should.equal("$lookup"); }); - it("strips inherited Object.prototype keys (constructor / __proto__)", function() { - var p = [JSON.parse('{"constructor": {"x": 1}}'), JSON.parse('{"__proto__": {"y": 1}}'), {$limit: 5}]; + it("leaves the pipeline untouched when rejecting", function() { + var p = [{$lookup: {from: "apps", as: "d"}}, {$limit: 5}]; guard.sanitizeAggregation(p, USER); - p.length.should.equal(1); - p[0].should.have.property("$limit"); + p.length.should.equal(2); + p[0].should.have.property("$lookup"); }); }); - describe("nested stage stripping (structural, any depth)", function() { - it("strips a non-allowed stage nested in $facet", function() { - var p = [{$facet: {leak: [{$lookup: {from: "events", as: "e"}}]}}]; - var res = guard.sanitizeAggregation(p, USER); - res.changes.should.have.property("$lookup"); - JSON.stringify(p).indexOf("$lookup").should.equal(-1); - }); - it("strips a non-allowed stage in an arbitrary (non-$facet/.pipeline) nested shape", function() { - var p = [{$facet: {b: [{$set: {ok: 1}}]}}]; - // craft an allowed stage carrying a sub-pipeline under a custom field - p[0].$facet.b.push({$project: {z: 1}}); - var weird = [{$group: {_id: 1}}]; - weird.push({$lookup: {from: "events", as: "e"}}); - p[0].$facet.b.push({$set: {nested: {anything: weird}}}); // expression object holding a pipeline-shaped array - var res = guard.sanitizeAggregation(p, USER); - res.changes.should.have.property("$lookup"); - JSON.stringify(p).indexOf("$lookup").should.equal(-1); + describe("operator allow-list (non-global user)", function() { + it("accepts allow-listed stages", function() { + var res = guard.sanitizeAggregation([{$match: {a: 1}}, {$group: {_id: "$x"}}, {$limit: 5}], USER); + (res.error === null).should.equal(true); }); - it("does not mistake an ordinary expression array for a sub-pipeline", function() { - var p = [{$project: {full: {$concat: ["$a", "$b"]}}}]; - var res = guard.sanitizeAggregation(p, USER); - Object.keys(res.changes).length.should.equal(0); - p[0].$project.full.$concat.length.should.equal(2); + it("rejects $lookup", function() { + guard.sanitizeAggregation([{$lookup: {from: "events", as: "e"}}], USER).error.name.should.equal("$lookup"); }); - it("drops a $facet branch emptied by stripping", function() { - var p = [{$facet: {leak: [{$lookup: {from: "events", as: "e"}}]}}]; - guard.sanitizeAggregation(p, USER); - // leak emptied -> removed; $facet emptied -> stage removed - p.length.should.equal(0); + it("rejects write stages ($out, $merge) for everyone", function() { + guard.sanitizeAggregation([{$match: {a: 1}}, {$out: "stolen"}], USER).error.name.should.equal("$out"); + guard.sanitizeAggregation([{$merge: {into: "stolen"}}], ADMIN).error.name.should.equal("$merge"); + }); + it("rejects server introspection stages", function() { + guard.sanitizeAggregation([{$currentOp: {}}], USER).error.name.should.equal("$currentOp"); + guard.sanitizeAggregation([{$collStats: {}}], ADMIN).error.name.should.equal("$collStats"); + }); + it("rejects a stage element that names no operator", function() { + var res = guard.sanitizeAggregation([JSON.parse('{"constructor": {"x": 1}}')], USER); + res.error.type.should.equal("stage"); + }); + it("rejects an own __proto__ key used as a stage name", function() { + var res = guard.sanitizeAggregation([JSON.parse('{"__proto__": {"y": 1}}')], USER); + res.error.type.should.equal("stage"); + }); + }); + + describe("does not reject legitimate queries", function() { + it("accepts expression operators inside stages", function() { + var p = [{$project: {t: {$add: ["$a", "$b"]}, m: {$mergeObjects: [{x: 1}, {y: 2}]}}}]; + (guard.sanitizeAggregation(p, USER).error === null).should.equal(true); + }); + it("accepts $mergeObjects, which must not be confused with $merge", function() { + var p = [{$project: {m: {$mergeObjects: [{a: 1}, {b: 2}]}}}]; + (guard.sanitizeAggregation(p, USER).error === null).should.equal(true); + }); + it("accepts a value that looks like a disallowed operator", function() { + // {$literal: "$lookup"} returns the STRING "$lookup". Only keys are + // checked, so this is fine. + var p = [{$project: {lit: {$literal: "$lookup"}}}]; + (guard.sanitizeAggregation(p, USER).error === null).should.equal(true); + }); + it("accepts $setField naming a $-prefixed field", function() { + // MongoDB allows documents with $-prefixed field names, reachable only + // through $getField / $setField where the name is a value. + var p = [{$project: {b: {$setField: {field: {$literal: "$lookup"}, input: {}, value: 1}}}}]; + (guard.sanitizeAggregation(p, USER).error === null).should.equal(true); + }); + it("accepts complex $match operators", function() { + var p = [{$match: {$and: [{a: {$gt: 1}}, {b: {$in: [1, 2]}}], c: {$elemMatch: {d: 1}}}}]; + (guard.sanitizeAggregation(p, USER).error === null).should.equal(true); + }); + it("accepts accumulators and window operators", function() { + var p = [ + {$group: {_id: "$a", n: {$sum: 1}, all: {$push: "$b"}}}, + {$setWindowFields: {sortBy: {a: 1}, output: {r: {$rank: {}}}}} + ]; + (guard.sanitizeAggregation(p, USER).error === null).should.equal(true); + }); + it("accepts a real $facet", function() { + var p = [{$facet: {byA: [{$group: {_id: "$a", n: {$sum: 1}}}], byB: [{$sortByCount: "$b"}]}}]; + (guard.sanitizeAggregation(p, USER).error === null).should.equal(true); }); }); - describe("hard rule: server-side JavaScript (any role, any depth)", function() { + describe("server-side JavaScript (any role, any depth)", function() { it("rejects $function inside a $project expression", function() { var p = [{$project: {x: {$function: {body: "f", args: [], lang: "js"}}}}]; var res = guard.sanitizeAggregation(p, ADMIN); @@ -92,8 +159,8 @@ describe("dbviewer aggregation guard", function() { }); }); - describe("hard rule: joins into redacted collections (any role, any depth)", function() { - it("rejects a $lookup into members (even for global admin)", function() { + describe("joins into redacted collections (any role, any depth)", function() { + it("rejects a $lookup into members even for a global admin", function() { var res = guard.sanitizeAggregation([{$lookup: {from: "members", as: "m"}}], ADMIN); res.error.type.should.equal("join"); res.error.name.should.equal("members"); @@ -110,12 +177,17 @@ describe("dbviewer aggregation guard", function() { it("rejects a join into members nested inside $facet", function() { guard.sanitizeAggregation([{$facet: {leak: [{$lookup: {from: "members", as: "m"}}]}}], ADMIN).error.name.should.equal("members"); }); - it("rejects a $lookup into members via the cross-db object form {db, coll}", function() { + it("rejects a $lookup into members via the cross-db object form", function() { var res = guard.sanitizeAggregation([{$lookup: {from: {db: "countly", coll: "members"}, as: "m"}}], ADMIN); res.error.type.should.equal("join"); res.error.name.should.equal("members"); }); - it("rejects a $graphLookup into members via the cross-db object form {db, coll}", function() { + it("rejects a $lookup into password_reset, whose prid is a reset token", function() { + var res = guard.sanitizeAggregation([{$lookup: {from: "password_reset", as: "d"}}], ADMIN); + res.error.type.should.equal("join"); + res.error.name.should.equal("password_reset"); + }); + it("rejects a $graphLookup into members via the cross-db object form", function() { var res = guard.sanitizeAggregation([{$graphLookup: {from: {db: "countly", coll: "members"}, startWith: "$x", connectFromField: "a", connectToField: "b", as: "m"}}], ADMIN); res.error.type.should.equal("join"); res.error.name.should.equal("members"); @@ -123,25 +195,37 @@ describe("dbviewer aggregation guard", function() { }); describe("global admin allow-list", function() { - it("allows $lookup into a non-protected collection (kept, no error)", function() { + it("allows $lookup into a non-protected collection", function() { var p = [{$lookup: {from: "events", pipeline: [{$match: {a: 1}}], as: "e"}}, {$limit: 5}]; - var res = guard.sanitizeAggregation(p, ADMIN); - (res.error === null).should.equal(true); - Object.keys(res.changes).length.should.equal(0); - p[0].should.have.property("$lookup"); + (guard.sanitizeAggregation(p, ADMIN).error === null).should.equal(true); }); - it("still strips a non-allowed stage ($out) for an admin", function() { - var p = [{$match: {a: 1}}, {$out: "x"}]; - var res = guard.sanitizeAggregation(p, ADMIN); - (res.error === null).should.equal(true); - res.changes.should.have.property("$out"); + it("allows $unionWith into a non-protected collection", function() { + var p = [{$unionWith: {coll: "events", pipeline: [{$limit: 1}]}}]; + (guard.sanitizeAggregation(p, ADMIN).error === null).should.equal(true); }); - it("does NOT allow $lookup for a normal user (stripped)", function() { - var p = [{$lookup: {from: "events", as: "e"}}]; - var res = guard.sanitizeAggregation(p, USER); - (res.error === null).should.equal(true); - res.changes.should.have.property("$lookup"); - p.length.should.equal(0); + it("rejects the same $lookup for a non-global user", function() { + guard.sanitizeAggregation([{$lookup: {from: "events", as: "e"}}], USER).error.name.should.equal("$lookup"); + }); + }); + + describe("allow-list integrity", function() { + it("does not contain the operators it is meant to exclude", function() { + ["$out", "$merge", "$function", "$accumulator", "$where", "$currentOp", + "$collStats", "$indexStats", "$planCacheStats", "$listSessions", + "$mergeCursors", "$documents", "$changeStream"].forEach(function(op) { + (ADMIN[op] === true).should.equal(false); + }); + }); + it("does not carry the misspelling the previous version had", function() { + // the real stage is $shardedDataDistribution; the old list said + // $sharedDataDistribution, so the real one was never recognised + (ADMIN.$sharedDataDistribution === true).should.equal(false); + (ADMIN.$shardedDataDistribution === true).should.equal(false); + }); + it("gives non-global users no join operators", function() { + ["$lookup", "$graphLookup", "$unionWith"].forEach(function(op) { + (USER[op] === true).should.equal(false); + }); }); }); }); diff --git a/test/unit-tests/plugins.dbviewer.query-guard.js b/test/unit-tests/plugins.dbviewer.query-guard.js index 358e93ea061..728f840c3c0 100644 --- a/test/unit-tests/plugins.dbviewer.query-guard.js +++ b/test/unit-tests/plugins.dbviewer.query-guard.js @@ -1,46 +1,41 @@ require("should"); var qguard = require("../../plugins/dbviewer/api/parts/query_guard.js"); +// findDisallowedProjectionValue(projection) -> { name } | null +// +// A find() projection may only include or exclude fields. It used to have offending +// entries deleted and the query run anyway, which returned something other than what +// was asked for without saying so. It now returns the first offending field and the +// caller rejects the request, matching the aggregation guard. describe("dbviewer query guard", function() { - describe("sanitizeProjection", function() { - it("keeps plain include/exclude projections", function() { + describe("findDisallowedProjectionValue", function() { + it("accepts plain include/exclude projections", function() { var p = {name: 1, _id: 0, "a.b": 1, ok: true, no: false}; - var changes = qguard.sanitizeProjection(p); - Object.keys(changes).length.should.equal(0); - p.should.have.property("name", 1); - p.should.have.property("a.b", 1); - }); - it("drops a field-path alias value (e.g. {leak: \"$password\"})", function() { - var p = {leak: "$password", k: "$api_key", name: 1}; - var changes = qguard.sanitizeProjection(p); - changes.should.have.property("leak"); - changes.should.have.property("k"); - p.should.not.have.property("leak"); - p.should.not.have.property("k"); - p.should.have.property("name", 1); - }); - it("drops an expression-object value (e.g. {x: {$function: ...}})", function() { - var p = {x: {$function: {body: "f", args: [], lang: "js"}}, y: {$concat: ["$password", ""]}, name: 1}; - var changes = qguard.sanitizeProjection(p); - changes.should.have.property("x"); - changes.should.have.property("y"); - p.should.not.have.property("x"); - p.should.not.have.property("y"); - p.should.have.property("name", 1); - }); - it("drops numeric values that are not strictly 0 or 1", function() { - var p = {a: 2, b: NaN, c: -1, ok: 1, off: 0, t: true, f: false}; - var changes = qguard.sanitizeProjection(p); - changes.should.have.property("a"); - changes.should.have.property("b"); - changes.should.have.property("c"); - p.should.not.have.property("a"); - p.should.not.have.property("b"); - p.should.not.have.property("c"); - p.should.have.property("ok", 1); - p.should.have.property("off", 0); - p.should.have.property("t", true); - p.should.have.property("f", false); + (qguard.findDisallowedProjectionValue(p) === null).should.equal(true); + }); + it("rejects a field-path alias value (e.g. {leak: \"$password\"})", function() { + qguard.findDisallowedProjectionValue({leak: "$password", name: 1}).name.should.equal("leak"); + qguard.findDisallowedProjectionValue({name: 1, k: "$api_key"}).name.should.equal("k"); + }); + it("rejects an expression-object value (e.g. {x: {$function: ...}})", function() { + qguard.findDisallowedProjectionValue({x: {$function: {body: "f", args: [], lang: "js"}}, name: 1}).name.should.equal("x"); + qguard.findDisallowedProjectionValue({y: {$concat: ["$password", ""]}}).name.should.equal("y"); + }); + it("rejects numeric values that are not strictly 0 or 1", function() { + [2, NaN, -1, "1"].forEach(function(bad) { + qguard.findDisallowedProjectionValue({ok: 1, a: bad}).name.should.equal("a"); + }); + }); + it("does not modify the projection it rejects", function() { + var p = {leak: "$password", name: 1}; + qguard.findDisallowedProjectionValue(p); + p.should.have.property("leak", "$password"); + Object.keys(p).length.should.equal(2); + }); + it("accepts a missing or non-object projection", function() { + (qguard.findDisallowedProjectionValue(undefined) === null).should.equal(true); + (qguard.findDisallowedProjectionValue(null) === null).should.equal(true); + (qguard.findDisallowedProjectionValue({}) === null).should.equal(true); }); }); diff --git a/test/unit-tests/plugins.dbviewer.redaction.js b/test/unit-tests/plugins.dbviewer.redaction.js new file mode 100644 index 00000000000..2ebff99aea3 --- /dev/null +++ b/test/unit-tests/plugins.dbviewer.redaction.js @@ -0,0 +1,69 @@ +require("should"); +var redaction = require("../../plugins/dbviewer/api/parts/redaction.js"); + +describe("dbviewer redaction", function() { + describe("hasRedactedFields", function() { + it("knows the collections that hold secrets", function() { + redaction.hasRedactedFields("members").should.equal(true); + redaction.hasRedactedFields("password_reset").should.equal(true); + }); + it("is false for ordinary collections", function() { + redaction.hasRedactedFields("events_data").should.equal(false); + redaction.hasRedactedFields("apps").should.equal(false); + }); + it("is false for Object.prototype keys, not truthy by inheritance", function() { + redaction.hasRedactedFields("constructor").should.equal(false); + redaction.hasRedactedFields("toString").should.equal(false); + redaction.hasRedactedFields("__proto__").should.equal(false); + }); + }); + + describe("redactFields", function() { + it("removes member credentials and keeps the rest", function() { + var doc = {_id: "1", email: "a@b.c", password: "hash", api_key: "k", two_factor_auth: {}, full_name: "A"}; + redaction.redactFields("members", doc); + doc.should.not.have.property("password"); + doc.should.not.have.property("api_key"); + doc.should.not.have.property("two_factor_auth"); + doc.should.have.property("email", "a@b.c"); + doc.should.have.property("full_name", "A"); + }); + it("removes the password-reset token and keeps the rest", function() { + var doc = {_id: "1", prid: "tok", user_id: "u", timestamp: 12345}; + redaction.redactFields("password_reset", doc); + doc.should.not.have.property("prid"); + doc.should.have.property("user_id", "u"); + doc.should.have.property("timestamp", 12345); + }); + it("leaves documents from other collections alone", function() { + var doc = {a: 1, password: "not a member doc"}; + redaction.redactFields("events_data", doc); + doc.should.have.property("password"); + }); + it("tolerates a missing document", function() { + (redaction.redactFields("members", null) === null).should.equal(true); + (redaction.redactFields("members", undefined) === undefined).should.equal(true); + }); + }); + + describe("redactionStage", function() { + it("excludes every member credential", function() { + redaction.redactionStage("members").should.eql({$project: {password: 0, api_key: 0, two_factor_auth: 0}}); + }); + it("excludes the password-reset token", function() { + redaction.redactionStage("password_reset").should.eql({$project: {prid: 0}}); + }); + it("is null for a collection with nothing to hide", function() { + (redaction.redactionStage("events_data") === null).should.equal(true); + (redaction.redactionStage("constructor") === null).should.equal(true); + }); + it("covers exactly the same fields the document path removes", function() { + // the two paths must not drift: a field dropped from a single-document + // read but not from an aggregation is a field that leaks + Object.keys(redaction.REDACTED_FIELDS).forEach(function(collection) { + var stage = redaction.redactionStage(collection); + Object.keys(stage.$project).sort().should.eql(redaction.REDACTED_FIELDS[collection].slice().sort()); + }); + }); + }); +});