From 9f6c45049287676505fe6656a2d2ff95c77873c0 Mon Sep 17 00:00:00 2001 From: wkh Date: Fri, 30 Oct 2026 09:45:18 +0100 Subject: [PATCH] Sicherheit: report/screen maskieren Shell-Eingaben, Sperren privat und atomar MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit report setzt Zellwerte in maskiert ein, screen verlangt gültige Variablennamen, lock/unlock nutzen /tmp/rdb-locks- mit flock, vilock sperrt per Hardlink. EDITOR darf Argumente haben. Doku und Tests ergänzt. --- CHANGELOG.md | 7 ++++ README.md | 8 ++++ doc/review.md | 92 +++++++++++++++++++++++++++++++++++++++++++ lib/rdb/cmd/access.rb | 63 +++++++++++++++++++++++------ lib/rdb/cmd/report.rb | 8 +++- lib/rdb/cmd/term.rb | 1 + test/rdb_test.rb | 38 ++++++++++++++++++ 7 files changed, 203 insertions(+), 14 deletions(-) create mode 100644 doc/review.md diff --git a/CHANGELOG.md b/CHANGELOG.md index 224476c..66e557b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,7 +16,14 @@ folgen [Semantic Versioning](https://semver.org/lang/de/). Die Version steht in - `gregorian -u` schreibt den Monat ohne führende Null (`6/01/89`), wie im Handbuch. - `mean` schreibt einen ganzzahligen Mittelwert ohne `.00` (`4786`, Handbuch S. 26). +### Sicherheit +- `report`: Spaltenwerte in `!>` wurden unmaskiert in einen Shell-Aufruf eingesetzt, eine Zelle konnte so Befehle ausführen. Sie werden jetzt mit `Shellwords.escape` eingesetzt. +- `screen`: Spaltennamen, die keine gültigen Shell-Variablennamen sind, brechen mit einer Meldung ab, statt ins erzeugte Skript zu gelangen. +- `lock`/`unlock`: Sperrdateien liegen in `/tmp/rdb-locks-/` (Modus 0700, Besitzer und Symlinks geprüft) statt lose in `/tmp`; Lesen und Schreiben laufen unter `flock`. +- `vilock`: die Sperre wird atomar über einen Hardlink genommen (vorher `exist?` und `rename` mit Lücke dazwischen). + ### Behoben +- `EDITOR` mit Argumenten (`nvim -f`) in `vilock` und `update.rdb` wurde als ein Programmname behandelt; der Editor startete nie. - Spaltennamen, die `justify` mit Leerzeichen aufgefüllt hat, wurden nicht mehr gefunden: `column Name` lieferte eine leere Spalte, `index` brach ab. Leerraum um die Namen im Kopf zählt jetzt nicht mehr. ## [0.7.0] - 2026-10-12 diff --git a/README.md b/README.md index bf7022a..6ca8d18 100644 --- a/README.md +++ b/README.md @@ -245,6 +245,14 @@ er nicht mehr passt (außer mit `-a`). Beide halten die Indizes aktuell. - Die Indexdateien sind dem Original nachgebaut, aber nicht gleich (Handbuch S. 103–108): die Hashtabelle `.h` hat 2n statt 2n+1 Plätze, und in `.i` steht der Schlüssel vor dem Offset statt dahinter. Alte /rdb-Indizes muss man neu bauen. +- `lock`/`unlock` legen ihre Sperrdateien in `/tmp/rdb-locks-/` ab (Modus 0700, `flock`), + nicht als `/tmp/L` wie in den Man-Pages. Eine Sperre eines abgestürzten Prozesses + wird nicht erkannt; `unlock` hebt sie auf. +- `vilock` sperrt atomar über einen Hardlink; `EDITOR` darf Argumente haben (`nvim -f`). + Nach `kill -9` bleibt `LOCK` liegen und muss von Hand zurückbenannt werden. +- `report`: Spaltenwerte in `!>` werden für die Shell maskiert und kommen + als ein Wort an; nicht in Anführungszeichen setzen. `screen` verlangt Spaltennamen, die + gültige Shell-Variablennamen sind (Buchstaben, Ziffern, `_`). - `sorttable` lässt Zeilen mit gleichem Schlüssel in ihrer Reihenfolge; das Original ordnet sie nach der ganzen Zeile, wie `sort` (Handbuch S. 94). - `subtotal` schreibt bei einer Summe von null ein leeres Feld; das Handbuch zeigt diff --git a/doc/review.md b/doc/review.md new file mode 100644 index 0000000..48cc39f --- /dev/null +++ b/doc/review.md @@ -0,0 +1,92 @@ +# Code-Review: Schwachstellen, Fehler, Verbesserungspotential + +Stand: 2026-10-30, Version 0.7.0. Die 114 Tests und der Handbuch-Test liefen grün. +„Bestätigt“ heißt: nachgestellt. Alles andere stammt aus dem Lesen des Codes. + +## Schwachstellen (alle vier behoben, siehe CHANGELOG) + +1. **Befehlseinschleusung über Tabellendaten in `report` (bestätigt).** + [lib/rdb/cmd/report.rb:11-12](../lib/rdb/cmd/report.rb): Bei `!>` werden + Zellwerte ungefiltert in einen Backtick-Aufruf eingesetzt. Eine Zelle mit + `touch PWNED` hat die Datei angelegt. Abhilfe: `Shellwords.escape`, oder in der + Doku als „nur vertrauenswürdige Daten“ kennzeichnen. + **Behoben:** Werte werden mit `Shellwords.escape` eingesetzt. +2. **`screen` erzeugt Shell-Code aus Spaltennamen** + ([report.rb:50-70](../lib/rdb/cmd/report.rb)). Namen stehen unquotiert in + `read NAME` und `echo "$NAME"`; Leerzeichen, `;` oder `$(…)` brechen das + Skript oder führen Code aus. Werte werden ebenfalls nicht maskiert. + **Behoben:** Spaltennamen müssen gültige Shell-Variablennamen sein. +3. **Lock-Dateien in `/tmp`** ([access.rb:121](../lib/rdb/cmd/access.rb)). + Vorhersagbare Namen, andere Benutzer können sie anlegen oder löschen. + `lock` liest und hängt an, ohne `flock` (Race). Keine Prüfung, ob die PID noch + lebt; nach einem Absturz bleibt das Schloss. + **Behoben:** privates Verzeichnis `/tmp/rdb-locks-` (0700) und `flock`. + Offen: verwaiste Schlösser nach Absturz (eine PID-Prüfung bricht Tests und Handbuch, + die erfundene PIDs nutzen). +4. **`vilock` nicht atomar** ([access.rb:134](../lib/rdb/cmd/access.rb)). + Race zwischen `File.exist?` und `rename`; nach `kill -9` bleibt `LOCK…` liegen. + **Behoben:** atomare Sperre über `File.link`. Offen: nach `kill -9` bleibt `LOCK…` liegen. + +## Fehler + +1. **Umlaute in Spaltennamen (bestätigt).** `to_awk` erkennt nur `[A-Za-z_#]` + und ASCII-`\w`. `row 'Über > 3'` scheitert mit awk-Fehler. Relevant für die + DE/AT/CH-Tarife. +2. **Nicht-UTF-8-Daten (bestätigt).** Latin-1-Datei führt zu + `invalid byte sequence in UTF-8` mit Stacktrace + ([table.rb:28](../lib/rdb/table.rb)). Abhilfe: binär lesen und `scrub`/ + `force_encoding`, oder Kodierungsoption. +3. **Kein Fehlerfang auf oberster Ebene (bestätigt).** Stacktraces statt + Meldungen bei ungültiger Regex (`update.rdb` mit `/[`, + [access.rb:277](../lib/rdb/cmd/access.rb); `sale`, + [act.rb:345](../lib/rdb/cmd/act.rb)), `Date::Error` in `to_julian`, `Errno::*`. + Abhilfe: `rescue StandardError` in `bin/rdb`/`Rdb.run`. +4. **`EDITOR` mit Argumenten (bestätigt, behoben).** `system(ENV['EDITOR'], datei)` in + `vilock` und `edit_cells` ([access.rb:138](../lib/rdb/cmd/access.rb), + [access.rb:150](../lib/rdb/cmd/access.rb)) behandelt `"nvim -f"` als einen + Programmnamen. Der Editor startet nie, `update.rdb` meldet trotzdem + „record replaced“. Abhilfe: `Shellwords.split`, Rückgabewert prüfen. +5. **Datenverlust-Risiko bei `update.rdb e`** + ([access.rb:289](../lib/rdb/cmd/access.rb)). Der Satz wird vor dem Editieren + geleert; bei Abbruch ist er weg. Erst nach erfolgreichem Editieren schreiben. +6. **Tab/Zeilenumbruch in Werten ungeprüft (bestätigt für Tab).** `append`, + `enter`, `replace`, `ve` schreiben Zellen unverändert; ein Tab erzeugte eine + Zeile mit 3 statt 2 Spalten (`check.rdb` meldet es). +7. **Nicht atomares Schreiben.** `splice`, `build`, `substitute` überschreiben + direkt, ohne temporäre Datei plus `rename`, ohne `flock`. +8. **Inkonsistente Vergleiche bei `-mb`/`-mi`** (nur im Code gesehen). Index + sortiert mit `to_s`, Suche vergleicht mit `strip`; führende Leerzeichen und + `-x` ohne Teilsuche können falsch ausfallen. +9. **Kleinigkeiten.** `sorttable -r` kehrt gleiche Schlüssel um (`reverse!` nach + stabiler Sortierung); `-nrf` gilt global für alle Spalten; `jointable -n` wird + ignoriert; `total`/`mean` lehnen `1e3` ab; eine Zeile aus lauter leeren + Zellen gilt als gelöscht ([fast.rb:21](../lib/rdb/fast.rb)). + +## Verbesserungspotential + +1. **Indexe sind nicht schnell.** Jede Suche liest Tabelle und Indexdatei + komplett ([fast.rb:151](../lib/rdb/fast.rb), [fast.rb:171](../lib/rdb/fast.rb), + `Data.new` pro Schlüssel in `delete`/`update.rdb`): O(n) pro Schlüssel. Abhilfe: + mit `seek`/`pread` direkt an der Fixbreiten-Position lesen. +2. **Schlechte Hash-Funktion** ([fast.rb:68](../lib/rdb/fast.rb)): + `bytes.sum % slots`. Bestätigt: 3000 fünfstellige Nummern landen in 30 Slots, + größter Bucket 223. Abhilfe: FNV-1a o. Ä., Primzahl-Slotzahl. +3. **Geld in `Float`** ([act.rb:11](../lib/rdb/cmd/act.rb), `to_f` im /act-Teil + und in `total`). Abhilfe: `BigDecimal` oder ganzzahlige Rappen. +4. **awk-Brücke** ([env.rb:119-138](../lib/rdb/env.rb)): ganze Tabelle als String + durch `capture3`, kein Streaming; handgeschriebener Parser erkennt Strings und + Regexe heuristisch (`prev`) und hängt vom awk-Dialekt ab. +5. **Globale Methoden auf `Object`** (`fast_opts`, `lock_file`, `num`, `money`, + `ask`, `need`, `show_record` …). In ein Modul kapseln und in `Env` einbinden. +6. **Doppelter Code.** `awk`/`awk_text`; Schlüsseltabellen-Parsing in `fast_opts`/ + `key_columns`; von Hand gebaute Kopf-/Strichzeilen trotz `Table.dash_row`. +7. **Tests.** Laufzeit ca. 66 s wegen eines Ruby-Prozesses pro Test. Fehlerfälle + (Encoding, Tab in Wert, Locks, Editor) nicht abgedeckt; Tests direkt gegen + `Rdb.run` im Prozess wären schneller. +8. **Repo.** `demo/fact.pl`, `fact_lc.pl`, `rule.pl` sind ungetrackt; + `demo/testall.old` (1187 Zeilen) wäre als Golden-File-Test besser gepflegt. + +## Empfohlene Reihenfolge + +Die Schwachstellen sind erledigt. Weiter mit: Umlaute (Fehler 1), Encoding (2), Top-Level-Rescue (3), `report`-Einschleusung +(Schwachstelle 1); danach Hash-Funktion und Index-Lesezugriffe. diff --git a/lib/rdb/cmd/access.rb b/lib/rdb/cmd/access.rb index 5d03274..fbca26f 100644 --- a/lib/rdb/cmd/access.rb +++ b/lib/rdb/cmd/access.rb @@ -185,44 +185,81 @@ Rdb.register('enter', 'Add rows to a table or list file without an editor', 'ent end require 'digest' +require 'shellwords' # ---- locks ------------------------------------------------------------------- +# Lock files live in a private per-user directory, not loose in the shared +# /tmp, so other users can neither forge nor delete them. +def lock_dir + dir = File.join(Dir.tmpdir, "rdb-locks-#{Process.uid}") + Dir.mkdir(dir, 0o700) unless File.directory?(dir) + st = File.lstat(dir) + Rdb.die("#{dir}: not a private directory of yours") if st.symlink? || st.uid != Process.uid || (st.mode & 0o077) != 0 + dir +end + def lock_file(name) - File.join(Dir.tmpdir, "L#{File.basename(name)}.#{Digest::MD5.hexdigest(File.expand_path(name))[0, 8]}") + File.join(lock_dir, "L#{File.basename(name)}.#{Digest::MD5.hexdigest(File.expand_path(name))[0, 8]}") +end + +# Read-modify-write of a lock file under an exclusive flock, so two lock or +# unlock calls cannot interleave. The block gets the held locks and returns +# the new list of lines, or nil to leave the file alone. +def with_lock_file(name) + File.open(lock_file(name), File::RDWR | File::CREAT, 0o600) do |f| + f.flock(File::LOCK_EX) + held = f.read.lines(chomp: true).map { |l| l.split.map(&:to_i) } + lines = yield held + next unless lines + + f.rewind + f.truncate(0) + f.write(lines.map { |l| "#{l}\n" }.join) + end end Rdb.register('lock', 'Lock a record or field of a file', 'lock filename processid from to indexfrom indexto') do |args| die 'usage: lock filename processid from to indexfrom indexto' if args.size < 4 name, pid, from, to = args - lf = lock_file(name) - held = File.exist?(lf) ? File.readlines(lf, chomp: true).map { |l| l.split.map(&:to_i) } : [] - clash = held.find { |p, f, t| p != pid.to_i && f < to.to_i && from.to_i < t } + clash = nil + with_lock_file(name) do |held| + clash = held.find { |p, f, t| p != pid.to_i && f < to.to_i && from.to_i < t } + clash ? nil : held.map { |l| l.join(' ') } << args.drop(1).join(' ') + end if clash warn "lock: #{name}: bytes #{from}-#{to} are locked by process #{clash[0]}" exit 1 end - File.open(lf, 'a') { |f| f.puts args.drop(1).join(' ') } end Rdb.register('unlock', 'Unlock a record or field of a file', 'unlock tableorlist processid from to indexfrom indexto') do |args| die 'usage: unlock filename processid from to indexfrom indexto' if args.size < 4 name = args.shift - lf = lock_file(name) - next unless File.exist?(lf) + next unless File.exist?(lock_file(name)) - keep = File.readlines(lf, chomp: true).reject { |l| l.split.first(3) == args.first(3) } - keep.empty? ? File.delete(lf) : File.write(lf, keep.join("\n") + "\n") + with_lock_file(name) do |held| + held.map { |l| l.join(' ') }.reject { |l| l.split.first(3) == args.first(3) } + end + lf = lock_file(name) + File.delete(lf) if File.zero?(lf) end Rdb.register('vilock', 'Lock a table before editing, unlock afterward', 'vilock tableorlist') do |args| file = args.first or die 'usage: vilock tableorlist' locked = File.join(File.dirname(file), "LOCK#{File.basename(file)}") - die "#{file} is locked or missing" if File.exist?(locked) || !File.exist?(file) + die "#{file} is locked or missing" unless File.exist?(file) - File.rename(file, locked) + # link fails if LOCK exists, so exactly one caller gets the table + # (File.exist? followed by rename would leave a race in between). begin - system(ENV['EDITOR'] || 'vi', locked) + File.link(file, locked) + rescue Errno::EEXIST + die "#{file} is locked or missing" + end + File.unlink(file) + begin + system(*Shellwords.split(ENV['EDITOR'] || 'vi'), locked) ensure File.rename(locked, file) end @@ -239,7 +276,7 @@ def edit_cells(head, cells) Tempfile.create('rdb-edit') do |f| f.write Rdb::Table.new(head, [cells], list: true).to_s f.flush - system(ENV['EDITOR'] || 'vi', f.path) + system(*Shellwords.split(ENV['EDITOR'] || 'vi'), f.path) t = Rdb::Table.parse(File.read(f.path)) return nil if t.rows.empty? diff --git a/lib/rdb/cmd/report.rb b/lib/rdb/cmd/report.rb index 5deabc5..a11f482 100644 --- a/lib/rdb/cmd/report.rb +++ b/lib/rdb/cmd/report.rb @@ -2,13 +2,17 @@ # Reports, forms and mailing lists. +require 'shellwords' + # Fill in a form for one row: is replaced by the value, # by the output of the shell command, and !> passes data to it. +# Values inside a command are shell-escaped, so a cell can never add commands +# of its own; they arrive as one word each (do not put them in quotes). def fill_form(form, head, row) values = head.zip(row).to_h { |h, v| [h.strip, v.to_s] } form.gsub(/|<([^<>!\n]*)>/m) do if Regexp.last_match(1) - cmd = Regexp.last_match(1).gsub(/<([^<>!\n]*)>/) { values.fetch(Regexp.last_match(1).strip, Regexp.last_match(0)) } + cmd = Regexp.last_match(1).gsub(/<([^<>!\n]*)>/) { Shellwords.escape(values.fetch(Regexp.last_match(1).strip, Regexp.last_match(0))) } `#{cmd}` else name = Regexp.last_match(2).strip @@ -57,6 +61,8 @@ Rdb.register('screen', 'Convert a form into a screen-input shell program', 'scre reads << format('cursor %2d %2d ; %s;', n + 1, col, m[1].strip) else names << m[2].strip + # the name becomes a shell variable in the generated program + die "screen: '#{m[2].strip}' is not a valid shell variable name" unless names.last.match?(/\A[A-Za-z_]\w*\z/) reads << format('cursor %2d %2d ; read %s;', n + 1, col, m[2].strip) end out[m.begin(0), m[0].size] = ' ' * m[0].size diff --git a/lib/rdb/cmd/term.rb b/lib/rdb/cmd/term.rb index 1043402..0fc0513 100644 --- a/lib/rdb/cmd/term.rb +++ b/lib/rdb/cmd/term.rb @@ -230,6 +230,7 @@ end Rdb.register('testall', 'Test all /rdb programs in directory $RDB/demo', 'testall [ > testall.new ]') do |_args| ENV['PATH'] = "#{File.join(Rdb::ROOT, 'bin')}:#{ENV['PATH']}" + ENV['EDITOR'] = 'true' # no real editor in an unattended run (vilock, update.rdb e) demo = File.join(ENV['RDB'] || Rdb::ROOT, 'demo') script = File.join(demo, 'testall.cmds') File.exist?(script) or die "#{script}: missing" diff --git a/test/rdb_test.rb b/test/rdb_test.rb index f77f6b4..e9e39b8 100644 --- a/test/rdb_test.rb +++ b/test/rdb_test.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true require_relative 'test_helper' +ENV['EDITOR'] = 'true' # a test must never open a real editor require 'pty' require_relative '../lib/rdb' @@ -416,6 +417,29 @@ class FastAccessTest < RdbCase run3('unlock', 'inventory', '333', '15', '25') end + def test_lock_directory_is_private + run3('lock', 'inventory', '111', '10', '20', '0', '0') + dir = File.join(Dir.tmpdir, "rdb-locks-#{Process.uid}") + assert_equal 0o700, File.stat(dir).mode & 0o777 + run3('unlock', 'inventory', '111', '10', '20') + end + + def test_vilock_refuses_a_table_that_is_already_locked + write('LOCKinventory', 'x') + _o, e, st = run3('vilock', 'inventory') + refute st.success? + assert_match(/is locked/, e) + assert File.exist?(File.join(@dir, 'inventory')) + end + + def test_vilock_edits_with_an_editor_that_has_arguments + out, _e, st = Open3.capture3({ 'EDITOR' => 'sh -c "echo edited >> \"$0\""', 'PATH' => "#{BIN}:#{ENV['PATH']}" }, + File.join(BIN, 'vilock'), 'inventory', chdir: @dir) + assert st.success?, out + assert_match(/edited/, File.read(File.join(@dir, 'inventory'))) + refute File.exist?(File.join(@dir, 'LOCKinventory')) + end + def test_enter_appends_rows out = rdb('enter', '-1', 'inventory', stdin: "8\n2\n3\n4\nnew thing\n") assert_includes out, 'Item: ' @@ -468,6 +492,20 @@ class ReportTest < RdbCase assert_equal "Ronald McDonald|Memphis\n|\nChiquita Banana|San Jose\n|\n", rdb('report', 'f2', file: 'maillist') end + def test_report_does_not_run_commands_hidden_in_data + write('evil', "Name\tCmd\n----\t---\nx\ttouch PWNED; echo\n") + write('f3', "!>\n") + out = rdb('report', 'f3', file: 'evil') + assert_equal "touch PWNED; echo\n\n", out + refute File.exist?(File.join(@dir, 'PWNED')) + end + + def test_screen_rejects_unsafe_variable_names + _o, e, st = run3('screen', stdin: "\n") + refute st.success? + assert_match(/not a valid shell variable name/, e) + end + def test_label assert_equal "Ronald McDonald\nMcDonalds\n123 Mac Attack\nMemphis, TENN 30000\n", rdb('label', file: 'maillist').lines.first(4).join end