From 450a68897dcf6e23b9bfd7fd29ca925384b78f90 Mon Sep 17 00:00:00 2001 From: Tim de Bruijn Date: Fri, 28 Aug 2026 22:33:23 +0200 Subject: [PATCH 1/4] A serial number off the bus is data, not code The dashboard built its per-device buttons as onclick="togglePanel('m:${esc(id)}')", and esc() does not make that safe. The browser HTML-decodes an attribute value before it compiles the inline handler, so the ' that esc() produced is an apostrophe again by the time the JS parser sees it -- the string closes early and whatever follows runs. A device id is driverId + '-' + the serial number the inverter reported over RS485, and the only filter on those bytes is isPrintable(), which spans 0x20..0x7E and therefore includes the apostrophe. A device supplying a crafted serial got script into the admin's authenticated session, where sessionStorage holds the Basic-auth token -- which unlocks read-only mode and factory reset. It needs a malicious or spoofed unit on the bus, not a remote attacker; bus noise landing an apostrophe would merely break the button. All nine interpolated handlers now pass their value as a data-* attribute, read back with getAttribute -- which yields the decoded string and never executable text. Two of the nine were exploitable (the readings and settings buttons); the rest carried loop indices and literals and were safe, but the shape was the defect, so the shape is gone. One delegated listener on document replaces them and survives every innerHTML repaint, which per-element handlers did not. Two gates keep it that way, both proven by breaking the code and watching them fail: check_web_js.py rejects ${...} inside any on* attribute, in both quote styles. It is shape-based rather than taint-based on purpose -- proving a given value safe means reading code, and these pages are re-authored from a design tool often enough that "someone will notice" is not a control. check_dashboard_layout.py renders a fleet whose EverSolar reports the serial x');window.__pwned=1;// and asserts both halves: the payload does not run, AND the button still opens its panel. Either alone passes for the wrong reason -- escaping harder satisfies the first, deleting the button satisfies both and ships a dead dashboard. With the old onclick restored alongside the new attribute it fails with "the payload ran when the button was clicked", so it detects execution rather than a missing attribute. Neither gate covered any of this before: the delegation could have been dead in all nine places and CI would have stayed green. --- src/web/assets/index_html.h | 43 ++++++++++++++++++----- tools/check_dashboard_layout.py | 60 +++++++++++++++++++++++++++++++++ tools/check_web_js.py | 43 +++++++++++++++++++++++ 3 files changed, 137 insertions(+), 9 deletions(-) diff --git a/src/web/assets/index_html.h b/src/web/assets/index_html.h index b66e846..3f7d19a 100644 --- a/src/web/assets/index_html.h +++ b/src/web/assets/index_html.h @@ -187,6 +187,31 @@ function goTab(name){ /// gets typed into the wrong section. function togglePanel(id){panel=panel===id?null:id;devDraft=null;paint()} +// Attribute-borne DATA, never attribute-borne CODE. The browser HTML-decodes an attribute +// value before the JS parser compiles an inline handler, so a quote that esc() turned into +// ' is a quote again by the time it runs -- and a device id here is driverId + '-' + +// the serial number the inverter reported over RS485, filtered only to printable ASCII. +// An apostrophe in that serial used to close the string and run whatever followed, in the +// admin's authenticated session, with the Basic-auth token sitting in sessionStorage. +// +// data-* values are read back with getAttribute, which yields the decoded string and never +// executable text, so the same bytes are inert. One delegated listener on document also +// survives every innerHTML repaint, which per-element handlers do not. +// +// tools/check_web_js.py fails the build if ${...} ever appears inside an on* attribute +// again -- the shape is what was wrong, so the shape is what is banned. +document.addEventListener('click',e=>{ + const t=e.target.closest('[data-act]'); + if(!t)return; + const a=t.getAttribute('data-act'), v=t.getAttribute('data-val')||''; + if(a==='panel')togglePanel(v); + else if(a==='tab-panel'){goTab(t.getAttribute('data-tab'));togglePanel(v)} + else if(a==='remove-extra')removeExtraAt(Number(v)); + else if(a==='save-device')saveDevice(Number(v)); + else if(a==='log-filter'){logFilter=v;loadLogs(true)} + else if(a==='wiz-pick')wizPick(v,JSON.parse(t.getAttribute('data-opts')||'{}')); +}); + // ---------------- admin auth ---------------- // fetch() never raises the browser's Basic-auth dialog: a 401 is just a 401. So ask once, keep // it for the tab only, and send the header ourselves. @@ -485,7 +510,7 @@ function firstRunCard(here){
Wire A, B and ground to the inverter first, then let the bridge look for it. Discovery listens at each driver's own line speed and reports what answered; nothing is written to the inverter and nothing is changed until you confirm.
-
+
${here?'':''}
`; } @@ -738,8 +763,8 @@ function paintInverters(){ ${!dev.label?`
Id ${esc(id)} — this is what the API, the MQTT topics and the Modbus unit mapping use.
`:''}
- - + +
${panel==='m:'+id?`
Everything this driver declares it can read. A channel is listed because the inverter has it; an em dash means it has it but is not reporting a value right now. @@ -775,12 +800,12 @@ function paintInverters(){ h+=`
${esc(e.label||'Inverter '+(i+2))} starts after a restart
- ${panel==='x:'+i?'':``}
+ ${panel==='x:'+i?'':``}
${esc((drv&&drv.display_name)||e.driver_id||'no driver chosen')}${ addr?' · address '+esc(addr):''} — added to the configuration, not polled yet. Correct it here before the restart if anything is off, or remove it.
- +
${panel==='x:'+i?deviceForm(i+1):''}
`; @@ -851,10 +876,10 @@ function deviceForm(slot){ ${drvId!==storedDrvId?'
A different driver from the one running. Its options below start at this driver\u2019s own defaults, not the stored ones.
':''}`; h+=optionFields(drv,stored,'dv_o_'); h+=`
- + - ${primary?'':``} + ${primary?'':``}
${primary?'This is the first inverter, which every build has. Point it at a different driver rather than removing it.' :'Removing it does not remove what it already published: the old entities stay in Home Assistant, available, showing their last value.'}
@@ -1046,7 +1071,7 @@ function paintHealth(){
show ${[['all','everything'],['warn','warnings & errors'],['bus','RS485 only']].map(([k,n])=> - ``).join('')} + ``).join('')} level