Skip to content

Securing OTA update #4700

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
blazoncek merged 2 commits into from
Jun 20, 2025
Merged

Securing OTA update #4700

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dcd3e07
Securing OTA update
blazoncek May 26, 2025
a147f41
Compile fix
blazoncek May 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions wled00/cfg.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -631,6 +631,7 @@ bool deserializeConfig(JsonObject doc, bool fromFS) {
CJSON(aOtaEnabled, ota[F("aota")]);
#endif
getStringFromJson(otaPass, pwd, 33); //normally not present due to security
CJSON(otaSameSubnet, ota[F("same-subnet")]);
}

#ifdef WLED_ENABLE_DMX
Expand Down Expand Up @@ -1127,6 +1128,7 @@ void serializeConfig(JsonObject root) {
#ifndef WLED_DISABLE_OTA
ota[F("aota")] = aOtaEnabled;
#endif
ota[F("same-subnet")] = otaSameSubnet;

#ifdef WLED_ENABLE_DMX
JsonObject dmx = root.createNestedObject("dmx");
Expand Down
3 changes: 3 additions & 0 deletions wled00/data/settings_sec.htm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,9 @@ <h2>Security & Update setup</h2>
<h3>Software Update</h3>
<button type="button" onclick="U()">Manual OTA Update</button><br>
<div id="aOTA">Enable ArduinoOTA: <input type="checkbox" name="AO"></div>
Only allow update from same network/WiFi: <input type="checkbox" name="SU"><br>
<i class="warn">&#9888; If you are using multiple VLANs (i.e. IoT or guest network) either set PIN or disable this option.<br>
Disabling this option will make your device less secure.</i><br>
<hr id="backup">
<h3>Backup & Restore</h3>
<div class="warn">&#9888; Restoring presets/configuration will OVERWRITE your current presets/configuration.<br>
Expand Down
19 changes: 16 additions & 3 deletions wled00/data/update.htm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,20 @@
<head>
<meta content='width=device-width' name='viewport'>
<title>WLED Update</title>
<script src="common.js" async type="text/javascript"></script>
<script>
function B() { window.history.back(); }
function U() { document.getElementById("uf").style.display="none";document.getElementById("msg").style.display="block"; }
var cnfr = false;
function cR() {
if (!cnfr) {
var bt = gId('rev');
bt.style.color = "red";
bt.innerText = "Revert!";
cnfr = true;
return;
}
window.open(getURL("/update?revert"),"_self");
}
function GetV() {/*injected values here*/}
</script>
<style>
Expand All @@ -15,15 +26,17 @@

<body onload="GetV()">
<h2>WLED Software Update</h2>
<form method='POST' action='./update' id='uf' enctype='multipart/form-data' onsubmit="U()">
<form method='POST' action='./update' id='upd' enctype='multipart/form-data' onsubmit="toggle('upd')">
Installed version: <span class="sip">##VERSION##</span><br>
Download the latest binary: <a href="https://github.com/wled-dev/WLED/releases" target="_blank"
style="vertical-align: text-bottom; display: inline-flex;">
<img src="https://img.shields.io/github/release/wled-dev/WLED.svg?style=flat-square"></a><br>
<input type='file' name='update' required><br> <!--should have accept='.bin', but it prevents file upload from android app-->
<button type="submit">Update!</button><br>
<hr class="sml">
<button id="rev" type="button" onclick="cR()">Revert update</button><br>
<button type="button" onclick="B()">Back</button>
</form>
<div id="msg"><b>Updating...</b><br>Please do not close or refresh the page :)</div>
<div id="Noupd" class="hide"><b>Updating...</b><br>Please do not close or refresh the page :)</div>
</body>
</html>
1 change: 1 addition & 0 deletions wled00/set.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -610,6 +610,7 @@ void handleSettingsSet(AsyncWebServerRequest *request, byte subPage)
aOtaEnabled = request->hasArg(F("AO"));
#endif
//createEditHandler(correctPIN && !otaLock);
otaSameSubnet = request->hasArg(F("SU"));
}
}

Expand Down
1 change: 1 addition & 0 deletions wled00/wled.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -590,6 +590,7 @@ WLED_GLOBAL bool aOtaEnabled _INIT(true); // ArduinoOTA allows easy upda
#else
WLED_GLOBAL bool aOtaEnabled _INIT(false); // ArduinoOTA allows easy updates directly from the IDE. Careful, it does not auto-disable when OTA lock is on
#endif
WLED_GLOBAL bool otaSameSubnet _INIT(true); // prevent OTA updates from other subnets (e.g. internet) if no PIN is set
WLED_GLOBAL char settingsPIN[5] _INIT(WLED_PIN); // PIN for settings pages
WLED_GLOBAL bool correctPIN _INIT(!strlen(settingsPIN));
WLED_GLOBAL unsigned long lastEditTime _INIT(0);
Expand Down
46 changes: 42 additions & 4 deletions wled00/wled_server.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ static const char s_redirecting[] PROGMEM = "Redirecting...";
static const char s_content_enc[] PROGMEM = "Content-Encoding";
static const char s_unlock_ota [] PROGMEM = "Please unlock OTA in security settings!";
static const char s_unlock_cfg [] PROGMEM = "Please unlock settings using PIN code!";
static const char s_rebooting [] PROGMEM = "Rebooting now...";
static const char s_notimplemented[] PROGMEM = "Not implemented";
static const char s_accessdenied[] PROGMEM = "Access Denied";
static const char _common_js[] PROGMEM = "/common.js";
Expand All @@ -31,6 +32,22 @@ static bool isIp(const String &str) {
return true;
}

