diff --git a/CLAUDE.md b/CLAUDE.md index fecb101..fd18ba1 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -653,6 +653,27 @@ against a real server whose `PATH` holds a fake `git`. The directory name is one path segment (`DIR_NAME_RE`), joined onto a parent that passed `isPathAllowed()` — a registered workdir widens that allowlist, which is why the gate cannot be skipped here any more than in `POST /api/projects`. +- **The AUTHORITY is user-supplied too, and the leading-`-` check does not see it.** + `ssh://-oProxyCommand=id/x` and `git@-oProxyCommand=id:a/b` are URLs whose HOST is + an ssh option (CVE-2017-1000117) — the option sits behind the scheme or the `@`. + `authorityIsSafe()` drops the `:` and refuses any `@`-part opening with `-`. + Modern git passes `--` to ssh itself, so this is a belt on a brace; the rule that + nothing handed to git may look like an option may not hold only on a patched binary. +- **The parent is re-checked after `realpath`, and the target is `lstat`ed.** + `isPathAllowed()` reads a STRING, so a symlink inside an allowed root is a legal + path pointing anywhere on disk — the same layer-2 reasoning as `pwd -P` in the + remote file browser. And `existsSync()` is false for a DANGLING symlink, which git + would happily follow and populate; `lstat` sees the link itself. +- **The timeout kills the process GROUP, and two clones run at a time.** `git clone` + is a parent to `git-remote-https` / `ssh` / `index-pack`; killing the `git` pid + alone leaves those writing into the tree the handler is about to `rmSync`. The + child is `detached` on POSIX for that reason (not on Windows, where it opens a + console). `MAX_CONCURRENT_CLONES` is 2 — each clone holds its request open for as + long as it runs, so an unbounded one is a disk-filling primitive behind one button. +- **The HOST is not filtered, deliberately.** Any reachable `http(s)`/`ssh`/`git` + host is clonable, including RFC1918 and loopback — cloning from an internal GitLab + is the feature. The endpoint sits BELOW `auth.authMiddleware`, so this is a + logged-in user's own fetch, not an open proxy. - **It must fail, not wait.** stdin is closed, `GIT_TERMINAL_PROMPT=0`, no TTY: a credential or host-key question fails at once (measured: a private/nonexistent GitHub URL answers in ~0.4 s with git's own line). `CCS_GIT_CLONE_TIMEOUT_MS` (10 min) is the diff --git a/git-clone.js b/git-clone.js index ef850aa..eedf118 100644 --- a/git-clone.js +++ b/git-clone.js @@ -18,6 +18,12 @@ // - THE DIRECTORY NAME IS A SINGLE PATH SEGMENT. Derived from the URL or given by // the user, it is joined onto a parent that passed isPathAllowed(); `..`, `/` or a // leading `.` would let the clone land somewhere else. +// - AND THE AUTHORITY IS PART OF THE ARGV. `ssh://-oProxyCommand=id/x` and +// `git@-oProxyCommand=id:x/y` are URLs whose HOST is an ssh option — CVE-2017-1000117. +// The leading `-` check on the whole string does not see them, because the option is +// behind the scheme or the `@`. Current git passes `--` to ssh itself, so this is a +// belt on top of a brace; the rule above says nothing we hand git may look like an +// option, and a rule that holds only on a patched binary is not that rule. const ALLOWED_PROTOCOLS = ['http', 'https', 'ssh', 'git']; @@ -31,6 +37,12 @@ const URL_RE = /^(?:https?|ssh|git):\/\/[^\s/@]+(?:@[^\s/@]+)?(?::\d+)?\/[^\s]+$ // scp-like: git@github.com:user/repo.git — no scheme, exactly one ':' after the host. const SCP_RE = /^[A-Za-z0-9._-]+@[A-Za-z0-9._-]+:[^\s:]+$/; +// Every `@`-separated part of an authority reaches ssh/git as argv; none may open +// with `-`. The trailing `:` is dropped first so a port cannot hide the host. +function authorityIsSafe(authority) { + return authority.replace(/:\d+$/, '').split('@').every(part => part && !part.startsWith('-')); +} + /** * @param {unknown} raw * @returns {{ url: string, repoName: string } | null} null when the URL is not one @@ -41,6 +53,12 @@ function parseCloneUrl(raw) { const url = raw.trim(); if (!url || url.startsWith('-') || /[\s\0]/.test(url)) return null; if (!URL_RE.test(url) && !SCP_RE.test(url)) return null; + // `@[:port]` — the part before the path in a URL, before the `:` in + // an scp-like address. + const authority = URL_RE.test(url) + ? url.slice(url.indexOf('://') + 3).split('/')[0] + : url.split(':')[0]; + if (!authorityIsSafe(authority)) return null; const tail = url.replace(/\/+$/, '').split(/[/:]/).pop().replace(/\.git$/i, ''); if (!DIR_NAME_RE.test(tail)) return null; return { url, repoName: tail }; @@ -68,4 +86,4 @@ function cloneEnv(base = process.env) { return { ...base, GIT_TERMINAL_PROMPT: '0', GIT_ALLOW_PROTOCOL: ALLOWED_PROTOCOLS.join(':') }; } -module.exports = { ALLOWED_PROTOCOLS, parseCloneUrl, isValidDirName, isValidBranch, cloneArgs, cloneEnv }; +module.exports = { ALLOWED_PROTOCOLS, parseCloneUrl, authorityIsSafe, isValidDirName, isValidBranch, cloneArgs, cloneEnv }; diff --git a/server.js b/server.js index 3a78988..5b77f71 100644 --- a/server.js +++ b/server.js @@ -9607,6 +9607,11 @@ function registerLocalProject(name, workdir) { // Local projects only: a clone on a remote host would need to run over SSH, and // the remote project flow already takes an existing path. const GIT_CLONE_TIMEOUT_MS = parseInt(process.env.CCS_GIT_CLONE_TIMEOUT_MS || '600000', 10) || 600000; +// A clone holds its request open for as long as it runs, writes into the filesystem +// and costs a network fetch. Two at a time is generous for a tool one person drives; +// without a cap, a page that fires the button in a loop fills the disk. +const MAX_CONCURRENT_CLONES = 2; +let clonesInFlight = 0; app.post('/api/projects/clone', (req, res) => { const { url, branch = '', parentDir, name = '', dirName = '', shallow = false } = req.body || {}; const parsed = gitClone.parseCloneUrl(url); @@ -9615,28 +9620,47 @@ app.post('/api/projects/clone', (req, res) => { if (dirName && !gitClone.isValidDirName(dirName)) return res.status(400).json({ error: 'invalid directory name' }); if (!parentDir || typeof parentDir !== 'string') return res.status(400).json({ error: 'parentDir required' }); if (!isPathAllowed(parentDir)) return res.status(403).json({ error: 'path not allowed' }); - const parent = path.resolve(parentDir); + // The parent is resolved to its PHYSICAL path and re-checked: isPathAllowed() reads + // a string, so a symlink inside an allowed root otherwise points the clone anywhere + // on disk. Same rule the remote browser's layer 2 applies with `pwd -P`. + let parent; + try { parent = fs.realpathSync(path.resolve(parentDir)); } catch { return res.status(400).json({ error: 'parent directory does not exist' }); } + if (!isPathAllowed(parent)) return res.status(403).json({ error: 'path not allowed' }); let parentIsDir = false; try { parentIsDir = fs.statSync(parent).isDirectory(); } catch {} if (!parentIsDir) return res.status(400).json({ error: 'parent directory does not exist' }); const dir = dirName || parsed.repoName; const target = path.join(parent, dir); - if (fs.existsSync(target)) return res.status(409).json({ error: `already exists: ${target}` }); + // lstat, not existsSync: a DANGLING symlink at the target does not "exist", and git + // would follow it and populate whatever it names. + let targetTaken = true; + try { fs.lstatSync(target); } catch { targetTaken = false; } + if (targetTaken) return res.status(409).json({ error: `already exists: ${target}` }); + if (clonesInFlight >= MAX_CONCURRENT_CLONES) return res.status(429).json({ error: 'another clone is already running' }); const args = gitClone.cloneArgs({ url: parsed.url, target, branch, shallow: !!shallow }); let stderr = ''; let child; + // `git clone` is a parent to git-remote-https / ssh / index-pack, and killing it + // alone leaves those writing into the tree the timeout handler is about to remove. + // A detached child is its own process GROUP, so one kill reaches all of them. + const ownGroup = process.platform !== 'win32'; try { // stdin is closed, GIT_TERMINAL_PROMPT=0 and no TTY: a credential or host-key // question fails at once instead of holding the request until the timeout. - child = spawnProc('git', args, { cwd: parent, env: gitClone.cloneEnv(), stdio: ['ignore', 'ignore', 'pipe'] }); + child = spawnProc('git', args, { cwd: parent, env: gitClone.cloneEnv(), stdio: ['ignore', 'ignore', 'pipe'], detached: ownGroup }); } catch (e) { return res.status(500).json({ error: e.message }); } + clonesInFlight++; child.stderr.on('data', d => { if (stderr.length < 8192) stderr += String(d); }); - const timer = setTimeout(() => { try { child.kill('SIGKILL'); } catch {} }, GIT_CLONE_TIMEOUT_MS); + const timer = setTimeout(() => { + try { if (ownGroup) process.kill(-child.pid, 'SIGKILL'); else child.kill('SIGKILL'); } + catch { try { child.kill('SIGKILL'); } catch {} } + }, GIT_CLONE_TIMEOUT_MS); let settled = false; const finish = (code, err) => { if (settled) return; settled = true; + clonesInFlight--; clearTimeout(timer); if (err || code !== 0) { // git removes its own half-clone on failure; a SIGKILL from the timer does not. diff --git a/test/git-clone.test.js b/test/git-clone.test.js index 4f2abbc..6f94ba5 100644 --- a/test/git-clone.test.js +++ b/test/git-clone.test.js @@ -39,6 +39,13 @@ check('embedded whitespace is refused', G.parseCloneUrl('git@h:a b'), null); check('a repo name of `..` is refused', G.parseCloneUrl('https://h/x/..'), null); check('a dot-leading repo name is refused', G.parseCloneUrl('https://h/x/.git'), null); check('non-string input is refused', G.parseCloneUrl({ toString: () => 'https://h/a/b' }), null); +// CVE-2017-1000117: the host itself is an ssh option. The leading `-` check on the +// whole string does not see it — it sits behind the scheme or the `@`. +check('an option-shaped host behind the scheme is refused', G.parseCloneUrl('ssh://-oProxyCommand=id/x/y'), null); +check('an option-shaped host behind the @ is refused', G.parseCloneUrl('git@-oProxyCommand=id:a/b'), null); +check('an option-shaped user is refused', G.parseCloneUrl('ssh://-u@h/a/b'), null); +check('a port does not hide the host', G.parseCloneUrl('ssh://git@host:2222/a/b.git').repoName, 'b'); +check('a password in the userinfo is still a URL', G.parseCloneUrl('https://user:pw@h/a/b').repoName, 'b'); console.log('\n— git-clone.js: argv and environment —'); check('argv puts -- before the URL and target', G.cloneArgs({ url: 'U', target: 'T' }), ['clone', '--', 'U', 'T']); @@ -65,6 +72,10 @@ process.on('exit', () => { for (const d of [APP_DIR, HOME_DIR, BIN_DIR, OUTSIDE] fs.mkdirSync(path.join(APP_DIR, 'data'), { recursive: true }); const WORKSPACE = path.join(APP_DIR, 'workspace'); fs.mkdirSync(WORKSPACE, { recursive: true }); +// The endpoint answers with the PHYSICAL parent (realpath), so on macOS, where +// /var is a symlink to /private/var, the path it returns is not the one composed +// here. Requests still send the unresolved WORKSPACE — that is what exercises it. +const WS_REAL = fs.realpathSync(WORKSPACE); const GIT_LOG = path.join(APP_DIR, 'git-calls.log'); // Fake git: one line of JSON per call — argv, cwd, the two env vars we care about. @@ -75,6 +86,7 @@ PATH=/usr/bin:/bin:$PATH printf '%s\\n' "$(node -e 'console.log(JSON.stringify({argv:process.argv.slice(1),cwd:process.cwd(),prompt:process.env.GIT_TERMINAL_PROMPT,allow:process.env.GIT_ALLOW_PROTOCOL}))' -- "$@")" >> "${GIT_LOG}" for last; do :; done mkdir -p "$last/.git" +case "$*" in *slow*) sleep 3;; esac case "$*" in *boom*) echo "fatal: repository 'boom' not found" >&2; exit 128;; esac exit 0 `, { mode: 0o755 }); @@ -122,17 +134,24 @@ const gitCalls = () => fs.existsSync(GIT_LOG) ? fs.readFileSync(GIT_LOG, 'utf8') check('a parent that does not exist is 400', (await clone({ url: 'https://h/a/b', parentDir: path.join(WORKSPACE, 'nope') })).status, 400); fs.mkdirSync(path.join(WORKSPACE, 'taken')); check('an existing target is 409', (await clone({ url: 'https://h/a/taken.git', parentDir: WORKSPACE })).status, 409); + // A symlink inside an allowed root is a string that passes isPathAllowed() and a + // directory that is somewhere else — so the parent is re-checked after realpath. + fs.symlinkSync(OUTSIDE, path.join(WORKSPACE, 'linkout')); + check('a parent that is a symlink out of the allowed roots is 403', (await clone({ url: 'https://h/a/b', parentDir: path.join(WORKSPACE, 'linkout') })).status, 403); + // A DANGLING symlink does not "exist" — git would follow it and populate its target. + fs.symlinkSync(path.join(OUTSIDE, 'nowhere'), path.join(WORKSPACE, 'ghost')); + check('a target that is a dangling symlink is 409, not a clone', (await clone({ url: 'https://h/a/ghost.git', parentDir: WORKSPACE })).status, 409); check('none of the refusals reached git', gitCalls().length, 0); console.log('\n— a clone that succeeds —'); const ok = await clone({ url: 'https://github.com/acme/widget.git', parentDir: WORKSPACE, branch: 'dev' }); check('answers 200 ok', [ok.status, ok.json?.ok], [200, true]); - const target = path.join(WORKSPACE, 'widget'); + const target = path.join(WS_REAL, 'widget'); check('workdir is /', ok.json?.workdir, target); check('the directory exists', fs.existsSync(path.join(target, '.git')), true); const call = gitCalls()[0]; check('git argv: clone --branch dev -- ', call.argv, ['clone', '--branch', 'dev', '--', 'https://github.com/acme/widget.git', target]); - check('git ran in the parent', fs.realpathSync(call.cwd), fs.realpathSync(WORKSPACE)); + check('git ran in the parent', fs.realpathSync(call.cwd), WS_REAL); check('git was told never to prompt', call.prompt, '0'); check('git was pinned to the transport allowlist', call.allow, 'http:https:ssh:git'); const projects = (await api('GET', '/api/projects')).json; @@ -146,12 +165,20 @@ const gitCalls = () => fs.existsSync(GIT_LOG) ? fs.readFileSync(GIT_LOG, 'utf8') const bad = await clone({ url: 'https://h/a/boom.git', parentDir: WORKSPACE, name: 'named' }); check('answers 502', bad.status, 502); check('with git\'s own last stderr line', bad.json?.error, "fatal: repository 'boom' not found"); - check('the half-made directory is removed', fs.existsSync(path.join(WORKSPACE, 'boom')), false); + check('the half-made directory is removed', fs.existsSync(path.join(WS_REAL, 'boom')), false); check('and no project was registered', (await api('GET', '/api/projects')).json.some(p => p.name === 'named'), false); + console.log('\n— the concurrency cap —'); + const slowA = clone({ url: 'https://h/a/slow1.git', parentDir: WORKSPACE }); + const slowB = clone({ url: 'https://h/a/slow2.git', parentDir: WORKSPACE }); + await sleep(400); + check('a third clone while two run is 429', (await clone({ url: 'https://h/a/slow3.git', parentDir: WORKSPACE })).status, 429); + check('the two that were admitted still succeed', (await Promise.all([slowA, slowB])).map(r => r.status), [200, 200]); + check('and the slot is given back', (await clone({ url: 'https://h/a/after.git', parentDir: WORKSPACE })).status, 200); + console.log('\n— dirName and name overrides —'); const named = await clone({ url: 'git@github.com:acme/widget.git', parentDir: WORKSPACE, dirName: 'widget2', name: 'Widget Two', shallow: true }); - check('dirName picks the folder', named.json?.workdir, path.join(WORKSPACE, 'widget2')); + check('dirName picks the folder', named.json?.workdir, path.join(WS_REAL, 'widget2')); check('shallow adds --depth 1', gitCalls().pop().argv.slice(0, 3), ['clone', '--depth', '1']); check('name is the project name', (await api('GET', '/api/projects')).json.find(p => p.id === named.json.id)?.name, 'Widget Two');