Skip to content

[knx] Allow decoding of KNX Data Secure frames#12434

Merged
lsiepel merged 4 commits into
openhab:mainfrom
holgerfriedrich:pr-knx-ip-secure
Sep 15, 2024
Merged

[knx] Allow decoding of KNX Data Secure frames#12434
lsiepel merged 4 commits into
openhab:mainfrom
holgerfriedrich:pr-knx-ip-secure

Conversation

@holgerfriedrich

@holgerfriedrich holgerfriedrich commented Mar 6, 2022

Copy link
Copy Markdown
Member
  • add passive (listening only) access for KNX Data Secure frames, [knx] support for KNX secure #8872
  • add config options for KNX keyring file and password
  • ease setup if IP Secure, as required parameters can be read from keyring
  • add tests for security functions
  • update user documentation

Signed-off-by: Holger Friedrich mail@holger-friedrich.de

@holgerfriedrich

holgerfriedrich commented Mar 6, 2022

Copy link
Copy Markdown
Member Author

This PR brings support to KNX IP secure, and partial support for KNX data secure (listening only). See previous discussions in #8872.

What has been tested:

  • Secure tunneling, seems to work fine for a few days

What is to be tested by someone else:

  • Secure routing (don't have a secure router at hand)

@holgerfriedrich holgerfriedrich force-pushed the pr-knx-ip-secure branch 2 times, most recently from 2e2d5d3 to ca6de24 Compare March 6, 2022 16:33
@holgerfriedrich

Copy link
Copy Markdown
Member Author

@kaikreuzer I followed your advise and have only included the passive support for data secure. Outgoing writes and poll requests for secure group adresses are dropped at lowest level.

Though, when I change a switch connected to a secure GA, the event log still shows the "Item ... predicted to become ..." (which of course will not happen as I drop the write). Is there a clean way to signal a failed write to the upper layer? Throwing an exception is probably too much, as it gets logged with a full stack trace.

@wborn wborn added the enhancement An enhancement or new feature for an existing add-on label Mar 6, 2022
<default>false</default>
<advanced>true</advanced>
</parameter>
<parameter name="keyringFile" type="text" groupName="knxsecure">

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it make sense to add that directly to the JSON database (i.e. add the file content directly here instead of a filename).

@holgerfriedrich holgerfriedrich Mar 7, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@J-N-K thanks for looking into this. I am not sure. At least there are a few practical reasons to keep it as a separate file:

  • keyring files need to be replaced everytime you add devices or secure group adresses, this is much easier if you can copy it over using scp
  • Calimero Keyring expects an URI for a file location, and does not provide other constructors besides Keyring::load()
  • Keyring files contain a signature, which might be invalid if we use a copy-paste approach here....

@wborn wborn added the work in progress A PR that is not yet ready to be merged label Mar 9, 2022
@holgerfriedrich holgerfriedrich changed the title [knx] add initial support for KNX secure [WIP] [knx] add initial support for KNX secure Mar 11, 2022
@openhab-bot

Copy link
Copy Markdown
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/knx-secure-initial-implementation/134133/1

@holgerfriedrich holgerfriedrich force-pushed the pr-knx-ip-secure branch 2 times, most recently from 2553802 to 021cd88 Compare March 14, 2022 23:40
@holgerfriedrich holgerfriedrich force-pushed the pr-knx-ip-secure branch 7 times, most recently from 84bc872 to 14daa8d Compare April 9, 2022 19:42
@holgerfriedrich

holgerfriedrich commented Apr 12, 2022

Copy link
Copy Markdown
Member Author

@slueder
During the last days, I tried to factor out everything which was not related to the feature itself. Give me a few days until I have a "release candidate" to test.

Not sure about the problem with Keyring files you mentioned in #8872. I have added several keyrings to the tests in this PR, all working fine. Spaces may be a problem, the code uses trim() to strip whitespace (maybe I will remove that).
Overall, for the first merge it might be way to remove the whole keyring stuff and go for the password parameters. If we cannot find out why keyrings are not working for you, this will be the way. Though, for me the keyring is working fine and gives the additional benefit that openHAB can listen to and decode data secure GAs as well....

Which branch did you use for your testing?

@slueder

slueder commented Apr 14, 2022

Copy link
Copy Markdown

@holgerfriedrich

I retested the keyring approach with the latest version of ETS (6.0.4) and reasonably strong keyring export passwords (8 characters, consisting of upper & lower letters, digits and a limited set of special chars :#,;) and it worked now.

I am using commit ID holgerfriedrich@c7114f5

Happy to re-test as soon as you let me know.

@holgerfriedrich holgerfriedrich force-pushed the pr-knx-ip-secure branch 2 times, most recently from baa44eb to 9dbea72 Compare May 25, 2024 05:15
@florian-h05

Copy link
Copy Markdown
Contributor

@holgerfriedrich Can we start with the review process here?

I understand your security concerns wrt to putting the keyring password in cleartext into the config, but IMO this is no major security problem. I would consider the openHAB server as a secure environment (use reasonable strong SSH password or even better public key auth), and in that case we have two layers of security that have to be breached to get access to the keyring: One needs to get the password for the keyring, either via REST API or via filesystem, and one has to access the keyring itself via filesystem. Only attacking REST API therefore wouldn‘t breach the security, and access to the filesystem can be secured very well (no SMB, FTP, NFS, only SSHFS and strong SSH security).
There is a remaining risk but the added security compared to unsecured KNX outweighs that risk, hence openHAB should implement KNX Data Secure even if not a „perfect“ implementation.

@holgerfriedrich

Copy link
Copy Markdown
Member Author

@florian-h05 agreed, let me rebase and go over the docs again. Then we can start the review.

@lsiepel

lsiepel commented Aug 18, 2024

Copy link
Copy Markdown
Contributor

I would consider the openHAB server as a secure environment (use reasonable strong SSH password or even better public key auth), and in that case we have two layers of security that have to be breached to get access to the keyring: One needs to get the password for the keyring, either via

How bad would it if some unauthorized individual got access to this keyring?
Does it only store the KNX secret?

@florian-h05

florian-h05 commented Aug 18, 2024

Copy link
Copy Markdown
Contributor

The keyring together with the password would allow an attacker who has physical access to the KNX bus or the network where the IP interface is located, to gain full access to the bus.

Current situation is that you cannot use KNX data secure at all, so if you had the physical or network access to KNX at the moment, you could directly attack it. With data secure support in openHAB and data secure enabled, there is the additional step of stealing the keyring.

I have already had a conversation with Holger and the best way would be to be a KNX member as openHAB and register openHAB as a bus member via the ETS software, because this would allow us to get data secure keys that are only valid for a limited set of group addresses (simply said: parts of the bus) and not everything; but this would be a huge effort.

For a successful attack, you would hence either need to break into the building and get access to the KNX wire or in case of KNX RF be in range of the radio frequency, or have access to the network where the IP interface is. A obviously doesn’t allow remote attacks, and B usually requires to breach the firewall of the internet router or possibly additional firewalls.

If I were to attack such a system, I would rather use openHAB to send commands to KNX instead of stealing the keyring — because to steal the keyring, I need access to the server which I would then use to attack openHAB directly.

TLDR: It would allow an attacker to have full access to the bus IF he has physical (wired or radio frequency) or network-based (breach a firewall, create a VPN tunnel …) access.
This keyring only stores keys required for de- and encrypting secured KNX telegrams.

@lsiepel

lsiepel commented Aug 18, 2024

Copy link
Copy Markdown
Contributor

TLDR: It would allow an attacker to have full access to the bus IF he has physical (wired or radio frequency) or network-based (breach a firewall, create a VPN tunnel …) access. This keyring only stores keys required for de- and encrypting secured KNX telegrams.

Thanks for the extensive outline. My concerns are not toward the KNX bus, adding this security is an imrovement eitherway, totally agree. You confirmed the thoughts i had.
My concern are that this keyring would also be a store to other secrets. But as you confirmed it is exclusive to KNX, i'm fine and let's move forward.

@florian-h05

Copy link
Copy Markdown
Contributor

Just FYI you program KNX using a program called ETS and the keyring required here is exported from the ETS, it’s the same keyring that allows the ETS full access to the KNX bus.

@lsiepel

lsiepel commented Aug 20, 2024

Copy link
Copy Markdown
Contributor

@holgerfriedrich did this PR get some test mileage on different installations ?

@holgerfriedrich

Copy link
Copy Markdown
Member Author

@lsiepel unfortunately not yet, at least not after the rebase and latest modifications.

I typically deploy to RPI, and I am using Enertex IP interface (connected via IP, with IP secure), Weinzierl 730 (IP w/o IP secure), or Weinzierl kBerry (RPI hat, using serial connection).

Give me some time to double check that data secure (decoding incoming encrypted frames) still works as expected.....

Comment thread bundles/org.openhab.binding.knx/README.md Outdated
Comment thread bundles/org.openhab.binding.knx/src/main/resources/OH-INF/thing/ip.xml Outdated

@lsiepel lsiepel left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left two comments. LGTM, but ift would also be nice of @J-N-K and @florian-h05 to confirm.

@florian-h05

Copy link
Copy Markdown
Contributor

I am currently busy till beginning of October, but then should be able to review.

@lsiepel lsiepel left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, let's wait for @florian-h05's review before merge.

* add passive (listening only) access for KNX Data Secure frames, openhab#8872
* add config options for KNX keyring file and password
* ease setup if IP Secure, as required parameters can be read from keyring
* add tests for security functions
* update user documentation

Signed-off-by: Holger Friedrich <mail@holger-friedrich.de>
Signed-off-by: Holger Friedrich <mail@holger-friedrich.de>
@holgerfriedrich holgerfriedrich added the rebuild Triggers Jenkins PR build label Sep 15, 2024
@holgerfriedrich

Copy link
Copy Markdown
Member Author

Accidentally pushed to this PR when working on a followup PR, rolled back, rebased to current main as a force push was anyway necessary.
Sorry for the noise.

Signed-off-by: Holger Friedrich <mail@holger-friedrich.de>

@florian-h05 florian-h05 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM, thanks! Just one minor comment:

I would have loved to give it a try, but I don't have KNX Data nor IP Secure devices.
I have of course tested it against my system - everything still works as expected.

Signed-off-by: Holger Friedrich <mail@holger-friedrich.de>
@florian-h05

Copy link
Copy Markdown
Contributor

@lsiepel You can merge now 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement An enhancement or new feature for an existing add-on

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants