mirror of
https://github.com/fusion44/blitz_api.git
synced 2026-08-20 12:57:23 +02:00
fix(setup): make input validation effective and tighten setup endpoints
password_valid()/name_valid() ended in `return re.match(...)`, which yields a Match object or None. The setup router tests them with `is False`, and `None is False` is False, so the charset check never rejected anything -- only the length and space rules were doing any work. The validators now return real booleans, and use fullmatch: with re.match the trailing `$` also matches just before a final newline. Impact is bounded, and this is a correctness bug rather than a vulnerability. These call sites are reachable only while the node is in `state=waitsetup`, and that same endpoint hands the caller a signed admin JWT by design, on a device where blitzapi has passwordless sudo. A malformed hostname reaching the setup file -- which provisioning sources as bash -- therefore grants nothing the caller does not already have, and in practice the value comes from the operator's own setup form. What it does cost: a hostname containing a quote or `$` corrupts the setup file and breaks provisioning, and the charset gate would not hold as a defence if the setup flow ever gains operator binding. Also in this change: - Gate /setup-start-done and /setup-start-info on setupPhase != "done", as /setup/shutdown already does. `state` lives in the unauthenticated key-value store, so a local process could flip it back to "waitsetup" on a fully provisioned node and be handed an admin JWT. Unlike the above, that is a real escalation, because setup is supposed to be closed at that point. - raise HTTPException instead of returning it (18 sites). FastAPI serialised the returned object as a 200 body, so rejections looked like successes; the WebUI stored that body as its access token. - Fix the status.status.HTTP_405_METHOD_NOT_ALLOWED typo (3 sites) that raised AttributeError and surfaced as an unhandled 500. - Create the setup file 0600. It holds passwords A/B/C in cleartext and provisioning appends the wallet seed words, on a tmpfs mounted mode=0777. Regression tests in tests/test_setup_input_validation.py; all nine fail before this change and pass after. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
fusion44
parent
d694f8498a
commit
97cbd4cb55
3 changed files with 171 additions and 21 deletions
|
|
@ -1,5 +1,6 @@
|
|||
import asyncio
|
||||
import logging
|
||||
import os
|
||||
|
||||
from fastapi import APIRouter, HTTPException, status
|
||||
from fastapi.params import Depends
|
||||
|
|
@ -78,9 +79,14 @@ async def setup_start_info():
|
|||
# first check that node is really in setup state
|
||||
setupPhase = await redis_get("setupPhase")
|
||||
state = await redis_get("state")
|
||||
# `state` is attacker-writable (the key-value store is unauthenticated), so
|
||||
# also check setupPhase, which only becomes "done" after provisioning.
|
||||
if setupPhase == "done":
|
||||
logging.warning("/setup-start-info blocked: node setup is already finalized")
|
||||
raise HTTPException(status.HTTP_405_METHOD_NOT_ALLOWED)
|
||||
if state != "waitsetup":
|
||||
logging.warning("/setup-start-info can only be called when nodes awaits setup")
|
||||
return HTTPException(status.status.HTTP_405_METHOD_NOT_ALLOWED)
|
||||
raise HTTPException(status.HTTP_405_METHOD_NOT_ALLOWED)
|
||||
|
||||
# get all the additional info needed to do setup dialog
|
||||
hddGotMigrationData = await redis_get("hddGotMigrationData")
|
||||
|
|
@ -100,7 +106,12 @@ async def setup_start_info():
|
|||
|
||||
|
||||
def write_text_file(filename: str, arrayOfLines):
|
||||
with open(filename, "w", encoding="utf-8") as f:
|
||||
# Holds passwords in cleartext, and provisioning appends the seed words to
|
||||
# it, on a mode=0777 tmpfs. fchmod also covers a pre-existing file, whose
|
||||
# permissions os.open() would otherwise leave alone.
|
||||
fd = os.open(filename, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600)
|
||||
os.fchmod(fd, 0o600)
|
||||
with os.fdopen(fd, "w", encoding="utf-8") as f:
|
||||
f.write("\n".join(arrayOfLines))
|
||||
|
||||
|
||||
|
|
@ -123,9 +134,13 @@ async def setup_start_done(data: StartDoneData):
|
|||
state = await redis_get("state")
|
||||
hddGotBlockchain = await redis_get("hddBlocksBitcoin")
|
||||
|
||||
# Hands out an admin JWT, so it must never be reachable post-setup.
|
||||
if setupPhase == "done":
|
||||
logging.warning("/setup-start-done blocked: node setup is already finalized")
|
||||
raise HTTPException(status.HTTP_405_METHOD_NOT_ALLOWED)
|
||||
if state != "waitsetup":
|
||||
logging.warning("/setup-start-done can only be called when nodes awaits setup")
|
||||
return HTTPException(status.HTTP_405_METHOD_NOT_ALLOWED)
|
||||
raise HTTPException(status.HTTP_405_METHOD_NOT_ALLOWED)
|
||||
|
||||
# check if a fresh setup is forced
|
||||
if data.forceFreshSetup:
|
||||
|
|
@ -136,7 +151,7 @@ async def setup_start_done(data: StartDoneData):
|
|||
if setupPhase == "setup":
|
||||
if name_valid(data.hostname) is False:
|
||||
logging.warning("hostname is not valid")
|
||||
return HTTPException(status.HTTP_400_BAD_REQUEST)
|
||||
raise HTTPException(status.HTTP_400_BAD_REQUEST)
|
||||
if (
|
||||
data.lightning != "lnd"
|
||||
and data.lightning != "cl"
|
||||
|
|
@ -145,16 +160,16 @@ async def setup_start_done(data: StartDoneData):
|
|||
logging.warning("lightning is not valid")
|
||||
if password_valid(data.passwordA) is False:
|
||||
logging.warning("passwordA is not valid")
|
||||
return HTTPException(status.HTTP_400_BAD_REQUEST)
|
||||
raise HTTPException(status.HTTP_400_BAD_REQUEST)
|
||||
if password_valid(data.passwordB) is False:
|
||||
logging.warning("passwordB is not valid")
|
||||
return HTTPException(status.HTTP_400_BAD_REQUEST)
|
||||
raise HTTPException(status.HTTP_400_BAD_REQUEST)
|
||||
if data.lightning != "none" and password_valid(data.passwordC) is False:
|
||||
logging.warning("passwordC is not valid")
|
||||
return HTTPException(status.HTTP_400_BAD_REQUEST)
|
||||
raise HTTPException(status.HTTP_400_BAD_REQUEST)
|
||||
if hddGotBlockchain != "1" and data.keepBlockchain:
|
||||
logging.warning("cannot keep blockchain that does not exists")
|
||||
return HTTPException(status.HTTP_400_BAD_REQUEST)
|
||||
raise HTTPException(status.HTTP_400_BAD_REQUEST)
|
||||
if data.keepBlockchain:
|
||||
formatHDD = 0
|
||||
cleanHDD = 1
|
||||
|
|
@ -185,7 +200,7 @@ async def setup_start_done(data: StartDoneData):
|
|||
logging.warning("check recovery data")
|
||||
if password_valid(data.passwordA) is False:
|
||||
logging.warning("passwordA is not valid")
|
||||
return HTTPException(status.HTTP_400_BAD_REQUEST)
|
||||
raise HTTPException(status.HTTP_400_BAD_REQUEST)
|
||||
write_text_file(
|
||||
setupFilePath, ["setPasswordA=1", f"passwordA='{data.passwordA}'"]
|
||||
)
|
||||
|
|
@ -196,16 +211,16 @@ async def setup_start_done(data: StartDoneData):
|
|||
hddGotMigrationData = await redis_get("hddGotMigrationData")
|
||||
if hddGotMigrationData == "":
|
||||
logging.warning("hddGotMigrationData is not available")
|
||||
return HTTPException(status.HTTP_400_BAD_REQUEST)
|
||||
raise HTTPException(status.HTTP_400_BAD_REQUEST)
|
||||
if password_valid(data.passwordA) is False:
|
||||
logging.warning("passwordA is not valid")
|
||||
return HTTPException(status.HTTP_400_BAD_REQUEST)
|
||||
raise HTTPException(status.HTTP_400_BAD_REQUEST)
|
||||
if password_valid(data.passwordB) is False:
|
||||
logging.warning("passwordB is not valid")
|
||||
return HTTPException(status.HTTP_400_BAD_REQUEST)
|
||||
raise HTTPException(status.HTTP_400_BAD_REQUEST)
|
||||
if password_valid(data.passwordC) is False:
|
||||
logging.warning("passwordC is not valid")
|
||||
return HTTPException(status.HTTP_400_BAD_REQUEST)
|
||||
raise HTTPException(status.HTTP_400_BAD_REQUEST)
|
||||
write_text_file(
|
||||
setupFilePath,
|
||||
[
|
||||
|
|
@ -221,7 +236,7 @@ async def setup_start_done(data: StartDoneData):
|
|||
|
||||
else:
|
||||
logging.warning(f"not handled setupPhase state ({setupPhase})")
|
||||
return HTTPException(status.HTTP_405_METHOD_NOT_ALLOWED)
|
||||
raise HTTPException(status.HTTP_405_METHOD_NOT_ALLOWED)
|
||||
|
||||
await _call_script("/home/admin/_cache.sh set state waitprovision")
|
||||
|
||||
|
|
@ -262,7 +277,7 @@ async def setup_final_info():
|
|||
logging.warning(
|
||||
f"/setup-final-info can only be called when nodes awaits final ({state})"
|
||||
)
|
||||
return HTTPException(status.HTTP_405_METHOD_NOT_ALLOWED)
|
||||
raise HTTPException(status.HTTP_405_METHOD_NOT_ALLOWED)
|
||||
|
||||
result_lines = []
|
||||
with open(setupFilePath, "r") as setup_file:
|
||||
|
|
@ -284,7 +299,7 @@ async def setup_final_done():
|
|||
state = await redis_get("state")
|
||||
if state != "waitfinal":
|
||||
logging.warning("/setup-final-done can only be called when nodes awaits final")
|
||||
return HTTPException(status.HTTP_405_METHOD_NOT_ALLOWED)
|
||||
raise HTTPException(status.HTTP_405_METHOD_NOT_ALLOWED)
|
||||
|
||||
await _call_script("/home/admin/_cache.sh set state donefinal")
|
||||
return {"state": "donefinal"}
|
||||
|
|
@ -297,10 +312,10 @@ async def get_shutdown():
|
|||
state = await redis_get("state")
|
||||
if setupPhase == "done":
|
||||
logging.warning("can only be called when the nodes is not finalized yet")
|
||||
return HTTPException(status.status.HTTP_405_METHOD_NOT_ALLOWED)
|
||||
raise HTTPException(status.HTTP_405_METHOD_NOT_ALLOWED)
|
||||
if state != "waitsetup":
|
||||
logging.warning("can only be called when nodes awaits setup")
|
||||
return HTTPException(status.status.HTTP_405_METHOD_NOT_ALLOWED)
|
||||
raise HTTPException(status.HTTP_405_METHOD_NOT_ALLOWED)
|
||||
|
||||
# do the shutdown
|
||||
system = RaspiBlitzSystem()
|
||||
|
|
@ -314,7 +329,7 @@ async def setup_sync_info():
|
|||
setupPhase = await redis_get("setupPhase")
|
||||
if setupPhase != "done":
|
||||
logging.warning("sync info not available yet")
|
||||
return HTTPException(status.HTTP_405_METHOD_NOT_ALLOWED)
|
||||
raise HTTPException(status.HTTP_405_METHOD_NOT_ALLOWED)
|
||||
|
||||
try:
|
||||
blitz_sync_initial_done = await redis_get("blitz_sync_initial_done")
|
||||
|
|
|
|||
|
|
@ -1,12 +1,16 @@
|
|||
import re
|
||||
|
||||
# These values reach shell command strings, so this charset is the boundary.
|
||||
# fullmatch, not match: `$` would also match before a trailing newline.
|
||||
_ALLOWED = re.compile(r"^[.a-zA-Z0-9\-_]*$")
|
||||
|
||||
|
||||
def password_valid(password: str):
|
||||
if len(password) < 8:
|
||||
return False
|
||||
if password.find(" ") >= 0:
|
||||
return False
|
||||
return re.match("^[\.a-zA-Z0-9-_]*$", password)
|
||||
return _ALLOWED.fullmatch(password) is not None
|
||||
|
||||
|
||||
def name_valid(password: str):
|
||||
|
|
@ -14,4 +18,4 @@ def name_valid(password: str):
|
|||
return False
|
||||
if password.find(" ") >= 0:
|
||||
return False
|
||||
return re.match("^[\.a-zA-Z0-9-_]*$", password)
|
||||
return _ALLOWED.fullmatch(password) is not None
|
||||
|
|
|
|||
131
tests/test_setup_input_validation.py
Normal file
131
tests/test_setup_input_validation.py
Normal file
|
|
@ -0,0 +1,131 @@
|
|||
"""
|
||||
Regression tests for the RaspiBlitz setup input validation and file handling.
|
||||
|
||||
Values from POST /setup/setup-start-done are written to the setup file, which
|
||||
provisioning sources as bash as root, so the charset validators are the
|
||||
boundary between an unauthenticated request body and root command execution.
|
||||
"""
|
||||
|
||||
import os
|
||||
import stat
|
||||
|
||||
from app.system.impl.raspiblitz_utils import name_valid, password_valid
|
||||
|
||||
# Values that must never reach a sourced-as-bash file. No spaces: the validators
|
||||
# reject those separately, and an attacker does not need them ($IFS, redirects).
|
||||
SHELL_METACHAR_INPUTS = [
|
||||
"abcdefgh';touch/tmp/pwned;'",
|
||||
"abcdefgh$(touch/tmp/pwned)",
|
||||
"abcdefgh`touch/tmp/pwned`",
|
||||
"abcdefgh;touch/tmp/pwned",
|
||||
"abcdefgh|touch/tmp/pwned",
|
||||
"abcdefgh&touch/tmp/pwned",
|
||||
"abcdefgh>/tmp/pwned",
|
||||
"abcdefgh\ntouch/tmp/pwned",
|
||||
]
|
||||
|
||||
|
||||
def test_validators_return_real_booleans():
|
||||
"""The setup router checks `is False`, so returning None was a bypass."""
|
||||
assert password_valid("bad'chars") is False
|
||||
assert name_valid("bad'chars") is False
|
||||
assert password_valid("goodpassword123") is True
|
||||
assert name_valid("goodhostname") is True
|
||||
|
||||
|
||||
def test_password_valid_rejects_shell_metacharacters():
|
||||
for candidate in SHELL_METACHAR_INPUTS:
|
||||
assert password_valid(candidate) is False, f"accepted: {candidate!r}"
|
||||
|
||||
|
||||
def test_name_valid_rejects_shell_metacharacters():
|
||||
for candidate in SHELL_METACHAR_INPUTS:
|
||||
assert name_valid(candidate) is False, f"accepted: {candidate!r}"
|
||||
|
||||
|
||||
def test_validators_reject_trailing_newline():
|
||||
"""re.match's `$` also matches before a final newline; fullmatch does not."""
|
||||
assert password_valid("abcd1234\n") is False
|
||||
assert name_valid("hostname\n") is False
|
||||
|
||||
|
||||
def test_validators_still_accept_legitimate_values():
|
||||
assert password_valid("Sat0shi-Nakamoto_2009.") is True
|
||||
assert name_valid("my-blitz_01.node") is True
|
||||
# length and space rules must keep working
|
||||
assert password_valid("short7") is False
|
||||
assert password_valid("has space here") is False
|
||||
assert name_valid("ab") is False
|
||||
|
||||
|
||||
async def _call_start_done(monkeypatch, redis_values, **overrides):
|
||||
"""Invoke setup_start_done with a faked key-value store."""
|
||||
import pytest
|
||||
from fastapi import HTTPException
|
||||
|
||||
from app.setup.impl.raspiblitz import router as setup_router
|
||||
|
||||
async def fake_redis_get(key):
|
||||
return redis_values.get(key, "")
|
||||
|
||||
monkeypatch.setattr(setup_router, "redis_get", fake_redis_get)
|
||||
|
||||
payload = {
|
||||
"hostname": "myblitz",
|
||||
"forceFreshSetup": False,
|
||||
"keepBlockchain": False,
|
||||
"lightning": "none",
|
||||
"passwordA": "goodpassword1",
|
||||
"passwordB": "goodpassword1",
|
||||
"passwordC": "",
|
||||
}
|
||||
payload.update(overrides)
|
||||
data = setup_router.StartDoneData(**payload)
|
||||
|
||||
with pytest.raises(HTTPException) as excinfo:
|
||||
await setup_router.setup_start_done(data)
|
||||
return excinfo.value
|
||||
|
||||
|
||||
async def test_finalized_node_rejects_setup_start_done(monkeypatch):
|
||||
"""`state` is attacker-writable; `setupPhase` stays "done" after setup."""
|
||||
exc = await _call_start_done(
|
||||
monkeypatch, {"state": "waitsetup", "setupPhase": "done"}
|
||||
)
|
||||
assert exc.status_code == 405
|
||||
|
||||
|
||||
async def test_setup_start_done_rejects_shell_metacharacter_hostname(monkeypatch):
|
||||
"""hostname is interpolated unquoted into the sourced-as-root file."""
|
||||
exc = await _call_start_done(
|
||||
monkeypatch,
|
||||
{"state": "waitsetup", "setupPhase": "setup"},
|
||||
hostname="blitz;touch/tmp/pwned",
|
||||
)
|
||||
assert exc.status_code == 400
|
||||
|
||||
|
||||
async def test_setup_start_done_rejects_shell_metacharacter_password(monkeypatch):
|
||||
exc = await _call_start_done(
|
||||
monkeypatch,
|
||||
{"state": "waitsetup", "setupPhase": "setup"},
|
||||
passwordA="abcdefgh';touch/tmp/pwned;'",
|
||||
)
|
||||
assert exc.status_code == 400
|
||||
|
||||
|
||||
def test_setup_file_is_created_private(tmp_path):
|
||||
"""Holds cleartext passwords and seed words on a mode=0777 tmpfs."""
|
||||
from app.setup.impl.raspiblitz.router import write_text_file
|
||||
|
||||
target = tmp_path / "raspiblitz.setup"
|
||||
write_text_file(str(target), ["passwordA='secret123'", ""])
|
||||
|
||||
mode = stat.S_IMODE(os.stat(target).st_mode)
|
||||
assert mode == 0o600, f"expected 0600, got {oct(mode)}"
|
||||
|
||||
# rewriting an existing file must not silently widen it either
|
||||
os.chmod(target, 0o644)
|
||||
write_text_file(str(target), ["passwordA='secret123'", ""])
|
||||
mode = stat.S_IMODE(os.stat(target).st_mode)
|
||||
assert mode == 0o600, f"rewrite left mode {oct(mode)}"
|
||||
Loading…
Add table
Add a link
Reference in a new issue