Security hardening and Adminer upgrade - #580
Open
hydrospheric0 wants to merge 8 commits into
Open
Conversation
- Upgrade bundled Adminer files to 5.4.2 (adminer.php, adminer-de.php, adminer-fr.php) - Harden shell command construction in config/play/species tools using escapeshellarg - Tighten path validation for delete/rename flows - Replace deprecated FILTER_SANITIZE_STRING usage with FILTER_SANITIZE_FULL_SPECIAL_CHARS - Make notifications resilient when apprise is unavailable and use context manager for body file reads - Fix get_wav_files path handling in helpers.py
P1 - the path-validation guards added by the security hardening used str_contains()/str_starts_with(), which are PHP 8.0+. Debian 11 ships PHP 7.4, so every guarded path (delete, rename, species-delete) died with "Call to undefined function str_contains()". Add function_exists-guarded polyfills in common.php (loaded by both call sites) so the guards run on 7.4 and stay correct on 8+. P2 - performance on the Pi 3: - get_com_en_name() and get_db() tested `isset()` on a plain local, so the guard was always false: the ~331KB labels_en.json was re-read and re-decoded, and a new SQLite handle opened, on every call - i.e. once per rendered detection row. Make both `static`. - stats.php drained one SQLite3Result cursor at :61 then iterated it again at :197 with no reset(), so the second loop never ran: the Best Recordings table always rendered empty and $excludelines stayed empty, rewriting disk_check_exclude.txt with an empty block on every page view. - birdnet_analysis join()ed the report queue before every put(), forcing analysis and reporting to run strictly serially. Bound the queue instead (maxsize=1): identical backpressure, but the next chunk's inference can overlap the previous chunk's reporting. Verified on the target: php -l clean on PHP 7.4.33, polyfill semantics match PHP 8, py_compile clean.
Leaks / correctness (python):
- reporting.spectrogram(): os.remove(tmp_file) only ran on the success path, so
a sox failure (check=True) or the RuntimeError below leaked the temp PNG. /tmp
is a tmpfs, so each leak was RAM on a 1GB box. Wrap in try/finally and use
`with Image.open(...)` so the handle closes too.
- reporting.write_to_db(): con.close() sat inside the try, leaking a connection
per failed attempt, and after 3 failures the loop just fell through - the
detection was dropped from the DB while its wav, extraction, spectrogram and
BirdDB.txt line all still existed, silently desyncing DB from filesystem. Use
contextlib.closing + PRAGMA busy_timeout, catch sqlite3.Error (not
BaseException) and raise after the final attempt.
- birdnet_analysis: os.remove() was the last statement in both try blocks, so
any earlier error stranded the wav forever (nothing else reaps StreamData).
Move to finally. process_file only removes when it failed *before* handing the
file to the reporting thread, which otherwise owns it.
- birdnet_analysis: except BaseException -> except Exception, so KeyboardInterrupt
and SystemExit are no longer swallowed at shutdown.
- db.get_records(): swallowed the error, slept 2s (retrying nothing, in the
analysis critical path) and returned [], which callers cannot distinguish from
a real empty result - a DB hiccup silently dropped notifications. Now logs via
the logger (not print) and raises. The notification call sites absorb it so a
failed count skips only that notification, never the whole detection.
- notifications: cache failed image-API lookups, otherwise a down API meant a
fresh 10s timeout on every detection of that species.
- helpers.ANALYZING_NOW hardcoded ~/BirdSongs, ignoring RECS_DIR; a relocated
RECS_DIR made open() raise for every file, which the caller swallowed -
analysis stopped while the service still looked healthy. Now derived from
RECS_DIR via get_analyzing_now_path(), and spectrogram.sh derives the same
path from RECS_DIR so both sides stay in step.
Shell:
- disk_check.sh: $used was measured once and never recomputed, so the second,
more destructive purge (rm -rf $PROCESSED) fired unconditionally whenever the
first did - the sleep 1 between them shows re-measuring was the intent. Also
guard the divide-by-zero divisor, and derive from RECS_DIR.
- dump_logs.sh: services was a string, so "${services[@]}" expanded to one
element containing all names joined by newlines and the -L test never matched
- every support bundle shipped with no journals. Make it an array (as
uninstall.sh already does) and prefer the canonical birdnet.conf path.
PHP:
- play.php: busyTimeout was set on the read-only handle, leaving $db_writable
with none, so DELETE failed instantly with SQLITE_BUSY exactly when the
analysis service was inserting.
- advanced.php: compared $freqshift_hi and $RAW_SPECTROGRAM - the wrong and
undefined variables - instead of $freqshift_reconnect_delay/$raw_spectrogram.
- history.php / weekly_report.php: error_reporting is process-global and both
are include()d by views.php, so E_ALL + display_errors sprayed notices and
absolute filesystem paths into the rendered page. Align with the other views.
Verified: py_compile + bash -n clean, php -l clean on the target's PHP 7.4.33,
and the spectrogram-cleanup and write_to_db-raises paths behaviorally tested.
Root RCE via shiftfile (play.php)
The hardening escaped the ffmpeg/sox *arguments* but $shifted_path.$dir -
derived from the unvalidated $_GET['shiftfile'] via pathinfo() - was
concatenated raw into a shell_exec running under sudo, so
?shiftfile=a/$(cmd)/b.wav&doshift=true executed as root.
FILTER_SANITIZE_FULL_SPECIAL_CHARS encodes & < > " ' but NOT ; | ` $ ( ) or
newline, so it never protected this. Validate the path (the guard the delete
and rename paths already had, now factored into reject_unsafe_relpath() so the
three cannot drift again) and escapeshellarg the mkdir path. Also escape the
FREQSHIFT_* config values interpolated into the same command - the settings
pages write those, so they are not trustworthy either.
RCE via the config writers (config.php, advanced.php)
Raw $_GET was written into /etc/birdnet/birdnet.conf, which bash `source`s
(restart_services.sh et al), so LATITUDE=$(id) executed on the next restart -
triggered in the same request. advanced.php had no filter_input_array at all.
Quoting cannot fix this: the file has three parsers - bash `source`, PHP
parse_ini_string, and python ConfigParser+strip('"') - which forces the
KEY="value" convention, and inside bash double quotes $(...), `...` and $VAR
still expand. So sanitise by type at the point of read instead: conf_safe_
{string,number,token,url,device} in common.php. This also removes the
preg_replace() backreference hazard for free, since a sanitised value can no
longer contain '$' or '\'.
Verified end-to-end: a $(touch /tmp/PWNED) payload survives as an inert
literal, /tmp/PWNED is not created, and all three parsers still read the file
identically (including apostrophes in species names).
SQL injection (common.php)
fetch_species_array/fetch_best_detection/fetch_all_detections called prepare()
with the value interpolated into the SQL - no placeholder, no bind - and
callers reach them via htmlspecialchars_decode($_GET[...]), which restores the
very quotes the input filter encoded. Bind properly (as species_tools.php
already did). The decode calls stay: they are required to recover real species
names ("Anna's Hummingbird"), and are safe once the value is bound.
todays_detections.php
Raw $_GET['searchterm'] concatenated into a LIKE literal, blocked only by the
input filter's quote-encoding. Bind it. This also fixes a live bug: the filter
turned ' into &Nachtzuster#39;, so searching for a species with an apostrophe always
returned zero matches.
homepage/views.php, homepage/index.php
Still on FILTER_SANITIZE_STRING - the two actual HTTP entry points the
migration missed. Deprecated in PHP 8.1 and slated for removal, at which point
they would silently stop filtering. Also restore the `?: []` fallback.
play.php / advanced.php: display_errors=1 -> 0 (fatals leaked absolute paths).
Verified: php -l clean on the target PHP 7.4.33; sanitisers behaviourally tested
against $(id), `id`, "; id; #, $VAR and newline-injection payloads.
- common.php get_info_url(): `require` (not require_once) of the ~6,500-entry /
243KB ebird.php array literal sat inside the function body, so the whole array
was rebuilt on EVERY call - and callers invoke it once per rendered row. Load
once into a static. Use __DIR__ too: the relative path silently depended on the
caller's CWD being homepage/. Guard with the static rather than require_once,
which would become a silent no-op (leaving $ebirds undefined) if anything else
ever includes the file first.
- spectrogram.php: glob() sorts ascending and the filenames are date-time
prefixed, so $files[0] was the OLDEST file - despite being assigned to
$newest_file, and despite the RTSP branch deliberately keeping the newest.
Take the last entry.
- system_controls.php: ran a blocking, networked `git fetch` on every render, so
a slow/absent uplink hung the page for the full DNS+TCP timeout. views.php
already backgrounds the fetch and caches the result in the session for 86400s
- reuse that shape instead of duplicating it differently.
- birdnet_recording.sh: the RTSP path hardcoded `-ac 2`, ignoring ${CHANNELS}
that the arecord path honours, so RTSP users could never record mono.
- livestream.sh: `-re` sat after the output URL, where it is a no-op ("Trailing
option(s) found in the command: may be ignored") - it is an input option and is
meaningless for an inherently realtime ALSA capture anyway.
- disk_check.sh / cleanup.sh / custom_recording.sh / restart_services.sh: drop
the unconditional `set -x`. These run on a timer or at every restart and every
traced line is a journal write, i.e. SD-card wear. Left enabled in the
install/uninstall/clear_all_data scripts, which run rarely and interactively
where the trace is genuinely useful.
- system_controls.php / advanced.php / play.php: display_errors 1 -> 0.
Verified: php -l clean on the target PHP 7.4.33, bash -n clean.
Regressions I introduced (found by adversarially re-reviewing 5cc3c6a..HEAD): - advanced.php CUSTOM_IMAGE: I used conf_safe_string on an UNQUOTED value. That function only strips what bash expands INSIDE double quotes ($ ` " \) and deliberately permits ; | & and spaces - which are live when unquoted. So CUSTOM_IMAGE=a.png;cmd executed on the next `source`. Reachable from the settings form, which has no pattern attribute. In other words P4 closed three injection sinks and opened a fourth. Now written double-quoted like its sibling CUSTOM_IMAGE_TITLE. Verified: the payload is now an inert literal and bash/PHP both read it back unchanged. - birdnet_analysis: making write_to_db raise (P3) widened the blast radius from one detection to the whole chunk - a failure on detection Nachtzuster#2 of 5 dropped Nachtzuster#3-5 AND skipped apprise/bird_weather/heartbeat, where a missed heartbeat can trip a false "station down" alert. Worse, write_to_file had already appended Nachtzuster#2 to BirdDB.txt, so it caused a wider desync than the silent drop it replaced. Contain per-detection; pass only successfully reported detections onward (apprise/bird_weather read file_name_extr, unset if extract_detection failed). - disk_check.sh: my divide-by-zero guard used `exit 0`, which skipped the second block - the PROCESSED purge. On a full disk with an empty By_Date that is the fallback that actually frees space, i.e. exactly the recovery path the script exists for. Skip only the By_Date loop. - conf_safe_number defaulted to '0' on rejection, silently destroying a working config: SENSITIVITY=0 is outside BirdNET's legal 0.5-1.5 and CONFIDENCE=0 means record everything. All call sites now pass the CURRENT config value as the fallback, so a rejected input preserves the existing setting. Regex also accepts a leading '.' (".5" previously became 0). New findings from a fresh review of previously-unreviewed files: - stop_core_services.sh ran `sudo rm -rf ${RECS_DIR}/$(date ...)/*` while never sourcing the config - and install_config.sh sets RECS_DIR without export, so it was empty even when a parent had sourced it. It expanded to an absolute path like /July-2026/16-Thursday/*: the documented cleanup silently never happened, and it sat one layout change away from real damage under sudo. Source the config and refuse to run unless RECS_DIR is a real directory. - api.php is routed before any auth or filtering and a cache miss drives blocking outbound HTTP + a DB write, unbounded and unauthenticated - a loop over junk names pins the whole PHP-FPM pool. Cannot simply require auth: utils/notifications.py fetches it from localhost with no credentials. So allow loopback, require auth otherwise; bound the species pattern; honour the IMAGE_PROVIDER off-switch (which previously did not reach this endpoint at all); add a 5s timeout to ImageProvider's stream context (it was inheriting default_socket_timeout=60). - birdnet_changeidentification.sh: play.php escapeshellarg's the args, which protects the shell layer only - the values then land raw inside single-quoted SQL against a writable DB. Escape at the SQL layer. Its own guard was also `grep -q "$NEWNAME"`, treating input as a regex, satisfiable via alternation while smuggling a quote: use grep -qF --. - plotly_streamlit.py: y_downscale_factor could be 0 for a species detected in a <3h window on one day, making the [::0] slices raise "slice step cannot be zero" - precisely the rare species the view exists to inspect. max(1, ...). - common.php: rawurlencode the species name before it becomes a path segment in the Wikipedia request. - weekly_report.php: was 1+2N queries (prior-week count and first-seen check ran per species inside the loop). Pre-aggregate into two lookups: 3 grouped queries total, zero prepare() calls inside the loop. Values bound. Verified behaviourally identical to the old logic on a synthetic DB. - dump_logs.sh: `arecord --dump-hw-params` blocks forever on a live station because the capture device is always held - so logs could not be collected precisely when needed. Bound with timeout and report the omission. - createdb.sh/update_birdnet_snippets.sh: add composite (Sci_Name, Date) index. overview.php's correlated subqueries filter on both; with only the Sci_Name index each probe scans every historical row for that species. createdb.sh DROPs the table so it only helps new installs - migrate existing DBs too. Verified: py_compile + bash -n clean; php -l clean on the target PHP 7.4.33; the CUSTOM_IMAGE payload proven inert across bash and PHP; conf_safe_number proven to preserve the current value on rejection.
Strip the explanatory comments added across 66eba70, 23c810d, d5b121f, 99dc2ff and 93076c2. They should not have been added: the stated preference for this repo is comment-free code. Pre-existing comments are preserved. The reasoning they carried is recorded in ~/GITHUB/birdnet-optimization-TODO.md instead, where it belongs. update_caddyfile.sh emitted `http:// ${BIRDNETPI_URL}`, pinning the site block to one hardcoded address. The deployed Caddyfile still carried a stale 192.168.12.27 from before BIRDNETPI_URL was corrected, and the host now has several addresses that change with the network (eth0 on one LAN, wlan0 on another). Caddy already listens on *:80, so bind the site to :80 and match any host rather than swapping one hardcoded IP for another that breaks on the next move. Verified: php -l clean on the target PHP 7.4.33, py_compile and bash -n clean.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Validation