fix(csrf): Fix SCRF vulnerability in OTA examples and libraries #11530
Conversation
👋 Hello me-no-dev, we appreciate your contribution to this project! 📘 Please review the project's Contributions Guide for key guidelines on code, documentation, testing, and more. 🖊️ Please also make sure you have read and signed the Contributor License Agreement for this project. Click to see more instructions ...
Review and merge process you can expect ...
|
Memory usage test (comparing PR against master branch)The table below shows the summary of memory usage change (decrease - increase) in bytes and percentage for each target.
Click to expand the detailed deltas report [usage change in BYTES]
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@JLLeitschuh PTAL |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a CSRF vulnerability by adding authentication and CSRF header checks to OTA update endpoints across various examples.
- Enforces authentication on update routes
- Introduces CSRF header collection and validation in multiple files
- Applies similar security improvements in both WebUpdate and OTA updater code, as well as in the HTTPUpdateServer
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| libraries/WebServer/examples/WebUpdate/WebUpdate.ino | Added authentication and CSRF header validation for OTA updates |
| libraries/Update/examples/OTAWebUpdater/OTAWebUpdater.ino | Integrated CSRF checks and authentication in OTA update flow |
| libraries/HTTPUpdateServer/src/HTTPUpdateServer.h | Updated CSRF header collection and verification in the update server |
Comments suppressed due to low confidence (3)
libraries/WebServer/examples/WebUpdate/WebUpdate.ino:64
- Consider adding an inline comment clarifying the CSRF header check logic, explaining the rationale for comparing 'Origin' with 'http://' concatenated with the Host header to improve future maintainability.
String origin = server.header(String(csrfHeaders[0]));
libraries/Update/examples/OTAWebUpdater/OTAWebUpdater.ino:74
- Adding an inline comment to document the CSRF validation steps here would help clarify why the origin is compared to 'http://' + host for maintaining secure updates.
String origin = server.header(String(csrfHeaders[0]));
libraries/HTTPUpdateServer/src/HTTPUpdateServer.h:111
- Consider sending an explicit HTTP error response (with an appropriate status code) when the CSRF check fails, to clearly communicate the failure to the client.
String origin = _server->header(String(csrfHeaders[0]));
Test Results 76 files 76 suites 13m 8s ⏱️ Results for commit 5f74e65. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
This will work, but I'd suggest implementing this as a standard middleware implementation over this solution. That way the logic is reusable. But that's my ten cents. One concern around the use of the Host and Origin header is that this won't work behind middleware proxies.
That concern is raised here:
Use the Host header value: If you want your application to find its own target so it doesn't have to be configured for each deployed instance, we recommend using the Host family of headers. The Host header is meant to contain the target origin of the request. But, if your app server is sitting behind a proxy, the Host header value is most likely changed by the proxy to the target origin of the URL behind the proxy, which is different than the original URL. This modified Host header origin won't match the source origin in the original Origin or Referer headers.
I've more frequently seen CSRF protection implemented via a CSRF token in the form post body, but this will likely also work.
| @@ -7,11 +7,16 @@ | |||
|
|
|||
| #define SSID_FORMAT "ESP32-%06lX" // 12 chars total | |||
| //#define PASSWORD "test123456" // generate if remarked | |||
| const char * authUser = "admin"; | |||
| const char * authPass = "admin"; | |||
There was a problem hiding this comment.
I'd suggest generating a random password that you print to the console to let the user know what it is over using a hard-coded password.
| @@ -11,11 +11,16 @@ | |||
| const char *host = "esp32-webupdate"; | |||
| const char *ssid = "........"; | |||
| const char *password = "........"; | |||
| const char * authUser = "admin"; | |||
| const char * authPass = "admin"; | |||
There was a problem hiding this comment.
Same feedback here. Generate a random password, and serial print it. I would suggest avoiding having this hard-coded in this example
There was a problem hiding this comment.
I think you are missing the point that this is an Arduino basic example, whose purpose is not to secure every possible scenario, but instead to showcase a particular use (the Updater class). No sane person will use admin:admin to secure anything. I can put comment to change the default values or define the user/pass as dots (like the SSID and Pass for WiFi), but anything beyond that is over-complication of otherwise basic example that needs to be readable and easy to understand by novice users. Same goes for uses behind proxies, sessions, etc things that are beyond the scope of the examples and whose implementation is not straight-forward on memory constrained devices. Surely feel free to to propose changes in form of Pull Request that will satisfy your requirements :)
Using Host and Origin was stated to be enough of a protection by the links in your initial report. Session tokens are not an option in our case at all.
Closes: https://github.com/espressif/arduino-esp32/security/advisories/GHSA-9vfw-wx65-c872