From 69fa74d5de3f5004d1780bb6c6cedcd82a07b510 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rmungandrk?= <47859767+odaysec@users.noreply.github.com> Date: Tue, 14 Oct 2025 00:05:18 +0700 Subject: [PATCH] fix: Implement path validation, rate limiting and Input safety across core modules --- package.json | 3 ++- src/common/crypt.ts | 63 ++++++++++++++++++++++++++++++++++++++++----- src/common/repo.ts | 16 +++++++++--- src/common/utils.ts | 12 +++++++++ src/server.ts | 37 +++++++++++++++++++------- 5 files changed, 110 insertions(+), 21 deletions(-) diff --git a/package.json b/package.json index c6746a27fd..9c494de602 100644 --- a/package.json +++ b/package.json @@ -37,7 +37,8 @@ "validator": "13.15.15", "yaml": "2.8.1", "yargs": "^17.7.2", - "zx": "8.8.3" + "zx": "8.8.3", + "express-rate-limit": "^8.1.0" }, "description": "Otomi Core is an opinionated stack of Kubernetes apps and configurations. Part of Otomi Container Platform.", "devDependencies": { diff --git a/src/common/crypt.ts b/src/common/crypt.ts index caf9dd685c..b5a3d27fcd 100644 --- a/src/common/crypt.ts +++ b/src/common/crypt.ts @@ -1,7 +1,8 @@ import { EventEmitter } from 'events' -import { existsSync } from 'fs' +import { existsSync, realpathSync } from 'fs' import { readFile, stat, utimes, writeFile } from 'fs/promises' import { chunk } from 'lodash' +import * as pathModule from 'path' import { $, cd, ProcessOutput } from 'zx' import { terminal } from './debug' import { cleanEnv, cliEnvSpec, env, isCli } from './envalid' @@ -94,10 +95,32 @@ const processFileChunk = async (crypt: CR, files: string[]): Promise<(ProcessOut const runOnSecretFiles = async (path: string, crypt: CR, filesArgs: string[] = []): Promise => { const d = terminal(`common:crypt:runOnSecretFiles`) let files: string[] = filesArgs + const MAX_FILES = 1000 + + // Defensive: ensure files is a real array and safely sized + if (!Array.isArray(files)) { + d.error('Expected files to be an Array') + throw new Error("Files parameter must be an Array") + } + // Bound the files length + if (typeof files.length !== 'number' || !Number.isInteger(files.length) || files.length < 0 || files.length > MAX_FILES) { + d.warn(`Too many files supplied (${files.length}), truncating to ${MAX_FILES}`) + files = files.slice(0, MAX_FILES) + } if (files.length === 0) { files = await getAllSecretFiles(path) + // Defensive: ensure safe array and length for auto-discovered files as well + if (!Array.isArray(files)) { + d.error('Expected files from getAllSecretFiles to be an Array') + throw new Error("getAllSecretFiles must return an Array") + } + if (typeof files.length !== 'number' || !Number.isInteger(files.length) || files.length < 0 || files.length > MAX_FILES) { + d.warn(`Too many files discovered (${files.length}), truncating to ${MAX_FILES}`) + files = files.slice(0, MAX_FILES) + } } + files = files.filter(async (f) => { const suffix = crypt.cmd === CryptType.ENCRYPT ? '.dec' : '' let file = `${f}${suffix}` @@ -146,17 +169,30 @@ const matchTimestamps = async (path, file: string) => { export const decrypt = async (path = env.ENV_DIR, ...files: string[]): Promise => { const d = terminal(`common:crypt:decrypt`) - if (!existsSync(`${path}/.sops.yaml`)) { + // Validate that `path` is within rootDir + const envRoot = rootDir + let resolvedPath: string + try { + resolvedPath = realpathSync(pathModule.resolve(envRoot, pathModule.relative(envRoot, path))) + } catch (err) { + d.warn(`Failed to resolve environment path '${path}': ${err}`) + return + } + if (!resolvedPath.startsWith(envRoot)) { + d.warn(`Denied access to environment path: '${path}' (resolved: '${resolvedPath}')`) + return + } + if (!existsSync(`${resolvedPath}/.sops.yaml`)) { d.info('Skipping decryption') return } d.info('Starting decryption') await runOnSecretFiles( - path, + resolvedPath, { cmd: CryptType.DECRYPT, - post: async (f) => matchTimestamps(path, f), + post: async (f) => matchTimestamps(resolvedPath, f), }, files, ) @@ -166,13 +202,26 @@ export const decrypt = async (path = env.ENV_DIR, ...files: string[]): Promise => { const d = terminal(`common:crypt:encrypt`) - if (!existsSync(`${path}/.sops.yaml`)) { + // Validate that `path` is within rootDir + const envRoot = rootDir + let resolvedPath: string + try { + resolvedPath = realpathSync(pathModule.resolve(envRoot, pathModule.relative(envRoot, path))) + } catch (err) { + d.warn(`Failed to resolve environment path '${path}': ${err}`) + return + } + if (!resolvedPath.startsWith(envRoot)) { + d.warn(`Denied access to environment path: '${path}' (resolved: '${resolvedPath}')`) + return + } + if (!existsSync(`${resolvedPath}/.sops.yaml`)) { d.info('Skipping encryption') return } d.info('Starting encryption') await runOnSecretFiles( - path, + resolvedPath, { condition: async (file: string): Promise => { if (!existsSync(file)) { @@ -206,7 +255,7 @@ export const encrypt = async (path = env.ENV_DIR, ...files: string[]): Promise matchTimestamps(path, f), + post: async (f: string) => matchTimestamps(resolvedPath, f), }, files, ) diff --git a/src/common/repo.ts b/src/common/repo.ts index f023a6fda8..39e3be0bb1 100644 --- a/src/common/repo.ts +++ b/src/common/repo.ts @@ -7,6 +7,8 @@ import path from 'path' import { getDirNames, loadYaml } from './utils' import { objectToYaml, writeValuesToFile } from './values' +// Define the base directory allowed for all envDir input (adjust as appropriate for the project) +const ROOT = path.resolve('/var/data') // Change /var/data to desired allowed directory export async function getTeamNames(envDir: string): Promise> { const teamsDir = path.join(envDir, 'env', 'teams') return await getDirNames(teamsDir, { skipHidden: true }) @@ -515,21 +517,27 @@ export async function setValuesFile( envDir: string, deps = { pathExists: existsSync, loadValues, writeFile }, ): Promise { - const valuesPath = path.join(envDir, 'values-repo.yaml') + const safeEnvDir = path.resolve(ROOT, envDir) + if (!safeEnvDir.startsWith(ROOT)) throw new Error('Invalid envDir: outside permitted directory') + const valuesPath = path.join(safeEnvDir, 'values-repo.yaml') // if (await deps.pathExists(valuesPath)) return valuesPath - const allValues = await deps.loadValues(envDir) + const allValues = await deps.loadValues(safeEnvDir) await deps.writeFile(valuesPath, objectToYaml(allValues)) return valuesPath } export async function unsetValuesFile(envDir: string): Promise { - const valuesPath = path.join(envDir, 'values-repo.yaml') + const safeEnvDir = path.resolve(ROOT, envDir) + if (!safeEnvDir.startsWith(ROOT)) throw new Error('Invalid envDir: outside permitted directory') + const valuesPath = path.join(safeEnvDir, 'values-repo.yaml') await rm(valuesPath, { force: true }) return valuesPath } export function unsetValuesFileSync(envDir: string): string { - const valuesPath = path.join(envDir, 'values-repo.yaml') + const safeEnvDir = path.resolve(ROOT, envDir) + if (!safeEnvDir.startsWith(ROOT)) throw new Error('Invalid envDir: outside permitted directory') + const valuesPath = path.join(safeEnvDir, 'values-repo.yaml') rmSync(valuesPath, { force: true }) return valuesPath } diff --git a/src/common/utils.ts b/src/common/utils.ts index 64e9162382..fb5eca0939 100644 --- a/src/common/utils.ts +++ b/src/common/utils.ts @@ -17,6 +17,13 @@ const packagePath = process.cwd() // we keep the rootDir for zx, but have to fix it for drone, which starts in /home/app/stack/env (to accommodate write perms): export const rootDir = process.cwd() === '/home/app/stack/env' ? '/home/app/stack' : process.cwd() export const pkg = JSON.parse(readFileSync(`${rootDir}/package.json`, 'utf8')) +// Ensure a resolved path is within rootDir +const isSubPath = (target: string, base: string): boolean => { + // Ensure trailing slash for correct startsWith + const baseNorm = resolve(base) + '/' + const targetNorm = resolve(target) + return targetNorm === resolve(base) || targetNorm.startsWith(baseNorm) +} export const getFilename = (path: string): string => path.split('/').pop()?.split('.')[0] as string export const asArray = (args: string | string[]): string[] => { @@ -35,6 +42,11 @@ export const removeBlankAttributes = (obj: Record): Record => { + // Validate that dir is within rootDir + if (!isSubPath(dir, rootDir)) { + terminal('common:utils').warn(`Refusing to recurse outside rootDir: ${dir}`) + return [] + } const dirs = await readdir(dir, { withFileTypes: true }) const files = await Promise.all( dirs.map(async (dirOrFile) => { diff --git a/src/server.ts b/src/server.ts index 730277a4a5..f6c032870e 100644 --- a/src/server.ts +++ b/src/server.ts @@ -1,6 +1,9 @@ import $RefParser, { JSONSchema } from '@apidevtools/json-schema-ref-parser' import express, { Request, Response } from 'express' +import rateLimit from 'express-rate-limit' +import { copyFile } from 'fs/promises' import { Server } from 'http' +import * as path from 'path' import { bootstrapSops } from 'src/cmd/bootstrap' import { decrypt, encrypt } from 'src/common/crypt' import { terminal } from 'src/common/debug' @@ -8,12 +11,18 @@ import { hfValues } from './common/hf' import { setValuesFile, unsetValuesFile } from './common/repo' import { loadYaml, rootDir } from './common/utils' import { objectToYaml } from './common/values' -import { copyFile } from 'fs/promises' const d = terminal('server') const app = express() let server: Server +// Rate limiter for expensive routes +const prepareLimiter = rateLimit({ + windowMs: 1 * 60 * 1000, // 1 minute + max: 10, // Limit each IP to 10 requests per minute + message: 'Too many requests, please try again later.', +}) + export const stopServer = (): void => { server?.close() } @@ -39,18 +48,28 @@ app.get('/init', async (req: Request, res: Response): Promise => { } }) -app.get('/prepare', async (req: Request, res: Response): Promise => { +app.get('/prepare', prepareLimiter, async (req: Request, res: Response): Promise => { const { envDir, files } = req.query as QueryParams + // Define the allowed environment root directory + const allowedEnvsRoot = path.resolve(rootDir, '.envs') + let resolvedEnvDir = path.resolve(allowedEnvsRoot, envDir || '') try { - d.log('Request to prepare values repo on', envDir) + // Check if the resolved environment directory is inside the allowed root + if (!resolvedEnvDir.startsWith(allowedEnvsRoot)) { + res.status(403).send('Forbidden: Invalid envDir path.') + return + } + d.log('Request to prepare values repo on', resolvedEnvDir) const file = '.editorconfig' - await copyFile(`${rootDir}/.values/${file}`, `${envDir}/${file}`) - await bootstrapSops(envDir) - await setValuesFile(envDir) + const srcFile = path.resolve(rootDir, '.values', file) + const destFile = path.resolve(resolvedEnvDir, file) + await copyFile(srcFile, destFile) + await bootstrapSops(resolvedEnvDir) + await setValuesFile(resolvedEnvDir) // Encrypt ensures that a brand new secret file is encrypted in place - await encrypt(envDir, ...(files ?? [])) + await encrypt(resolvedEnvDir, ...(files ?? [])) // Decrypt ensures that a brand new encrypted secret file is decrypted to the .dec file - await decrypt(envDir, ...(files ?? [])) + await decrypt(resolvedEnvDir, ...(files ?? [])) res.status(200).send('ok') } catch (error) { const err = `${error}` @@ -61,7 +80,7 @@ app.get('/prepare', async (req: Request, res: Response): Promise => { } res.status(status).send(err) } finally { - await unsetValuesFile(envDir) + await unsetValuesFile(resolvedEnvDir) } })