Skip to content

Commit 0ff683e

Browse files
committed
fix(ssh): comprehensive security enhancements and validation improvements
This commit addresses multiple security concerns identified in the PR review: **Security Enhancements:** - Add SSH agent socket path validation to prevent command injection - Implement repository path validation with stricter rules (hostname, no traversal, .git extension) - Add host key verification using hardcoded trusted fingerprints (prevents MITM attacks) - Add chunk count limit (10,000) to prevent memory fragmentation attacks - Fix timeout cleanup in error paths to prevent memory leaks **Type Safety Improvements:** - Add SSH2ServerOptions interface for proper server configuration typing - Add SSH2ConnectionInternals interface for internal ssh2 protocol types - Replace Function type with proper signature in _handlers **Configuration Changes:** - Use fixed path for proxy host keys (.ssh/proxy_host_key) - Ensure consistent host key location across all SSH operations **Security Tests:** - Add comprehensive security test suite (test/ssh/security.test.ts) - Test repository path validation (traversal, special chars, invalid formats) - Test command injection prevention - Test pack data chunk limits All 34 SSH tests passing (27 server + 7 security tests).
1 parent 4230bc5 commit 0ff683e

File tree

6 files changed

+601
-99
lines changed

6 files changed

+601
-99
lines changed

src/config/index.ts

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -314,20 +314,21 @@ export const getMaxPackSizeBytes = (): number => {
314314
};
315315

