From 5f2f69dd1979f2b27a86d413b4bccefb6609f711 Mon Sep 17 00:00:00 2001 From: ce5rpy <169016246+ce5rpy@users.noreply.github.com> Date: Mon, 20 Jul 2026 22:31:11 -0400 Subject: [PATCH] fix: accept NUL-padded RPTC/RPTO from ipsc2hbp (#52) * fix: accept NUL-padded RPTC callsigns in login check Fixed-width RPTC fields may be padded with NUL (ipsc2hbp) or spaces (MMDVM). str.rstrip() left trailing NULs so matching DB callsigns were rejected with MSTNAK after a successful passphrase exchange. * fix: strip NUL padding from RPTO OPTIONS in logs and storage ipsc2hbp pads the fixed-width RPTO body with NULs; logs showed ^@ noise and peer CALLSIGN as raw bytes on the options line. Normalize on ingest and in redact_pass_in_options. * fix: strip NUL padding from monitor export and shared HBP fields Centralize fixed-width NUL/space trimming in domain helpers and use them for dashboard_state peer fields, OPTIONS parsing, proxy self-service, and remaining CALLSIGN log lines. --- src/adn_server/application/report/payloads.py | 19 +++------ src/adn_server/domain/hbp_protocol.py | 24 ++++++++++++ .../infrastructure/options_redaction.py | 14 ++++--- .../proxy/self_service_bridge.py | 7 ++-- .../twisted_adapters/udp_hbp.py | 39 ++++++++++++++----- tests/application/test_report_payloads.py | 24 ++++++++++++ tests/domain/test_hbp_protocol.py | 37 ++++++++++++++++++ .../infrastructure/test_hbp_auth_handshake.py | 39 +++++++++++++++++++ .../infrastructure/test_options_redaction.py | 5 +++ 9 files changed, 177 insertions(+), 31 deletions(-) create mode 100644 tests/domain/test_hbp_protocol.py diff --git a/src/adn_server/application/report/payloads.py b/src/adn_server/application/report/payloads.py index 79476d1..49a0ab1 100644 --- a/src/adn_server/application/report/payloads.py +++ b/src/adn_server/application/report/payloads.py @@ -32,6 +32,7 @@ from adn_server.application.routing.helpers import ( peer_rf_mode, ) from adn_server.domain import int_id +from adn_server.domain.hbp_protocol import normalize_fixed_width_ascii from adn_server.domain.ua_timer import normalize_ua_timer_minutes, ua_timer_is_infinite REPORT_PROTOCOL = 2 @@ -68,11 +69,8 @@ def _parse_options_kv(options: Any) -> dict[str, str]: """Parse RPTO OPTIONS string into upper-case keys (legacy normalisation).""" if options is None: return {} - if isinstance(options, bytes): - text = options.decode("utf-8", errors="replace") - else: - text = str(options) - text = text.rstrip("\x00").encode("ascii", "ignore").decode() + text = normalize_fixed_width_ascii(options) + text = text.encode("ascii", "ignore").decode() text = re.sub(r"['\"]", "", text).strip() if not text: return {} @@ -262,10 +260,7 @@ def _peer_field_json(value: Any) -> str | None: """Sanitize a legacy peer field for JSON (no secrets).""" if value is None: return None - if isinstance(value, bytes): - text = value.decode("utf-8", errors="replace").strip() - return text or None - text = str(value).strip() + text = normalize_fixed_width_ascii(value) return text or None @@ -301,11 +296,7 @@ def _sanitized_peer_options_text(options: Any) -> str | None: """RPTO OPTIONS for monitor display (omit ``PASS=`` secrets).""" if options is None: return None - if isinstance(options, bytes): - text = options.decode("utf-8", errors="replace") - else: - text = str(options) - text = text.rstrip("\x00").strip() + text = normalize_fixed_width_ascii(options) if not text: return None parts: list[str] = [] diff --git a/src/adn_server/domain/hbp_protocol.py b/src/adn_server/domain/hbp_protocol.py index f546d2d..4963041 100644 --- a/src/adn_server/domain/hbp_protocol.py +++ b/src/adn_server/domain/hbp_protocol.py @@ -28,6 +28,10 @@ belong in the domain layer so application use cases can reference them without importing infrastructure. """ +from __future__ import annotations + +from typing import Any + # Frame types (bits) HBPF_VOICE = 0x0 HBPF_VOICE_SYNC = 0x1 @@ -41,3 +45,23 @@ PROTO_VER = 5 # Stream timeout (seconds) for contention (legacy const.py) STREAM_TO = 0.36 + + +def normalize_fixed_width_ascii(value: Any) -> str: + """Decode fixed-width HBP ASCII (RPTC field, RPTO body) and strip NUL/space pad. + + MMDVMHost pads with spaces; some bridges (e.g. ipsc2hbp) NUL-pad to width. + ``str.strip()`` / ``str.rstrip()`` without ``\\x00`` leave false suffix bytes. + """ + if value is None: + return "" + if isinstance(value, (bytes, bytearray)): + text = bytes(value).decode("utf-8", errors="replace") + else: + text = str(value) + return text.rstrip("\x00 ") + + +def normalize_fixed_width_bytes(value: bytes) -> bytes: + """Strip NUL/space padding from a fixed-width RPTO/RPTC byte slice.""" + return value.rstrip(b"\x00 ") diff --git a/src/adn_server/infrastructure/options_redaction.py b/src/adn_server/infrastructure/options_redaction.py index 3deedf5..5744923 100644 --- a/src/adn_server/infrastructure/options_redaction.py +++ b/src/adn_server/infrastructure/options_redaction.py @@ -5,15 +5,19 @@ from __future__ import annotations import re from typing import Any +from adn_server.domain.hbp_protocol import normalize_fixed_width_ascii + _PASS_RE = re.compile(r"(?i)(PASS=)[^;]*") +def normalize_options_text(options: Any) -> str: + """Decode OPTIONS and strip fixed-width NUL/space padding (e.g. ipsc2hbp RPTO).""" + return normalize_fixed_width_ascii(options) + + def redact_pass_in_options(options: Any) -> str: """OPTIONS text for logging with ``PASS=`` secret replaced by ``PASS=*******``.""" - if options is None: + text = normalize_options_text(options) + if not text: return "" - if isinstance(options, (bytes, bytearray)): - text = bytes(options).decode("utf-8", errors="replace") - else: - text = str(options) return _PASS_RE.sub(r"\1*******", text) diff --git a/src/adn_server/infrastructure/proxy/self_service_bridge.py b/src/adn_server/infrastructure/proxy/self_service_bridge.py index 665763b..271969a 100644 --- a/src/adn_server/infrastructure/proxy/self_service_bridge.py +++ b/src/adn_server/infrastructure/proxy/self_service_bridge.py @@ -38,6 +38,7 @@ from adn_server.application.ports import ( ProxySelfServiceStore, ) from adn_server.application.proxy import ProxyUseCases +from adn_server.domain.hbp_protocol import normalize_fixed_width_ascii, normalize_fixed_width_bytes from adn_server.domain.proxy import ClientEndpoint from adn_server.domain.value_objects import int_id from adn_server.infrastructure.hbp_constants import RPTACK, RPTC, RPTCL, RPTO @@ -128,7 +129,7 @@ class ProxySelfServiceBridge: self._log.info( "(SELF_SERVICE) RPTO from %s:%s peer=%s len=%d payload=%s", host, port, int_id(peer_id), len(data), - redact_pass_in_options(data[8:8 + 40]), + redact_pass_in_options(data[8:] if len(data) > 8 else b""), ) return self._handle_rpto(data, peer_id, host, port) if command == RPTC and len(data) >= 5 and data[:5] != RPTCL: @@ -149,7 +150,7 @@ class ProxySelfServiceBridge: ) return mode = data[97:98].decode("utf-8", errors="replace") if len(data) >= 98 else "4" - callsign = data[8:16].rstrip().decode("utf-8", errors="replace") + callsign = normalize_fixed_width_ascii(data[8:16]) self._store.ins_conf(int_id(peer_id), peer_id, callsign, host, mode) if peer_id in self._mysql_option_peers: self._fetch_options_now(peer_id) @@ -173,7 +174,7 @@ class ProxySelfServiceBridge: host, port, int_id(peer_id), ) return False - payload = data[8:].rstrip(b"\x00") + payload = normalize_fixed_width_bytes(data[8:]) if len(data) > 8 else b"" if payload.upper().startswith(b"PASS="): psswd_raw = payload[5:] if len(psswd_raw) < 6: diff --git a/src/adn_server/infrastructure/twisted_adapters/udp_hbp.py b/src/adn_server/infrastructure/twisted_adapters/udp_hbp.py index 35cae61..c78b9b2 100644 --- a/src/adn_server/infrastructure/twisted_adapters/udp_hbp.py +++ b/src/adn_server/infrastructure/twisted_adapters/udp_hbp.py @@ -77,6 +77,7 @@ from ...application.server_voice import all_server_voice_ids from ...domain import bytes_3, bytes_4, int_id from ...domain.dmr import decode from ...domain.dmr.const import LC_OPT +from ...domain.hbp_protocol import normalize_fixed_width_ascii, normalize_fixed_width_bytes from ...domain.mesh_routing import MeshEgress, MeshIngress, PeerMeshConfig from ...domain.talker_alias import ( DMRA_PACKET_LEN, @@ -148,6 +149,11 @@ def _get_passphrase_bytes(sys_cfg: dict) -> bytes: return p if isinstance(p, bytes) else p.encode("utf-8") +def _rptc_field_str(raw: bytes) -> str: + """Decode a fixed-width RPTC ASCII field (space- or NUL-padded).""" + return normalize_fixed_width_ascii(raw) + + def _calc_hash(salt_str: bytes, password: bytes) -> bytes: """Same as legacy: bhex(sha256(salt_str + password).hexdigest()).""" return bhex(sha256(salt_str + password).hexdigest()) @@ -970,7 +976,7 @@ class HBPProtocol(DatagramProtocol): if mode == "MASTER": for _peer in self._peers: self.send_peer(_peer, b"".join([MSTCL, _peer])) - logger.info("(%s) De-Registration sent to Peer: %s (%s)", self._system, self._peers[_peer].get("CALLSIGN", b""), self._peers[_peer].get("RADIO_ID", b"")) + logger.info("(%s) De-Registration sent to Peer: %s (%s)", self._system, _rptc_field_str(self._peers[_peer].get("CALLSIGN", b"")), self._peers[_peer].get("RADIO_ID", b"")) elif mode == "PEER": self.send_master(b"".join([RPTCL, self._config.get("RADIO_ID", b"\x00\x00\x00\x00")])) logger.info("(%s) De-Registration sent to Master: %s:%s", self._system, self._config.get("MASTER_SOCKADDR", ("?", "?"))[0], self._config.get("MASTER_SOCKADDR", ("?", "?"))[1]) @@ -1073,7 +1079,7 @@ class HBPProtocol(DatagramProtocol): for peer in remove_list: logger.info( "(%s) Peer %s (%s) has timed out and is being removed", - self._system, self._peers[peer].get("CALLSIGN", b""), self._peers[peer].get("RADIO_ID", b""), + self._system, _rptc_field_str(self._peers[peer].get("CALLSIGN", b"")), self._peers[peer].get("RADIO_ID", b""), ) self.transport.write(b"".join([MSTCL, peer]), self._peers[peer]["SOCKADDR"]) self._remove_peer(peer) @@ -1496,7 +1502,7 @@ class HBPProtocol(DatagramProtocol): if _data[:5] == RPTCL: _peer_id = _data[5:9] if _peer_id in self._peers and self._peers[_peer_id]["CONNECTION"] == "YES" and self._peers[_peer_id]["SOCKADDR"] == _sockaddr: - logger.info("(%s) Peer is closing down: %s (%s)", self._system, self._peers[_peer_id]["CALLSIGN"], int_id(_peer_id)) + logger.info("(%s) Peer is closing down: %s (%s)", self._system, _rptc_field_str(self._peers[_peer_id]["CALLSIGN"]), int_id(_peer_id)) self.transport.write(b"".join([MSTNAK, _peer_id]), _sockaddr) self._remove_peer(_peer_id) sys_cfg = self._CONFIG.get("SYSTEMS", {}).get(self._system, {}) @@ -1530,16 +1536,25 @@ class HBPProtocol(DatagramProtocol): _this_peer["URL"] = _data[98:222] _this_peer["SOFTWARE_ID"] = _data[222:262] _this_peer["PACKAGE_ID"] = _data[262:302] - if ("ALLOW_UNREG_ID" in self._config and not self._config["ALLOW_UNREG_ID"]) and _this_peer["CALLSIGN"].decode("utf8", errors="replace").rstrip() != self.validate_id(_peer_id): + _sent_call = _rptc_field_str(_this_peer["CALLSIGN"]) + if ("ALLOW_UNREG_ID" in self._config and not self._config["ALLOW_UNREG_ID"]) and _sent_call != self.validate_id(_peer_id): self._remove_peer(_peer_id) if self._config.get("PROXY_CONTROL"): self.proxy_IPBlackList(_peer_id, _sockaddr) self.transport.write(b"".join([MSTNAK, _peer_id]), _sockaddr) self._CONFIG.setdefault("SYSTEMS", {}).setdefault(self._system, {})["_reset"] = True - logger.info("(%s) Callsign does not match subscriber database: ID: %s, Sent Call: %s, DB call %s", self._system, int_id(_peer_id), _this_peer["CALLSIGN"].decode("utf8", errors="replace").rstrip(), self.validate_id(_peer_id)) + logger.info("(%s) Callsign does not match subscriber database: ID: %s, Sent Call: %s, DB call %s", self._system, int_id(_peer_id), _sent_call, self.validate_id(_peer_id)) else: self.send_peer(_peer_id, b"".join([RPTACK, _peer_id])) - logger.info("(%s) Peer %s (%s) has sent repeater configuration, Package ID: %s, Software ID: %s, Desc: %s", self._system, _this_peer["CALLSIGN"], _this_peer["RADIO_ID"], self._peers[_peer_id]["PACKAGE_ID"].decode("utf8", errors="replace").rstrip(), self._peers[_peer_id]["SOFTWARE_ID"].decode("utf8", errors="replace").rstrip(), self._peers[_peer_id]["DESCRIPTION"].decode("utf8", errors="replace").rstrip()) + logger.info( + "(%s) Peer %s (%s) has sent repeater configuration, Package ID: %s, Software ID: %s, Desc: %s", + self._system, + _sent_call, + _this_peer["RADIO_ID"], + _rptc_field_str(self._peers[_peer_id]["PACKAGE_ID"]), + _rptc_field_str(self._peers[_peer_id]["SOFTWARE_ID"]), + _rptc_field_str(self._peers[_peer_id]["DESCRIPTION"]), + ) self._refresh_connected_peer_count() self._mark_downlink_index_dirty() self._config_push_throttle.note_peer_connected() @@ -1559,12 +1574,18 @@ class HBPProtocol(DatagramProtocol): _peer_id = _data[4:8] if _peer_id in self._peers and self._peers[_peer_id]["SOCKADDR"] == _sockaddr: _this_peer = self._peers[_peer_id] - _this_peer["OPTIONS"] = _data[8:] + # RPTO body is fixed-width; bridges may NUL-pad (same as RPTC fields). + _this_peer["OPTIONS"] = normalize_fixed_width_bytes(_data[8:]) invalidate_peer_options_cache(_this_peer) self._mark_downlink_index_dirty() self.send_peer(_peer_id, b"".join([RPTACK, _peer_id])) _opt_log = redact_pass_in_options(_this_peer["OPTIONS"]) - logger.info("(%s) Peer %s has sent options %s", self._system, _this_peer["CALLSIGN"], _opt_log) + logger.info( + "(%s) Peer %s has sent options %s", + self._system, + _rptc_field_str(_this_peer["CALLSIGN"]), + _opt_log, + ) # Inject-only multi-hotspot: OPTIONS live on each peer; do not let last RPTO # overwrite the shared SYSTEM row (legacy had one peer per virtual master). if not is_proxy_inject_only(self._CONFIG, self._system): @@ -1602,7 +1623,7 @@ class HBPProtocol(DatagramProtocol): self._peers[_peer_id]["PINGS_RECEIVED"] += 1 self._peers[_peer_id]["LAST_PING"] = time.time() self.send_peer(_peer_id, b"".join([MSTPONG, _peer_id])) - logger.log(logging.TRACE if hasattr(logging, "TRACE") else logging.DEBUG, "(%s) Received and answered RPTPING from peer %s (%s)", self._system, self._peers[_peer_id]["CALLSIGN"], int_id(_peer_id)) + logger.log(logging.TRACE if hasattr(logging, "TRACE") else logging.DEBUG, "(%s) Received and answered RPTPING from peer %s (%s)", self._system, _rptc_field_str(self._peers[_peer_id]["CALLSIGN"]), int_id(_peer_id)) else: self.transport.write(b"".join([MSTNAK, _peer_id]), _sockaddr) logger.info("(%s) Ping from Radio ID that is not logged in: %s", self._system, int_id(_peer_id)) diff --git a/tests/application/test_report_payloads.py b/tests/application/test_report_payloads.py index 7a029cd..45d8c51 100644 --- a/tests/application/test_report_payloads.py +++ b/tests/application/test_report_payloads.py @@ -323,6 +323,30 @@ def test_build_topology_includes_peer_display_fields() -> None: assert peer["slots"] == "2" +def test_topology_strips_nul_padded_callsign() -> None: + systems = { + "MASTER-A": { + "MODE": "MASTER", + "ENABLED": True, + "PEERS": { + bytes_3(3120001): { + "CONNECTION": "YES", + "CALLSIGN": b"Bridge\x00\x00", + } + }, + } + } + doc = build_topology(systems, seq=1, ts=1.0) + assert doc["systems"][0]["peers"][0]["callsign"] == "Bridge" + + +def test_parse_peer_options_static_strips_nul_padding() -> None: + raw = b"TS1=73080;TS2=73081;SINGLE=1;" + b"\x00" * 12 + ts1, ts2 = parse_peer_options_static(raw) + assert ts1 == ["73080"] + assert ts2 == ["73081"] + + def test_hello_connected_system_names_only_live_systems() -> None: systems = { "SYSTEM-0": { diff --git a/tests/domain/test_hbp_protocol.py b/tests/domain/test_hbp_protocol.py new file mode 100644 index 0000000..fce0ee2 --- /dev/null +++ b/tests/domain/test_hbp_protocol.py @@ -0,0 +1,37 @@ +# ADN DMR Peer Server - domain HBP protocol helpers +# +# Copyright (C) 2026 Rodrigo Pérez, CE5RPY +# +############################################################################### +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software Foundation, +# Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA +############################################################################### + +from __future__ import annotations + +from adn_server.domain.hbp_protocol import ( + normalize_fixed_width_ascii, + normalize_fixed_width_bytes, +) + + +def test_normalize_fixed_width_ascii_strips_nul_and_space() -> None: + assert normalize_fixed_width_ascii(b"CE5RPY\x00\x00") == "CE5RPY" + assert normalize_fixed_width_ascii(b"CE5RPY ") == "CE5RPY" + assert normalize_fixed_width_ascii("Bridge\x00 ") == "Bridge" + + +def test_normalize_fixed_width_bytes_strips_nul_and_space() -> None: + raw = b"TS1=730;TS2=73081" + b"\x00" * 8 + b" " + assert normalize_fixed_width_bytes(raw) == b"TS1=730;TS2=73081" diff --git a/tests/infrastructure/test_hbp_auth_handshake.py b/tests/infrastructure/test_hbp_auth_handshake.py index 96148e5..1572ba5 100644 --- a/tests/infrastructure/test_hbp_auth_handshake.py +++ b/tests/infrastructure/test_hbp_auth_handshake.py @@ -118,3 +118,42 @@ def test_hbp_auth_wrong_password_mstnak() -> None: assert nak.startswith(MSTNAK) assert addr == _CLIENT_ADDR assert _PEER not in hbp._peers + + +def _auth_to_waiting_config(hbp: HBPProtocol, transport: _RecordingTransport) -> None: + hbp.datagramReceived(RPTL + _PEER, _CLIENT_ADDR) + salt_str = bytes_4(hbp._peers[_PEER]["SALT"]) + auth_hash = _calc_hash(salt_str, _get_passphrase_bytes(hbp._config)) + transport.sent.clear() + hbp.datagramReceived(RPTK + _PEER + auth_hash, _CLIENT_ADDR) + transport.sent.clear() + + +def test_rptc_accepts_nul_padded_callsign_with_allow_unreg_false() -> None: + """RPTC callsign fields may be NUL-padded (ipsc2hbp); must match DB like space pad.""" + hbp, transport = _master_protocol() + hbp._config["ALLOW_UNREG_ID"] = False + hbp._CONFIG["_SUB_IDS"] = {1234567: "Bridge"} + _auth_to_waiting_config(hbp, transport) + + # 8-byte field: "Bridge" + two NUL pads (not spaces) + rptc = RPTC + _PEER + b"Bridge\x00\x00" + b"\x00" * 85 + b"4" + hbp.datagramReceived(rptc, _CLIENT_ADDR) + + assert len(transport.sent) == 1 + assert transport.sent[0][0].startswith(RPTACK) + assert hbp._peers[_PEER]["CONNECTION"] == "YES" + + +def test_rptc_rejects_wrong_callsign_with_allow_unreg_false() -> None: + hbp, transport = _master_protocol() + hbp._config["ALLOW_UNREG_ID"] = False + hbp._CONFIG["_SUB_IDS"] = {1234567: "Bridge"} + _auth_to_waiting_config(hbp, transport) + + rptc = RPTC + _PEER + b"WRONG " + b"\x00" * 85 + b"4" + hbp.datagramReceived(rptc, _CLIENT_ADDR) + + assert len(transport.sent) == 1 + assert transport.sent[0][0].startswith(MSTNAK) + assert _PEER not in hbp._peers diff --git a/tests/infrastructure/test_options_redaction.py b/tests/infrastructure/test_options_redaction.py index d5e3981..0910e10 100644 --- a/tests/infrastructure/test_options_redaction.py +++ b/tests/infrastructure/test_options_redaction.py @@ -45,3 +45,8 @@ def test_redact_pass_accepts_str() -> None: def test_redact_pass_none_returns_empty() -> None: assert redact_pass_in_options(None) == "" + + +def test_redact_pass_strips_nul_padding() -> None: + raw = b"TS1=73080;TS2=73081" + b"\x00" * 20 + assert redact_pass_in_options(raw) == "TS1=73080;TS2=73081"