Skip to content
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.

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

Set rtps_protection_kind to ENCRYPT by default #171

Open
wants to merge 1 commit into
base: rolling
Choose a base branch
from

Conversation

vmayoral
Copy link
Member

@vmayoral vmayoral commented Dec 6, 2019

Currently, sros2 defaults leak node-related information from which an attacker can obtain additional information about the ROS graph.

This issue was discussed during the Security workshop at ROSCon 2019 (credit and thanks to @mikaelarguedas for pointing out the fix). In addition and for compleness, the flaw has been captured at the following ticket within the Robot Vulnerability Database (RVD): aliasrobotics/RVD#922

A reproduction of this bug with the latest ROS 2 (master, foxy) packages is illustrated in https://asciinema.org/a/SSnSAMlOEoHfqhAuzC1R98STF. The same issue has been confirmed for dashing and eloquent.

Signed-off-by: Víctor Mayoral Vilches <v.mayoralv@gmail.com>
@mikaelarguedas
Copy link
Member

Thanks @vmayoral for the follow-up. I tried the fix right after ROSCon and think the leaking was still happening :s so I held on PRing it.

Did you have a different experience in your testing ?

@vmayoral
Copy link
Member Author

vmayoral commented Dec 6, 2019

@mikaelarguedas I believe the issue here is bigger than this simple fix. Refer to #172 for some further testing on our side and discussion.

In case it helps, we're using internally the following configuration file to reproduce the flaw:

sros2 leak reproduction file
networks:
  - network:
    - driver: overlay
    - name: net1
    - encryption: false

containers:
  - container:
    - name: subject1
    - modules:
         - base: registry.gitlab.com/aliasrobotics/offensive/alurity/ros2/ros2:latest
         - network: net1
  - container:
    - name: subject2
    - modules:
         - base: registry.gitlab.com/aliasrobotics/offensive/alurity/ros2/ros2:latest
         - volume: registry.gitlab.com/aliasrobotics/offensive/alurity/deve_atom
         - network: net1
  - container:
    - name: attacker
    - modules:
         - base: registry.gitlab.com/aliasrobotics/offensive/alurity/ros2/ros2:latest
         - volume: registry.gitlab.com/aliasrobotics/offensive/alurity/reco_aztarna
         - network: net1

flow:
  - container:
    - name: subject1
    - window:
        - name: unsecure
        - commands:
          - command: "source /opt/ros2_ws/install/setup.bash"
          - command: "export ROS_DOMAIN_ID=0"
          - command: "ros2 run demo_nodes_cpp talker"
          - split: horizontal
          - command: "source /opt/ros2_ws/install/setup.bash"
          - command: "export ROS_DOMAIN_ID=0"
          - command: "env | grep ROS"  # this shows there's no security enabled at this point          
    - select: unsecure
  - container:
    - name: subject2
    - window:
        - name: secure
        - commands:
          - command: "source /opt/ros2_ws/install/setup.bash"
          - command: "export ROS_DOMAIN_ID=1"
          - command: "env | grep ROS"  # this shows there's no security enabled at this point
          - command: "ros2 run demo_nodes_cpp talker"
          - command: "export ROS_SECURITY_ENABLE=true"
          - command: "export ROS_SECURITY_STRATEGY=Enforce"
          - command: "export ROS_SECURITY_ROOT_DIRECTORY=/opt/ros2_ws/keystore"
          - command: "export ROS_SECURITY_LOOKUP_TYPE=MATCH_PREFIX"                    
          - command: "env | grep ROS"  # from this point on, there's security enabled
          - command: "ros2 run demo_nodes_cpp talker"
          - split: horizontal
          - command: "source /opt/ros2_ws/install/setup.bash"
          - command: "export ROS_DOMAIN_ID=1"
          - command: "cd /opt/ros2_ws/"          
          - command: "mkdir policy"
          # generate a security policy based on our current graph
          - command: "ros2 security generate_policy policy/my_policy.xml"
          - command: "cat policy/my_policy.xml"
          #  populated the keystore for all profiles
          - command: "ros2 security generate_artifacts -k keystore -p policy/my_policy.xml -n /_ros2cli"
          - command: "kill -9 $(pidof talker)"
    - select: secure
  - container:
    - name: attacker
    - window:
        - name: attacker_window
        - commands:
          - command: "source /opt/ros2_ws/install/setup.bash"
          - command: "aztarna -t ros2 -d 0 --daemon" 
          - split: horizontal
          - command: "source /opt/ros2_ws/install/setup.bash"
          - type: "aztarna -t ros2 -d 1 --daemon"          
    - select: attacker_window
  - attach: subject2

This enables our internal security toolbox to launch and configure automatically the robotic scenario with the attacker connected to the same network, etc. We should be releasing this toolbox soon and make it available for testing which should simplify debugging things like this one. It's nevertheless easily reproducible at this point.

@mikaelarguedas shall we maybe consider accepting this simple fix and moving the discussion on this "deeper" issue to #172? I've allocated some bandwidth next week to look at this more closely.

@mikaelarguedas
Copy link
Member

shall we maybe consider accepting this simple fix and moving the discussion on this "deeper" issue to #172? I've allocated some bandwidth next week to look at this more closely.

My original thinking was that this may add significant overhead to using security, as it seems that each data payload would be encrypted once and then the global rtps frame will be encrypted again. It may also diminish the usefulness of exposing configuration options like #135 if the rtps frame gets encrypted anyway.

So I would prefer us to be sure this is needed by a tested/working solution to not leak node information rather than merging it based on a hunch I had that hasn't proven valid yet.

Happy to know you allocated time to look into it! Feel free to ping me if I can help

@vmayoral
Copy link
Member Author

vmayoral commented Jan 9, 2020

For completeness, reporting back in here from #172 (comment).

Didn't manage to put time into this beyond locating the vulnerability and reporting it, This might change in the coming weeks. I hope so.

@audrow audrow changed the base branch from master to rolling June 28, 2022 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants