Securing OTA update #4700
Securing OTA update #4700
Conversation
- prevent settings change if not using private IP address or same subnet - prevent OTA from differnet subnet if PIN is not set - ability to revert firmware
WalkthroughThe changes introduce a new boolean configuration parameter, Changes
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (19)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
wled00/cfg.cpp(2 hunks)wled00/data/settings_sec.htm(1 hunks)wled00/data/update.htm(2 hunks)wled00/set.cpp(1 hunks)wled00/wled.h(1 hunks)wled00/wled_server.cpp(7 hunks)wled00/xml.cpp(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
wled00/wled_server.cpp (1)
wled00/fcn_declare.h (1)
serveMessage(611-611)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (esp32dev_V4)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (esp01_1m_full)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
- GitHub Check: wled_build / Build Environments (usermods)
🔇 Additional comments (10)
wled00/wled.h (1)
593-593: Verify JSON serialization/deserialization of the new setting
Ensure thatotaSameSubnetis correctly persisted in the configuration: it must be read from and written to the JSON config (incfg.cpp) under the"same-subnet"key so user changes survive a reboot.wled00/set.cpp (1)
613-613: Correct integration ofotaSameSubnetin the settings handler
The new line properly captures the "SU" checkbox when password checks pass or OTA lock is disabled. This ensures the flag is updated from the HTTP request.wled00/xml.cpp (1)
594-594: ExposeotaSameSubnetin the security settings UI
AddingprintSetFormCheckbox(settingsScript, PSTR("SU"), otaSameSubnet);correctly injects the checkbox state into the JS for the security page, making the new setting visible and editable.wled00/data/settings_sec.htm (1)
60-62: LGTM! Good security enhancement with clear user guidance.The new checkbox for subnet-based OTA restriction is well-integrated into the UI with an appropriate warning message that helps users understand the implications for multi-VLAN setups.
wled00/cfg.cpp (1)
634-634: Configuration handling is properly implemented.The
otaSameSubnetsetting is correctly integrated into the configuration serialization/deserialization flow, following the established pattern for OTA settings.Also applies to: 1131-1131
wled00/data/update.htm (2)
9-19: Well-implemented rollback feature with proper user confirmation.The two-step confirmation mechanism for the rollback button prevents accidental triggering, with clear visual feedback (red color and text change) before executing the rollback.
Also applies to: 36-37
6-6:Details
✅ Verification successful
Verify that the
toggle()function is properly defined in common.js.The form submission now relies on
toggle('upd')but we need to ensure this function exists and properly handles the form submission and div visibility toggling.Also applies to: 29-29, 40-40
🏁 Script executed:
#!/bin/bash # Description: Verify that the toggle() function is defined in common.js # Search for toggle function definition in common.js fd -e js "common.js" --exec grep -l "function toggle" {} \; # If found, show the implementation fd -e js "common.js" --exec grep -A 10 "function toggle" {} \;Length of output: 654
toggle() Verified in common.js
The
toggle(el)function is defined inwled00/data/common.jsand correctly toggles thehideclass on bothelandNo + elelements:
- File: wled00/data/common.js
Implementation:function toggle(el) { gId(el).classList.toggle("hide"); gId('No'+el).classList.toggle("hide"); }No further action required.
wled00/wled_server.cpp (3)
19-19: Good practice: Centralizing reboot messages.Using a common flash string constant for reboot messages improves consistency and reduces flash memory usage.
Also applies to: 290-291, 405-405
599-604: Good security enhancement for settings protection.Restricting settings POST requests to local subnets adds an important security layer. The implementation correctly uses the
inLocalSubnetcheck which includes same subnet verification.
35-50:Details
✅ Verification successful
Subnet checking implementation looks solid with one minor concern.
The helper functions correctly implement subnet membership checks using bitwise operations. However, the WLED AP subnet check on line 47 assumes the AP always uses 4.3.2.0/24, which might not be configurable.
🏁 Script executed:
#!/bin/bash # Description: Verify if WLED AP network is always 4.3.2.0/24 or if it's configurable # Search for AP IP configuration rg -A 5 "4\.3\.2\.[0-9]" --type cpp # Search for AP network configuration rg -A 5 "apIP|AP_IP|ap.*ip" --type cpp -iLength of output: 44499
AP IP is fixed at 4.3.2.1/24 by design; no changes needed
I verified that the AP address is hard-coded in the codebase (not loaded from user-config):
• wled00/wled.cpp:
WiFi.softAPConfig(IPAddress(4,3,2,1), IPAddress(4,3,2,1), IPAddress(255,255,255,0));
• wled00/cfg.cpp:
Default “ap.ip” is generated as [4,3,2,1], and legacy code to read it is commented out.Since the AP subnet is intentionally fixed, the
inLocalSubnetcheck remains accurate and no further action is required.
| IPAddress client = request->client()->remoteIP(); | ||
| if (((otaSameSubnet && !inSameSubnet(client)) && !strlen(settingsPIN)) || (!otaSameSubnet && !inLocalSubnet(client))) { | ||
| DEBUG_PRINTLN(F("Attempted OTA update from different/non-local subnet!")); | ||
| request->send(401, FPSTR(CONTENT_TYPE_PLAIN), FPSTR(s_accessdenied)); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Security logic needs clarification.
The OTA update access control condition is complex and might have unintended behavior:
if (((otaSameSubnet && !inSameSubnet(client)) && !strlen(settingsPIN)) || (!otaSameSubnet && !inLocalSubnet(client)))
This logic means:
- When
otaSameSubnetis true: denies access if client is not in same subnet AND no PIN is set - When
otaSameSubnetis false: denies access if client is not in local subnet
This could allow OTA updates from different subnets if a PIN is set, even when otaSameSubnet is true. Is this the intended behavior?
Consider simplifying the logic:
-if (((otaSameSubnet && !inSameSubnet(client)) && !strlen(settingsPIN)) || (!otaSameSubnet && !inLocalSubnet(client))) {
+if ((otaSameSubnet && !inSameSubnet(client)) || (!otaSameSubnet && !inLocalSubnet(client))) {🤖 Prompt for AI Agents
In wled00/wled_server.cpp around lines 409 to 414, the OTA update access control
condition is overly complex and may unintentionally allow OTA updates from
different subnets when a PIN is set and otaSameSubnet is true. Simplify the
condition to clearly enforce that if otaSameSubnet is true, only clients in the
same subnet are allowed regardless of PIN, and if otaSameSubnet is false, only
clients in the local subnet are allowed. Adjust the logic to explicitly check
subnet membership first, then verify PIN presence if needed, ensuring no
unintended access is granted.
There was a problem hiding this comment.
This was the intended behaviour. I admit it may not be the best.
There was a problem hiding this comment.
@blazoncek Thank you for clarifying that this was the intended behavior. I understand the design choice - allowing PIN-protected OTA updates from different subnets when otaSameSubnet is true does provide flexibility for users who need remote access.
The trade-off is between convenience (remote OTA with PIN) and security (strict subnet enforcement). If you ever decide to revisit this, you might consider:
- Making the PIN requirement more explicit in the UI/documentation
- Or separating the subnet restriction from PIN authentication entirely
But I appreciate you confirming the intentional design!
✏️ Learnings added
Learnt from: blazoncek
PR: wled/WLED#4700
File: wled00/wled_server.cpp:409-414
Timestamp: 2025-05-26T16:09:34.316Z
Learning: In wled00/wled_server.cpp, the OTA update access control logic intentionally allows OTA updates from different subnets when otaSameSubnet is true AND a PIN is set. This was a conscious design decision by blazoncek to provide flexibility for remote OTA updates with PIN protection, though they acknowledged it may not be the optimal security model.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| return inSubnet(client, IPAddress(10,0,0,0), IPAddress(255,0,0,0)) // 10.x.x.x | ||
| || inSubnet(client, IPAddress(192,168,0,0), IPAddress(255,255,0,0)) // 192.168.x.x | ||
| || inSubnet(client, IPAddress(172,16,0,0), IPAddress(255,240,0,0)) // 172.16.x.x |
There was a problem hiding this comment.
Those darn old ESP32 platforms. :( In a modern system (ESP8266, ESP32 v4 or newer) we should be using client.isLocal() instead.
| return inSubnet(client, IPAddress(10,0,0,0), IPAddress(255,0,0,0)) // 10.x.x.x | ||
| || inSubnet(client, IPAddress(192,168,0,0), IPAddress(255,255,0,0)) // 192.168.x.x | ||
| || inSubnet(client, IPAddress(172,16,0,0), IPAddress(255,240,0,0)) // 172.16.x.x | ||
| || (inSubnet(client, IPAddress(4,3,2,0), IPAddress(255,255,255,0)) && apActive) // WLED AP |
There was a problem hiding this comment.
Do we need to test for this explicitly? Shouldn't this also pass inSameSubnet()?
There was a problem hiding this comment.
Arg, I forget that the AP might as well be a separate ethernet device as the client mode. Only real note here is that 'apActive' should probably be tested first; it'll save a couple of cycles in the average case, and be clearer that this line is different than the above.
| #ifdef ARDUINO_ARCH_ESP32 | ||
| if (request->hasArg(F("revert")) && inLocalSubnet(request->client()->remoteIP()) && Update.canRollBack()) { | ||
| doReboot = Update.rollBack(); | ||
| if (doReboot) { | ||
| serveMessage(request, 200, F("Reverted to previous version!"), FPSTR(s_rebooting), 133); | ||
| } else { | ||
| serveMessage(request, 500, F("Rollback failed!"), F("Please reboot and retry."), 254); | ||
| } | ||
| return; | ||
| } | ||
| #endif |
There was a problem hiding this comment.
Bikeshedding: Could we implement this in the handler at wled_server.cpp:387 instead, like the update POST? It doesn't feel like it ought to be mixed in with the settings code.
There was a problem hiding this comment.
I'll look into it.
There was a problem hiding this comment.
Ok, the issue is another set of checking and handling for PIN.
There was a problem hiding this comment.
Yes, 'revert' should have the same set of checks as a regular firmware upload. Best to put them in a common function I think.
There was a problem hiding this comment.
Tried it, does not add anything really useful beyond duplicating handling of PIN from serveSettings() into callback function. serveSettings() does all checks.
Perhaps my view is skewed and you may have a different POV.
There was a problem hiding this comment.
At the heart of it, my concern is that serveSettings() doesn't just serve settings pages anymore -- it's becoming an octopus function that call paths are being forced to multiplex in to, then fan out of, just to get access to the authentication code. Triggering a revert isn't serving a settings page, so my $0.02 is that it doesn't belong in serveSettings().
Also it looks to me like authentication validation is replicated in the upload POST handler as well. This is a strong indication to me that the authentication validation code should be pulled out to its own function that can be called from different contexts, independent of serveSettings().
That said: I do not consider this a blocker on this patch -- it can be addressed later.
There was a problem hiding this comment.
This is what I came up with:
server.on(_update, HTTP_GET, [](AsyncWebServerRequest *request){
if (otaLock) {
serveMessage(request, 401, FPSTR(s_accessdenied), FPSTR(s_unlock_ota), 254);
} else {
#ifdef ARDUINO_ARCH_ESP32
if (!correctPIN && strlen(settingsPIN) > 0) {
handleStaticContent(request, "", 401, FPSTR(CONTENT_TYPE_HTML), PAGE_settings_pin, PAGE_settings_pin_length);
return;
}
if (request->hasArg(F("revert")) && inLocalSubnet(request->client()->remoteIP()) && Update.canRollBack()) {
doReboot = Update.rollBack();
if (doReboot) {
serveMessage(request, 200, F("Reverted to previous version!"), FPSTR(s_rebooting), 133);
} else {
serveMessage(request, 500, F("Rollback failed!"), F("Please reboot and retry."), 254);
}
return;
}
#endif
serveSettings(request); // checks for "/update" in URL and also handles PIN
}
});
server.on(_update, HTTP_POST, [](AsyncWebServerRequest *request){
if (request->hasArg(F("PIN"))) {
checkSettingsPIN(request->arg(F("PIN")).c_str());
if (correctPIN) serveMessage(request, 200, F("PIN accepted"), FPSTR(s_redirecting), 1);
else serveMessage(request, 401, F("PIN rejected"), FPSTR(s_redirecting), 3);
return;
}
// if OTA locked or too frequent PIN entry requests fail hard
if (otaLock || !correctPIN) {
serveMessage(request, 401, FPSTR(s_accessdenied), FPSTR(s_unlock_ota), 254);
return;
}
...Which only moves PIN checking into GET handler (which is still included in serveSettings() for other purposes).
|
This would certainly help reduce the risk for those that (unwisely) use port forwarding to expose WLED to the internet. I would not help anyone that uses a reverse proxy like NGINX to do similar |
|
Given this is security related, should we be looking to include the the 0.15.1 bugfix release? |
Possibly yes. But I would put in a warning. |
Update to that version would be fine. It's just then trying to upgrade after this or downgrade that they might see issues. Good to flag in the release notes thought if we do include |
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation