mirror of
https://github.com/CyberMind-FR/secubox-deb.git
synced 2026-07-29 12:34:38 +00:00
fix(wireguard): address review — close private-key disclosure in _parse_wg_show, delegate webui handlers (no JS-string interp), invalidate cache on mutations
- _parse_wg_show: parse all-dump by field count (5=iface/9=peer), drop private_key (sudo route had made it leak 48 bits via /stats,/summary,/peers/status); also restores peer parsing (old detection misclassified every peer line) - webui: data-attr + delegated click handlers; guard peer-drawer loading flicker - api: clear read cache after up/down/add/remove so mutations show immediately
This commit is contained in:
parent
f3acebdc4a
commit
24972969a9
|
|
@ -150,7 +150,16 @@ def _human_bytes(b: int) -> str:
|
|||
|
||||
|
||||
def _parse_wg_show() -> Dict[str, Any]:
|
||||
"""Parse wg show output for all interfaces."""
|
||||
"""Parse `wg show all dump` for all interfaces.
|
||||
|
||||
With the `all` prefix, every line begins with the interface name:
|
||||
interface line (5 fields): iface, private-key, public-key, listen-port, fwmark
|
||||
peer line (9 fields): iface, public-key, preshared-key, endpoint,
|
||||
allowed-ips, latest-handshake, rx, tx, keepalive
|
||||
The interface private key (field 2 of the interface line) is deliberately
|
||||
never read into the result — this runs as root, so the dump contains the
|
||||
real key and any of it in a response would be a disclosure.
|
||||
"""
|
||||
result = {"interfaces": {}}
|
||||
try:
|
||||
proc = subprocess.run(
|
||||
|
|
@ -160,35 +169,33 @@ def _parse_wg_show() -> Dict[str, Any]:
|
|||
if proc.returncode != 0:
|
||||
return result
|
||||
|
||||
current_iface = None
|
||||
for line in proc.stdout.strip().split("\n"):
|
||||
if not line:
|
||||
continue
|
||||
parts = line.split("\t")
|
||||
iface = parts[0]
|
||||
|
||||
if len(parts) >= 4 and parts[1] != "(none)":
|
||||
# Interface line
|
||||
iface = parts[0]
|
||||
current_iface = iface
|
||||
if len(parts) == 5:
|
||||
# Interface line — private key (parts[1]) intentionally dropped.
|
||||
result["interfaces"][iface] = {
|
||||
"private_key": parts[1][:8] + "...",
|
||||
"public_key": parts[2],
|
||||
"listen_port": int(parts[3]) if parts[3] != "(none)" else None,
|
||||
"listen_port": int(parts[3]) if parts[3] not in ("(none)", "off") else None,
|
||||
"peers": []
|
||||
}
|
||||
elif len(parts) >= 8 and current_iface:
|
||||
# Peer line
|
||||
peer = {
|
||||
"public_key": parts[0],
|
||||
"preshared_key": parts[1] != "(none)",
|
||||
"endpoint": parts[2] if parts[2] != "(none)" else None,
|
||||
"allowed_ips": parts[3].split(",") if parts[3] != "(none)" else [],
|
||||
"last_handshake": int(parts[4]) if parts[4] != "0" else None,
|
||||
"rx_bytes": int(parts[5]),
|
||||
"tx_bytes": int(parts[6]),
|
||||
"persistent_keepalive": int(parts[7]) if parts[7] != "off" else None
|
||||
}
|
||||
result["interfaces"][current_iface]["peers"].append(peer)
|
||||
elif len(parts) == 9:
|
||||
entry = result["interfaces"].setdefault(
|
||||
iface, {"public_key": "", "listen_port": None, "peers": []}
|
||||
)
|
||||
entry["peers"].append({
|
||||
"public_key": parts[1],
|
||||
"preshared_key": parts[2] != "(none)",
|
||||
"endpoint": parts[3] if parts[3] != "(none)" else None,
|
||||
"allowed_ips": parts[4].split(",") if parts[4] != "(none)" else [],
|
||||
"last_handshake": int(parts[5]) if parts[5] != "0" else None,
|
||||
"rx_bytes": int(parts[6]),
|
||||
"tx_bytes": int(parts[7]),
|
||||
"persistent_keepalive": int(parts[8]) if parts[8] != "off" else None,
|
||||
})
|
||||
except Exception as e:
|
||||
log.error("Error parsing wg show: %s", e)
|
||||
|
||||
|
|
@ -391,6 +398,13 @@ async def _run_ctl_cached(*args, ttl: float = 8.0, timeout: int = 30) -> dict:
|
|||
return value
|
||||
|
||||
|
||||
def _invalidate_ctl_cache() -> None:
|
||||
"""Drop all cached wgctl reads. Called after a mutation (interface up/down,
|
||||
peer add/remove) so the next /status /interfaces /peers reflects it
|
||||
immediately instead of serving the pre-mutation snapshot for up to the TTL."""
|
||||
_CTL_CACHE.clear()
|
||||
|
||||
|
||||
# === Three-Fold Architecture Endpoints ===
|
||||
|
||||
@app.get("/components")
|
||||
|
|
@ -424,14 +438,18 @@ async def list_interfaces():
|
|||
async def interface_up(name: str, user=Depends(require_jwt)):
|
||||
"""Bring interface up."""
|
||||
log.info("Bringing up interface: %s", name)
|
||||
return await _run_ctl("interface", "up", name)
|
||||
result = await _run_ctl("interface", "up", name)
|
||||
_invalidate_ctl_cache()
|
||||
return result
|
||||
|
||||
|
||||
@app.post("/interface/{name}/down")
|
||||
async def interface_down(name: str, user=Depends(require_jwt)):
|
||||
"""Bring interface down."""
|
||||
log.info("Bringing down interface: %s", name)
|
||||
return await _run_ctl("interface", "down", name)
|
||||
result = await _run_ctl("interface", "down", name)
|
||||
_invalidate_ctl_cache()
|
||||
return result
|
||||
|
||||
|
||||
# === Peer Management ===
|
||||
|
|
@ -453,7 +471,9 @@ class PeerAddRequest(BaseModel):
|
|||
async def add_peer(req: PeerAddRequest, user=Depends(require_jwt)):
|
||||
"""Add new peer with auto-generated config."""
|
||||
log.info("Adding peer: %s to %s", req.name, req.interface)
|
||||
return await _run_ctl("peer", "add", req.name, req.interface)
|
||||
result = await _run_ctl("peer", "add", req.name, req.interface)
|
||||
_invalidate_ctl_cache()
|
||||
return result
|
||||
|
||||
|
||||
class PeerRemoveRequest(BaseModel):
|
||||
|
|
@ -466,8 +486,11 @@ async def remove_peer(req: PeerRemoveRequest, user=Depends(require_jwt)):
|
|||
"""Remove peer by name or public key."""
|
||||
log.info("Removing peer: %s", req.identifier)
|
||||
if req.interface:
|
||||
return await _run_ctl("peer", "remove", req.identifier, req.interface)
|
||||
return await _run_ctl("peer", "remove", req.identifier)
|
||||
result = await _run_ctl("peer", "remove", req.identifier, req.interface)
|
||||
else:
|
||||
result = await _run_ctl("peer", "remove", req.identifier)
|
||||
_invalidate_ctl_cache()
|
||||
return result
|
||||
|
||||
|
||||
@app.get("/peer/{name}/config")
|
||||
|
|
|
|||
|
|
@ -10,6 +10,13 @@ secubox-wireguard (1.0.2-1~bookworm1) bookworm; urgency=medium
|
|||
concurrent dashboard polls collapse to one wgctl run.
|
||||
* fix: kill the wgctl child on API timeout (asyncio.wait_for left it running
|
||||
→ zombie CPU pileup); service NoNewPrivileges=false so sudo -n wgctl works.
|
||||
* security: fix _parse_wg_show — parse `wg show all dump` by field count and
|
||||
never read the interface private key (the sudo route had activated a
|
||||
truncated-key disclosure via /stats,/summary,/peers/status); also fixes
|
||||
peers being dropped by the old interface/peer line misdetection.
|
||||
* webui: interface-list actions via data-attrs + event delegation (no name
|
||||
interpolated into inline handlers); peer drawers no longer blink on refresh.
|
||||
* api: invalidate the read cache after interface up/down and peer add/remove.
|
||||
|
||||
-- Gerald KERMA <devel@cybermind.fr> Fri, 10 Jul 2026 07:30:00 +0200
|
||||
|
||||
|
|
|
|||
|
|
@ -247,7 +247,7 @@
|
|||
const open = openIfaces.has(i.name);
|
||||
return '' +
|
||||
'<div class="iface ' + (isUp ? 'up' : 'down') + (open ? ' open' : '') + '" id="if-' + esc(i.name) + '">' +
|
||||
'<div class="iface-head" onclick="toggleIface(\'' + esc(i.name) + '\')">' +
|
||||
'<div class="iface-head" data-act="toggle" data-iface="' + esc(i.name) + '">' +
|
||||
'<div class="iface-emoji">' + meta.emoji + '</div>' +
|
||||
'<div class="iface-id"><div class="name">' + esc(i.name) + '</div><div class="role">' + esc(meta.role) + '</div></div>' +
|
||||
'<div class="iface-meta">' +
|
||||
|
|
@ -266,9 +266,9 @@
|
|||
'</div>' +
|
||||
'<div class="iface-actions">' +
|
||||
(isUp
|
||||
? '<button class="btn danger sm" onclick="ifaceToggle(\'' + esc(i.name) + '\',\'down\')">⏹ Bring down</button>'
|
||||
: '<button class="btn good sm" onclick="ifaceToggle(\'' + esc(i.name) + '\',\'up\')">▶ Bring up</button>') +
|
||||
'<button class="btn primary sm" onclick="event.stopPropagation();showAddPeer(\'' + esc(i.name) + '\')">+ Add peer</button>' +
|
||||
? '<button class="btn danger sm" data-act="down" data-iface="' + esc(i.name) + '">⏹ Bring down</button>'
|
||||
: '<button class="btn good sm" data-act="up" data-iface="' + esc(i.name) + '">▶ Bring up</button>') +
|
||||
'<button class="btn primary sm" data-act="addpeer" data-iface="' + esc(i.name) + '">+ Add peer</button>' +
|
||||
'</div>' +
|
||||
'<div class="peer-host" id="peers-' + esc(i.name) + '"></div>' +
|
||||
'</div>' +
|
||||
|
|
@ -304,10 +304,15 @@
|
|||
if (peerCount > PEER_AUTOLOAD_MAX && !forcedPeers.has(name)) {
|
||||
host.innerHTML = '<div class="peer-note">🧰 <b>' + peerCount + ' peers</b> on this tunnel — listing suppressed to keep the page fast ' +
|
||||
'(these are transient transparent-proxy clients).<br><br>' +
|
||||
'<button class="btn sm" onclick="forcePeers(\'' + esc(name) + '\',' + peerCount + ')">Load anyway (slow)</button></div>';
|
||||
'<button class="btn sm" data-act="force" data-iface="' + esc(name) + '" data-count="' + peerCount + '">Load anyway (slow)</button></div>';
|
||||
return;
|
||||
}
|
||||
host.innerHTML = '<div class="peer-note">Loading peers…</div>';
|
||||
// Only show the loading note on a cold drawer; on the 15s auto-refresh
|
||||
// the drawer already holds a peer list, so leave it in place (no blink,
|
||||
// no scroll reset) until the new data replaces it.
|
||||
if (!host.querySelector('.peer-list')) {
|
||||
host.innerHTML = '<div class="peer-note">Loading peers…</div>';
|
||||
}
|
||||
try {
|
||||
const res = await fetch(API + '/peers?interface=' + encodeURIComponent(name));
|
||||
const data = await res.json();
|
||||
|
|
@ -361,8 +366,12 @@
|
|||
'<div class="form-group"><label>Peer name</label>' +
|
||||
'<input id="peerName" placeholder="alice-phone" autocomplete="off"></div>' +
|
||||
'<div class="btn-group" style="margin-top:1rem">' +
|
||||
'<button class="btn primary" onclick="createPeer(\'' + esc(iface) + '\')">Generate config</button>' +
|
||||
'<button class="btn" onclick="hideModal()">Cancel</button></div>';
|
||||
'<button class="btn primary" id="peerGenBtn">Generate config</button>' +
|
||||
'<button class="btn" id="peerCancelBtn">Cancel</button></div>';
|
||||
// Bind via closures so the interface name never gets interpolated
|
||||
// into an inline handler string (avoids any JS-string breakout).
|
||||
mc.querySelector('#peerGenBtn').addEventListener('click', () => createPeer(iface));
|
||||
mc.querySelector('#peerCancelBtn').addEventListener('click', hideModal);
|
||||
document.getElementById('modal').classList.add('show');
|
||||
setTimeout(() => { const el = document.getElementById('peerName'); if (el) el.focus(); }, 50);
|
||||
}
|
||||
|
|
@ -409,6 +418,20 @@
|
|||
function hideModal() { document.getElementById('modal').classList.remove('show'); }
|
||||
document.getElementById('modal').addEventListener('click', e => { if (e.target.id === 'modal') hideModal(); });
|
||||
|
||||
// Delegated handler for the interface list. Actions carry data-act /
|
||||
// data-iface attributes instead of inline onclick, so interface names
|
||||
// (and any future peer-derived values) never enter a JS-string context.
|
||||
document.getElementById('ifaceList').addEventListener('click', e => {
|
||||
const t = e.target.closest('[data-act]');
|
||||
if (!t) return;
|
||||
const name = t.getAttribute('data-iface');
|
||||
const act = t.getAttribute('data-act');
|
||||
if (act === 'toggle') toggleIface(name);
|
||||
else if (act === 'up' || act === 'down') ifaceToggle(name, act);
|
||||
else if (act === 'addpeer') showAddPeer(name);
|
||||
else if (act === 'force') forcePeers(name, parseInt(t.getAttribute('data-count'), 10) || 0);
|
||||
});
|
||||
|
||||
async function renderConnect() {
|
||||
if (!accessData) {
|
||||
try { accessData = await (await fetch(API + '/access')).json(); } catch (e) { return; }
|
||||
|
|
|
|||
Loading…
Reference in New Issue
Block a user