Fix unauthenticated OTA/control endpoints, CSRF, and stored XSS - #11
Open
cesar-xyz wants to merge 1 commit into
Open
Fix unauthenticated OTA/control endpoints, CSRF, and stored XSS#11cesar-xyz wants to merge 1 commit into
cesar-xyz wants to merge 1 commit into
Conversation
Every state-changing and OTA endpoint (/ban, /addblock, /unblock, /forgetwifi, /upload, /update, /setupdate, /fetchnow) had zero authentication, and ArduinoOTA had no password set — anyone who could reach the device on the LAN could reflash it with arbitrary firmware or rewrite the blocklist with no credentials at all, on a device that sits in the path of every DNS query on the network. - Add HTTP Basic Auth (WEB_USER/WEB_PASS) gating every mutating endpoint, and ArduinoOTA.setPassword(OTA_PASS). New secrets.h fields, gitignored as before. - Add a CSRF check (custom X-Requested-With header) alongside auth on every mutating endpoint. Basic Auth alone doesn't stop CSRF: browsers auto-attach cached Basic Auth credentials to any request to an already-authenticated origin, including one triggered by an unrelated page the same browser visits later (e.g. a bare <img src="http:// c3adblock.local/forgetwifi">, no JS needed). A plain <img>/auto-submit <form> CSRF can't attach a custom header; only the dashboard's own same-origin fetch() calls can, which is what now sets it. This is why /forgetwifi is no longer a bare URL you can just visit — it's now a dashboard button. - For the two upload endpoints (/upload, /update), auth+CSRF are checked inside UPLOAD_FILE_START of the multipart callback (the "done" handler that would normally 401 fires after the body is already consumed), gating a bool that's re-evaluated on every new upload attempt and guards the WRITE/END/ABORTED cases too, so no partial write happens before the check passes. - Fix stored XSS in the dashboard: custom blocklist domain names (attacker-controllable via /addblock, only required to contain a dot) were interpolated raw into innerHTML, and a second copy was interpolated directly into an inline onclick's JS string (a second injection vector via JS-string breakout). Both now go through an esc() HTML-escaper; the onclick was replaced with a data-d attribute plus an event-delegated listener. - Warn (serial log + dashboard banner via a new stats.json "defcreds" field) if WEB_PASS/OTA_PASS are left at their secrets.example.h placeholder values, since those are public in the repo. - Document all of the above in a new README Security section, including what's explicitly out of scope (the WiFi setup portal's access point stays open by design, since it must be joinable without a password first) and an honest note that Basic Auth over plain HTTP is a LAN-trust-boundary control, not encryption — this chip has no realistic budget to run a TLS server. Verified on real ESP32-C3 hardware: unauthenticated requests to protected endpoints return 401/403, correct credentials + CSRF header return 200, DNS blocking and the dashboard continue to work normally, and the XSS payload renders as inert escaped text instead of executing.
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
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
Every state-changing and OTA endpoint (
/ban,/addblock,/unblock,/forgetwifi,/upload,/update,/setupdate,/fetchnow) had zero authentication, andArduinoOTAhad no password set. Anyone who could reach the device on the LAN could reflash it with arbitrary firmware or rewrite the blocklist with no credentials at all — worth fixing given the device sits in the path of every DNS query on the network. There was also a stored-XSS in the dashboard via custom blocklist domain names./ban,/addblock,/unblock,/forgetwifi,/setupdate,/fetchnow/upload(blocklist OTA),/update(firmware OTA)OTA_PASSrequiredinnerHTMLinterpolationWhat's in this PR
WEB_USER/WEB_PASS(newsecrets.hfields, still gitignored) gate every mutating endpoint viaweb.authenticate()/web.requestAuthentication().ArduinoOTA.setPassword(OTA_PASS)added.<img src="http://c3adblock.local/forgetwifi">, no JS needed). That defeats the "LAN attacker" threat model entirely — the attacker doesn't need network access, just to get the victim's browser to fire one request. A plain<img>/auto-submitted<form>CSRF can't attach a custom header, only same-originfetch()can, so every mutating request now also requiresX-Requested-With: c3-adblock. This is why/forgetwifiis no longer a bare URL you can just visit — it's now a Forget WiFi button on the dashboard./uploadand/update, theWebServerlibrary's "done" handler (where you'd normally send a 401) fires after the multipart body is already parsed, so checking auth there is too late to prevent a partial write. Auth+CSRF are instead checked insideUPLOAD_FILE_START, storing the result in a bool (upAuthOk/fwAuthOk) that's freshly recomputed on every new upload attempt and gates theWRITE/END/ABORTEDcases too — no destructive side effect (beginBlocklistSwap(),Update.begin()) runs before the check passes./addblock— the only validation is "must contain a dot") were interpolated raw intoinnerHTML, and a second copy was interpolated directly into an inlineonclick's JS string (a second injection vector via JS-string breakout, independent of theinnerHTMLone). Both now go through a smallesc()HTML-escaper; theonclickwas replaced with adata-dattribute plus an event-delegated click listener.WEB_PASS/OTA_PASSare left at theirsecrets.example.hplaceholder values, those are public (they're in this repo). The firmware now logs a warning over serial and shows a dashboard banner (newstats.json"defcreds"field) when this is the case — it still boots and runs (not blocking, to avoid bricking usability for anyone mid-setup), but it's loud about it./and/stats.jsonstay unauthenticated by design (read-only dashboard view).Test plan
Verified on real ESP32-C3 SuperMini hardware (flashed via
pio run -t upload && pio run -t uploadfs, blocklist built viatools/build_blocklist.py):espressif32/arduino), same flash/RAM footprint (+~2KB flash for the added checks)curl http://c3adblock.local/stats.json→ 200, no auth required (read-only view still open)curl "http://c3adblock.local/ban?ip=1.2.3.4"(no auth) → 401curl -u admin:<pass> "http://c3adblock.local/ban?ip=1.2.3.4"(correct auth) → 200dig @<device-ip> doubleclick.net→0.0.0.0(blocklist still resolving/blocking correctly)dig @<device-ip> github.com→ real IP (forwarding still works)<img src=x onerror=alert(1)>.evil.comvia/addblock→ stored as-is server-side, renders as inert escaped text on the dashboard (no execution)🤖 Generated with Claude Code