mirror of
https://github.com/Kpa-clawbot/meshcore-analyzer.git
synced 2026-09-26 02:43:44 +00:00
Closes #1977. Supersedes #1952. Follow-up filed as #1983. ## What §10.2 did ```bash q="SELECT COUNT(*) FROM transmissions WHERE from_node = '$TEST_PUBKEY';" qq=$(printf %q "$q") if ! count=$(ssh_t "docker exec … sqlite3 … $qq" 2>/dev/null); then count=$(ssh_t "sqlite3 … $qq" 2>/dev/null || echo "") fi ``` The injection is not reachable today — `TEST_PUBKEY` is hex-gated and the script `exit 2`s before the SQL is built. The problem is that the SQL layer's safety rests entirely on that outer gate rather than on the SQL layer itself. #1952 proposed doubling embedded quotes; that is string escaping, not parameterisation, which is why it was withdrawn in favour of this. ## What this does Per the four points in the sign-off on #1977: **1. Bind the value.** A constant `SELECT` and a bound `:pubkey`, fed to sqlite3 on stdin. The SQL no longer crosses the remote shell as a command word, so there is no `printf %q` on the query at all any more. **Why hex rather than `.parameter set :pk '<value>'`.** Dot-command arguments are split on whitespace, so a payload containing a space produces too many arguments — and sqlite3 responds by printing the `.parameter` help to **stdout**, exiting **0**, and leaving `:pk` **unbound**. `COUNT(*)` then returns 0, which reads exactly like a passing security fix. `-bail` does not catch it. Verified on 3.51.0: ``` $ printf ".parameter set :pk '' OR 1=1 --'\nSELECT COUNT(*) FROM transmissions WHERE from_node = :pk;\n" \ | sqlite3 -bail ptest.db .parameter CMD ... Manage SQL parameter bindings # <- help, on stdout … 0 # <- :pk never bound $ echo $? 0 ``` `.parameter set :pk 1+1` also binds the integer `2` — the value is evaluated as an SQL expression and only falls back to a text literal when evaluation fails. So interpolating into the `.parameter set` line trades one hazard for another. Hex-encoding removes the quoting layer instead of adding one: the value is bound as `cast(x'<hex>' as text)`, so its contribution to the SQL text is drawn from the alphabet `[0-9a-f]` only. Nothing to quote, no tokenizer arity hazard, and it holds for **arbitrary** input rather than only for hex-gated input — which is the point. Verified against a fixture table holding two rows, one of them `deadbeef`: | value | result | exit | |---|---|---| | `deadbeef`, bound as `cast(x'6465616462656566' as text)` | `1` | 0 | | `' OR 1=1 --`, bound the same way | `0` | 0 | | `' OR 1=1 --`, interpolated the current way | `2` (whole table) | 0 | | query against a DB with no `transmissions` table, `-bail` | `Parse error … no such table` on **stderr** | **1** | **2. Probe the capability, not a version.** `resolve_sqlite_runner` binds `corescope-probe-ok` and asserts it comes back — a round trip, not a bare `.parameter init`, so the positive control runs against the operator's actual binary rather than one we pin. If neither the container nor the host qualifies, it fails loudly and names what is needed: ``` ❌ retain-failed: no sqlite3 able to bind a parameter on the target tried: docker exec -i corescope-prod sqlite3, then sqlite3 on runner@example need: the sqlite3 CLI reachable over ssh, supporting '.parameter set' OCI runtime exec failed: exec: "sqlite3": executable file not found in $PATH bash: line 1: sqlite3: command not found ``` There is deliberately **no** interpolating fallback. That would leave the vulnerable path in place under a nicer name. **3. The hex gate is kept**, with its comment updated to say why: for the SQL layer it is now defence in depth rather than the only guard. Redundant is not the same as wrong. **4. The exit status and stderr survive.** `-batch -bail -init /dev/null -noheader -list` (stop at the first SQL error; ignore the operator's `~/.sqliterc`, where a stray `.mode` would make the count unparseable; stdout is exactly the number). Query stderr is captured and printed on failure rather than sent to `/dev/null`, so a broken query is distinguishable from a legitimately empty result. Probe stderr is collected too, and printed only if *both* probes fail — the container miss is the known-normal case, so surfacing it on every run would be noise. ## Also fixed An existing double-count in §10.2: the `TARGET_DB_PATH unset` branch incremented `$fails` and then left `count=""`, so the generic branch incremented it a **second** time for the same failure. `read_retain_count` now gives §10.2 exactly one increment point. Opportunistic cleanup in a file already being touched (AGENTS.md line 318). ## Tests New `qa/scripts/test-blacklist-sql.sh`, wired into the `go-test` job. 24 assertions, modelled on `scripts/staging/test-disk-monitor.sh`. Both directions are asserted, because a zero from a command that failed proves nothing: - **Positive control** — `deadbeef` still returns its row (`1`, exit 0), and so does `cafebabe`; an absent pubkey returns `0`. - **Negative** — `' OR 1=1 --` returns `0` while the table demonstrably holds 2 rows, and the old interpolated form is asserted to leak all `2`. That last assertion is what makes the `0` above worth something. - **Error surfacing** — the same SQL against a DB with no `transmissions` table exits non-zero with a message on stderr and nothing on stdout. - **Alphabet** — `sql_hex_literal` output matches `^x'[0-9a-f]*'$` for the SQL payloads, a backslash, `$(id)` / backticks, an embedded newline, `héllo`, and a 4096-byte repetitive string. That last one is a regression guard for `od -v`: without the flag `od` collapses repeated identical lines to `*`. - `run_sqlite` with no resolved runner refuses rather than guessing. Group 2 skips loudly (rather than silently) if `sqlite3` is not on PATH; group 1 needs no sqlite3 and always runs. **Mutation-tested** — each of these breaks the suite, so the assertions have teeth: | mutation | caught by | |---|---| | restore full interpolation | `injection payload → 0 rows — expected '0' got '2'` | | naive `.parameter set '%s'` | `expected '0' got '.parameter CMD ...'` | | drop `od -v` | alphabet failure on `*`, plus `expected '8192' got '33'` | Commit 1 is a behaviour-neutral refactor that moves the imperative body into `main()` behind a `BASH_SOURCE` guard, so the test can source the script and exercise individual helpers. Same idiom as `scripts/staging/disk-monitor.sh:99`. ## Verification - `bash qa/scripts/test-blacklist-sql.sh` → 24 passed, 0 failed - `bash -n` on both scripts - All three runtime paths exercised end to end with PATH shims for `ssh`/`docker`/`sqlite3` against a real fixture DB: success (`sqlite3 runner: host`, count 2), query failure (classified message + `Parse error … no such table`, `fails=1`), and no-capability (the loud block above, both probe stderrs, `fails=1` — not 2) - The new step lands inside `go-test`, which runs when `changes.outputs.code == 'true'`; `qa/scripts/*.sh` does not match that job's `^docs/|[.]md$|^LICENSE$` documentation filter, so it is not skipped ## Deliberately out of scope - **The `docker exec` branch is dead on current images** → filed as #1983. The app container has no `sqlite3` at all: `Dockerfile:15` is pure-Go SQLite with no CGO, and the `apk add` installs only `mosquitto mosquitto-clients supervisor caddy wget`. So the host "fallback" is the only path that has ever executed, silently, because both branches discarded stderr. This change keeps both branches and merely makes the outcome visible (`sqlite3 runner: …` on every run). - **`-readonly` on the target DB.** Tempting, and verified compatible with `.parameter` (the binding table lives in the TEMP database), but a WAL database needing journal recovery can refuse a read-only open. Adding it here risks exactly the "trades an unreachable injection for a script that does not run" outcome flagged in the #1952 thread. Worth its own issue. - **The other `2>/dev/null` sites** in this file, which also sit awkwardly with `qa/README.md`'s "Don't silence stderr". Only the §10.2 lines named in the sign-off are touched. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
136 lines
6.2 KiB
Bash
Executable File
136 lines
6.2 KiB
Bash
Executable File
#!/usr/bin/env bash
|
|
# test-blacklist-sql.sh — unit tests for the §10.2 SQL construction in
|
|
# qa/scripts/blacklist-test.sh (issue #1977). Sources the script and exercises
|
|
# its pure helpers, plus a real local sqlite3 against a throwaway fixture DB.
|
|
#
|
|
# Run: bash qa/scripts/test-blacklist-sql.sh
|
|
# Exits non-zero if any case fails.
|
|
#
|
|
# The point of the sqlite3 group is that BOTH directions are asserted. A test
|
|
# that only checks "the injection payload returns 0" passes just as happily when
|
|
# the query is silently broken and returns 0 for everything, so the legitimate
|
|
# pubkey must be shown to still return the row it should.
|
|
|
|
set -uo pipefail
|
|
|
|
SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)"
|
|
# shellcheck source=blacklist-test.sh
|
|
. "$SCRIPT_DIR/blacklist-test.sh"
|
|
|
|
PASS=0
|
|
FAIL=0
|
|
|
|
assert_eq() {
|
|
local label="$1" expected="$2" actual="$3"
|
|
if [ "$expected" = "$actual" ]; then
|
|
PASS=$((PASS + 1))
|
|
else
|
|
FAIL=$((FAIL + 1))
|
|
echo "FAIL: $label — expected '$expected' got '$actual'" >&2
|
|
fi
|
|
}
|
|
|
|
assert_match() {
|
|
local label="$1" pattern="$2" actual="$3"
|
|
if [[ "$actual" =~ $pattern ]]; then
|
|
PASS=$((PASS + 1))
|
|
else
|
|
FAIL=$((FAIL + 1))
|
|
echo "FAIL: $label — '$actual' does not match /$pattern/" >&2
|
|
fi
|
|
}
|
|
|
|
# ----- sql_hex_literal ------------------------------------------------------
|
|
# The security property: whatever goes in, the SQL text it produces is drawn
|
|
# from [0-9a-f] only. No caller-supplied byte can close a string literal or add
|
|
# a dot-command argument. Needs no sqlite3, so this group always runs.
|
|
assert_eq "hex of deadbeef" "x'6465616462656566'" "$(sql_hex_literal deadbeef)"
|
|
assert_eq "hex of empty" "x''" "$(sql_hex_literal "")"
|
|
|
|
HEX_ONLY="^x'[0-9a-f]*'\$"
|
|
assert_match "alphabet: sql quote payload" "$HEX_ONLY" "$(sql_hex_literal "' OR 1=1 --")"
|
|
assert_match "alphabet: drop table" "$HEX_ONLY" "$(sql_hex_literal '"; DROP TABLE transmissions; --')"
|
|
assert_match "alphabet: backslash" "$HEX_ONLY" "$(sql_hex_literal 'a\b')"
|
|
assert_match "alphabet: dollar and backtick" "$HEX_ONLY" "$(sql_hex_literal '$(id) `id`')"
|
|
assert_match "alphabet: embedded newline" "$HEX_ONLY" "$(sql_hex_literal "$(printf 'a\nb')")"
|
|
assert_match "alphabet: multibyte" "$HEX_ONLY" "$(sql_hex_literal 'héllo')"
|
|
|
|
# `od` without -v collapses runs of identical lines to '*'. A long repetitive
|
|
# value is the case that catches losing the flag.
|
|
LONG=$(printf 'x%.0s' $(seq 1 4096))
|
|
LONG_HEX=$(sql_hex_literal "$LONG")
|
|
assert_match "alphabet: 4096 repeated bytes" "$HEX_ONLY" "$LONG_HEX"
|
|
# 4096 bytes → 8192 hex digits, plus the 3 chars of x''. A collapsed run would
|
|
# be far shorter and would also fail the alphabet check on '*'.
|
|
assert_eq "no od line-collapse in 4096-byte value" "8192" "$(( ${#LONG_HEX} - 3 ))"
|
|
|
|
# ----- against a real sqlite3 ----------------------------------------------
|
|
if ! command -v sqlite3 >/dev/null 2>&1; then
|
|
echo "SKIP: sqlite3 not on PATH — skipping the ${#SQLITE_ARGS[@]}-flag query group" >&2
|
|
echo " (the alphabet assertions above still ran)" >&2
|
|
else
|
|
FIXTURE_DIR=$(mktemp -d)
|
|
trap 'rm -rf "$FIXTURE_DIR"' EXIT
|
|
DB="$FIXTURE_DIR/fixture.db"
|
|
EMPTY_DB="$FIXTURE_DIR/no-table.db"
|
|
sqlite3 "$DB" \
|
|
"CREATE TABLE transmissions(from_node TEXT); INSERT INTO transmissions VALUES('deadbeef'),('cafebabe');"
|
|
sqlite3 "$EMPTY_DB" "CREATE TABLE unrelated(x);"
|
|
|
|
run_local() { sqlite3 "${SQLITE_ARGS[@]}" "$1"; }
|
|
|
|
# The capability probe must round-trip on this machine, or the assertions
|
|
# below would be testing nothing.
|
|
assert_eq "probe round-trips" "$SQLITE_PROBE_TOKEN" "$(sqlite_probe_sql | run_local :memory:)"
|
|
|
|
# POSITIVE CONTROL: a legitimate pubkey still returns its row. Without this,
|
|
# a silently broken query looks like a passing security fix.
|
|
out=$(transmission_count_sql deadbeef | run_local "$DB"); rc=$?
|
|
assert_eq "legit pubkey → its row" "1" "$out"
|
|
assert_eq "legit pubkey → exit 0" "0" "$rc"
|
|
assert_eq "other legit pubkey" "1" "$(transmission_count_sql cafebabe | run_local "$DB")"
|
|
assert_eq "absent pubkey → 0" "0" "$(transmission_count_sql abc123 | run_local "$DB")"
|
|
|
|
# NEGATIVE: the payload binds as a literal that matches nothing. The table
|
|
# holds 2 rows, so a structural injection would return 2, not 0.
|
|
out=$(transmission_count_sql "' OR 1=1 --" | run_local "$DB"); rc=$?
|
|
assert_eq "injection payload → 0 rows" "0" "$out"
|
|
assert_eq "injection payload → exit 0" "0" "$rc"
|
|
assert_eq "table really does hold 2 rows" "2" \
|
|
"$(run_local "$DB" <<<'SELECT COUNT(*) FROM transmissions;')"
|
|
|
|
# Interpolating the same payload the old way returns the whole table. This is
|
|
# the behaviour the change removes; asserting it keeps the test honest about
|
|
# what "0" above is worth.
|
|
legacy="SELECT COUNT(*) FROM transmissions WHERE from_node = '' OR 1=1 --';"
|
|
assert_eq "old interpolated form leaked the table" "2" "$(run_local "$DB" <<<"$legacy")"
|
|
|
|
# Multibyte and whitespace values bind as themselves rather than erroring.
|
|
sqlite3 "$DB" "INSERT INTO transmissions VALUES('héllo wörld');"
|
|
assert_eq "multibyte value with a space binds" "1" \
|
|
"$(transmission_count_sql 'héllo wörld' | run_local "$DB")"
|
|
|
|
# ERROR SURFACING: a broken query must be distinguishable from an empty
|
|
# result — non-zero exit and something on stderr, not a silent "".
|
|
err_file="$FIXTURE_DIR/err"
|
|
out=$(transmission_count_sql deadbeef | run_local "$EMPTY_DB" 2>"$err_file"); rc=$?
|
|
if [ "$rc" -ne 0 ]; then PASS=$((PASS + 1)); else
|
|
FAIL=$((FAIL + 1)); echo "FAIL: missing table — expected non-zero exit, got $rc" >&2
|
|
fi
|
|
if [ -s "$err_file" ]; then PASS=$((PASS + 1)); else
|
|
FAIL=$((FAIL + 1)); echo "FAIL: missing table — expected a message on stderr" >&2
|
|
fi
|
|
assert_eq "missing table → no count on stdout" "" "$out"
|
|
|
|
# run_sqlite with no resolved runner must refuse rather than guess.
|
|
SQLITE_RUNNER=""
|
|
if run_sqlite </dev/null >/dev/null 2>&1; then
|
|
FAIL=$((FAIL + 1)); echo "FAIL: run_sqlite with no runner — expected non-zero exit" >&2
|
|
else
|
|
PASS=$((PASS + 1))
|
|
fi
|
|
fi
|
|
|
|
echo "test-blacklist-sql.sh: $PASS passed, $FAIL failed"
|
|
[ "$FAIL" -eq 0 ]
|