Skip to content

Commit 2930998

Browse files
authored
security(gpg): pipe private key via stdin instead of writing to /tmp (#798)
## Summary Eliminates the TOCTOU / information-disclosure hazard in `importGPGKey()`: the private key is now piped to \`gpg --batch --import\` via stdin instead of being written to a fixed, predictable path in `os.tmpdir()`. ## Motivation The previous implementation: ```ts const PRIVATE_KEY_FILE = path.join(tmpdir(), 'private-key.asc'); await fsPromises.writeFile(PRIVATE_KEY_FILE, privateKey); await spawnProcess('gpg', ['--batch', '--import', PRIVATE_KEY_FILE]); await fsPromises.unlink(PRIVATE_KEY_FILE); ``` On Linux, `/tmp` is mode 1777 (world-writable, with the sticky bit). The path is deterministic, so: 1. **Read race**: any co-resident process on the runner can read the key between `writeFile` and `unlink`. 2. **Write redirect via symlink**: an attacker who wins a race to `ln -s /some/target /tmp/private-key.asc` before the `writeFile` call causes Craft to overwrite \`/some/target\` with the private key. 3. **Crash persistence**: an unexpected exit between `writeFile` and `unlink` leaves the key on disk indefinitely. ## Fix Pass the key via stdin. `gpg --batch --import` reads a key from stdin when no file argument is given. `spawnProcess` already supports a `stdin` option — no new infrastructure needed. ```ts await spawnProcess('gpg', ['--batch', '--import'], {}, { stdin: privateKey }); ``` Benefits vs. `mkdtemp(0o700)` alternative: - Zero filesystem contact — no dir creation, no file write, no cleanup path to get wrong. - Key no longer appears in `argv` either, so it doesn't show up in `ps` / `/proc/<pid>/cmdline`. ## Tests `src/utils/__tests__/gpg.test.ts` is rewritten to assert the new invariants: - `spawnProcess` is called with `['--batch', '--import']` and `{ stdin: KEY }`. - No `fs.writeFile` / `fs.unlink` happens. - The key is not embedded in any argv entry (regression guard against future reintroduction). `pnpm test src/utils/__tests__/gpg.test.ts` → 3 tests pass. Full lint / build clean. ## Callers Only `src/targets/maven.ts:271` calls `importGPGKey`. The signature is unchanged (`importGPGKey(privateKey: string): Promise<void>`) — no caller updates needed.
1 parent e56aa0f commit 2930998

2 files changed

Lines changed: 53 additions & 26 deletions

File tree

‎src/utils/__tests__/gpg.test.ts‎

Lines changed: 35 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { vi, describe, test, expect } from 'vitest';
1+
import { vi, describe, test, expect, beforeEach } from 'vitest';
22
import { promises as fsPromises } from 'fs';
33
import { importGPGKey } from '../gpg';
44
import { spawnProcess } from '../system';
@@ -10,35 +10,52 @@ vi.mock('fs', async importOriginal => {
1010
return {
1111
...actual,
1212
promises: {
13+
...actual.promises,
1314
writeFile: vi.fn(() => Promise.resolve()),
14-
unlink: vi.fn(),
15+
unlink: vi.fn(() => Promise.resolve()),
16+
mkdtemp: vi.fn(() => Promise.resolve('/tmp/should-not-be-created')),
17+
rm: vi.fn(() => Promise.resolve()),
1518
},
1619
};
1720
});
1821

1922
describe('importGPGKey', () => {
2023
const KEY = 'very_private_key_like_for_real_really_private';
21-
const PRIVATE_KEY_FILE_MATCHER = expect.stringMatching(/private-key.asc$/);
2224

23-
test('should write key to temp file', async () => {
24-
importGPGKey(KEY);
25-
expect(fsPromises.writeFile).toHaveBeenCalledWith(
26-
PRIVATE_KEY_FILE_MATCHER,
27-
KEY,
25+
beforeEach(() => {
26+
vi.clearAllMocks();
27+
});
28+
29+
test('passes the key to gpg via stdin and never touches the filesystem', async () => {
30+
await importGPGKey(KEY);
31+
32+
// gpg is spawned with --batch --import, no file path argument.
33+
expect(spawnProcess).toHaveBeenCalledTimes(1);
34+
expect(spawnProcess).toHaveBeenCalledWith(
35+
'gpg',
36+
['--batch', '--import'],
37+
{},
38+
{ stdin: KEY },
2839
);
2940
});
3041

31-
test('should remove file with the key afterwards', async () => {
32-
importGPGKey(KEY);
33-
expect(spawnProcess).toHaveBeenCalledWith('gpg', [
34-
'--batch',
35-
'--import',
36-
PRIVATE_KEY_FILE_MATCHER,
37-
]);
42+
test('does not write or unlink any file', async () => {
43+
await importGPGKey(KEY);
44+
45+
// The old implementation wrote the key to `tmpdir()/private-key.asc`
46+
// and then unlinked it. Neither must happen in the new version.
47+
expect(fsPromises.writeFile).not.toHaveBeenCalled();
48+
expect(fsPromises.unlink).not.toHaveBeenCalled();
3849
});
3950

40-
test('should call gpg command to load the key', async () => {
41-
importGPGKey(KEY);
42-
expect(fsPromises.unlink).toHaveBeenCalledWith(PRIVATE_KEY_FILE_MATCHER);
51+
test('key is not embedded in argv (not visible in process list)', async () => {
52+
await importGPGKey(KEY);
53+
54+
const [, args] = (spawnProcess as any).mock.calls[0];
55+
// Argv should contain only the static gpg flags; the key must only
56+
// travel through stdin.
57+
for (const arg of args as string[]) {
58+
expect(arg).not.toContain(KEY);
59+
}
4360
});
4461
});

‎src/utils/gpg.ts‎

Lines changed: 18 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,22 @@
1-
import { tmpdir } from 'os';
2-
import { promises as fsPromises } from 'fs';
3-
import * as path from 'path';
41
import { spawnProcess } from './system';
52

3+
/**
4+
* Imports a GPG private key into the local keyring.
5+
*
6+
* The key is piped to `gpg --batch --import` via stdin — it is NEVER
7+
* written to disk. This avoids the previous TOCTOU / information
8+
* disclosure hazards of writing the key to a predictable path in
9+
* `tmpdir()`:
10+
*
11+
* - Co-resident processes on shared runners could read the key
12+
* between `writeFile` and `unlink` (typical `/tmp` is mode 1777).
13+
* - A symlink planted at `/tmp/private-key.asc` before `writeFile`
14+
* would redirect the write to an attacker-chosen destination.
15+
* - An unexpected crash between `writeFile` and `unlink` would
16+
* leave the key on disk indefinitely.
17+
*
18+
* @param privateKey ASCII-armored GPG private key contents.
19+
*/
620
export async function importGPGKey(privateKey: string): Promise<void> {
7-
const PRIVATE_KEY_FILE = path.join(tmpdir(), 'private-key.asc');
8-
9-
await fsPromises.writeFile(PRIVATE_KEY_FILE, privateKey);
10-
await spawnProcess(`gpg`, ['--batch', '--import', PRIVATE_KEY_FILE]);
11-
await fsPromises.unlink(PRIVATE_KEY_FILE);
21+
await spawnProcess('gpg', ['--batch', '--import'], {}, { stdin: privateKey });
1222
}

0 commit comments

Comments
 (0)