Skip to content

Add security concerns section #6

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
methylDragon wants to merge 1 commit into
base: evolving_message_types_rep
Choose a base branch
from
+16 −0
Open

Add security concerns section #6

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions rep-2011.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -877,6 +877,22 @@ Similar to the integration with launch, you could run ``ros2 bag play ...`` and

That option would handle the conversion on the fly using the same mechanisms as the command line tool.

Security Concerns
Copy link

@vmayoral vmayoral Sep 9, 2022

Choose a reason for hiding this comment

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

Suggested change
Security Concerns
Security considerations

I like concerns, don't get me wrong, it shows and remarks the relevance of the topic, but it might be a bit intimidating :).

=================

The additional features this REP proposes do not impose significantly increased security concerns, since the message conversions happen over topics and services.
Copy link

@vmayoral vmayoral Sep 9, 2022

Choose a reason for hiding this comment

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

I like where this is going but I would suggest rephrasing this a bit and avoiding strong claims. After all, we don't know if an unintended bug in the implementation of these features (or any of its dependencies, if any) would actually impose a significant security concern. In my experience security is a process, strong claims do not generally hold for long.

Here's a suggestion:

Suggested change
The additional features this REP proposes do not impose significantly increased security concerns, since the message conversions happen over topics and services.
The additional features this REP proposes happen over topics and services, as any ROS 2 application would. Correspondingly, the use of evolving message types shouldn't add significant additional complexity to security thread models however new ROS 2 abstractions will be created when using it, and these would need to be secured appropriately. To that end, this REP makes the following security considerations:


More precisely, transfer functions would just be another node that interfaces with topics and services to do the message conversions, and hence would be secured by the same security mechanisms that secure nodes, topics, and services in general.
Furthermore, transfer functions would have to be defined ahead of time (local to the machine running the transfer function), and cycles in the transfer function chain would throw assertions, making it less feasible to use the transfer function feature as an attack vector.
Copy link

@vmayoral vmayoral Sep 9, 2022

Choose a reason for hiding this comment

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

This confused me. Can you further elaborate on this consideration? Will transfer functions always be local to the machine running the transfer function and defined ahead of time? Wouldn't we be loosing evolving capabilities if this is case?

The transfer functions would also generally need to be opted into by the user (e.g. in a launch file or otherwise explicitly invoked), further reducing the attack surface.

Likewise, for the version hashing component, the hash gets appended to the type name that is sent via discovery.
This means that the same security mechanisms that secure topic type names sent on discovery will secure the version hash, so there is no increase in attack surface.
Copy link

@vmayoral vmayoral Sep 9, 2022

Choose a reason for hiding this comment

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

I believe this statement is not true. As far as I understood from the discussion, hashes will be sent as part of discovery and thereby without security, so there's an increase in the attack surface.

This increase could or could not be leveraged by an attacker, but technically, there's indeed an increase in the exposure.


Of course, it could be argued that since the version hash is a hashed version of some parsed version of the type description, there is an increase in the kinds of information that an attacker would gain access to if the hash was crackable/reversible, since information about the message's structure would then be obtained.
Copy link

@vmayoral vmayoral Sep 9, 2022

Choose a reason for hiding this comment

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

This then contradicts the previous statement. So i'd suggest merging both and reasoning about why this does not imply a high risk (which is essentially argued below).

However, an attacker would first have to gain access to the hash by bypassing the mechanisms securing the type names, and then find a way to reverse a hash that should not be easily reversible, since the input's length and variety would make it resistant to rainbow table attacks or other brute forcing methods.
Furthermore, using hash collisions to trick a transfer function into thinking that two types are compatible would not be a useful attack vector, since if such a type got through to the transfer function, it would just cause the transfer function to fail to do the conversion, and to even get to such a point, an attacker would have to gain access to the network to send a message to the transfer function in the first place.

Rationale
=========

Expand Down