fix: patch SQL injection, command injection, and config injection vul…#159
fix: patch SQL injection, command injection, and config injection vul…#159alexbelgium wants to merge 1 commit into
Conversation
…nerabilities
- play.php: Replace exec("sudo rm") with realpath() validation + unlink() to prevent
OS command injection via deletefile parameter (CVSSv3 9.8)
- play.php: Use parameterized query for filename lookup to prevent SQL injection
- common.php: Convert all 3 query functions (fetch_species_array, fetch_best_detection,
fetch_all_detections) from string interpolation to parameterized queries with bindValue()
- config.php: Sanitize all user-controlled values written to birdnet.conf using whitelist
patterns and numeric validation to prevent config injection leading to root RCE
- todays_detections.php: Replace 5 concatenated LIKE clauses with parameterized bindings
to prevent SQL injection via searchterm parameter
These fixes complement PR Nachtzuster#629 which addresses command injection in newname, shiftfile,
sendtest, and species_delete. Together they close all 7 identified vulnerabilities.
Ref: Nachtzuster#608
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ba94c497dc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| $contents = preg_replace("/LATITUDE=.*/", "LATITUDE=" . $safe_num($latitude), $contents); | ||
| $contents = preg_replace("/LONGITUDE=.*/", "LONGITUDE=" . $safe_num($longitude), $contents); | ||
| $contents = preg_replace("/BIRDWEATHER_ID=.*/", "BIRDWEATHER_ID=" . $safe($birdweather_id), $contents); | ||
| $contents = preg_replace("/APPRISE_NOTIFICATION_TITLE=.*/", "APPRISE_NOTIFICATION_TITLE=" . $safe($apprise_notification_title, '/[^a-zA-Z0-9._\- ]/'), $contents); |
There was a problem hiding this comment.
When users follow the settings page guidance to put variables such as $comname or $confidence in the notification title, this whitelist removes the $ before writing birdnet.conf. The notification code later calls render_template() on the title, so a title like New $comname is saved as New comname and future alerts no longer include the detected species. Please preserve the supported $... tokens while still escaping the config value.
Useful? React with 👍 / 👎.
| @unlink($file_pointer); | ||
| if (@unlink($file_pointer . ".png") || true) { |
There was a problem hiding this comment.
If PHP/caddy lacks write permission on a restored or externally mounted recordings directory, the audio unlink() can fail, but || true makes the handler still return OK and then remove the database row. That leaves the recording orphaned on disk while the UI reports success; check the audio deletion result and only tolerate a missing PNG separately before deleting the DB row.
Useful? React with 👍 / 👎.
1 participant
…nerabilities
These fixes complement PR Nachtzuster#629 which addresses command injection in newname, shiftfile, sendtest, and species_delete. Together they close all 7 identified vulnerabilities.
Ref: Nachtzuster#608