316316
export const getSSHConfig = () => {
317-
// Default host key paths - auto-generated if not present
317+
// The proxy host key is auto-generated at startup if not present
318+
// This key is only used to identify the proxy server to clients (like SSL cert)
319+
// It is NOT configurable to ensure consistent behavior
318320
const defaultHostKey = {
319-
privateKeyPath: '.ssh/host_key',
320-
publicKeyPath: '.ssh/host_key.pub',
321+
privateKeyPath: '.ssh/proxy_host_key',
322+
publicKeyPath: '.ssh/proxy_host_key.pub',
321323
};
322324

323325
try {
324326
const config = loadFullConfiguration();
325327
const sshConfig = config.ssh || { enabled: false };
326328

327-
// Always ensure hostKey is present with defaults
328-
// The hostKey identifies the proxy server to clients
329+
// The host key is a server identity, not user configuration
329330
if (sshConfig.enabled) {
330-
sshConfig.hostKey = sshConfig.hostKey || defaultHostKey;
331+
sshConfig.hostKey = defaultHostKey;
331332
}
332333

333334
return sshConfig;
@@ -340,9 +341,8 @@ export const getSSHConfig = () => {
340341
const userConfig = JSON.parse(userConfigContent);
341342
const sshConfig = userConfig.ssh || { enabled: false };
342343

343-
// Always ensure hostKey is present with defaults
344344
if (sshConfig.enabled) {
345-
sshConfig.hostKey = sshConfig.hostKey || defaultHostKey;
345+
sshConfig.hostKey = defaultHostKey;
346346
}
347347

348348
return sshConfig;

src/proxy/processors/push-action/PullRemoteSSH.ts

Lines changed: 148 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,10 @@
11
import { Action, Step } from '../../actions';
22
import { PullRemoteBase, CloneResult } from './PullRemoteBase';
33
import { ClientWithUser } from '../../ssh/types';
4+
import { DEFAULT_KNOWN_HOSTS } from '../../ssh/knownHosts';
45
import { spawn } from 'child_process';
6+
import { execSync } from 'child_process';
7+
import * as crypto from 'crypto';
58
import fs from 'fs';
69
import path from 'path';
710
import os from 'os';
@@ -11,6 +14,121 @@ import os from 'os';
1114
* Uses system git with SSH agent forwarding for cloning
1215
*/
1316
export class PullRemoteSSH extends PullRemoteBase {
17+
/**
18+
* Validate agent socket path to prevent command injection
19+
* Only allows safe characters in Unix socket paths
20+
*/
21+
private validateAgentSocketPath(socketPath: string | undefined): string {
22+
if (!socketPath) {
23+
throw new Error(
24+
'SSH agent socket path not found. ' +
25+
'Ensure SSH_AUTH_SOCK is set or agent forwarding is enabled.',
26+
);
27+
}
28+
29+
// Unix socket paths should only contain alphanumeric, dots, slashes, underscores, hyphens
30+
// and allow common socket path patterns like /tmp/ssh-*/agent.*
31+
const safePathRegex = /^[a-zA-Z0-9/_.\-*]+$/;
32+
if (!safePathRegex.test(socketPath)) {
33+
throw new Error(
34+
`Invalid SSH agent socket path: contains unsafe characters. Path: ${socketPath}`,
35+
);
36+
}
37+
38+
// Additional validation: path should start with / (absolute path)
39+
if (!socketPath.startsWith('/')) {
40+
throw new Error(
41+
`Invalid SSH agent socket path: must be an absolute path. Path: ${socketPath}`,
42+
);
43+
}
44+
45+
return socketPath;
46+
}
47+
48+
/**
49+
* Create a secure known_hosts file with hardcoded verified host keys
50+
* This prevents MITM attacks by using pre-verified fingerprints
51+
*
52+
* NOTE: We use hardcoded fingerprints from DEFAULT_KNOWN_HOSTS, NOT ssh-keyscan,
53+
* because ssh-keyscan itself is vulnerable to MITM attacks.
54+
*/
55+
private async createKnownHostsFile(tempDir: string, sshUrl: string): Promise<string> {
56+
const knownHostsPath = path.join(tempDir, 'known_hosts');
57+
58+
// Extract hostname from SSH URL (git@github.com:org/repo.git -> github.com)
59+
const hostMatch = sshUrl.match(/git@([^:]+):/);
60+
if (!hostMatch) {
61+
throw new Error(`Cannot extract hostname from SSH URL: ${sshUrl}`);
62+
}
63+
64+
const hostname = hostMatch[1];
65+
66+
// Get the known host key for this hostname from hardcoded fingerprints
67+
const knownFingerprint = DEFAULT_KNOWN_HOSTS[hostname];
68+
if (!knownFingerprint) {
69+
throw new Error(
70+
`No known host key for ${hostname}. ` +
71+
`Supported hosts: ${Object.keys(DEFAULT_KNOWN_HOSTS).join(', ')}. ` +
72+
`To add support for ${hostname}, add its ed25519 key fingerprint to DEFAULT_KNOWN_HOSTS.`,
73+
);
74+
}
75+
76+
// Fetch the actual host key from the remote server to get the public key
77+
// We'll verify its fingerprint matches our hardcoded one
78+
let actualHostKey: string;
79+
try {
80+
const output = execSync(`ssh-keyscan -t ed25519 ${hostname} 2>/dev/null`, {
81+
encoding: 'utf-8',
82+
timeout: 5000,
83+
});
84+
85+
// Parse ssh-keyscan output: "hostname ssh-ed25519 AAAAC3Nz..."
86+
const keyLine = output.split('\n').find((line) => line.includes('ssh-ed25519'));
87+
if (!keyLine) {
88+
throw new Error('No ed25519 key found in ssh-keyscan output');
89+
}
90+
91+
actualHostKey = keyLine.trim();
92+
93+
// Verify the fingerprint matches our hardcoded trusted fingerprint
94+
// Extract the public key portion
95+
const keyParts = actualHostKey.split(' ');
96+
if (keyParts.length < 2) {
97+
throw new Error('Invalid ssh-keyscan output format');
98+
}
99+
100+
const publicKeyBase64 = keyParts[1];
101+
const publicKeyBuffer = Buffer.from(publicKeyBase64, 'base64');
102+
103+
// Calculate SHA256 fingerprint
104+
const hash = crypto.createHash('sha256').update(publicKeyBuffer).digest('base64');
105+
const calculatedFingerprint = `SHA256:${hash}`;
106+
107+
// Verify against hardcoded fingerprint
108+
if (calculatedFingerprint !== knownFingerprint) {
109+
throw new Error(
110+
`Host key verification failed for ${hostname}!\n` +
111+
`Expected fingerprint: ${knownFingerprint}\n` +
112+
`Received fingerprint: ${calculatedFingerprint}\n` +
113+
`WARNING: This could indicate a man-in-the-middle attack!\n` +
114+
`If the host key has legitimately changed, update DEFAULT_KNOWN_HOSTS.`,
115+
);
116+
}
117+
118+
console.log(`[SSH] ✓ Host key verification successful for ${hostname}`);
119+
console.log(`[SSH] Fingerprint: ${calculatedFingerprint}`);
120+
} catch (error) {
121+
throw new Error(
122+
`Failed to verify host key for ${hostname}: ${error instanceof Error ? error.message : String(error)}`,
123+
);
124+
}
125+
126+
// Write the verified known_hosts file
127+
await fs.promises.writeFile(knownHostsPath, actualHostKey + '\n', { mode: 0o600 });
128+
129+
return knownHostsPath;
130+
}
131+
14132
/**
15133
* Convert HTTPS URL to SSH URL
16134
*/
@@ -27,6 +145,7 @@ export class PullRemoteSSH extends PullRemoteBase {
27145

28146
/**
29147
* Clone repository using system git with SSH agent forwarding
148+
* Implements secure SSH configuration with host key verification
30149
*/
31150
private async cloneWithSystemGit(
32151
client: ClientWithUser,
@@ -40,22 +159,34 @@ export class PullRemoteSSH extends PullRemoteBase {
40159

41160
step.log(`Cloning repository via system git: ${sshUrl}`);
42161

43-
// Create temporary SSH config to use proxy's agent socket
162+
// Create temporary directory for SSH config and known_hosts
44163
const tempDir = await fs.promises.mkdtemp(path.join(os.tmpdir(), 'git-proxy-ssh-'));
45164
const sshConfigPath = path.join(tempDir, 'ssh_config');
46165

47-
// Get the agent socket path from the client connection
48-
const agentSocketPath = (client as any)._agent?._sock?.path || process.env.SSH_AUTH_SOCK;
166+
try {
167+
// Validate and get the agent socket path
168+
const rawAgentSocketPath = (client as any)._agent?._sock?.path || process.env.SSH_AUTH_SOCK;
169+
const agentSocketPath = this.validateAgentSocketPath(rawAgentSocketPath);
170+
171+
step.log(`Using SSH agent socket: ${agentSocketPath}`);
172+
173+
// Create secure known_hosts file with verified host keys
174+
const knownHostsPath = await this.createKnownHostsFile(tempDir, sshUrl);
175+
step.log(`Created secure known_hosts file with verified host keys`);
49176

50-
const sshConfig = `Host *
51-
StrictHostKeyChecking no
52-
UserKnownHostsFile /dev/null
177+
// Create secure SSH config with StrictHostKeyChecking enabled
178+
const sshConfig = `Host *
179+
StrictHostKeyChecking yes
180+
UserKnownHostsFile ${knownHostsPath}
53181
IdentityAgent ${agentSocketPath}
182+
# Additional security settings
183+
HashKnownHosts no
184+
PasswordAuthentication no
185+
PubkeyAuthentication yes
54186
`;
55187

56-
await fs.promises.writeFile(sshConfigPath, sshConfig);
188+
await fs.promises.writeFile(sshConfigPath, sshConfig, { mode: 0o600 });
57189

58-
try {
59190
await new Promise<void>((resolve, reject) => {
60191
const gitProc = spawn(
61192
'git',
@@ -64,7 +195,7 @@ export class PullRemoteSSH extends PullRemoteBase {
64195
cwd: action.proxyGitPath,
65196
env: {
66197
...process.env,
67-
GIT_SSH_COMMAND: `ssh -F ${sshConfigPath}`,
198+
GIT_SSH_COMMAND: `ssh -F "${sshConfigPath}"`,
68199
},
69200
},
70201
);
@@ -82,10 +213,15 @@ export class PullRemoteSSH extends PullRemoteBase {
82213

83214
gitProc.on('close', (code) => {
84215
if (code === 0) {
85-
step.log(`Successfully cloned repository (depth=1)`);
216+
step.log(`Successfully cloned repository (depth=1) with secure SSH verification`);
86217
resolve();
87218
} else {
88-
reject(new Error(`git clone failed (code ${code}): ${stderr}`));
219+
reject(
220+
new Error(
221+
`git clone failed (code ${code}): ${stderr}\n` +
222+
`This may indicate a host key verification failure or network issue.`,
223+
),
224+
);
89225
}
90226
});
91227

@@ -94,7 +230,7 @@ export class PullRemoteSSH extends PullRemoteBase {
94230
});
95231
});
96232
} finally {
97-
// Cleanup temp SSH config
233+
// Cleanup temp SSH config and known_hosts
98234
await fs.promises.rm(tempDir, { recursive: true, force: true });
99235
}
100236
}

0 commit comments

Comments
 (0)