static bool inSubnet(const IPAddress &ip, const IPAddress &subnet, const IPAddress &mask) {
return ((ip & mask) == (subnet & mask));
}

static bool inSameSubnet(const IPAddress &client) {
return inSubnet(client, Network.localIP(), Network.subnetMask());
}

static bool inLocalSubnet(const IPAddress &client) {
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
Comment on lines +44 to +46
Copy link
Member

@willmmiles willmmiles May 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those darn old ESP32 platforms. :( In a modern system (ESP8266, ESP32 v4 or newer) we should be using client.isLocal() instead.

|| (inSubnet(client, IPAddress(4,3,2,0), IPAddress(255,255,255,0)) && apActive) // WLED AP
Copy link
Member

@willmmiles willmmiles May 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to test for this explicitly? Shouldn't this also pass inSameSubnet()?

Copy link
Member

@willmmiles willmmiles May 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

|| inSameSubnet(client); // same subnet as WLED device
}

/*
* Integrated HTTP web server page declarations
*/
Expand Down Expand Up @@ -130,7 +147,7 @@ static String msgProcessor(const String& var)
if (optt < 60) //redirect to settings after optionType seconds
{
messageBody += F("<script>setTimeout(RS,");
messageBody +=String(optt*1000);
messageBody += String(optt*1000);
messageBody += F(")</script>");
} else if (optt < 120) //redirect back after optionType-60 seconds, unused
{
Expand Down Expand Up @@ -270,7 +287,7 @@ void initServer()
});

server.on(F("/reset"), HTTP_GET, [](AsyncWebServerRequest *request){
serveMessage(request, 200,F("Rebooting now..."),F("Please wait ~10 seconds..."),129);
serveMessage(request, 200, FPSTR(s_rebooting), F("Please wait ~10 seconds."), 131);
doReboot = true;
});

Expand Down Expand Up @@ -385,10 +402,16 @@ void initServer()
if (Update.hasError()) {
serveMessage(request, 500, F("Update failed!"), F("Please check your file and retry!"), 254);
} else {
serveMessage(request, 200, F("Update successful!"), F("Rebooting..."), 131);
serveMessage(request, 200, F("Update successful!"), FPSTR(s_rebooting), 131);
doReboot = true;
}
},[](AsyncWebServerRequest *request, String filename, size_t index, uint8_t *data, size_t len, bool isFinal){
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;
}
Comment on lines +409 to +414
Copy link
Contributor

@coderabbitai coderabbitai bot May 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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 otaSameSubnet is true: denies access if client is not in same subnet AND no PIN is set
  • When otaSameSubnet is 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.

Copy link
Contributor Author

@blazoncek blazoncek May 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was the intended behaviour. I admit it may not be the best.

Copy link
Contributor

@coderabbitai coderabbitai bot May 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

if (!correctPIN || otaLock) return;
if(!index){
DEBUG_PRINTLN(F("OTA Update Start"));
Expand Down Expand Up @@ -573,6 +596,11 @@ void serveSettings(AsyncWebServerRequest* request, bool post) {
}

if (post) { //settings/set POST request, saving
IPAddress client = request->client()->remoteIP();
if (!inLocalSubnet(client)) { // includes same subnet check
serveMessage(request, 401, FPSTR(s_accessdenied), FPSTR(s_redirecting), 123);
return;
}
if (subPage != SUBPAGE_WIFI || !(wifiLock && otaLock)) handleSettingsSet(request, subPage);

char s[32];
Expand Down Expand Up @@ -624,7 +652,17 @@ void serveSettings(AsyncWebServerRequest* request, bool post) {
case SUBPAGE_DMX : content = PAGE_settings_dmx; len = PAGE_settings_dmx_length; break;
#endif
case SUBPAGE_UM : content = PAGE_settings_um; len = PAGE_settings_um_length; break;
case SUBPAGE_UPDATE : content = PAGE_update; len = PAGE_update_length; break;
case SUBPAGE_UPDATE : content = PAGE_update; len = PAGE_update_length;
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;
}
break;
#ifndef WLED_DISABLE_2D
case SUBPAGE_2D : content = PAGE_settings_2D; len = PAGE_settings_2D_length; break;
#endif
Expand Down
1 change: 1 addition & 0 deletions wled00/xml.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -591,6 +591,7 @@ void getSettingsJS(byte subPage, Print& settingsScript)
printSetFormCheckbox(settingsScript,PSTR("NO"),otaLock);
printSetFormCheckbox(settingsScript,PSTR("OW"),wifiLock);
printSetFormCheckbox(settingsScript,PSTR("AO"),aOtaEnabled);
printSetFormCheckbox(settingsScript,PSTR("SU"),otaSameSubnet);
char tmp_buf[128];
snprintf_P(tmp_buf,sizeof(tmp_buf),PSTR("WLED %s (build %d)"),versionString,VERSION);
printSetClassElementHTML(settingsScript,PSTR("sip"),0,tmp_buf);
Expand Down
Loading