Repository: XKNX/xknx
Pull request #943 (2022-05-03 19:59:10)

Fix MAC calculation for 22-byte long secure payloads
<!--
  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.
-->
The calculation of b0 blocks was wrong if the data length was already a multiple of 16. This only shows on KNXIPFrames with a length of 22 bytes - which is eg. a TunnelingRequest with a 1 byte payload.

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:

- [x] I have performed a self-review of my own code
- [ ] The documentation has been adjusted accordingly
- [x] The changes generate no new warnings
- [x] Tests have been added that prove the fix is effective or that the feature works
- [x] The changes are documented in the changelog
- [ ] The Homeassistant plugin has been adjusted in case of new config options


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

[![Impacted file tree graph](https://codecov.io/gh/XKNX/xknx/pull/943/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/943?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XKNX)

```diff
@@           Coverage Diff           @@
##             main     #943   +/-   ##
=======================================
  Coverage   95.96%   95.96%           
=======================================
  Files         130      130           
  Lines        7777     7778    +1     
=======================================
+ Hits         7463     7464    +1     
  Misses        314      314           
```


| [Impacted Files](https://codecov.io/gh/XKNX/xknx/pull/943?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XKNX) | Coverage Δ | |
|---|---|---|
| [xknx/secure/util.py](https://codecov.io/gh/XKNX/xknx/pull/943/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XKNX#diff-eGtueC9zZWN1cmUvdXRpbC5weQ==) | `100.00% <100.00%> (ø)` | |


Patch:
diff --git a/changelog.md b/changelog.md
index df08a4ce..9f6966b1 100644
--- a/changelog.md
+++ b/changelog.md
@@ -2,6 +2,10 @@
 
 ## Unreleased changes
 
+### Bugfixes
+
+- IP Secure: Fix MAC calculation for 22-byte payloads
+
 ### Internals
 
 - Rename TaskRegistry.register and Task `task` attribute to `async_func` to avoid confusion; return Task from `start()`
diff --git a/xknx/secure/util.py b/xknx/secure/util.py
index 38eb34a1..20fc9076 100644
--- a/xknx/secure/util.py
+++ b/xknx/secure/util.py
@@ -14,13 +14,10 @@ def bytes_xor(a: bytes, b: bytes) -> bytes:  # pylint: disable=invalid-name
 
 
 def byte_pad(data: bytes, block_size: int) -> bytes:
-    """
-    Pad data with 0x00 until its length is a multiple of block_size.
-
-    Add a whole block of 0 if data lenght is already a multiple of block_size.
-    """
-    padding = bytes(block_size - (len(data) % block_size))
-    return data + padding
+    """Pad data with 0x00 until its length is a multiple of block_size."""
+    if remainder := len(data) % block_size:
+        return data + bytes(block_size - remainder)
+    return data
 
 
 def sha256_hash(data: bytes) -> bytes:


Test patch:
diff --git a/test/secure_tests/util_test.py b/test/secure_tests/util_test.py
index fec23d94..ebf66f54 100644
--- a/test/secure_tests/util_test.py
+++ b/test/secure_tests/util_test.py
@@ -52,13 +52,17 @@ def test_byte_xor_error():
         (
             4,
             bytes([1, 23, 0, 0]),
-            bytes([1, 23, 0, 0, 0, 0, 0, 0]),
+            bytes([1, 23, 0, 0]),
         ),
         (
             16,
             bytes([123]),
             bytes([123, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]),
         ),
+        (16, bytes(16), bytes(16)),
+        (16, bytes(17), bytes(32)),
+        (16, bytes(32), bytes(32)),
+        (16, bytes(47), bytes(48)),
     ],
 )
 def test_byte_pad(block_size, data, result):