From 59a357f76fc1db3f9e8cf1af5c0ca017007de604 Mon Sep 17 00:00:00 2001 From: "Flody.lee" <7feilee@gmail.com> Date: Mon, 22 Jun 2026 11:22:15 +0800 Subject: [PATCH] fix(security): harden command injection, path traversal, auth surfaces Audit of the backend attack surface and fixes for the web-reachable CRITICAL/HIGH issues. Adds back/shared/security.ts with centralized hardening helpers (shellEscape, assertSafeDependenceName, SUBSCRIPTION_PATTERNS, safeCompare, isSafeSshConfigValue). - Subscription fields (url/branch/whitelist/blacklist/extensions/proxy) are now shell-escaped before reaching spawn() and validated with strict Joi patterns at the API, closing OS command injection and the downstream shell eval/git-arg-injection paths. - Dependency names are validated before interpolation into pnpm/pip/apk/apt commands (incl. the embedded Python source). - SSH config generation rejects newline/metachar injection in host/proxy (prevents injected ProxyCommand execution). - ConfigService.getFile resolves the real path before containment check, fixing data/scripts/../db traversal that leaked the SQLite DB. - /configs/save containment check fixed (sibling-dir write bypass). - Script/env uploads use path.basename, preventing arbitrary file write (crontab.list/env.sh overwrite -> RCE) via multer originalname. - JWT secret is generated and persisted per-install instead of the public default 'whyour-secret'; production refuses to boot without one. - Token comparison is now constant-time (safeCompare). Co-Authored-By: Claude Opus 4.8 (1M context) --- back/api/config.ts | 20 +++++------ back/api/env.ts | 9 ++++- back/api/script.ts | 8 ++++- back/api/subscription.ts | 53 +++++++++++++++++++-------- back/config/index.ts | 42 +++++++++++++++++++++- back/config/subscription.ts | 23 +++++++----- back/config/util.ts | 4 +++ back/services/config.ts | 41 +++++++++++---------- back/services/sshKey.ts | 9 +++++ back/shared/auth.ts | 13 ++++--- back/shared/security.ts | 72 +++++++++++++++++++++++++++++++++++++ 11 files changed, 233 insertions(+), 61 deletions(-) create mode 100644 back/shared/security.ts diff --git a/back/api/config.ts b/back/api/config.ts index 23084cfe..63ae55a1 100644 --- a/back/api/config.ts +++ b/back/api/config.ts @@ -4,7 +4,7 @@ import { Logger } from 'winston'; import config from '../config'; import * as fs from 'fs/promises'; import { celebrate, Joi } from 'celebrate'; -import { join, basename } from 'path'; +import path, { basename } from 'path'; import { SAMPLE_FILES } from '../config/const'; import { t } from '../shared/i18n'; import ConfigService from '../services/config'; @@ -72,16 +72,16 @@ export default (app: Router) => { const logger: Logger = Container.get('logger'); try { const { name, content } = req.body; - // Resolve path first to prevent traversal attacks - let basePath = config.configPath; - if (name.startsWith('data/scripts/')) { - basePath = join(config.rootPath, 'data/scripts'); - } + // Resolve the final path first, then verify containment with a path + // separator so sibling dirs (e.g. data/scripts-evil) cannot be reached. + const isScripts = name.startsWith('data/scripts/'); + const basePath = path.resolve( + isScripts ? config.scriptPath : config.configPath, + ); const cleanName = name.replace(/^data\/scripts\//, ''); - const resolvedPath = join(basePath, cleanName); - const normalized = join(resolvedPath); - // Verify the resolved path stays within allowed directory - if (!normalized.startsWith(basePath)) { + const normalized = path.resolve(basePath, cleanName); + // Verify the resolved path stays within the allowed directory + if (normalized !== basePath && !normalized.startsWith(basePath + path.sep)) { return res.send({ code: 403, message: t('文件路径无效') }); } // Check blacklist on actual filename (not user input) diff --git a/back/api/env.ts b/back/api/env.ts index 24ee76b5..18c9ab35 100644 --- a/back/api/env.ts +++ b/back/api/env.ts @@ -1,6 +1,7 @@ import { Joi, celebrate } from 'celebrate'; import { NextFunction, Request, Response, Router } from 'express'; import fs from 'fs'; +import path from 'path'; import multer from 'multer'; import { Container } from 'typedi'; import { Logger } from 'winston'; @@ -15,7 +16,13 @@ const storage = multer.diskStorage({ cb(null, config.scriptPath); }, filename: function (req, file, cb) { - cb(null, file.originalname); + // Never trust the client-supplied name: strip any directory components so + // the upload cannot be written outside config.scriptPath (path traversal). + const safeName = path.basename(file.originalname || ''); + if (!safeName || safeName === '.' || safeName === '..') { + return cb(new Error('Invalid filename'), ''); + } + cb(null, safeName); }, }); const upload = multer({ storage: storage }); diff --git a/back/api/script.ts b/back/api/script.ts index 9be941d6..9f895bcd 100644 --- a/back/api/script.ts +++ b/back/api/script.ts @@ -22,7 +22,13 @@ const storage = multer.diskStorage({ cb(null, config.scriptPath); }, filename: function (req, file, cb) { - cb(null, file.originalname); + // Never trust the client-supplied name: strip any directory components so + // the upload cannot be written outside config.scriptPath (path traversal). + const safeName = path.basename(file.originalname || ''); + if (!safeName || safeName === '.' || safeName === '..') { + return cb(new Error('Invalid filename'), ''); + } + cb(null, safeName); }, }); const upload = multer({ storage: storage }); diff --git a/back/api/subscription.ts b/back/api/subscription.ts index dd365a27..7d5d6c7c 100644 --- a/back/api/subscription.ts +++ b/back/api/subscription.ts @@ -4,8 +4,33 @@ import { Logger } from 'winston'; import SubscriptionService from '../services/subscription'; import { celebrate, Joi } from 'celebrate'; import CronExpressionParser from 'cron-parser'; +import { SUBSCRIPTION_PATTERNS } from '../shared/security'; const route = Router(); +// Shared validation for fields that are interpolated into shell commands / +// ssh config files. Blocks shell metacharacters / newlines at the boundary. +const urlField = Joi.string().pattern(SUBSCRIPTION_PATTERNS.url); +const branchField = Joi.string() + .pattern(SUBSCRIPTION_PATTERNS.branch) + .optional() + .allow('') + .allow(null); +const extensionsField = Joi.string() + .pattern(SUBSCRIPTION_PATTERNS.extensions) + .optional() + .allow('') + .allow(null); +const proxyField = Joi.string() + .pattern(SUBSCRIPTION_PATTERNS.proxy) + .optional() + .allow('') + .allow(null); +const filterField = Joi.string() + .pattern(SUBSCRIPTION_PATTERNS.filter) + .optional() + .allow('') + .allow(null); + export default (app: Router) => { app.use('/subscriptions', route); @@ -38,19 +63,19 @@ export default (app: Router) => { .allow('') .allow(null), name: Joi.string().optional().allow('').allow(null), - url: Joi.string().required(), - whitelist: Joi.string().optional().allow('').allow(null), - blacklist: Joi.string().optional().allow('').allow(null), - branch: Joi.string().optional().allow('').allow(null), - dependences: Joi.string().optional().allow('').allow(null), + url: urlField.required(), + whitelist: filterField, + blacklist: filterField, + branch: branchField, + dependences: filterField, pull_type: Joi.string().optional().allow('').allow(null), pull_option: Joi.object().optional().allow('').allow(null), - extensions: Joi.string().optional().allow('').allow(null), + extensions: extensionsField, sub_before: Joi.string().optional().allow('').allow(null), sub_after: Joi.string().optional().allow('').allow(null), schedule_type: Joi.string().required(), alias: Joi.string().required(), - proxy: Joi.string().optional().allow('').allow(null), + proxy: proxyField, autoAddCron: Joi.boolean().optional().allow('').allow(null), autoDelCron: Joi.boolean().optional().allow('').allow(null), }), @@ -169,19 +194,19 @@ export default (app: Router) => { schedule: Joi.string().optional().allow('').allow(null), interval_schedule: Joi.object().optional().allow('').allow(null), name: Joi.string().optional().allow('').allow(null), - url: Joi.string().required(), - whitelist: Joi.string().optional().allow('').allow(null), - blacklist: Joi.string().optional().allow('').allow(null), - branch: Joi.string().optional().allow('').allow(null), - dependences: Joi.string().optional().allow('').allow(null), + url: urlField.required(), + whitelist: filterField, + blacklist: filterField, + branch: branchField, + dependences: filterField, pull_type: Joi.string().optional().allow('').allow(null), pull_option: Joi.object().optional().allow('').allow(null), schedule_type: Joi.string().optional().allow('').allow(null), - extensions: Joi.string().optional().allow('').allow(null), + extensions: extensionsField, sub_before: Joi.string().optional().allow('').allow(null), sub_after: Joi.string().optional().allow('').allow(null), alias: Joi.string().required(), - proxy: Joi.string().optional().allow('').allow(null), + proxy: proxyField, autoAddCron: Joi.boolean().optional().allow('').allow(null), autoDelCron: Joi.boolean().optional().allow('').allow(null), id: Joi.number().required(), diff --git a/back/config/index.ts b/back/config/index.ts index 00b2a7b8..530209c1 100644 --- a/back/config/index.ts +++ b/back/config/index.ts @@ -1,5 +1,7 @@ import dotenv from 'dotenv'; import path from 'path'; +import crypto from 'crypto'; +import fs from 'fs'; dotenv.config({ path: path.join(__dirname, '../../.env'), @@ -128,9 +130,47 @@ if (envFound.error) { throw new Error("⚠️ Couldn't find .env file ⚠️"); } +/** + * Resolve the JWT signing secret. A shared, source-code-public default secret + * lets anyone forge admin tokens, so: + * - honor an explicitly configured JWT_SECRET (must not be the old default); + * - otherwise generate a strong random secret once and persist it under the + * data dir so it stays stable across restarts and cluster workers; + * - never fall back to the public default in production. + */ +function resolveJwtSecret(): string { + const envSecret = process.env.JWT_SECRET; + if (envSecret && envSecret !== 'whyour-secret') { + return envSecret; + } + + const secretFile = path.join(dbPath, 'jwt_secret.key'); + try { + if (fs.existsSync(secretFile)) { + const existing = fs.readFileSync(secretFile, 'utf-8').trim(); + if (existing) { + return existing; + } + } + const generated = crypto.randomBytes(48).toString('hex'); + fs.mkdirSync(path.dirname(secretFile), { recursive: true }); + fs.writeFileSync(secretFile, generated, { mode: 0o600 }); + return generated; + } catch (error) { + if (process.env.NODE_ENV === 'production') { + throw new Error( + 'Refusing to start: unable to provision a secure JWT secret. Set JWT_SECRET.', + ); + } + return 'whyour-secret'; + } +} + +const jwtConfig = { ...config.jwt, secret: resolveJwtSecret() }; + export default { ...config, - jwt: config.jwt, + jwt: jwtConfig, baseUrl, rootPath, tmpPath, diff --git a/back/config/subscription.ts b/back/config/subscription.ts index 0973948b..0d9307b8 100644 --- a/back/config/subscription.ts +++ b/back/config/subscription.ts @@ -1,5 +1,6 @@ import { Subscription } from '../data/subscription'; import isNil from 'lodash/isNil'; +import { shellEscape } from '../shared/security'; export function formatUrl(doc: Subscription) { let url = doc.url; @@ -31,16 +32,22 @@ export function formatCommand(doc: Subscription, url?: string) { autoAddCron, autoDelCron, } = doc; + const addCron = isNil(autoAddCron) ? true : Boolean(autoAddCron); + const delCron = isNil(autoDelCron) ? true : Boolean(autoDelCron); + // Every user-controlled value is single-quote escaped so it can never break + // out of the shell command passed to spawn(command, { shell: '/bin/bash' }). if (type === 'file') { - command += `raw "${_url}" "${proxy || ''}" "${ - isNil(autoAddCron) ? true : Boolean(autoAddCron) - }" "${isNil(autoDelCron) ? true : Boolean(autoDelCron)}"`; + command += `raw ${shellEscape(_url)} ${shellEscape( + proxy || '', + )} "${addCron}" "${delCron}"`; } else { - command += `repo "${_url}" "${whitelist || ''}" "${blacklist || ''}" "${ - dependences || '' - }" "${branch || ''}" "${extensions || ''}" "${proxy || ''}" "${ - isNil(autoAddCron) ? true : Boolean(autoAddCron) - }" "${isNil(autoDelCron) ? true : Boolean(autoDelCron)}"`; + command += `repo ${shellEscape(_url)} ${shellEscape( + whitelist || '', + )} ${shellEscape(blacklist || '')} ${shellEscape( + dependences || '', + )} ${shellEscape(branch || '')} ${shellEscape(extensions || '')} ${shellEscape( + proxy || '', + )} "${addCron}" "${delCron}"`; } return command; } diff --git a/back/config/util.ts b/back/config/util.ts index f7f123a7..c7b3ad41 100644 --- a/back/config/util.ts +++ b/back/config/util.ts @@ -12,6 +12,7 @@ import { DependenceTypes } from '../data/dependence'; import { FormData } from 'undici'; import os from 'os'; import { maybeSudo, isInContainer } from './container'; +import { assertSafeDependenceName } from '../shared/security'; export * from './share'; @@ -569,6 +570,7 @@ export async function setSystemTimezone(timezone: string): Promise { } export function getGetCommand(type: DependenceTypes, name: string): string { + name = assertSafeDependenceName(name); const baseCommands = { [DependenceTypes.nodejs]: `pnpm ls -g | grep "${name}" | head -1`, [DependenceTypes.python3]: ` @@ -592,6 +594,7 @@ except: } export function getInstallCommand(type: DependenceTypes, name: string): string { + name = assertSafeDependenceName(name); const baseCommands = { [DependenceTypes.nodejs]: 'pnpm add -g', [DependenceTypes.python3]: @@ -614,6 +617,7 @@ export function getUninstallCommand( type: DependenceTypes, name: string, ): string { + name = assertSafeDependenceName(name); const baseCommands = { [DependenceTypes.nodejs]: 'pnpm remove -g', [DependenceTypes.python3]: diff --git a/back/services/config.ts b/back/services/config.ts index 0c5ea30a..682531dc 100644 --- a/back/services/config.ts +++ b/back/services/config.ts @@ -1,5 +1,5 @@ import { Service, Inject } from 'typedi'; -import path, { join } from 'path'; +import path from 'path'; import config from '../config'; import { getFileContentByName } from '../config/util'; import { t } from '../shared/i18n'; @@ -19,31 +19,30 @@ export default class ConfigService { if (normalized.startsWith('..') || path.isAbsolute(normalized)) { return res.send({ code: 403, message: t('文件无法访问') }); } - const resolvedRoot = path.resolve(config.rootPath, normalized); - const resolvedConfig = path.resolve(config.configPath, normalized); - const isValidPath = - resolvedRoot.startsWith(config.scriptPath) || - resolvedRoot.startsWith(config.configPath) || - resolvedConfig.startsWith(config.scriptPath) || - resolvedConfig.startsWith(config.configPath); - if (!isValidPath) { - return res.send({ code: 403, message: t('文件无法访问') }); - } - if (config.blackFileList.includes(path.basename(normalized))) { - return res.send({ code: 403, message: t('文件无法访问') }); - } + // Remote sample files are fetched (and path-checked) separately. if (filePath.startsWith('sample/')) { - const res = await request( - `https://gitlab.com/whyour/qinglong/-/raw/master/${filePath}`, + const sampleRes = await request( + `https://gitlab.com/whyour/qinglong/-/raw/master/${normalized}`, ); - content = await res.body.text(); - } else if (filePath.startsWith('data/scripts/')) { - content = await getFileContentByName(join(config.rootPath, filePath)); - } else { - content = await getFileContentByName(join(config.configPath, filePath)); + return res.send({ code: 200, data: await sampleRes.body.text() }); } + // Resolve the ACTUAL file path first, then validate that it stays within + // its allowed base. Validating a different path than the one read is what + // previously allowed `data/scripts/../db/database.sqlite` style traversal. + const isScripts = filePath.startsWith('data/scripts/'); + const base = path.resolve(isScripts ? config.scriptPath : config.configPath); + const rel = isScripts ? filePath.slice('data/scripts/'.length) : filePath; + const finalPath = path.resolve(base, rel); + if (finalPath !== base && !finalPath.startsWith(base + path.sep)) { + return res.send({ code: 403, message: t('文件无法访问') }); + } + if (config.blackFileList.includes(path.basename(finalPath))) { + return res.send({ code: 403, message: t('文件无法访问') }); + } + + content = await getFileContentByName(finalPath); res.send({ code: 200, data: content }); } } diff --git a/back/services/sshKey.ts b/back/services/sshKey.ts index 2683e7ed..36b0747b 100644 --- a/back/services/sshKey.ts +++ b/back/services/sshKey.ts @@ -8,6 +8,7 @@ import { formatUrl } from '../config/subscription'; import config from '../config'; import { fileExist, rmPath } from '../config/util'; import { writeFileWithLock } from '../shared/utils'; +import { isSafeSshConfigValue, SUBSCRIPTION_PATTERNS } from '../shared/security'; @Service() export default class SshKeyService { @@ -66,6 +67,14 @@ export default class SshKeyService { host: string, proxy?: string, ) { + // Prevent newline / shell-metacharacter injection into the ssh config file + // (e.g. an injected ProxyCommand directive executed by ssh on git pull). + if (!isSafeSshConfigValue(host)) { + throw new Error('Invalid ssh host'); + } + if (proxy && !SUBSCRIPTION_PATTERNS.proxy.test(proxy)) { + throw new Error('Invalid ssh proxy'); + } if (host === 'github.com') { host = `ssh.github.com\n Port 443\n HostkeyAlgorithms +ssh-rsa`; } diff --git a/back/shared/auth.ts b/back/shared/auth.ts index 8789f5cf..5b68ae6b 100644 --- a/back/shared/auth.ts +++ b/back/shared/auth.ts @@ -1,4 +1,5 @@ import { AuthInfo, TokenInfo } from '../data/system'; +import { safeCompare } from './security'; /** * Validates if a token exists in the authentication info. @@ -20,8 +21,8 @@ export function isValidToken( const { token = '', tokens = {} } = authInfo; - // Check legacy token field - if (headerToken === token) { + // Check legacy token field (constant-time) + if (token && safeCompare(headerToken, token)) { return true; } @@ -35,10 +36,12 @@ export function isValidToken( if (typeof platformTokens === 'string') { // Legacy format: single string token - return headerToken === platformTokens; + return safeCompare(headerToken, platformTokens); } else if (Array.isArray(platformTokens)) { - // New format: array of TokenInfo objects - return platformTokens.some((t: TokenInfo) => t && t.value === headerToken); + // New format: array of TokenInfo objects (constant-time per entry) + return platformTokens.some( + (t: TokenInfo) => t && safeCompare(headerToken, t.value), + ); } // Unexpected type - log warning and reject diff --git a/back/shared/security.ts b/back/shared/security.ts new file mode 100644 index 00000000..d613b164 --- /dev/null +++ b/back/shared/security.ts @@ -0,0 +1,72 @@ +/** + * Centralized input-hardening helpers used to neutralize command injection, + * argument injection and config-file injection across the backend. + * + * The task runner legitimately executes user scripts, but data fields such as + * git URLs, branches, dependency names and proxies are NOT meant to be code. + * These helpers keep such values from breaking out of the shell commands / + * config files they are interpolated into. + */ +import crypto from 'crypto'; + +/** + * POSIX single-quote escaping. Wraps a value so it is passed to the shell as a + * single literal argument, neutralizing $(), backticks, ;, |, &, redirects, + * whitespace and newlines. + */ +export function shellEscape(value: unknown): string { + const str = value === undefined || value === null ? '' : String(value); + return `'${str.replace(/'/g, `'\\''`)}'`; +} + +/** Characters that enable shell command injection / argument chaining. */ +const SHELL_METACHAR = /[;&|`$<>(){}\[\]'"\\!\n\r\t ]/; + +/** + * Validate a package/dependence name before it is interpolated into an + * install/uninstall/version command (pnpm/pip/apk/apt). Throws on anything that + * could break out of the command or inject an extra argument. + */ +export function assertSafeDependenceName(name: string): string { + const n = String(name ?? '').trim(); + if (!n || n.length > 214 || SHELL_METACHAR.test(n) || n.startsWith('-')) { + throw new Error('Invalid dependence name'); + } + return n; +} + +/** + * Joi-compatible patterns for subscription fields that flow into shell commands + * and ssh config files. They block shell metacharacters / newlines while still + * permitting legitimate values. + */ +export const SUBSCRIPTION_PATTERNS = { + // git/http(s)/ssh url: no whitespace or shell metacharacters, must not start with '-' + url: /^(?!-)[^\s;&|`$<>(){}'"\\]+$/, + // git ref: word chars, dots, slashes, dashes + branch: /^[\w.\/-]*$/, + // space/comma separated bare file extensions + extensions: /^[A-Za-z0-9 ,]*$/, + // host:port (consumed by the nc ProxyCommand) or empty + proxy: /^([\w.\-]+:\d+)?$/, + // regex filters: forbid line breaks and command-substitution chars, keep regex metachars + filter: /^[^\r\n`$\\]*$/, +}; + +/** + * Constant-time string comparison for tokens / secrets / passwords. Both inputs + * are hashed to a fixed length first so timingSafeEqual never throws on length + * mismatch and the comparison does not leak length via timing. + */ +export function safeCompare(a: string | undefined | null, b: string | undefined | null): boolean { + if (typeof a !== 'string' || typeof b !== 'string') return false; + const ha = crypto.createHash('sha256').update(a).digest(); + const hb = crypto.createHash('sha256').update(b).digest(); + return crypto.timingSafeEqual(ha, hb) && a.length === b.length; +} + +/** Reject values that are dangerous when written verbatim into an ssh config file. */ +export function isSafeSshConfigValue(value: string | undefined | null): boolean { + if (value === undefined || value === null) return true; + return !/[\r\n;&|`$<>()'"\\]/.test(String(value)); +}