lcm-proj / lcm

Lightweight Communications and Marshalling
GNU Lesser General Public License v2.1
944 stars 385 forks source link

Proposal: Security functionalities for LCM #467

Open mojasp opened 10 months ago

mojasp commented 10 months ago

Motivation

LCM does not provide any security features. Tunneling LCM over a secure protocol (when using the UDP-Multicast provider) is difficult due to the - mostly - multicast-based nature of LCM. Therefore, i expect most of the users of this library to use LCM in some sort of isolated network. This however violates against the zero trust principle: "The entire enterprise private network is not considered an implicit trust zone" [1]

Solution

We have developed and implemented a set of security features of LCM. A paper has been submitted for peer review and is available here. The mentioned security features have been implemented in a fork called LCMsec.

The goals of LCMsec are as follows:

Non-goals are:

Proposal

Merge LCMsec into the LCM library as an optional, secured version of the protocol.

If there is indeed interest, i would be happy to work towards a merge. In that case, would one of the maintainers be willing to coordinate with me and make a plan as to what is / is not required to merge?

[1] https://nvlpubs.nist.gov/nistpubs/SpecialPublications/NIST.SP.800-207.pdf

ihilt commented 10 months ago

Thank you for this proposal! This does indeed look rather interesting.

From a maintainer perspective, I have a few thoughts:

Regarding your question to users of LCM, you may want to post to the Google group https://groups.google.com/g/lcm-dev or https://groups.google.com/g/lcm-users to sample the broader community's interest.

I think that's it from my point of view. I appreciate your interest in contributing to LCM!

mojasp commented 10 months ago

I appreciate your interest :) If you are open to move forward with this, provided your comments are met, i will do some cleanup to reduce intrusion etc.

As you mentioned in the proposal, LCMsec should be an opt-in configuration such that users who aren't interested in the security features aren't forced to do anything different after updating than what they are doing currently, e.g. install more dependencies, add CMake options to disable features, etc.

It is not right now, but it should not be a problem to achieve this.

It should be as isolated as possible so updates to LCMsec have minimal impact on the rest of the code base. Obviously this is a judgment call so we could work that out if this moves forward.

Most of the code is indeed seperate, and in a seperate folder. However, i will give you an overview of places where there is some intrusion:

I hope i did not miss anything, but if there are other changes, i think they are either very small or unnesscary and could be cleaned up.

Similar to bullet 1, the existing method of connecting nodes and the LCM message format isn't modified in the non-secure case so existing users aren't required to update all of their nodes to the LCMsec version.

This is the case - a seperate header and Magic Number is used for the secure case. Since i intercept the udp packets, the message format itself is not touched.

I would be happy to hear your thoughts about this - especially about the current level of intermixing between LCM and LCMsec.

ihilt commented 10 months ago

I appreciate your interest :) If you are open to move forward with this, provided your comments are met, i will do some cleanup to reduce intrusion etc. It is not right now, but it should not be a problem to achieve this.

Excellent!

Most of the code is indeed seperate, and in a seperate folder. However, i will give you an overview of places where there is some intrusion:

  • There are changes in lcm.h, and lcm.c, to provide an API for lcmsec. However, the API (and ABI, i think) for the unsecured version does not change.

Yes, taking a look at the changes, this appears to be the case.

  • For the C++ interface, The API does not change. I think that the ABI is fine as well, since LCM class methods are thankfully inlined.

Agreed.

  • The create function in _lcm_provider_vtable_tis changed to accommodate the new API. Unfortunately i needed a way to pass the user-provided security parameters to the udpm provider - There are quite a few parameters, in my opinion it would be hacky to use the url for this purpose... This means that the files for all other providers are touched as well (to include the additional parameter).

This should also work since that struct is accessed as an opaque object from users of the LCM library.

  • There are additional headers used (long and short) for the secured version (changes to udpm_util.h) changes to lcm_udpm.c, since i "intercept" packages on the socket layer to do encryption and decryption. The secured versions of lcm_udpm_send and recv* could be moved to a different file if desired, but a few changes remain.

That might be a good idea, though, I might need to see a version with the changes you've mentioned thus far. Overall, this all sounds good.