Repository: XKNX/xknx
Pull request #1136 (2023-01-07 14:54:27)

SecureRouting: verify MAC before timer handling
<!--
  You are awesome! Thanks for contributing to our project!
  Please, DO NOT DELETE ANY TEXT from this template!.
-->
## Description
<!--
Please include a summary of the change and which issue is fixed.
Please also include relevant motivation and context.
List any dependencies that are required for this change.
-->
SecureRouting: verify MAC before timer handling

- Verify MAC of received TimerNotify frames.
- Verify and handle timer value of received SecureWrapper frames after verification of MAC to ensure authenticity.
- Discard received unencrypted RoutingIndication frames.

Fixes # (issue)

## Type of change
<!--
Please tick the applicable options.
NOTE: Ticking multiple options most likely indicates
that your change is to big and it is suggested to split it into several smaller PRs.
-->

- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
- [ ] This change requires a documentation update

## Checklist:

- [ ] The documentation has been adjusted accordingly
- [x] Tests have been added that prove the fix is effective or that the feature works
- [x] The changes are documented in the changelog (docs/changelog.md)

codecov[bot]: # [Codecov](https://codecov.io/gh/XKNX/xknx/pull/1136?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XKNX) Report
> Merging [#1136](https://codecov.io/gh/XKNX/xknx/pull/1136?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XKNX) (2b0e0a1) into [main](https://codecov.io/gh/XKNX/xknx/commit/28a90437bad7321ed5b9c46835ae4101d485cad0?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XKNX) (28a9043) will **increase** coverage by `0.00%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/XKNX/xknx/pull/1136/graphs/tree.svg?width=650&height=150&src=pr&token=irWbIygS84&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XKNX)](https://codecov.io/gh/XKNX/xknx/pull/1136?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XKNX)

```diff
@@           Coverage Diff           @@
##             main    #1136   +/-   ##
=======================================
  Coverage   96.33%   96.34%           
=======================================
  Files         138      138           
  Lines        8650     8662   +12     
=======================================
+ Hits         8333     8345   +12     
  Misses        317      317           
```


| [Impacted Files](https://codecov.io/gh/XKNX/xknx/pull/1136?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XKNX) | Coverage Δ | |
|---|---|---|
| [xknx/io/ip\_secure.py](https://codecov.io/gh/XKNX/xknx/pull/1136?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XKNX#diff-eGtueC9pby9pcF9zZWN1cmUucHk=) | `95.38% <100.00%> (+0.17%)` | :arrow_up: |


Patch:
diff --git a/docs/changelog.md b/docs/changelog.md
index f571dad4..4e618628 100644
--- a/docs/changelog.md
+++ b/docs/changelog.md
@@ -8,6 +8,12 @@ nav_order: 2
 
 # Unreleased changes
 
+### IP Secure
+
+- SecureRouting: verify MAC of received TimerNotify frames.
+- SecureRouting: verify and handle timer value of received SecureWrapper frames after verification of MAC.
+- SecureRouting: Discard received unencrypted RoutingIndication frames.
+
 ### Internals
 
 - Parse T_Data_Broadcast TPCI. Forward these telegrams to the Management class.
diff --git a/xknx/io/ip_secure.py b/xknx/io/ip_secure.py
index 52ee86fa..f49ae05b 100644
--- a/xknx/io/ip_secure.py
+++ b/xknx/io/ip_secure.py
@@ -21,6 +21,7 @@ from xknx.knxip import (
     HPAI,
     KNXIPFrame,
     KNXIPServiceType,
+    RoutingIndication,
     SecureWrapper,
     SessionResponse,
     SessionStatus,
@@ -421,25 +422,26 @@ class SecureGroup(UDPTransport, _IPSecureTransportLayer):
 
     def handle_knxipframe(self, knxipframe: KNXIPFrame, source: HPAI) -> None:
         """Handle KNXIP Frame and call all callbacks matching the service type ident."""
-        # TODO: disallow unencrypted frames with exceptions for discovery etc. eg. SearchRequest
+        if isinstance(knxipframe.body, RoutingIndication):
+            ip_secure_logger.info(
+                "Discarding received unencrypted RoutingIndication: %s",
+                knxipframe,
+            )
+            return
         if isinstance(knxipframe.body, TimerNotify):
             self.secure_timer.handle_timer_notify(knxipframe.body)
             return
         if isinstance(knxipframe.body, SecureWrapper):
-            if not self.secure_timer.validate_secure_wrapper(knxipframe.body):
-                ip_secure_logger.warning(
-                    "Discarding SecureWrapper with invalid timer value: %s",
-                    knxipframe,
-                )
-                return
             if not self.secure_timer.timer_authenticated:
                 ip_secure_logger.debug(
                     "Discarding received SecureWrapper before timer synchronisazion finished: %s",
                     knxipframe,
                 )
                 return
+            secure_wrapper = knxipframe.body
             try:
                 knxipframe = self.decrypt_frame(knxipframe)
+                print(secure_wrapper)
             except KNXSecureValidationError as err:
                 ip_secure_logger.warning("Could not decrypt KNXIPFrame: %s", err)
                 # Frame shall be discarded
@@ -450,7 +452,14 @@ class SecureGroup(UDPTransport, _IPSecureTransportLayer):
                     couldnotparseknxip.description,
                 )
                 return
+            if not self.secure_timer.validate_secure_wrapper(secure_wrapper):
+                ip_secure_logger.warning(
+                    "Discarding SecureWrapper with invalid timer value: %s",
+                    secure_wrapper,
+                )
+                return
             knx_logger.debug("Decrypted frame: %s", knxipframe)
+        # handle decrypted frames and plain frames (e.g. SearchRequest for discovery)
         super().handle_knxipframe(knxipframe, source)
 
     def send(self, knxipframe: KNXIPFrame, addr: tuple[str, int] | None = None) -> None:
@@ -477,6 +486,8 @@ class SecureSequenceTimer:
     According to AN159 v06 KNXnet-IP Secure AS §2.2.2.3 Timer synchronizing
     """
 
+    TIMER_NOTIFY_HEADER = bytes.fromhex("06 10 09 55 00 24")
+
     min_delay_time_keeper_periodic_notify: Final = 10  # pylint: disable=invalid-name
     min_delay_time_keeper_update_notify: Final = 0.1  # pylint: disable=invalid-name
     sync_latency_fraction: int = 10  # 10%
@@ -605,10 +616,9 @@ class SecureSequenceTimer:
         b_0 = timer_bytes + serial_number + _message_tag + b"\x00\x00"
         # TODO: get header data and total_length from TimerNotify class
         #       or handle mac calculation in TimerNotify class directly
-        timer_notify_header = bytes.fromhex("06 10 09 55 00 24")
         mac_cbc = calculate_message_authentication_code_cbc(
             key=self._backbone_key,
-            additional_data=timer_notify_header,
+            additional_data=self.TIMER_NOTIFY_HEADER,
             block_0=b_0,
         )
         c_0 = timer_bytes + serial_number + _message_tag + b"\xff\x00"
@@ -629,6 +639,34 @@ class SecureSequenceTimer:
             None,
         )
 
+    def verify_timer_notify_mac(self, timer_notify: TimerNotify) -> None:
+        """Verify MAC of timer notify."""
+        timer_bytes = timer_notify.timer_value.to_bytes(6, "big")
+        b_0 = (
+            timer_bytes
+            + timer_notify.serial_number
+            + timer_notify.message_tag
+            + b"\x00\x00"
+        )
+        c_0 = (
+            timer_bytes
+            + timer_notify.serial_number
+            + timer_notify.message_tag
+            + b"\xff\x00"
+        )
+        _, mac_tr = decrypt_ctr(
+            key=self._backbone_key,
+            counter_0=c_0,
+            mac=timer_notify.message_authentication_code,
+        )
+        mac_cbc = calculate_message_authentication_code_cbc(
+            key=self._backbone_key,
+            additional_data=self.TIMER_NOTIFY_HEADER,
+            block_0=b_0,
+        )
+        if mac_cbc != mac_tr:
+            raise KNXSecureValidationError("MAC verification failed")
+
     async def synchronize(self) -> None:
         """Synchronize timer with remote time keeper."""
         message_tag = random.randbytes(2)
@@ -685,6 +723,13 @@ class SecureSequenceTimer:
     def handle_timer_notify(self, timer_notify: TimerNotify) -> None:
         """Handle received TimerNotify frame."""
         local_timer_value = self.current_timer_value()
+        try:
+            self.verify_timer_notify_mac(timer_notify)
+        except KNXSecureValidationError:
+            ip_secure_logger.warning(
+                "Discarding TimerNotify with invalid MAC: %s", timer_notify
+            )
+            return
         received_timer_value = timer_notify.timer_value
         # §2.2.2.3.2.5 Events: E11
         if (
@@ -693,7 +738,7 @@ class SecureSequenceTimer:
             and timer_notify.message_tag == self._expected_notify_handler[0]
         ):
             fut = self._expected_notify_handler[1]
-            fut.set_result(timer_notify.timer_value)
+            fut.set_result(received_timer_value)
             return
         # §2.2.2.3.2.5 Events: E1 - E4
         if received_timer_value > local_timer_value:


Test patch:
diff --git a/test/io_tests/routing_test.py b/test/io_tests/routing_test.py
index 9d5e218a..17a80fe0 100644
--- a/test/io_tests/routing_test.py
+++ b/test/io_tests/routing_test.py
@@ -31,7 +31,6 @@ class TestRouting:
         routing = Routing(
             self.xknx,
             individual_address=None,
-            # telegram_received_callback=self.tg_received_mock,
             telegram_received_callback=self.xknx.knxip_interface.telegram_received,
             local_ip="192.168.1.1",
         )
diff --git a/test/io_tests/secure_group_test.py b/test/io_tests/secure_group_test.py
index 6fdb3dd1..58f5d7bd 100644
--- a/test/io_tests/secure_group_test.py
+++ b/test/io_tests/secure_group_test.py
@@ -1,10 +1,10 @@
 """Test Secure Group."""
 import asyncio
-from unittest.mock import patch
+from unittest.mock import Mock, patch
 
 from xknx.io.const import DEFAULT_MCAST_GRP, DEFAULT_MCAST_PORT, XKNX_SERIAL_NUMBER
 from xknx.io.ip_secure import SecureGroup
-from xknx.knxip import HPAI, KNXIPFrame, SecureWrapper, TimerNotify
+from xknx.knxip import HPAI, KNXIPFrame, RoutingIndication, SecureWrapper, TimerNotify
 from xknx.knxip.cemi_frame import CEMIFrame
 from xknx.knxip.routing_indication import RoutingIndication
 from xknx.telegram import GroupAddress, Telegram, apci
@@ -88,6 +88,9 @@ class TestSecureGroup:
                 timer_value=0,
                 serial_number=bytes.fromhex("00 fa 12 34 56 78"),
                 message_tag=bytes.fromhex("12 34"),
+                message_authentication_code=bytes.fromhex(
+                    "3195051bb981941d57e6c5b55355f341"
+                ),
             )
         )
         secure_group.handle_knxipframe(timer_invalid, HPAI(*self.mock_addr))
@@ -135,7 +138,9 @@ class TestSecureGroup:
                 message_tag=_message_tag,
             )
         )
-        secure_group.handle_knxipframe(timer_update, HPAI(*self.mock_addr))
+        with patch("xknx.io.ip_secure.SecureSequenceTimer.verify_timer_notify_mac"):
+            # TimerNotify MAC is random so we don't verify it in tests
+            secure_group.handle_knxipframe(timer_update, HPAI(*self.mock_addr))
         await time_travel(0)
         _leeway_for_ci = 50  # ms
         assert (
@@ -180,12 +185,25 @@ class TestSecureGroup:
         with patch.object(
             secure_timer, "reschedule", wraps=secure_timer.reschedule
         ) as mock_reschedule:
+            # TimerNotify with invalid MAC shall be discarded
+            timer_invalid_mac = KNXIPFrame.init_from_body(
+                TimerNotify(
+                    timer_value=ONE_HOUR_MS,
+                    serial_number=bytes.fromhex("00 fa 12 34 56 78"),
+                    message_tag=bytes.fromhex("12 34"),
+                )
+            )
+            secure_group.handle_knxipframe(timer_invalid_mac, HPAI(*self.mock_addr))
+            mock_reschedule.assert_not_called()
             # E1
             timer_newer = KNXIPFrame.init_from_body(
                 TimerNotify(
                     timer_value=ONE_HOUR_MS,
                     serial_number=bytes.fromhex("00 fa 12 34 56 78"),
                     message_tag=bytes.fromhex("12 34"),
+                    message_authentication_code=bytes.fromhex(
+                        "d2fad5657a5788a36cdd8a3ef84c90ab"
+                    ),
                 )
             )
             secure_group.handle_knxipframe(timer_newer, HPAI(*self.mock_addr))
@@ -200,6 +218,9 @@ class TestSecureGroup:
                     timer_value=ONE_HOUR_MS,
                     serial_number=bytes.fromhex("00 fa 12 34 56 78"),
                     message_tag=bytes.fromhex("12 34"),
+                    message_authentication_code=bytes.fromhex(
+                        "d2fad5657a5788a36cdd8a3ef84c90ab"
+                    ),
                 )
             )
             secure_group.handle_knxipframe(timer_exact, HPAI(*self.mock_addr))
@@ -216,6 +237,9 @@ class TestSecureGroup:
                     + 1,
                     serial_number=bytes.fromhex("00 fa 12 34 56 78"),
                     message_tag=bytes.fromhex("12 34"),
+                    message_authentication_code=bytes.fromhex(
+                        "7572b70b4f986d7ae68891a00c0d46d3"
+                    ),
                 )
             )
             secure_group.handle_knxipframe(timer_valid, HPAI(*self.mock_addr))
@@ -230,6 +254,9 @@ class TestSecureGroup:
                     timer_value=ONE_HOUR_MS - secure_timer.sync_latency_tolerance_ms,
                     serial_number=bytes.fromhex("00 fa 12 34 56 78"),
                     message_tag=bytes.fromhex("12 34"),
+                    message_authentication_code=bytes.fromhex(
+                        "3d70cc44607ad9b4a4425de95101a54c"
+                    ),
                 )
             )
             secure_group.handle_knxipframe(timer_tolerable_1, HPAI(*self.mock_addr))
@@ -240,6 +267,9 @@ class TestSecureGroup:
                     timer_value=ONE_HOUR_MS - secure_timer.latency_tolerance_ms + 1,
                     serial_number=bytes.fromhex("00 fa 12 34 56 78"),
                     message_tag=bytes.fromhex("12 34"),
+                    message_authentication_code=bytes.fromhex(
+                        "66b8b2e52196bcce3dce2da7e0dc50ec"
+                    ),
                 )
             )
             secure_group.handle_knxipframe(timer_tolerable_2, HPAI(*self.mock_addr))
@@ -250,6 +280,9 @@ class TestSecureGroup:
                     timer_value=ONE_HOUR_MS - secure_timer.latency_tolerance_ms,
                     serial_number=bytes.fromhex("00 fa 12 34 56 78"),
                     message_tag=bytes.fromhex("12 34"),
+                    message_authentication_code=bytes.fromhex(
+                        "873d9a5a25b8508446bbd5ff1bb4b465"
+                    ),
                 )
             )
             secure_group.handle_knxipframe(timer_invalid, HPAI(*self.mock_addr))
@@ -434,3 +467,33 @@ class TestSecureGroup:
                 KNXIPFrame.init_from_body(RoutingIndication(raw_cemi=raw_test_cemi))
             )
             mock_reschedule.assert_not_called()
+
+    async def test_receive_plain_frames(
+        self,
+        mock_super_send,
+        mock_super_connect,
+    ):
+        """Test class for KNXnet/IP secure routing."""
+        frame_received_mock = Mock()
+        secure_group = SecureGroup(
+            local_addr=self.mock_addr,
+            remote_addr=(DEFAULT_MCAST_GRP, DEFAULT_MCAST_PORT),
+            backbone_key=self.mock_backbone_key,
+            latency_ms=1000,
+        )
+        secure_group.register_callback(frame_received_mock)
+        plain_routing_indication = bytes.fromhex("0610 0530 0010 2900b06010fa10ff0080")
+        secure_group.data_received_callback(
+            plain_routing_indication, ("192.168.1.2", 3671)
+        )
+        frame_received_mock.assert_not_called()
+        plain_search_response = bytes.fromhex(
+            "0610020c006608010a0100280e57360102001000000000082d40834de000170c"
+            "000ab3274a3247697261204b4e582f49502d526f757465720000000000000000"
+            "000000000e02020203020402050207010901140700dc10f1fffe10f2ffff10f3"
+            "ffff10f4ffff"
+        )
+        secure_group.data_received_callback(
+            plain_search_response, ("192.168.1.2", 3671)
+        )
+        frame_received_mock.assert_called_once()