mirror of
https://github.com/wekan/wekan.git
synced 2026-11-07 11:02:33 -05:00
Lauri Ojansivu
0a9eb0ef4f
Attachments blocked client-supplied `versions.*.path`; avatars did not. The avatar allow rule was the single line `update: isOwner` - being the owner let you write ANY field of your own avatar document, `versions.original.path` included. That field is server-managed metadata: it says where on disk the bytes are. Board export then read it. For every member whose avatar is a local WeKan file, the exporter opens `versions.original.path` and embeds the bytes as base64 in `profile.avatarFile`, so an authenticated user could point their own avatar at /etc/passwd - or at anything else under WRITABLE_PATH - export a board they are a member of, and decode the file out of the JSON. Arbitrary file read as the WeKan OS user (CWE-22, CWE-73), from any account that can own an avatar. Both halves are closed. The WRITE: the guards that attachments had are now in one module, models/lib/fileVersionFields.js, and BOTH permission files import it - avatars had gone without them precisely because each collection wrote its own copy. Avatars refuse an insert carrying versions.*.path or .storage, refuse any update touching the versions subtree, and restrict updates to the same field whitelist attachments use. Being the owner is still required; it is no longer sufficient. The READ: nothing is read from a stored path unless the path RESOLVES to somewhere inside WeKan's own storage (models/lib/storagePathContainment.js). That is the half that also holds when a path is poisoned some other way - a document written before this fix, a restored backup, a bad migration. The check sits at getBase64Data, the one place the export turns a stored path into bytes for attachments and avatars alike, and at both avatar call sites. The download path in fileStoreStrategy.js has always checked containment this way; its private copy of the function is gone and it uses the shared one, so the download and the export ask the same question. tests/avatarVersionPathTraversal.test.cjs pins the payload being refused on insert and on update, that renaming an avatar still works, that ..-escapes are resolved rather than string-matched, that a sibling directory with the root as a prefix is not inside it, and that no unguarded read of a stored version path remains in the exporter. Thanks to Alpastx and xet7 ! Fixes GHSA-4mxf-m8pq-xc9p.
85 lines
3.4 KiB
JavaScript
85 lines
3.4 KiB
JavaScript
// Pure, dependency-free containment checks for a file path that came out of the
|
|
// DATABASE rather than off the disk. No Meteor imports, so it is unit tested
|
|
// directly with plain Node (tests/storagePathContainment.test.cjs).
|
|
//
|
|
// GHSA-4mxf-m8pq-xc9p: the board exporter read `versions.original.path` from an
|
|
// avatar document and streamed that file into the export as base64, trusting
|
|
// whatever Mongo held. The avatar allow rule let the owner write that field, so
|
|
// the export embedded the bytes of any file the WeKan process could read.
|
|
// Blocking the write is the first half of the fix; this is the second, and it is
|
|
// the half that also holds if a path is poisoned some other way - a document
|
|
// written before the fix, a restored backup, a bad migration.
|
|
//
|
|
// The DOWNLOAD path in models/lib/fileStoreStrategy.js has always checked
|
|
// containment this way; that function now lives here, so the download and the
|
|
// export ask the same question instead of each having their own answer. Drift
|
|
// between two copies of a rule is exactly what this advisory was.
|
|
|
|
const path = require('path');
|
|
|
|
/** Resolve, and lowercase on Windows, whose paths are case-insensitive. */
|
|
function normalizeForCompare(inputPath) {
|
|
const normalized = path.resolve(inputPath);
|
|
return process.platform === 'win32' ? normalized.toLowerCase() : normalized;
|
|
}
|
|
|
|
/**
|
|
* True when `targetPath` is `basePath` or lies under it.
|
|
*
|
|
* Resolving first is the whole point: '..' segments are what a plain
|
|
* `startsWith` gets wrong, so `/data/files/avatars/../../etc/passwd` is NOT
|
|
* inside `/data/files/avatars`. Comparing the RELATIVE path also means
|
|
* `/data/files/avatars-evil` is not inside `/data/files/avatars`, which a
|
|
* `startsWith` on the raw strings would accept.
|
|
*
|
|
* @param {string} basePath the directory
|
|
* @param {string} targetPath the path to test
|
|
* @return {boolean}
|
|
*/
|
|
function isPathInsideBase(basePath, targetPath) {
|
|
if (typeof basePath !== 'string' || !basePath) return false;
|
|
if (typeof targetPath !== 'string' || !targetPath) return false;
|
|
|
|
const normalizedBase = normalizeForCompare(basePath);
|
|
const normalizedTarget = normalizeForCompare(targetPath);
|
|
const relative = path.relative(normalizedBase, normalizedTarget);
|
|
|
|
return relative === '' || (!relative.startsWith('..') && !path.isAbsolute(relative));
|
|
}
|
|
|
|
/**
|
|
* The same question asked the other way round, for reading a FILE: is this
|
|
* candidate somewhere under that root? The root ITSELF is not an answer - a
|
|
* storage root is a directory, never the file being read - which is the one
|
|
* difference from isPathInsideBase().
|
|
*
|
|
* @param {string} candidate the path to check (as stored)
|
|
* @param {string} root the directory it must be under
|
|
* @return {boolean}
|
|
*/
|
|
function isPathInside(candidate, root) {
|
|
if (typeof candidate !== 'string' || !candidate) return false;
|
|
if (typeof root !== 'string' || !root) return false;
|
|
if (normalizeForCompare(candidate) === normalizeForCompare(root)) return false;
|
|
|
|
return isPathInsideBase(root, candidate);
|
|
}
|
|
|
|
/**
|
|
* Same, against several roots at once (attachments and avatars, say).
|
|
* @param {string} candidate
|
|
* @param {string[]} roots
|
|
* @return {boolean}
|
|
*/
|
|
function isPathInsideAny(candidate, roots) {
|
|
if (!Array.isArray(roots)) return false;
|
|
return roots.some((root) => isPathInside(candidate, root));
|
|
}
|
|
|
|
module.exports = {
|
|
normalizeForCompare,
|
|
isPathInsideBase,
|
|
isPathInside,
|
|
isPathInsideAny,
|
|
};
|