diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 02a24595..d68d3712 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -49,6 +49,12 @@ jobs: - name: Verify release changelogs exist for this versionCode run: ./scripts/check-release-changelogs.sh + # Assert the guard's rules before trusting them. A member access split + # after a trailing dot, and a class name assembled by concatenation, both + # walked past the scanner and nothing would have noticed. + - name: Self-test the RNG hygiene guard + run: ./scripts/test-rng-hygiene.sh + - name: Check for silent RNG fallbacks and GCM IV reuse run: ./scripts/check-rng-hygiene.sh diff --git a/scripts/check-rng-hygiene.sh b/scripts/check-rng-hygiene.sh index 07ba7dcc..5654ab0c 100755 --- a/scripts/check-rng-hygiene.sh +++ b/scripts/check-rng-hygiene.sh @@ -124,21 +124,28 @@ preprocess() { } function emit() { if (buf != "" && !bufopt) printf "%s:%d:%s\n", fname, bufline, buf - buf = ""; bufopt = 0; open = 0 + buf = ""; bufopt = 0; open = 0; dotcont = 0 } - BEGIN { inblock = 0; instr = 0; blockopt = 0; buf = ""; open = 0; bufopt = 0 } + BEGIN { inblock = 0; instr = 0; blockopt = 0; buf = ""; open = 0; bufopt = 0; dotcont = 0 } { code = trim(strip($0)) marked = index(cmt, optout); cmt = "" if (code == "") { if (marked) blockopt = 1; next } - if (open > 0) { - buf = buf " " code + # A line ending in `.` continues a member access: Kotlin accepts + # `Math.` on one line and `random()` on the next as the same call, and a + # per-physical-line matcher sees neither half. Buffered like an + # unbalanced paren so the rules match the joined expression. + if (open > 0 || dotcont) { + # Join directly after a trailing dot: `Math.` + ` ` + `random()` would + # reassemble as `Math. random()`, which no longer matches `Math[.]random`. + buf = buf (dotcont ? "" : " ") code if (marked && !keepoptout) bufopt = 1 open += count(code, "(") - count(code, ")") - if (open <= 0) emit() + dotcont = (code ~ /\.[ \t]*$/) + if (open <= 0 && !dotcont) emit() next } @@ -147,7 +154,8 @@ preprocess() { buf = code; bufline = FNR; bufopt = 0 open = count(code, "(") - count(code, ")") - if (open <= 0) emit() + dotcont = (code ~ /\.[ \t]*$/) + if (open <= 0 && !dotcont) emit() } END { if (open > 0) emit() } ' "$f") || rc=2 @@ -191,9 +199,16 @@ scan() { # $1 = ERE, matched against code with string literals blanked } scan_with_strings() { # $1 = ERE, matched against code with literals intact + # Concatenation is collapsed before matching: `"java.util." + "Random"` builds + # the same class name a literal match would catch, and Class.forName reaches + # the same PRNG. Reported against the raw line so output shows what was + # written, not the normalised form. printf '%s\n' "$CODE_STR" | awk -v pat="$1" '{ + raw = $0 line = $0; sub(/^[^:]*:[0-9]+:/, "", line) - if (line ~ pat) print $0 + norm = line + gsub(/"[ \t]*\+[ \t]*"/, "", norm) + if (line ~ pat || norm ~ pat) print raw }' || scanner_died } @@ -216,6 +231,16 @@ report() { # $1 = findings, $2 = headline, $3.. = hints # refactor away. # The `(^|[^a-zA-Z0-9_.])` guard deliberately does NOT apply to the fully # qualified names: `java.util.concurrent.ThreadLocalRandom.current()` has a `.` +# ------------------------------ reflection reaches a PRNG by name, not by token ---- +# `Class.forName("java.util." + "Random")` builds the banned name at runtime, so +# no literal match can see it, and reflection can reach any generator this file +# forbids by spelling. There is no legitimate use in this codebase today, so the +# rule is: don't. A marker documents it if that ever changes. +reflect_bad=$(scan '(Class[.]forName|[.]loadClass)[ \t]*[(]') +report "$reflect_bad" "loads a class by name, which can reach a banned generator without naming it:" \ + 'construct the type directly so the rules above can see which one it is.' \ + "Mark a deliberate use: // $OPT_OUT - " + # in front of the token, so anchoring it would make qualified use invisible. weak_rng=$(scan 'kotlin[.]random|java[.]util[.]Random|Math[.]random|ThreadLocalRandom|SplittableRandom|(^|[^a-zA-Z0-9_.])(Random[(]|Random[.]next)|[.]random[(][)]|SecureRandom[(][^)]') report "$weak_rng" "non-cryptographic randomness in production code:" \ diff --git a/scripts/test-rng-hygiene.sh b/scripts/test-rng-hygiene.sh new file mode 100755 index 00000000..cea1acc3 --- /dev/null +++ b/scripts/test-rng-hygiene.sh @@ -0,0 +1,130 @@ +#!/usr/bin/env bash +# Self-test for check-rng-hygiene.sh. +# +# This guard keeps non-CSPRNG generators, weakened SecureRandom and +# caller-supplied GCM IVs out of production Kotlin and Java. A scanner that +# quietly stops scanning reports a clean tree exactly like a clean tree does, so +# the rules are asserted rather than trusted. +# +# Probes go under app/src/main/kotlin: the guard deliberately skips test, +# androidTest and testFixtures, so a probe placed there passes and looks like a +# bypass when it is only a misplaced probe. The positive control catches that. +# +# Each reject case requires the guard to NAME the probe file. Without that, a +# guard aborting for an unrelated reason would be credited as a detection and +# every case below would pass while detecting nothing. +# +# Probes are staged into a throwaway GIT_INDEX_FILE, so the real index is never +# touched. The file itself must exist on disk while the guard runs, because the +# scanner reads bytes; it is removed on every path including the EXIT trap. +set -uo pipefail + +cd "$(dirname "$0")/.." || { echo "FAIL: cannot cd to the repo root"; exit 1; } +GUARD=scripts/check-rng-hygiene.sh +[ -x "$GUARD" ] || { echo "FAIL: $GUARD not found or not executable"; exit 1; } + +TMPD=$(mktemp -d) +PROBE="" +cleanup() { rm -rf "$TMPD"; [ -n "$PROBE" ] && rm -f "$PROBE"; } +trap cleanup EXIT + +fails=0 + +# run_probe +run_probe() { + local name="$1" content="$2" expect="$3" desc="$4" + + if [ -e "$name" ]; then + echo " HARNESS BROKEN: $name exists; refusing to overwrite a real file" + fails=$((fails + 1)); return + fi + PROBE="$name" + printf '%s' "$content" > "$name" + + rm -f "$TMPD/index" + GIT_INDEX_FILE="$TMPD/index" git read-tree HEAD 2>/dev/null + GIT_INDEX_FILE="$TMPD/index" git add -f "$name" 2>/dev/null + + # Both that the tree is populated AND that this probe is in it. Discarding + # the git errors above means a failed `git add` would otherwise leave the + # guard scanning a probe-free tree, and the case would be judged on a file + # the guard never saw. + local staged + staged=$(GIT_INDEX_FILE="$TMPD/index" git ls-files | wc -l) + if [ "$staged" -lt 10 ]; then + echo " HARNESS BROKEN: only $staged file(s) staged; the guard would scan almost nothing" + fails=$((fails + 1)); rm -f "$name"; PROBE=""; return + fi + if ! GIT_INDEX_FILE="$TMPD/index" git ls-files --error-unmatch "$name" >/dev/null 2>&1; then + echo " HARNESS BROKEN: $name was not staged; the guard would never see it" + fails=$((fails + 1)); rm -f "$name"; PROBE=""; return + fi + + local rc=0 out + out=$(GIT_INDEX_FILE="$TMPD/index" "$GUARD" 2>&1) || rc=$? + rm -f "$name"; PROBE="" + + if [ "$expect" = fail ]; then + if [ "$rc" -eq 0 ]; then + echo " BYPASS: $desc"; fails=$((fails + 1)) + elif ! printf '%s' "$out" | grep -qF "$name"; then + echo " WRONG REASON: $desc (guard failed without naming $name)"; fails=$((fails + 1)) + else + echo " ok: $desc" + fi + else + if [ "$rc" -ne 0 ]; then + echo " FALSE POSITIVE: $desc" + printf '%s\n' "$out" | sed 's/^/ /' | head -4 + fails=$((fails + 1)) + else + echo " ok: $desc" + fi + fi +} + +echo "== rejects what it must reject ==" + +SRC=app/src/main/kotlin/io/privkey/keep + +run_probe $SRC/ProbeCtl.kt 'val x = Math.random() +' fail "Math.random() (positive control: if this passes, nothing below means anything)" + +run_probe $SRC/ProbeSplit.kt 'val x = Math. +random() +' fail "member access split after a trailing dot" + +run_probe $SRC/ProbeSplit2.kt 'val r = java.util. +Random() +' fail "qualified name split after the package dot" + +run_probe $SRC/ProbeReflect.kt 'val c = Class.forName("java.util." + "Random") +' fail "reflection with a concatenated class name" + +run_probe $SRC/ProbeLoad.kt 'val c = javaClass.classLoader.loadClass("java.util.Random") +' fail "loadClass reaching a generator by name" + +run_probe $SRC/ProbeSeed.kt 'val r = java.security.SecureRandom(); r.setSeed(1L) +' fail "setSeed weakening SecureRandom" + +run_probe $SRC/ProbeTlr.kt 'val x = ThreadLocalRandom.current().nextInt() +' fail "ThreadLocalRandom" + +echo "== accepts what it must accept ==" + +run_probe $SRC/ProbeClean.kt 'val x = 1 +' pass "ordinary code" + +run_probe $SRC/ProbeComment.kt '// Math.random() is named here in prose only +val x = 1 +' pass "a banned token inside a comment is not code" + +run_probe $SRC/ProbeTest.kt 'val x = 1 +' pass "ordinary code in the production tree" + +echo +if [ "$fails" -ne 0 ]; then + echo "FAIL: $fails case(s) did not behave as required" + exit 1 +fi +echo "OK: check-rng-hygiene.sh rejects every known bypass and accepts sanctioned use"