mirror of
https://github.com/CherryHQ/cherry-studio.git
synced 2026-01-11 08:19:01 +08:00
fix(security): prevent path traversal vulnerability in DXT plugin system (#12377)
* fix(security): prevent path traversal vulnerability in DXT plugin system Add input validation to prevent path traversal attacks in DXT plugin handling: - Add sanitizeName() to filter dangerous characters from manifest.name - Add validateCommand() to reject commands with path traversal sequences - Add validateArgs() to validate command arguments - Remove unsafe fallback logic in cleanupDxtServer() The vulnerability allowed attackers to write files to arbitrary locations on Windows by crafting malicious DXT packages with path traversal sequences (e.g., "..\\..\\Windows\\System32\\") in manifest.name or command fields. * refactor: use path validation instead of input sanitization --------- Co-authored-by: defi-failure <159208748+defi-failure@users.noreply.github.com>
This commit is contained in:
亢奋猫
GitHub
parent
bdf8f103c8
commit
c5ea42ca3a
@ -8,6 +8,27 @@ import { v4 as uuidv4 } from 'uuid'
|
||||
|
||||
const logger = loggerService.withContext('DxtService')
|
||||
|
||||
/**
|
||||
* Ensure a target path is within the base directory to prevent path traversal attacks.
|
||||
* This is the correct approach: validate the final resolved path rather than sanitizing input.
|
||||
*
|
||||
* @param basePath - The base directory that the target must be within
|
||||
* @param targetPath - The target path to validate
|
||||
* @returns The resolved target path if valid
|
||||
* @throws Error if the target path escapes the base directory
|
||||
*/
|
||||
export function ensurePathWithin(basePath: string, targetPath: string): string {
|
||||
const resolvedBase = path.resolve(basePath)
|
||||
const resolvedTarget = path.resolve(path.normalize(targetPath))
|
||||
|
||||
// Must be direct child of base directory, no subdirectories allowed
|
||||
if (path.dirname(resolvedTarget) !== resolvedBase) {
|
||||
throw new Error('Path traversal detected: target path must be direct child of base directory')
|
||||
}
|
||||
|
||||
return resolvedTarget
|
||||
}
|
||||
|
||||
// Type definitions
|
||||
export interface DxtManifest {
|
||||
dxt_version: string
|
||||
@ -68,6 +89,76 @@ export interface DxtUploadResult {
|
||||
error?: string
|
||||
}
|
||||
|
||||
/**
|
||||
* Validate and sanitize a command to prevent path traversal attacks.
|
||||
* Commands should be either:
|
||||
* 1. Simple command names (e.g., "node", "python", "npx") - looked up in PATH
|
||||
* 2. Absolute paths (e.g., "/usr/bin/node", "C:\\Program Files\\node\\node.exe")
|
||||
* 3. Relative paths starting with ./ or .\ (relative to extractDir)
|
||||
*
|
||||
* Rejects commands containing path traversal sequences (..)
|
||||
*
|
||||
* @param command - The command to validate
|
||||
* @returns The validated command
|
||||
* @throws Error if command contains path traversal or is invalid
|
||||
*/
|
||||
export function validateCommand(command: string): string {
|
||||
if (!command || typeof command !== 'string') {
|
||||
throw new Error('Invalid command: command must be a non-empty string')
|
||||
}
|
||||
|
||||
const trimmed = command.trim()
|
||||
if (!trimmed) {
|
||||
throw new Error('Invalid command: command cannot be empty')
|
||||
}
|
||||
|
||||
// Check for path traversal sequences
|
||||
// This catches: .., ../, ..\, /../, \..\, etc.
|
||||
if (/(?:^|[/\\])\.\.(?:[/\\]|$)/.test(trimmed) || trimmed === '..') {
|
||||
throw new Error(`Invalid command: path traversal detected in "${command}"`)
|
||||
}
|
||||
|
||||
// Check for null bytes
|
||||
if (trimmed.includes('\0')) {
|
||||
throw new Error('Invalid command: null byte detected')
|
||||
}
|
||||
|
||||
return trimmed
|
||||
}
|
||||
|
||||
/**
|
||||
* Validate command arguments to prevent injection attacks.
|
||||
* Rejects arguments containing path traversal sequences.
|
||||
*
|
||||
* @param args - The arguments array to validate
|
||||
* @returns The validated arguments array
|
||||
* @throws Error if any argument contains path traversal
|
||||
*/
|
||||
export function validateArgs(args: string[]): string[] {
|
||||
if (!Array.isArray(args)) {
|
||||
throw new Error('Invalid args: must be an array')
|
||||
}
|
||||
|
||||
return args.map((arg, index) => {
|
||||
if (typeof arg !== 'string') {
|
||||
throw new Error(`Invalid args: argument at index ${index} must be a string`)
|
||||
}
|
||||
|
||||
// Check for null bytes
|
||||
if (arg.includes('\0')) {
|
||||
throw new Error(`Invalid args: null byte detected in argument at index ${index}`)
|
||||
}
|
||||
|
||||
// Check for path traversal in arguments that look like paths
|
||||
// Only validate if the arg contains path separators (indicating it's meant to be a path)
|
||||
if ((arg.includes('/') || arg.includes('\\')) && /(?:^|[/\\])\.\.(?:[/\\]|$)/.test(arg)) {
|
||||
throw new Error(`Invalid args: path traversal detected in argument at index ${index}`)
|
||||
}
|
||||
|
||||
return arg
|
||||
})
|
||||
}
|
||||
|
||||
export function performVariableSubstitution(
|
||||
value: string,
|
||||
extractDir: string,
|
||||
@ -134,12 +225,16 @@ export function applyPlatformOverrides(mcpConfig: any, extractDir: string, userC
|
||||
// Apply variable substitution to all string values
|
||||
if (resolvedConfig.command) {
|
||||
resolvedConfig.command = performVariableSubstitution(resolvedConfig.command, extractDir, userConfig)
|
||||
// Validate command after substitution to prevent path traversal attacks
|
||||
resolvedConfig.command = validateCommand(resolvedConfig.command)
|
||||
}
|
||||
|
||||
if (resolvedConfig.args) {
|
||||
resolvedConfig.args = resolvedConfig.args.map((arg: string) =>
|
||||
performVariableSubstitution(arg, extractDir, userConfig)
|
||||
)
|
||||
// Validate args after substitution to prevent path traversal attacks
|
||||
resolvedConfig.args = validateArgs(resolvedConfig.args)
|
||||
}
|
||||
|
||||
if (resolvedConfig.env) {
|
||||
@ -271,10 +366,8 @@ class DxtService {
|
||||
}
|
||||
|
||||
// Use server name as the final extract directory for automatic version management
|
||||
// Sanitize the name to prevent creating subdirectories
|
||||
const sanitizedName = manifest.name.replace(/\//g, '-')
|
||||
const serverDirName = `server-${sanitizedName}`
|
||||
const finalExtractDir = path.join(this.mcpDir, serverDirName)
|
||||
const serverDirName = `server-${manifest.name}`
|
||||
const finalExtractDir = ensurePathWithin(this.mcpDir, path.join(this.mcpDir, serverDirName))
|
||||
|
||||
// Clean up any existing version of this server
|
||||
if (fs.existsSync(finalExtractDir)) {
|
||||
@ -354,27 +447,15 @@ class DxtService {
|
||||
|
||||
public cleanupDxtServer(serverName: string): boolean {
|
||||
try {
|
||||
// Handle server names that might contain slashes (e.g., "anthropic/sequential-thinking")
|
||||
// by replacing slashes with the same separator used during installation
|
||||
const sanitizedName = serverName.replace(/\//g, '-')
|
||||
const serverDirName = `server-${sanitizedName}`
|
||||
const serverDir = path.join(this.mcpDir, serverDirName)
|
||||
const serverDirName = `server-${serverName}`
|
||||
const serverDir = ensurePathWithin(this.mcpDir, path.join(this.mcpDir, serverDirName))
|
||||
|
||||
// First try the sanitized path
|
||||
if (fs.existsSync(serverDir)) {
|
||||
logger.debug(`Removing DXT server directory: ${serverDir}`)
|
||||
fs.rmSync(serverDir, { recursive: true, force: true })
|
||||
return true
|
||||
}
|
||||
|
||||
// Fallback: try with original name in case it was stored differently
|
||||
const originalServerDir = path.join(this.mcpDir, `server-${serverName}`)
|
||||
if (fs.existsSync(originalServerDir)) {
|
||||
logger.debug(`Removing DXT server directory: ${originalServerDir}`)
|
||||
fs.rmSync(originalServerDir, { recursive: true, force: true })
|
||||
return true
|
||||
}
|
||||
|
||||
logger.warn(`Server directory not found: ${serverDir}`)
|
||||
return false
|
||||
} catch (error) {
|
||||
|
||||
202
src/main/services/__tests__/DxtService.test.ts
Normal file
202
src/main/services/__tests__/DxtService.test.ts
Normal file
@ -0,0 +1,202 @@
|
||||
import path from 'path'
|
||||
import { describe, expect, it } from 'vitest'
|
||||
|
||||
import { ensurePathWithin, validateArgs, validateCommand } from '../DxtService'
|
||||
|
||||
describe('ensurePathWithin', () => {
|
||||
const baseDir = '/home/user/mcp'
|
||||
|
||||
describe('valid paths', () => {
|
||||
it('should accept direct child paths', () => {
|
||||
expect(ensurePathWithin(baseDir, '/home/user/mcp/server-test')).toBe('/home/user/mcp/server-test')
|
||||
expect(ensurePathWithin(baseDir, '/home/user/mcp/my-server')).toBe('/home/user/mcp/my-server')
|
||||
})
|
||||
|
||||
it('should accept paths with unicode characters', () => {
|
||||
expect(ensurePathWithin(baseDir, '/home/user/mcp/服务器')).toBe('/home/user/mcp/服务器')
|
||||
expect(ensurePathWithin(baseDir, '/home/user/mcp/サーバー')).toBe('/home/user/mcp/サーバー')
|
||||
})
|
||||
})
|
||||
|
||||
describe('path traversal prevention', () => {
|
||||
it('should reject paths that escape base directory', () => {
|
||||
expect(() => ensurePathWithin(baseDir, '/home/user/mcp/../../../etc')).toThrow('Path traversal detected')
|
||||
expect(() => ensurePathWithin(baseDir, '/etc/passwd')).toThrow('Path traversal detected')
|
||||
expect(() => ensurePathWithin(baseDir, '/home/user')).toThrow('Path traversal detected')
|
||||
})
|
||||
|
||||
it('should reject subdirectories', () => {
|
||||
expect(() => ensurePathWithin(baseDir, '/home/user/mcp/sub/dir')).toThrow('Path traversal detected')
|
||||
expect(() => ensurePathWithin(baseDir, '/home/user/mcp/a/b/c')).toThrow('Path traversal detected')
|
||||
})
|
||||
|
||||
it('should reject Windows-style path traversal', () => {
|
||||
const winBase = 'C:\\Users\\user\\mcp'
|
||||
expect(() => ensurePathWithin(winBase, 'C:\\Users\\user\\mcp\\..\\..\\Windows\\System32')).toThrow(
|
||||
'Path traversal detected'
|
||||
)
|
||||
})
|
||||
|
||||
it('should reject null byte attacks', () => {
|
||||
const maliciousPath = path.join(baseDir, 'server\x00/../../../etc/passwd')
|
||||
expect(() => ensurePathWithin(baseDir, maliciousPath)).toThrow('Path traversal detected')
|
||||
})
|
||||
|
||||
it('should handle encoded traversal attempts', () => {
|
||||
expect(() => ensurePathWithin(baseDir, '/home/user/mcp/../escape')).toThrow('Path traversal detected')
|
||||
})
|
||||
})
|
||||
|
||||
describe('edge cases', () => {
|
||||
it('should reject base directory itself', () => {
|
||||
expect(() => ensurePathWithin(baseDir, '/home/user/mcp')).toThrow('Path traversal detected')
|
||||
})
|
||||
|
||||
it('should handle relative path construction', () => {
|
||||
const target = path.join(baseDir, 'server-name')
|
||||
expect(ensurePathWithin(baseDir, target)).toBe('/home/user/mcp/server-name')
|
||||
})
|
||||
})
|
||||
})
|
||||
|
||||
describe('validateCommand', () => {
|
||||
describe('valid commands', () => {
|
||||
it('should accept simple command names', () => {
|
||||
expect(validateCommand('node')).toBe('node')
|
||||
expect(validateCommand('python')).toBe('python')
|
||||
expect(validateCommand('npx')).toBe('npx')
|
||||
expect(validateCommand('uvx')).toBe('uvx')
|
||||
})
|
||||
|
||||
it('should accept absolute paths', () => {
|
||||
expect(validateCommand('/usr/bin/node')).toBe('/usr/bin/node')
|
||||
expect(validateCommand('/usr/local/bin/python3')).toBe('/usr/local/bin/python3')
|
||||
expect(validateCommand('C:\\Program Files\\nodejs\\node.exe')).toBe('C:\\Program Files\\nodejs\\node.exe')
|
||||
})
|
||||
|
||||
it('should accept relative paths starting with ./', () => {
|
||||
expect(validateCommand('./node_modules/.bin/tsc')).toBe('./node_modules/.bin/tsc')
|
||||
expect(validateCommand('.\\scripts\\run.bat')).toBe('.\\scripts\\run.bat')
|
||||
})
|
||||
|
||||
it('should trim whitespace', () => {
|
||||
expect(validateCommand(' node ')).toBe('node')
|
||||
expect(validateCommand('\tpython\n')).toBe('python')
|
||||
})
|
||||
})
|
||||
|
||||
describe('path traversal prevention', () => {
|
||||
it('should reject commands with path traversal (Unix style)', () => {
|
||||
expect(() => validateCommand('../../../bin/sh')).toThrow('path traversal detected')
|
||||
expect(() => validateCommand('../../etc/passwd')).toThrow('path traversal detected')
|
||||
expect(() => validateCommand('/usr/../../../bin/sh')).toThrow('path traversal detected')
|
||||
})
|
||||
|
||||
it('should reject commands with path traversal (Windows style)', () => {
|
||||
expect(() => validateCommand('..\\..\\..\\Windows\\System32\\cmd.exe')).toThrow('path traversal detected')
|
||||
expect(() => validateCommand('..\\..\\Windows\\System32\\calc.exe')).toThrow('path traversal detected')
|
||||
expect(() => validateCommand('C:\\..\\..\\Windows\\System32\\cmd.exe')).toThrow('path traversal detected')
|
||||
})
|
||||
|
||||
it('should reject just ".."', () => {
|
||||
expect(() => validateCommand('..')).toThrow('path traversal detected')
|
||||
})
|
||||
|
||||
it('should reject mixed style path traversal', () => {
|
||||
expect(() => validateCommand('../..\\mixed/..\\attack')).toThrow('path traversal detected')
|
||||
})
|
||||
})
|
||||
|
||||
describe('null byte injection', () => {
|
||||
it('should reject commands with null bytes', () => {
|
||||
expect(() => validateCommand('node\x00.exe')).toThrow('null byte detected')
|
||||
expect(() => validateCommand('python\0')).toThrow('null byte detected')
|
||||
})
|
||||
})
|
||||
|
||||
describe('edge cases', () => {
|
||||
it('should reject empty strings', () => {
|
||||
expect(() => validateCommand('')).toThrow('command must be a non-empty string')
|
||||
expect(() => validateCommand(' ')).toThrow('command cannot be empty')
|
||||
})
|
||||
|
||||
it('should reject non-string input', () => {
|
||||
// @ts-expect-error - testing runtime behavior
|
||||
expect(() => validateCommand(null)).toThrow('command must be a non-empty string')
|
||||
// @ts-expect-error - testing runtime behavior
|
||||
expect(() => validateCommand(undefined)).toThrow('command must be a non-empty string')
|
||||
// @ts-expect-error - testing runtime behavior
|
||||
expect(() => validateCommand(123)).toThrow('command must be a non-empty string')
|
||||
})
|
||||
})
|
||||
|
||||
describe('real-world attack scenarios', () => {
|
||||
it('should prevent Windows system32 command injection', () => {
|
||||
expect(() => validateCommand('../../../../Windows/System32/cmd.exe')).toThrow('path traversal detected')
|
||||
expect(() => validateCommand('..\\..\\..\\..\\Windows\\System32\\powershell.exe')).toThrow(
|
||||
'path traversal detected'
|
||||
)
|
||||
})
|
||||
|
||||
it('should prevent Unix bin injection', () => {
|
||||
expect(() => validateCommand('../../../../bin/bash')).toThrow('path traversal detected')
|
||||
expect(() => validateCommand('../../../usr/bin/curl')).toThrow('path traversal detected')
|
||||
})
|
||||
})
|
||||
})
|
||||
|
||||
describe('validateArgs', () => {
|
||||
describe('valid arguments', () => {
|
||||
it('should accept normal arguments', () => {
|
||||
expect(validateArgs(['--version'])).toEqual(['--version'])
|
||||
expect(validateArgs(['-y', '@anthropic/mcp-server'])).toEqual(['-y', '@anthropic/mcp-server'])
|
||||
expect(validateArgs(['install', 'package-name'])).toEqual(['install', 'package-name'])
|
||||
})
|
||||
|
||||
it('should accept arguments with safe paths', () => {
|
||||
expect(validateArgs(['./src/index.ts'])).toEqual(['./src/index.ts'])
|
||||
expect(validateArgs(['/absolute/path/file.js'])).toEqual(['/absolute/path/file.js'])
|
||||
})
|
||||
|
||||
it('should accept empty array', () => {
|
||||
expect(validateArgs([])).toEqual([])
|
||||
})
|
||||
})
|
||||
|
||||
describe('path traversal prevention', () => {
|
||||
it('should reject arguments with path traversal', () => {
|
||||
expect(() => validateArgs(['../../../etc/passwd'])).toThrow('path traversal detected')
|
||||
expect(() => validateArgs(['--config', '../../secrets.json'])).toThrow('path traversal detected')
|
||||
expect(() => validateArgs(['..\\..\\Windows\\System32\\config'])).toThrow('path traversal detected')
|
||||
})
|
||||
|
||||
it('should only check path-like arguments', () => {
|
||||
// Arguments without path separators should pass even with dots
|
||||
expect(validateArgs(['..version'])).toEqual(['..version'])
|
||||
expect(validateArgs(['test..name'])).toEqual(['test..name'])
|
||||
})
|
||||
})
|
||||
|
||||
describe('null byte injection', () => {
|
||||
it('should reject arguments with null bytes', () => {
|
||||
expect(() => validateArgs(['file\x00.txt'])).toThrow('null byte detected')
|
||||
expect(() => validateArgs(['--config', 'path\0name'])).toThrow('null byte detected')
|
||||
})
|
||||
})
|
||||
|
||||
describe('edge cases', () => {
|
||||
it('should reject non-array input', () => {
|
||||
// @ts-expect-error - testing runtime behavior
|
||||
expect(() => validateArgs('not an array')).toThrow('must be an array')
|
||||
// @ts-expect-error - testing runtime behavior
|
||||
expect(() => validateArgs(null)).toThrow('must be an array')
|
||||
})
|
||||
|
||||
it('should reject non-string elements', () => {
|
||||
// @ts-expect-error - testing runtime behavior
|
||||
expect(() => validateArgs([123])).toThrow('must be a string')
|
||||
// @ts-expect-error - testing runtime behavior
|
||||
expect(() => validateArgs(['valid', null])).toThrow('must be a string')
|
||||
})
|
||||
})
|
||||
})
|
||||
Loading…
Reference in New Issue
Block a user