Sicherheit: report/screen maskieren Shell-Eingaben, Sperren privat und atomar

report setzt Zellwerte in <!befehl!> maskiert ein, screen verlangt gültige
Variablennamen, lock/unlock nutzen /tmp/rdb-locks-<UID> mit flock, vilock
sperrt per Hardlink. EDITOR darf Argumente haben. Doku und Tests ergänzt.

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 `<!befehl <Spalte>!>` 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-<UID>/` (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

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-<UID>/` ab (Modus 0700, `flock`),
+  nicht als `/tmp/L<Tabelle>` 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<Tabelle>` liegen und muss von Hand zurückbenannt werden.
+- `report`: Spaltenwerte in `<!befehl <Spalte>!>` 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

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 `<!cmd <Spalte>!>` 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-<UID>` (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.

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)
+  clash = nil
-  held = File.exist?(lf) ? File.readlines(lf, chomp: true).map { |l| l.split.map(&:to_i) } : []
+  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 = 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?(lock_file(name))
-  next unless File.exist?(lf)
 
-  keep = File.readlines(lf, chomp: true).reject { |l| l.split.first(3) == args.first(3) }
+  with_lock_file(name) do |held|
-  keep.empty? ? File.delete(lf) : File.write(lf, keep.join("\n") + "\n")
+    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<file> 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?
 

lib/rdb/cmd/report.rb

@@ -2,13 +2,17 @@
 
 # Reports, forms and mailing lists.
 
+require 'shellwords'
+
 # Fill in a form for one row: <Column> is replaced by the value, <!command!>
 # by the output of the shell command, and <!cmd <Column>!> 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

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"

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|<Nope>\nChiquita  Banana|San Jose\n|<Nope>\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', "<!echo <Cmd>!>\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: "<a b; touch X>\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