[lib-audit] rollback.sh sources a data file as bash: RCE under sudo (tsk-rw62vx)

changelog.d/tsk-rw62vx-rollback-record-parsed-not-sourced.md

@@ -0,0 +1,7 @@
+### Security
+- `taos rollback` no longer executes its own state file. `scripts/rollback.sh` used to `source <install>/.taos-rollback`, which runs the file as bash — and the script escalates with `sudo` to restart the service in the same run. The file lives in the install dir, which the installer chowns to the `taos` service account, so anything able to write as `taos` (a compromised agent container with a bind mount, an updater bug, a partial write after a power cut) could get its shell executed with root. The record is now parsed line by line as data, `prev_sha` is accepted only when it is a full 40- (or 64-) character object name, and a recorded branch is restored only when `git check-ref-format` accepts it and it does not start with a dash — git considers `refs/heads/--force` a valid ref, but `git checkout -B --force` would read it as an option.
+
+### Fixed
+- A truncated or corrupt `.taos-rollback` no longer loses the recovery route. A half-written record used to abort the script with a bash syntax error, and a record with an empty or malformed `prev_sha` dead-ended on "cannot resolve"; both now fall through to the newest `taos-pre-update-*` recovery tag, which is the whole point of having one. A branch name git would refuse (`feat/..evil`, a trailing `.lock`, a leading `.`) used to abort the rollback outright when both the plain and the `--force` checkout failed; it now costs only the branch, and the recorded commit is still restored.
+- `tinyagentos/rollback.py` applies the same object-name and ref-name rules when it reads a record, and refuses to write a target that is not a full object name, so both ends agree on what counts as usable — including whitespace: Python's `$` also matches before a final newline, so a `<40 hex>\n` value used to be writable while `scripts/rollback.sh` refused it, silently leaving the install with a rollback target that did not work.
+- `scripts/rollback.sh` strips a trailing `\r` before parsing a `key='value'` line. A CRLF-formatted record (e.g. written or edited on Windows) left the `\r` glued to the end of the value, so the shell's quote-stripping never matched, `sha_safe` rejected the quoted value, and the script fell back to the recovery tag while `tinyagentos/rollback.py` (whose `splitlines()` already normalizes CRLF) reported the real recorded target — the two readers disagreed about an otherwise valid record.

scripts/rollback.sh

@@ -7,7 +7,9 @@
 #   bash scripts/rollback.sh <ref>      # roll back to a specific tag/branch/sha
 #
 # The updater records the pre-update state in <install>/.taos-rollback before it
-# touches anything, so even a clean fast-forward has a restore point.
+# touches anything, so even a clean fast-forward has a restore point. That file
+# is read as DATA (see record_field below), never sourced: it is writable by the
+# taos service account and this script escalates with sudo.
 set -euo pipefail
 
 # --- locate the install (this script lives in <install>/scripts) ---
@@ -22,28 +24,80 @@ fi
 
 log(){ echo "[rollback] $*"; }
 
+# Read one `key='value'` field out of the record file WITHOUT executing it.
+# The record sits in the install dir, which the installer chowns to the taos
+# service account, and this script escalates with sudo further down -- so
+# `source` would hand arbitrary code from a writable data file straight to
+# root. tinyagentos/rollback.py writes the values single-quoted with the
+# usual '\'' escape for an embedded quote; anything else is returned verbatim
+# and rejected by the validation below.
+record_field(){
+  local key="$1" line val
+  line="$(grep -m1 -E "^[[:space:]]*${key}=" .taos-rollback 2>/dev/null)" || return 0
+  line="${line%$'\r'}"
+  val="${line#*=}"
+  if [[ "$val" == \'*\' ]]; then
+    val="${val:1:${#val}-2}"
+    val="${val//\'\\\'\'/\'}"
+  fi
+  printf '%s' "$val"
+}
+
+# A recorded commit is a FULL object name. The writer records `git rev-parse
+# HEAD`, which is 40 hex (64 in a sha256 checkout) and never abbreviated -- so a
+# short value in the record is a truncated or forged one, not a legitimate
+# prefix, even when git would happily resolve it.
+sha_safe(){
+  [[ "$1" =~ ^[0-9a-fA-F]{40}$ || "$1" =~ ^[0-9a-fA-F]{64}$ ]]
+}
+
+# A recorded branch is usable only if git itself calls it a valid ref name, and
+# only if it does not start with a dash: `git checkout -B --force <sha>` would
+# read the name as an option, and git considers `refs/heads/--force` a perfectly
+# valid ref. check-ref-format is the authority on the rest (no `..`, no leading
+# `.`, no trailing `.lock`, no `@{`, no control characters, space or ~^:?*[\) --
+# hand-rolling that grammar in a regex is how these checks drift out of date.
+ref_safe(){
+  [[ -n "$1" && "$1" != -* ]] && git check-ref-format "refs/heads/$1" 2>/dev/null
+}
+
 target_ref="${1:-}"
+prev_branch=""
+prev_sha=""
 
 if [[ -n "$target_ref" ]]; then
   # Explicit target: a tag, branch, or sha the user named.
-  prev_branch=""
   prev_sha="$target_ref"
   log "explicit target: $target_ref"
 elif [[ -f .taos-rollback ]]; then
-  # shellcheck disable=SC1091
-  source .taos-rollback
-  prev_branch="${prev_branch:-}"
-  prev_sha="${prev_sha:-}"
-  log "recorded target: branch='${prev_branch}' commit='${prev_sha:0:12}'"
-else
-  # Fallback for installs predating the recorded file: newest recovery tag.
+  prev_branch="$(record_field prev_branch)"
+  prev_sha="$(record_field prev_sha)"
+  # A truncated or tampered record must not reach git. An unusable commit sends
+  # the whole run to the recovery tag; an unusable branch costs only the branch,
+  # because getting the commit back still beats not rolling back at all.
+  if ! sha_safe "$prev_sha"; then
+    log "record file has no usable commit; falling back to the recovery tag"
+    prev_branch=""
+    prev_sha=""
+  else
+    if ! ref_safe "$prev_branch"; then
+      log "record file has no usable branch; restoring the commit only"
+      prev_branch=""
+    fi
+    log "recorded target: branch='${prev_branch}' commit='${prev_sha:0:12}'"
+  fi
+fi
+
+if [[ -z "$prev_sha" ]]; then
+  # No usable record (install predates the file, or it is missing/corrupt):
+  # fall back to the newest recovery tag.
   prev_sha="$(git tag --list 'taos-pre-update-*' --sort=-creatordate | head -1)"
   prev_branch=""
   if [[ -z "$prev_sha" ]]; then
-    echo "taos rollback: no recorded rollback target and no taos-pre-update-* tag found" >&2
+    echo "taos rollback: no usable rollback target and no taos-pre-update-* tag found" >&2
     exit 1
   fi
-  log "no record file; using newest recovery tag: $prev_sha"
+  log "using newest recovery tag: $prev_sha"
 fi
 
 # Best-effort fetch so an explicit branch/tag that only exists on the remote