Merge pull request #74 from mwolter805/fix/reader-parser-crash-safety

fix: add umbrella crash protection and length guards to reader/parser dispatch

src/meshcore/meshcore_parser.py

@@ -42,8 +42,30 @@ class MeshcorePacketParser:
             Returns :
             completed log_data
         """
+        # Minimum viable payload is 2 bytes (1 header + 1 path_byte) for a
+        # direct route. Anything shorter is provably broken — for example,
+        # the LOG_DATA branch in reader.py only requires `len(data) > 3`,
+        # which means a 4-byte LOG_DATA frame produces a 1-byte payload
+        # here, and `path_byte = pbuf.read(1)[0]` further down would raise
+        # IndexError on the empty buffer. Populate sentinel values so the
+        # caller's downstream `log_data['route_type']` etc. lookups don't
+        # KeyError, then return early.
+        if len(payload) < 2:
+            logger.debug(f"parsePacketPayload: payload too short ({len(payload)} bytes < 2), returning sentinel log_data")
+            log_data["route_type"] = -1
+            log_data["route_typename"] = "UNK"
+            log_data["payload_type"] = -1
+            log_data["payload_typename"] = "UNK"
+            log_data["payload_ver"] = 0
+            log_data["path_len"] = 0
+            log_data["path_hash_size"] = 1
+            log_data["path"] = ""
+            log_data["pkt_payload"] = b""
+            log_data["pkt_hash"] = 0
+            return log_data
+
         pbuf = io.BytesIO(payload)
-        
+
         header = pbuf.read(1)[0]
         route_type = header & 0x03
         payload_type = (header & 0x3c) >> 2
@@ -128,7 +150,7 @@ class MeshcorePacketParser:
                     uncrypted = cipher.decrypt(msg)
                     timestamp = int.from_bytes(uncrypted[0:4], "little", signed=False)
                     attempt = uncrypted[4] & 3
-                    txt_type = int.from_bytes(uncrypted[4:4], "little", signed=False) >> 2
+                    txt_type = int.from_bytes(uncrypted[4:5], "little", signed=False) >> 2
                     message = uncrypted[5:].strip(b"\0")
                     msg_hash = int.from_bytes(SHA256.new(timestamp.to_bytes(4, "little", signed=False) + message).digest()[0:4], "little", signed=False)
                     log_data["message"] = message.decode("utf-8", "ignore")
@@ -149,39 +171,42 @@ class MeshcorePacketParser:
                 del self.channels_log[:25]
 
         elif not payload is None and payload_type == 0x04: # Advert
-            pk_buf = io.BytesIO(pkt_payload)
+            try:
-            adv_key = pk_buf.read(32).hex()
+                pk_buf = io.BytesIO(pkt_payload)
-            adv_timestamp = int.from_bytes(pk_buf.read(4), "little", signed=False)
+                adv_key = pk_buf.read(32).hex()
-            signature = pk_buf.read(64).hex()
+                adv_timestamp = int.from_bytes(pk_buf.read(4), "little", signed=False)
-            flags = pk_buf.read(1)[0]
+                signature = pk_buf.read(64).hex()
-            adv_type = flags & 0x0F
+                flags = pk_buf.read(1)[0]
-            adv_lat = None
+                adv_type = flags & 0x0F
-            adv_lon = None
+                adv_lat = None
-            adv_feat1 = None
+                adv_lon = None
-            adv_feat2 = None
+                adv_feat1 = None
-            if flags & 0x10 > 0: #has location
+                adv_feat2 = None
-                adv_lat = int.from_bytes(pk_buf.read(4), "little", signed=True)/1000000.0
+                if flags & 0x10 > 0: #has location
-                adv_lon = int.from_bytes(pk_buf.read(4), "little", signed=True)/1000000.0
+                    adv_lat = int.from_bytes(pk_buf.read(4), "little", signed=True)/1000000.0
-            if flags & 0x20 > 0: #has feature1
+                    adv_lon = int.from_bytes(pk_buf.read(4), "little", signed=True)/1000000.0
-                adv_feat1 = pk_buf.read(2).hex()
+                if flags & 0x20 > 0: #has feature1
-            if flags & 0x40 > 0: #has feature2
+                    adv_feat1 = pk_buf.read(2).hex()
-                adv_feat2 = pk_buf.read(2).hex()
+                if flags & 0x40 > 0: #has feature2
-            if flags & 0x80 > 0: #has name
+                    adv_feat2 = pk_buf.read(2).hex()
-                adv_name = pk_buf.read().decode("utf-8", "ignore").strip("\x00")
+                if flags & 0x80 > 0: #has name
-                log_data["adv_name"] = adv_name
+                    adv_name = pk_buf.read().decode("utf-8", "ignore").strip("\x00")
+                    log_data["adv_name"] = adv_name
 
-            log_data["adv_key"] = adv_key
+                log_data["adv_key"] = adv_key
-            log_data["adv_timestamp"] = adv_timestamp
+                log_data["adv_timestamp"] = adv_timestamp
-            log_data["signature"] = signature
+                log_data["signature"] = signature
-            log_data["adv_flags"] = flags
+                log_data["adv_flags"] = flags
-            log_data["adv_type"] = adv_type
+                log_data["adv_type"] = adv_type
-            if not adv_lat is None :
+                if not adv_lat is None :
-                log_data["adv_lat"] = adv_lat
+                    log_data["adv_lat"] = adv_lat
-            if not adv_lon is None :
+                if not adv_lon is None :
-                log_data["adv_lon"] = adv_lon
+                    log_data["adv_lon"] = adv_lon
-            if not adv_feat1 is None:
+                if not adv_feat1 is None:
-                log_data["adv_feat1"] = adv_feat1
+                    log_data["adv_feat1"] = adv_feat1
-            if not adv_feat2 is None:
+                if not adv_feat2 is None:
-                log_data["adv_feat2"] = adv_feat2
+                    log_data["adv_feat2"] = adv_feat2
+            except (IndexError, ValueError) as e:
+                logger.debug(f"parsePacketPayload: malformed ADVERT payload ({type(e).__name__}: {e}), len={len(pkt_payload)}")
 
         return log_data

tests/unit/test_reader.py

@@ -1,6 +1,7 @@
 #!/usr/bin/env python3
 
 import asyncio
+import logging
 from unittest.mock import AsyncMock
 from meshcore.events import EventType
 from meshcore.reader import MessageReader
@@ -87,4 +88,193 @@ async def test_binary_response():
             print(f"⚠️ Unknown request type {request_type}, no specific event expected")
 
 if __name__ == "__main__":
-    asyncio.run(test_binary_response())
+    asyncio.run(test_binary_response())
+
+
+# ---------------------------------------------------------------------------
+# Reader/parser crash-safety verification tests
+# ---------------------------------------------------------------------------
+
+class _CapturingDispatcher:
+    """Quiet dispatcher that records every dispatched event."""
+    def __init__(self):
+        self.events = []
+
+    async def dispatch(self, event):
+        self.events.append(event)
+
+
+@pytest.mark.asyncio
+async def test_handle_rx_malformed_frame_logged_and_swallowed(caplog):
+    """Malformed frame must not propagate, must be logged with traceback."""
+    dispatcher = _CapturingDispatcher()
+    reader = MessageReader(dispatcher)
+
+    # 4-byte CHANNEL_MSG_RECV_V3 frame: type byte (0x11) + 1 SNR byte +
+    # 2 reserved bytes, but no channel_idx byte. The handler will raise
+    # IndexError on the next dbuf.read(1)[0] when the buffer is empty.
+    # The umbrella try/except must catch it, log the parse error, and
+    # return cleanly.
+    malformed = bytearray.fromhex("11100000")
+
+    with caplog.at_level(logging.ERROR, logger="meshcore"):
+        await reader.handle_rx(malformed)  # must not raise
+
+    error_records = [r for r in caplog.records if "handle_rx parse error" in r.message]
+    assert error_records, (
+        f"Expected an error log containing 'handle_rx parse error'; "
+        f"got: {[r.message for r in caplog.records]}"
+    )
+    # Traceback should be present in the log message
+    assert "Traceback" in error_records[0].message, (
+        "Umbrella log message must include a traceback"
+    )
+    # No CHANNEL_MSG_RECV event should have been dispatched
+    assert not any(e.type == EventType.CHANNEL_MSG_RECV for e in dispatcher.events)
+
+
+@pytest.mark.asyncio
+async def test_battery_short_frame_omits_storage_fields():
+    """Short BATTERY frame must not silently yield zero used_kb/total_kb."""
+    dispatcher = _CapturingDispatcher()
+    reader = MessageReader(dispatcher)
+
+    # 3-byte BATTERY frame: type 0x0c + 2 level bytes (no storage tail).
+    # Pre-fix the `len(data) > 3` gate would have let any frame >= 4 bytes
+    # through, producing a BATTERY event with bogus zero used_kb/total_kb
+    # because io.BytesIO.read() returns short data without raising.
+    # Post-fix (`len(data) >= 11`) the storage fields are skipped entirely.
+    short_battery = bytearray.fromhex("0c8000")
+
+    await reader.handle_rx(short_battery)
+
+    battery_events = [e for e in dispatcher.events if e.type == EventType.BATTERY]
+    assert len(battery_events) == 1, (
+        f"Expected exactly one BATTERY event, got {len(battery_events)}"
+    )
+    payload = battery_events[0].payload
+    assert payload["level"] == 0x0080, f"Unexpected level: {payload['level']}"
+    assert "used_kb" not in payload, (
+        "Short BATTERY frame must not include used_kb (would be a silent zero)"
+    )
+    assert "total_kb" not in payload, (
+        "Short BATTERY frame must not include total_kb (would be a silent zero)"
+    )
+
+
+@pytest.mark.asyncio
+async def test_battery_too_short_for_level(caplog):
+    """BATTERY frame shorter than 3 bytes must be dropped entirely (Option B).
+
+    A 1-byte frame (just the packet-type byte 0x0c, no level bytes) would cause
+    dbuf.read(2) to return b"" and int.from_bytes(b"", ...) to silently yield 0.
+    The fix adds an early return with a debug log.
+    """
+    dispatcher = _CapturingDispatcher()
+    reader = MessageReader(dispatcher)
+
+    # 1-byte BATTERY frame: only the type byte, no level payload.
+    too_short = bytearray.fromhex("0c")
+
+    with caplog.at_level(logging.DEBUG, logger="meshcore"):
+        await reader.handle_rx(too_short)
+
+    battery_events = [e for e in dispatcher.events if e.type == EventType.BATTERY]
+    assert len(battery_events) == 0, (
+        "BATTERY frame shorter than 3 bytes must not dispatch an event"
+    )
+    debug_records = [
+        r for r in caplog.records if "BATTERY frame too short" in r.message
+    ]
+    assert debug_records, "Expected a debug log about the short BATTERY frame"
+
+
+@pytest.mark.asyncio
+async def test_status_response_short_frame_skipped(caplog):
+    """Short STATUS_RESPONSE push frame must be skipped, not parsed with bogus zeros."""
+    dispatcher = _CapturingDispatcher()
+    reader = MessageReader(dispatcher)
+
+    # 30-byte STATUS_RESPONSE push frame, well below the 60-byte minimum.
+    # First byte is the type (0x87 = PacketType.STATUS_RESPONSE), the rest
+    # is arbitrary filler. parse_status with offset=8 reads up through
+    # data[56:60], so anything < 60 bytes would yield short reads and
+    # silent zero values pre-fix.
+    short_status = bytearray([0x87] + [0xAA] * 29)
+    assert len(short_status) == 30
+
+    with caplog.at_level(logging.DEBUG, logger="meshcore"):
+        await reader.handle_rx(short_status)
+
+    status_events = [e for e in dispatcher.events if e.type == EventType.STATUS_RESPONSE]
+    assert len(status_events) == 0, (
+        "Short STATUS_RESPONSE push frame must not dispatch a parsed event"
+    )
+    assert any(
+        "STATUS_RESPONSE push frame too short" in r.message for r in caplog.records
+    ), "Expected a debug log line for short STATUS_RESPONSE frames"
+
+
+@pytest.mark.asyncio
+async def test_parse_packet_payload_txt_type_decodes_high_bits():
+    """txt_type must decode the high 6 bits of byte 4, not always be 0."""
+    from Crypto.Cipher import AES
+    from Crypto.Hash import HMAC, SHA256
+    from meshcore.meshcore_parser import MeshcorePacketParser
+
+    parser = MeshcorePacketParser()
+    parser.decrypt_channels = True
+
+    # Set up a synthetic channel with a known 16-byte AES key. Direct dict
+    # assignment matches how the parser stores channels (newChannel is async
+    # and serves the same purpose).
+    channel_secret = b"\x01" * 16
+    channel_hash_byte = 0xAB
+    parser.channels[0] = {
+        "channel_idx": 0,
+        "channel_name": "test",
+        "channel_hash": "ab",
+        "channel_secret": channel_secret,
+    }
+
+    # 16-byte plaintext (one AES block):
+    #   bytes 0-3 = sender_timestamp (little-endian)
+    #   byte 4    = (txt_type << 2) | attempt
+    #   bytes 5-15 = message + null padding
+    # Pick txt_type=5, attempt=1 → byte 4 = (5 << 2) | 1 = 0x15.
+    # Pre-fix uncrypted[4:4] is empty so txt_type would be 0;
+    # post-fix uncrypted[4:5] yields 0x15 >> 2 = 5.
+    plaintext = b"\x00\x00\x00\x00\x15hello\x00\x00\x00\x00\x00\x00"
+    assert len(plaintext) == 16
+
+    encrypted = AES.new(channel_secret, AES.MODE_ECB).encrypt(plaintext)
+
+    # cipher_mac = first 2 bytes of HMAC-SHA256(channel_secret, encrypted)
+    h = HMAC.new(channel_secret, digestmod=SHA256)
+    h.update(encrypted)
+    cipher_mac = h.digest()[:2]
+
+    # pkt_payload layout: 1-byte chan_hash + 2-byte cipher_mac + ciphertext
+    pkt_payload = bytes([channel_hash_byte]) + cipher_mac + encrypted
+
+    # parsePacketPayload expects the full payload buffer:
+    #   header byte (route_type=1 DIRECT, payload_type=5 channel, ver=0)
+    #   path_byte  (path_len=0, path_hash_size=1) → 0x00
+    #   pkt_payload
+    header = 0x15  # route_type=1, payload_type=5, payload_ver=0
+    path_byte = 0x00
+    payload = bytes([header, path_byte]) + pkt_payload
+
+    log_data = await parser.parsePacketPayload(payload, log_data={})
+
+    assert log_data["payload_type"] == 0x05
+    assert "txt_type" in log_data, (
+        f"txt_type missing from log_data — channel decrypt path was not reached. "
+        f"log_data keys: {list(log_data.keys())}"
+    )
+    assert log_data["txt_type"] == 5, (
+        f"Expected txt_type=5, got {log_data['txt_type']}"
+    )
+    assert log_data["attempt"] == 1, (
+        f"Expected attempt=1, got {log_data['attempt']}"
+    )