diff --git a/__tests__/unit/rivet-fetch.test.js b/__tests__/unit/rivet-fetch.test.js index a025791..e8a4407 100644 --- a/__tests__/unit/rivet-fetch.test.js +++ b/__tests__/unit/rivet-fetch.test.js @@ -1,8 +1,12 @@ import { EventEmitter } from 'node:events'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; import { hasRivetYaml, extractTarballBuffer, - fetchAndExtractTarball + fetchAndExtractTarball, + assertNoEscapingSymlinks } from '../../src/rivet-fetch.js'; function mockOctokit({ contents = null, contentsStatus = 200, tarball = null } = {}) { @@ -27,7 +31,7 @@ function mockOctokit({ contents = null, contentsStatus = 200, tarball = null } = }; } -function makeFakeSpawn({ exitCode = 0, errorOnSpawn = null, stderr = '' } = {}) { +function makeFakeSpawn({ exitCode = 0, errorOnSpawn = null, stderr = '', stderrChunks = null } = {}) { return jest.fn(() => { const proc = new EventEmitter(); proc.stdin = new EventEmitter(); @@ -38,15 +42,28 @@ function makeFakeSpawn({ exitCode = 0, errorOnSpawn = null, stderr = '' } = {}) proc.stdin.write = jest.fn(); proc.stderr = new EventEmitter(); proc.stdout = new EventEmitter(); + // Provide resume() so the production code's drain path is exercised. + proc.stdout.resume = jest.fn(); if (errorOnSpawn) { setImmediate(() => proc.emit('error', new Error(errorOnSpawn))); - } else if (stderr) { - setImmediate(() => proc.stderr.emit('data', Buffer.from(stderr))); + } else { + if (stderrChunks) { + setImmediate(() => { + for (const c of stderrChunks) proc.stderr.emit('data', Buffer.from(c)); + }); + } else if (stderr) { + setImmediate(() => proc.stderr.emit('data', Buffer.from(stderr))); + } } return proc; }); } +/** Make a real, empty temp dir for tests that hit the post-extract walk. */ +function freshTmpDir() { + return fs.mkdtempSync(path.join(os.tmpdir(), 'temper-rivet-test-')); +} + describe('hasRivetYaml', () => { it('returns true on 200 (file exists)', async () => { const oct = mockOctokit({ contents: { name: 'rivet.yaml' } }); @@ -65,35 +82,193 @@ describe('hasRivetYaml', () => { }); describe('extractTarballBuffer', () => { - it('resolves on tar exit code 0', async () => { + let dest; + beforeEach(() => { dest = freshTmpDir(); }); + afterEach(() => { try { fs.rmSync(dest, { recursive: true, force: true }); } catch { /* ignore */ } }); + + it('resolves on tar exit code 0 and passes hardening flags', async () => { const spawnFn = makeFakeSpawn({ exitCode: 0 }); - await expect(extractTarballBuffer(Buffer.from('x'), '/tmp/dest', { spawnFn })) + await expect(extractTarballBuffer(Buffer.from('x'), dest, { spawnFn })) .resolves.toBeUndefined(); expect(spawnFn).toHaveBeenCalledWith( 'tar', - ['-xz', '--strip-components=1', '-C', '/tmp/dest'], + ['-xz', '--strip-components=1', '--no-same-owner', '-C', dest], expect.objectContaining({ stdio: ['pipe', 'pipe', 'pipe'] }) ); }); + it('drains stdout so a piped `tar` cannot stall on a full pipe buffer', async () => { + const spawnFn = makeFakeSpawn({ exitCode: 0 }); + let proc; + const wrappedSpawn = jest.fn((...args) => { + proc = spawnFn(...args); + return proc; + }); + await extractTarballBuffer(Buffer.from('x'), dest, { spawnFn: wrappedSpawn }); + expect(proc.stdout.resume).toHaveBeenCalled(); + }); + it('rejects on non-zero exit with stderr in message', async () => { const spawnFn = makeFakeSpawn({ exitCode: 1, stderr: 'tar: bad gzip' }); - await expect(extractTarballBuffer(Buffer.from('x'), '/tmp/dest', { spawnFn })) + await expect(extractTarballBuffer(Buffer.from('x'), dest, { spawnFn })) .rejects.toThrow(/tar exited 1/); }); + it('caps stderr at 64 KiB and marks the message truncated', async () => { + // Emit ~256 KiB of stderr — well over the 64 KiB cap. Without bounding + // we'd happily concat all of it into a string in this process. + const big = 'x'.repeat(64 * 1024); + const spawnFn = makeFakeSpawn({ + exitCode: 1, + stderrChunks: [big, big, big, big] + }); + let caught; + try { + await extractTarballBuffer(Buffer.from('x'), dest, { spawnFn }); + } catch (err) { + caught = err; + } + expect(caught).toBeDefined(); + // The thrown message must not contain the full 256 KiB — it should be + // close to the 64 KiB cap plus a small prefix/suffix. + expect(caught.message.length).toBeLessThan(64 * 1024 + 256); + expect(caught.message).toMatch(/stderr truncated/); + }); + it('rejects when tar cannot be spawned', async () => { const spawnFn = makeFakeSpawn({ errorOnSpawn: 'ENOENT' }); - await expect(extractTarballBuffer(Buffer.from('x'), '/tmp/dest', { spawnFn })) + await expect(extractTarballBuffer(Buffer.from('x'), dest, { spawnFn })) .rejects.toThrow(/ENOENT/); }); + + it('rejects when post-extract walk finds an escaping symlink', async () => { + // Simulate `tar` "successfully" extracting a tree that contains a + // malicious symlink. We materialise the post-extract state by hand. + const evilTarget = '/etc/passwd'; + const spawnFn = jest.fn(() => { + const proc = new EventEmitter(); + proc.stdin = new EventEmitter(); + proc.stdin.write = jest.fn(); + proc.stderr = new EventEmitter(); + proc.stdout = new EventEmitter(); + proc.stdout.resume = jest.fn(); + proc.stdin.end = jest.fn(() => { + // Drop a nasty symlink BEFORE signalling tar exit, so the + // post-extract walk has something to find. + try { + fs.symlinkSync(evilTarget, path.join(dest, 'rivet.yaml')); + } catch { /* ignore — symlinks may be denied; test is best-effort */ } + setImmediate(() => proc.emit('exit', 0)); + }); + return proc; + }); + let caught; + try { + await extractTarballBuffer(Buffer.from('x'), dest, { spawnFn }); + } catch (err) { + caught = err; + } + if (caught) { + expect(caught.message).toMatch(/symlink escaping destDir/); + // The bad link must have been removed. + expect(fs.existsSync(path.join(dest, 'rivet.yaml'))).toBe(false); + } else { + // Symlink creation was unsupported on this runner (e.g. Windows + // without dev-mode). The hardening still ran; nothing to assert. + expect(spawnFn).toHaveBeenCalled(); + } + }); + + it('accepts a benign tree (no symlinks) without complaint', async () => { + const spawnFn = jest.fn(() => { + const proc = new EventEmitter(); + proc.stdin = new EventEmitter(); + proc.stdin.write = jest.fn(); + proc.stderr = new EventEmitter(); + proc.stdout = new EventEmitter(); + proc.stdout.resume = jest.fn(); + proc.stdin.end = jest.fn(() => { + // Realistic post-extract state: a couple of files and a subdir. + fs.writeFileSync(path.join(dest, 'rivet.yaml'), 'project: test\n'); + fs.mkdirSync(path.join(dest, 'src')); + fs.writeFileSync(path.join(dest, 'src', 'foo.js'), '// hi\n'); + setImmediate(() => proc.emit('exit', 0)); + }); + return proc; + }); + await expect( + extractTarballBuffer(Buffer.from('x'), dest, { spawnFn }) + ).resolves.toBeUndefined(); + }); + + it('accepts a relative symlink that stays inside destDir', async () => { + const spawnFn = jest.fn(() => { + const proc = new EventEmitter(); + proc.stdin = new EventEmitter(); + proc.stdin.write = jest.fn(); + proc.stderr = new EventEmitter(); + proc.stdout = new EventEmitter(); + proc.stdout.resume = jest.fn(); + proc.stdin.end = jest.fn(() => { + fs.mkdirSync(path.join(dest, 'src')); + fs.writeFileSync(path.join(dest, 'src', 'real.js'), '// real\n'); + try { + fs.symlinkSync('./real.js', path.join(dest, 'src', 'alias.js')); + } catch { /* ignore on platforms without symlink perms */ } + setImmediate(() => proc.emit('exit', 0)); + }); + return proc; + }); + await expect( + extractTarballBuffer(Buffer.from('x'), dest, { spawnFn }) + ).resolves.toBeUndefined(); + }); +}); + +describe('assertNoEscapingSymlinks', () => { + let dest; + beforeEach(() => { dest = freshTmpDir(); }); + afterEach(() => { try { fs.rmSync(dest, { recursive: true, force: true }); } catch { /* ignore */ } }); + + it('rejects an absolute escaping symlink', async () => { + try { + fs.symlinkSync('/etc/passwd', path.join(dest, 'evil')); + } catch { + // Symlink unsupported — skip rather than false-pass. + return; + } + await expect(assertNoEscapingSymlinks(dest)).rejects.toThrow(/escaping destDir/); + }); + + it('rejects a relative `..` symlink that climbs out', async () => { + try { + fs.symlinkSync('../../../../etc/passwd', path.join(dest, 'climb')); + } catch { + return; + } + await expect(assertNoEscapingSymlinks(dest)).rejects.toThrow(/escaping destDir/); + }); + + it('accepts a symlink whose target is inside destDir', async () => { + fs.writeFileSync(path.join(dest, 'real'), 'ok'); + try { + fs.symlinkSync('./real', path.join(dest, 'alias')); + } catch { + return; + } + await expect(assertNoEscapingSymlinks(dest)).resolves.toBeUndefined(); + }); }); describe('fetchAndExtractTarball', () => { + let dest; + beforeEach(() => { dest = freshTmpDir(); }); + afterEach(() => { try { fs.rmSync(dest, { recursive: true, force: true }); } catch { /* ignore */ } }); + it('requests the tarball and pipes it through tar', async () => { const oct = mockOctokit({ tarball: Buffer.from('hello-tar') }); const spawnFn = makeFakeSpawn({ exitCode: 0 }); - await fetchAndExtractTarball(oct, 'o', 'r', 'sha123', '/tmp/dest', { spawnFn }); + await fetchAndExtractTarball(oct, 'o', 'r', 'sha123', dest, { spawnFn }); expect(oct.request).toHaveBeenCalledWith( 'GET /repos/{owner}/{repo}/tarball/{ref}', { owner: 'o', repo: 'r', ref: 'sha123' } @@ -107,7 +282,7 @@ describe('fetchAndExtractTarball', () => { const oct = mockOctokit({ tarball: buf }); const spawnFn = makeFakeSpawn({ exitCode: 0 }); await expect( - fetchAndExtractTarball(oct, 'o', 'r', 'sha123', '/tmp/dest', { spawnFn }) + fetchAndExtractTarball(oct, 'o', 'r', 'sha123', dest, { spawnFn }) ).resolves.toBeUndefined(); }); }); diff --git a/src/rivet-fetch.js b/src/rivet-fetch.js index 6a311e5..1563eb1 100644 --- a/src/rivet-fetch.js +++ b/src/rivet-fetch.js @@ -37,29 +37,136 @@ export async function hasRivetYaml(octokit, owner, repo, ref) { } } +/** Cap stderr capture from `tar` to avoid OOM on pathological output. */ +const TAR_STDERR_CAP_BYTES = 64 * 1024; + /** * Pipe a Buffer (the tarball bytes) through `tar -xz` into destDir. Strips * the leading "{owner}-{repo}-{sha}/" component so the resulting tree mirrors * the repo root. + * + * Hardening (Bug #15, wave-1 Security auditor): + * - `--no-same-owner`: ignore archive uid/gid (we run single-user anyway). + * - `-P` is *not* passed: tar must strip leading `/` and reject `..` + * components. (Default behaviour on GNU tar and bsdtar; explicit because + * a hostile patch could otherwise re-enable absolute paths.) + * - Post-extract walk: every symlink whose resolved target escapes + * `destDir` is removed, and the extraction is rejected. A subsequent + * `runRivetOracle` therefore cannot read `rivet.yaml` (or anything else) + * from outside the sandbox. + * - Stderr buffer is capped at 64 KiB so a malicious tarball cannot + * trigger unbounded memory growth in this process. + * - `tar.stdout` is drained — it's piped but the parent never reads from + * it; without a consumer the pipe buffer can fill and stall `tar`. */ export function extractTarballBuffer(buffer, destDir, opts = {}) { - const { spawnFn = spawn } = opts; + const { spawnFn = spawn, postExtractCheck = assertNoEscapingSymlinks } = opts; return new Promise((resolve, reject) => { - const tar = spawnFn('tar', ['-xz', '--strip-components=1', '-C', destDir], { - stdio: ['pipe', 'pipe', 'pipe'] - }); + const tar = spawnFn( + 'tar', + ['-xz', '--strip-components=1', '--no-same-owner', '-C', destDir], + { stdio: ['pipe', 'pipe', 'pipe'] } + ); let stderr = ''; - tar.stderr.on('data', (chunk) => { stderr += chunk.toString(); }); + let stderrTruncated = false; + tar.stderr.on('data', (chunk) => { + if (stderr.length >= TAR_STDERR_CAP_BYTES) { + stderrTruncated = true; + return; + } + const remaining = TAR_STDERR_CAP_BYTES - stderr.length; + const piece = chunk.toString(); + if (piece.length > remaining) { + stderr += piece.slice(0, remaining); + stderrTruncated = true; + } else { + stderr += piece; + } + }); + // Drain stdout so the pipe buffer never fills and stalls `tar`. + if (tar.stdout && typeof tar.stdout.resume === 'function') { + tar.stdout.resume(); + } else if (tar.stdout && typeof tar.stdout.on === 'function') { + tar.stdout.on('data', () => {}); + } tar.on('error', reject); tar.on('exit', (code) => { - if (code === 0) resolve(); - else reject(new Error(`tar exited ${code}: ${stderr.trim()}`)); + if (code !== 0) { + const suffix = stderrTruncated ? ' [stderr truncated]' : ''; + reject(new Error(`tar exited ${code}: ${stderr.trim()}${suffix}`)); + return; + } + // Walk the extracted tree and reject any symlink that escapes destDir. + Promise.resolve() + .then(() => postExtractCheck(destDir)) + .then(resolve, reject); }); tar.stdin.on('error', reject); tar.stdin.end(buffer); }); } +/** + * Recursively walk `destDir`. Any symlink whose resolved target lies outside + * `destDir` is unlinked and the walk is rejected with a descriptive error. + * Intentionally conservative: we resolve the link target relative to the + * directory containing the link (mimicking how a later reader would resolve + * it) and compare against the absolute, resolved `destDir`. + * + * Exported for tests; not re-exported as part of the module surface. + */ +export async function assertNoEscapingSymlinks(destDir) { + const root = path.resolve(destDir); + const rootWithSep = root.endsWith(path.sep) ? root : root + path.sep; + await walkAndCheck(root, root, rootWithSep); +} + +async function walkAndCheck(current, root, rootWithSep) { + let entries; + try { + entries = await fs.promises.readdir(current, { withFileTypes: true }); + } catch (err) { + // If the directory disappeared mid-walk, treat as benign; if we can't + // read it for other reasons, surface the failure. + if (err.code === 'ENOENT') return; + throw err; + } + for (const entry of entries) { + const full = path.join(current, entry.name); + if (entry.isSymbolicLink()) { + let target; + try { + target = await fs.promises.readlink(full); + } catch { + // Unreadable link — remove defensively rather than trust it. + try { fs.unlinkSync(full); } catch { /* best-effort */ } + throw new Error( + `tarball symlink unreadable and removed: ${path.relative(root, full)}` + ); + } + const resolved = path.resolve(path.dirname(full), target); + const resolvedWithSep = resolved.endsWith(path.sep) + ? resolved + : resolved + path.sep; + const escapes = + resolved !== root && + !resolvedWithSep.startsWith(rootWithSep); + if (escapes) { + try { fs.unlinkSync(full); } catch { /* best-effort */ } + throw new Error( + `tarball contains symlink escaping destDir: ` + + `${path.relative(root, full)} -> ${target}` + ); + } + // Don't follow symlinks during the walk — even safe ones. + continue; + } + if (entry.isDirectory()) { + await walkAndCheck(full, root, rootWithSep); + } + } +} + /** * Fetch the GitHub tarball for a ref and extract it into destDir. *