diff --git a/app/setup/impl/raspiblitz/router.py b/app/setup/impl/raspiblitz/router.py index 2592e36..dc7bea4 100644 --- a/app/setup/impl/raspiblitz/router.py +++ b/app/setup/impl/raspiblitz/router.py @@ -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") diff --git a/app/system/impl/raspiblitz_utils.py b/app/system/impl/raspiblitz_utils.py index c6a1304..7604c93 100644 --- a/app/system/impl/raspiblitz_utils.py +++ b/app/system/impl/raspiblitz_utils.py @@ -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 diff --git a/tests/test_setup_input_validation.py b/tests/test_setup_input_validation.py new file mode 100644 index 0000000..1a4a46d --- /dev/null +++ b/tests/test_setup_input_validation.py @@ -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)}"