From 623b93ad708f87901a488e983c50b469219d0df1 Mon Sep 17 00:00:00 2001 From: Till Heidrich Date: Tue, 1 Sep 2026 02:21:47 +0000 Subject: [PATCH] fix: close the path-escape hole in delivery (gallery and filename) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The wiki flagged this twice as open: `recipe.picdrop_gallery` had the same weakness the print module already fixed. `posixpath.join('/kunden/lea', '../../../etc/cron.d')` is `/etc/cron.d`, and the upload creates the directory first. Gallery and file names come from the database -- recipes, folders, settings -- and therefore ultimately from user input. Checked in `safeSegment()` immediately before the write, in both `uploadBuffer` (Picdrop and every FTP target) and `uploadToNas`. That is the one place nobody delivering can bypass, rather than a check somewhere further up that a new call site would miss. Deliberately a denylist of the dangerous constructs -- separators, control characters, all-dot names -- not a narrow allowlist. A customer folder may well be called "Müller & Söhne (2026)"; that is harmless, and rejecting it would have been a self-inflicted outage. Covered both ways by tests. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01BNhLunz586g6DeYMa7Qsd1 --- src/lib/nas.ts | 8 ++++++-- src/lib/picdrop.ts | 32 ++++++++++++++++++++++++++++---- tests/naming.test.ts | 26 ++++++++++++++++++++++++++ 3 files changed, 60 insertions(+), 6 deletions(-) diff --git a/src/lib/nas.ts b/src/lib/nas.ts index e40754a..ea6d849 100644 --- a/src/lib/nas.ts +++ b/src/lib/nas.ts @@ -45,8 +45,12 @@ export async function testNas(cfg: NasCfg): Promise<{ ok: boolean; message: stri } async function uploadToNas(cfg: NasCfg, remoteDir: string, filename: string, buf: Buffer): Promise { - const dir = posixpath.join(cfg.basePath || '/', remoteDir); - const finalPath = posixpath.join(dir, filename); + // Gleiche Prüfung wie bei Picdrop, aus demselben Grund: `posixpath.join` schützt + // nicht vor `..` — der Dateiname kommt aus der Datenbank und wird deshalb hier, + // unmittelbar vor dem Schreiben, geprüft. + const { safeSegment } = await import('./picdrop'); + const dir = posixpath.join(cfg.basePath || '/', safeSegment(remoteDir, 'Galerie')); + const finalPath = posixpath.join(dir, safeSegment(filename, 'Dateiname')); const { Readable } = await import('node:stream'); if (cfg.protocol === 'sftp') { const SftpClient = (await import('ssh2-sftp-client')).default; diff --git a/src/lib/picdrop.ts b/src/lib/picdrop.ts index 55e9125..4993962 100644 --- a/src/lib/picdrop.ts +++ b/src/lib/picdrop.ts @@ -1,8 +1,8 @@ // Picdrop-Auslieferung via SFTP (ssh2-sftp-client) oder FTPS (basic-ftp). // Konfiguration kommt aus settings (verschlüsseltes Passwort). import posixpath from 'node:path/posix'; -import { one } from './db'; -import { decrypt } from './crypto'; +import { one } from './db.ts'; +import { decrypt } from './crypto.ts'; export interface PicdropCfg { host: string; protocol: 'ftps' | 'sftp'; port: number; @@ -58,11 +58,35 @@ export async function testConnection(cfg: PicdropCfg): Promise<{ ok: boolean; me } } +/** + * Ein Pfadstück, das gefahrlos an den Basisordner angehängt werden kann. + * + * `posixpath.join` ist **kein** Schutz: `join('/kunden/lea', '../../../etc/cron.d')` + * ergibt `/etc/cron.d`, und der Upload legt das Verzeichnis vorher an. Galerienamen + * und Dateinamen stammen aus der Datenbank (Rezepte, Ordner, Einstellungen) und + * damit letztlich aus Nutzereingaben — sie werden deshalb **unmittelbar vor dem + * Schreiben** geprüft, nicht irgendwo weiter oben. Diese Stelle kann niemand + * umgehen, der ausliefert. + */ +export function safeSegment(value: string, was: 'Galerie' | 'Dateiname'): string { + const v = String(value || '').trim(); + if (!v) return ''; + // Bewusst eine Sperrliste der *gefährlichen* Konstrukte statt einer engen + // Erlaubnisliste: Ein Kundenordner darf „Müller & Söhne (2026)" heißen — das ist + // harmlos. Gefährlich sind nur Trennzeichen, Steuerzeichen und reine Punktnamen, + // denn allein damit lässt sich der Basisordner verlassen. + if (/[\/\\]/.test(v)) throw new Error(`Ungültiger ${was}: keine Schrägstriche erlaubt.`); + if (/[\u0000-\u001f\u007f]/.test(v)) throw new Error(`Ungültiger ${was}: Steuerzeichen.`); + if (/^\.+$/.test(v)) throw new Error(`Ungültiger ${was}: „.“ und „..“ sind keine Namen.`); + if (v.length > 128) throw new Error(`Ungültiger ${was}: zu lang.`); + return v; +} + /** Lädt einen Puffer direkt unter dem finalen Namen hoch. * (Picdrop verträgt das Temp→Rename-Muster nicht — es blieben `.tmp-…`-Dateien liegen.) */ export async function uploadBuffer(cfg: PicdropCfg, gallery: string, filename: string, buf: Buffer): Promise { - const dir = posixpath.join(cfg.basePath || '/', gallery || ''); - const finalPath = posixpath.join(dir, filename); + const dir = posixpath.join(cfg.basePath || '/', safeSegment(gallery, 'Galerie')); + const finalPath = posixpath.join(dir, safeSegment(filename, 'Dateiname')); const { Readable } = await import('node:stream'); if (cfg.protocol === 'sftp') { diff --git a/tests/naming.test.ts b/tests/naming.test.ts index 2ed7037..dedfb9b 100644 --- a/tests/naming.test.ts +++ b/tests/naming.test.ts @@ -128,3 +128,29 @@ test('Die Schleife gibt auch bei dauerhaft belegten Namen etwas zurück', async const n = await uniqueFilename(ID1, 'lea', 'a4', 'png', AT, async () => true); assert.ok(n.endsWith('.png') && n.length > 10, `unbrauchbarer Rückfall: ${n}`); }); + +// --- Pfad-Ausbruch beim Ausliefern ------------------------------------------- +// `posixpath.join('/kunden/lea','../../../etc/cron.d')` ergibt `/etc/cron.d`, und +// der Upload legt das Verzeichnis vorher an. Deshalb wird unmittelbar vor dem +// Schreiben geprüft — hier die Prüfung selbst. +import { safeSegment } from '../src/lib/picdrop.ts'; + +test('Galerie- und Dateinamen können den Basisordner nicht verlassen', () => { + const boese = ['..', '../..', '../../../etc/cron.d', 'a/b', 'a\\b', '.', '...', + 'POSTER LEA/../..', 'y\ny', 'z\u0000z']; + for (const b of boese) { + assert.throws(() => safeSegment(b, 'Galerie'), /Ungültige/, `durchgerutscht: ${JSON.stringify(b)}`); + } +}); + +test('Echte Kundenordner bleiben erlaubt — die Prüfung ist keine Schikane', () => { + const gut = ['POSTER LEA', 'TheFrame-Backgrounds', 'Müller & Söhne (2026)', + "Hochzeit O'Brien", 'Kita 2026_Gruppe 3', 'a.b.c', + '2026-08-20_143207_lea-badge_the-frame_a3f9.jpg']; + for (const g of gut) assert.equal(safeSegment(g, 'Galerie'), g, `fälschlich abgelehnt: ${g}`); +}); + +test('Leerer Name ist erlaubt und bedeutet „kein Unterordner"', () => { + assert.equal(safeSegment('', 'Galerie'), ''); + assert.equal(safeSegment(' ', 'Galerie'), ''); +});