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

Add ROS 2 Threat Model article #218

Merged
merged 9 commits into from Mar 27, 2019

Conversation

@thomas-moulard
Copy link
Collaborator

thomas-moulard commented Mar 20, 2019

This adds an article describing a threat model for
ROS 2 robotic applications.

The objective of this work is to drive ROS 2 security
work and define, end-to-end, what it takes for a robotic
company to build a secure ROS 2 robotic application.

Signed-off-by: Thomas Moulard tmoulard@amazon.com

@tfoote tfoote added the in progress label Mar 20, 2019

@thomas-moulard thomas-moulard marked this pull request as ready for review Mar 20, 2019

@gbiggs

This comment has been minimized.

Copy link
Contributor

gbiggs commented Mar 20, 2019

Wow.

@gavanderhoorn

This comment has been minimized.

Copy link
Contributor

gavanderhoorn commented Mar 20, 2019

@thomas-moulard: nice article. I notice however that there is just a single reference to related/existing work. Was there really no usable literature available, or was it all non-applicable? Grounding these sort of articles in the existing body of knowledge -- also (or preferably?) from outside the ROS community -- would significantly increase the value of them. Edit: oops, I should scroll to the right.

Perhaps a References section would be a good idea?

@olaldiko

This comment has been minimized.

Copy link

olaldiko commented Mar 20, 2019

Great work @thomas-moulard! We have reviewed it and it looks very promising. I think that most of the attack vectors have been covered. Just a small concern, several risks for physical attacks to the OpenCR controller and the Raspberry Pi have been taken as acceptable for this architecture. Shouldn't we advise at least for physically protecting access to sensitive components? I would suggest a combination of shields or covers + usage of the ZimKey perimeter breach detection mechanisms. In our experience with commercial robots, this is the way to go.

@thomas-moulard thomas-moulard referenced this pull request Mar 20, 2019

Closed

ROS 2 Threat Model #672

@thomas-moulard thomas-moulard force-pushed the thomas-moulard:b-ros2-672 branch 2 times, most recently from c569dc2 to 0a7926b Mar 20, 2019

Add ROS 2 Threat Model article
This adds an article describing a threat model for
ROS 2 robotic applications.

The objective of this work is to drive ROS 2 security
work and define, end-to-end, what it takes for a robotic
company to build a secure ROS 2 robotic application.

Signed-off-by: Thomas Moulard <tmoulard@amazon.com>

@thomas-moulard thomas-moulard force-pushed the thomas-moulard:b-ros2-672 branch from 0a7926b to 18744d2 Mar 20, 2019


### Robot Application Actors, Assets, Business Goals and Entry Points

This section defines actors, assets, business goals and entry points for this

This comment has been minimized.

Copy link
@jacobperron

jacobperron Mar 20, 2019

Member

This section does not explicitly define business goals. For clarity, consider organizing the entities as done in the threat analysis section.

This comment has been minimized.

Copy link
@vmayoral

vmayoral Mar 20, 2019

+1, this was confusing.

This comment has been minimized.

Copy link
@ryanewel

ryanewel Apr 5, 2019

Contributor

This is addressed in this PR #227 (review) .

Show resolved Hide resolved articles/ros2_threat_model.md Outdated
Show resolved Hide resolved articles/ros2_threat_model.md
Show resolved Hide resolved articles/ros2_threat_model.md Outdated
Show resolved Hide resolved articles/ros2_threat_model.md Outdated
<td class="danger">✘</td>
<td class="danger">✘</td>
<td class="danger">✘</td>

This comment has been minimized.

Copy link
@vmayoral

vmayoral Mar 20, 2019

Great diagram and great reading! I've learn tons from this article. Very good contribution. Blue lines in the diagram below are representing "DDS protocols" do you mean RTPS solely? If not, what else are you capturing here?

This comment has been minimized.

Copy link
@thomas-moulard

thomas-moulard Mar 22, 2019

Author Collaborator

We don't explicitly distinguish RTPS from XRCE in this diagram (both are in blue). The main reason is that we don't consider (at this point) protocol-level attack.
We should improve on that as XRCE does not support in-transit data encryption (see https://www.slideshare.net/GerardoPardo/overview-of-the-ddsxrce-specification slide 35)

This comment has been minimized.

Copy link
@ryanewel

ryanewel Apr 9, 2019

Contributor

Some more detail on the DDS protocol used is added in this PR #231. This also addresses an earlier comment on attacks for the OpenCR board.

Show resolved Hide resolved articles/ros2_threat_model.md Outdated
Show resolved Hide resolved articles/ros2_threat_model.md Outdated
Show resolved Hide resolved articles/ros2_threat_model.md Outdated
Show resolved Hide resolved articles/ros2_threat_model.md Outdated
Show resolved Hide resolved articles/ros2_threat_model.md Outdated
@olaldiko
Copy link

olaldiko left a comment

Here go some typo fixes and suggestions!

<td class="danger">✘</td>
<td class="danger">✘</td>
<td class="danger">✘</td>

This comment has been minimized.

Copy link
@olaldiko

olaldiko Mar 20, 2019

Fix indentation

Show resolved Hide resolved articles/ros2_threat_model.md Outdated
Show resolved Hide resolved articles/ros2_threat_model.md Outdated
Show resolved Hide resolved articles/ros2_threat_model.md
<td class="danger">✘</td>
<td class="danger">✘</td>
<td class="danger">✘</td>

This comment has been minimized.

Copy link
@olaldiko

olaldiko Mar 20, 2019

Add physical security measures, anti-tampering? (ZimKey perimeter breach detection)

<td class="danger">✘</td>
<td class="danger">✘</td>
<td class="danger">✘</td>

This comment has been minimized.

Copy link
@olaldiko

olaldiko Mar 20, 2019

Add usage of TPM devices as well for the rest of the platforms?

<tr><th colspan="29">Embedded / Hardware / Sensors</th></tr>

<tr>
<td>An attacker spoofs a robot sensor (by e.g. replacing the sensor itself

This comment has been minimized.

Copy link
@vmayoral

vmayoral Mar 20, 2019

True, I agree with this threat evaluation overall but I'm missing in the "Embedded / Hardware / <Sensor/Actuators>" section considerations of physical attacks that involve complete removal (not simple replacement) of the sensor. I still find shocking how several commercial and industrial robots don't provide any mechanism to secure hardware physically and simply present sensors totally exposed. I collaborated in the production of a framework (with a slightly different aim) that touched into these aspects https://arxiv.org/pdf/1806.04042.pdf.

Would you like me to make a few additional row suggestions? @olaldiko was also involved on this so maybe he'd like to jump in directly?

This comment has been minimized.

Copy link
@olaldiko

olaldiko Mar 20, 2019

This would be in line with adding physical security measures or tamper detections. In would combine them into spoofing/removal of the sensor, and add alerting/logging of physical intrusions and sensor communication losses as mitigation.

This comment has been minimized.

Copy link
@thomas-moulard

thomas-moulard Mar 22, 2019

Author Collaborator

I agree this is a real concern. @vmayoral yes, adding a few rows in the doc would be great.

<td class="danger">✘</td>
<td>
<ul>
<li>If a joint command is exceeding the joint limits, a specific code

This comment has been minimized.

Copy link
@vmayoral

vmayoral Mar 20, 2019

I don't quite see how this could be realized @thomas-moulard. Do you imply that all trajectories should be monitored for an "out of bounds" potential action and in case such is found a particular routine should get executed? Which routine? Wouldn't it be simpler to just discard the trajectory completely?

This comment has been minimized.

Copy link
@olaldiko

This comment has been minimized.

Copy link
@vmayoral

vmayoral Mar 20, 2019

Maybe I wasn't clear enough, sorry, I don't quite understand how to implement the countermeasure proposed and I'm arguing about whether simply discarding the trajectory would be safer. Does that make more sense @olaldiko?

This comment has been minimized.

Copy link
@gbiggs

gbiggs Mar 21, 2019

Contributor

That has safety implications. Simply discarding the trajectory may not be safe in all conditions or for all systems.

Monitoring every single command input (e.g. every trajectory) for dangerous ones is a common technique in safety.

This comment has been minimized.

Copy link
@thomas-moulard

thomas-moulard Mar 22, 2019

Author Collaborator

Yes, @gbiggs is correct. Coming from a humanoid robotic background, stopping a walking robot can be more dangerous than allowing a slight violation of a joint max velocity or effort. The same idea can apply to any robot where "velocity = 0" is not a safe state (drones, planes, self-driving cars on a highway).

This comment has been minimized.

Copy link
@vmayoral

vmayoral Mar 25, 2019

All right, that makes sense, however does the same rationale apply for this mobile robot application? It feels like in this particular case we may want to elaborate? I still believe I’d rather have the TB3 stopping than falling/hitting something.

Show resolved Hide resolved articles/ros2_threat_model.md Outdated
Show resolved Hide resolved articles/ros2_threat_model.md Outdated
Show resolved Hide resolved articles/ros2_threat_model.md Outdated
<td class="danger">✘</td>
<td class="danger">✘</td>
<td class="danger">✘</td>

This comment has been minimized.

Copy link
@vmayoral

vmayoral Mar 20, 2019

How about adding another section to treat vulnerabilities in the Communications Middleware? Something like "Embedded / Software / Communication Middleware". There're several issues opened in ros2/ros2 describing how middleware implementations start demanding a considerable amount of resources as more publishers (or subscribers) are enabled into them. We ourselves while building robots found considerable limitations in several DDS implementations. This can lead to simple DoS attacks by simply accessing the local network of the robot. While we haven't yet tested it extensively (@LanderU, feel free to comment), we've been able to limit the implications to a certain extend by properly configuring the DDS layer.

This comment has been minimized.

Copy link
@olaldiko

olaldiko Mar 20, 2019

I think this would fall on "An attacker prevents a communication channel from being usable."

This comment has been minimized.

Copy link
@thomas-moulard

thomas-moulard Mar 22, 2019

Author Collaborator

@vmayoral My understanding is that enabling SROS and enforcing all nodes to be secure would mitigate this. Maybe adding somewhere "the DDS network should be kept isolated as much as possible" would be good?

By the way, we've been running TSan on ROS 2 and we detected potential deadlocks when the graph changes. So what you're highlighting here is a real concern, due to the way DDS discovery works, one could imagine that spamming a network with new DDS participants could put ROS 2 nodes into a dead-locked state (note that we didn't attempt this yet so we don't know how hard it would be to reproduce this issue that TSan detected).

<td class="danger">✘</td>
<td class="danger">✘</td>
<td class="danger">✘</td>

This comment has been minimized.

Copy link
@vmayoral

This comment has been minimized.

Copy link
@vmayoral

vmayoral Mar 20, 2019

This is a nice thing to add to the micro-ROS project. @imuguruza please consider it.

Show resolved Hide resolved articles/ros2_threat_model.md Outdated
<td class="danger">✘</td>
<td class="danger">✘</td>
<td class="danger">✘</td>

This comment has been minimized.

Copy link
@vmayoral

vmayoral Mar 20, 2019

Are you open to consider more communication channels? E.g. serial ports such as SPI or UART should be available both from the RPi. Similarly, OpenCR exposes communication ports.

This comment has been minimized.

Copy link
@vmayoral

vmayoral Mar 20, 2019

On a second reading, there're several more entry points worth mentioning here @thomas-moulard. It may be worth considering them. There're some simple aspects such as unused communication ports which can be simply disabled by configuring the embedded computer (and the underlying OS) appropriately. Would you be open for contributions that would affect (slightly to include those columns below) the threat model below? @XabierPB and @o-olalde, would you be interested on this?

This comment has been minimized.

Copy link
@thomas-moulard

thomas-moulard Mar 22, 2019

Author Collaborator

Yes, that'd be great. As with your other suggestion, it may be better to get this merged and iterate on the doc, but I don't have a very strong preference there.

<td class="danger">✘</td>
<td class="danger">✘</td>
<td class="danger">✘</td>

This comment has been minimized.

Copy link
@vmayoral

vmayoral Mar 20, 2019

Suggested change
*TurtleBot Threat Diagram - An attacker deploys a malicious node on the robot*

This comment has been minimized.

Copy link
@vmayoral

vmayoral Mar 20, 2019

I think the last space wasn't needed.

This comment has been minimized.

Copy link
@vmayoral

vmayoral Mar 20, 2019

Or maybe this is intended to be a title? If so, a different format would be required

@vmayoral

This comment has been minimized.

Copy link

vmayoral commented Mar 20, 2019

That was very nice reading @thomas-moulard! Thanks a lot for the contribution. Very, very nice.

A few further comments:

  • I'd support @gavanderhoorn above about a references section. It'll help many of us get the references across the document into a single place for further reading. I'm happy drafting a first Pull Request if needed.
  • I find the first part of the article particularly well suited for the title of the document however the Threat Analysis that follows (the TurtleBot 3 one) is only of of many we could (and I hope we) produce. May I suggest to bring that second part of the article to yet another article? I can envision having a dedicated section in http://design.ros2.org to "Security" (at the same level as "Overview" or "Middleware"). @jacobperron, @nuclearsandwich, @dirk-thomas, @wjwwood is this actually feasible? It will certainly boost the contributions on security IMHO.

@rutvih rutvih referenced this pull request Mar 20, 2019

Open

ROS 2 Dashing Diademata #607

5 of 19 tasks complete
@thomas-moulard

This comment has been minimized.

Copy link
Collaborator Author

thomas-moulard commented Mar 21, 2019

Thanks a lot everyone for the feedback - I'll integrate all of those points and push an update to this PR ASAP.

thomas-moulard added some commits Mar 22, 2019

Apply suggestions from code review
Fix typos and improve formatting.

Co-Authored-By: thomas-moulard <thomas.moulard@gmail.com>

Signed-off-by: Thomas Moulard <tmoulard@amazon.com>
Add a "references" section
Signed-off-by: Thomas Moulard <tmoulard@amazon.com>
Fix typos, phrasing and minor formatting issues
Signed-off-by: Thomas Moulard <tmoulard@amazon.com>
Add table columns description
Signed-off-by: Thomas Moulard <tmoulard@amazon.com>

@thomas-moulard thomas-moulard force-pushed the thomas-moulard:b-ros2-672 branch from 7ff2ef7 to 712af74 Mar 22, 2019

@thomas-moulard

This comment has been minimized.

Copy link
Collaborator Author

thomas-moulard commented Mar 22, 2019

@gavanderhoorn @vmayoral I added a references section - open for ideas there if you feel it could be improved.

I think splitting the doc could be a good idea, but it could also probably be done after this first pass.

All feedback is not addressed yet and I'll take care of that tomorrow.

@ruffsl

This comment has been minimized.

Copy link
Member

ruffsl commented Mar 22, 2019

I think the tables are a bit too wide to reasonably read with one screen:

image

Perhaps we could collapse the columns for:

  • Threat Category (STRIDE)
  • Threat Risk Assessment (DREAD)
  • Impacted Assets
  • Impacted Entry Points
  • Impacted Business Goals

And use a fixed width encoding of flags and values, e.g:

Threat Category Threat Risk Assessment ...
S✓ T✘ R✘ I✓ D✓ E✓ D3 R1 E1 A2 D3 = 10 ...
... .. ...
@gbiggs

This comment has been minimized.

Copy link
Contributor

gbiggs commented Mar 24, 2019

I think the tables are a bit too wide to reasonably read with one screen

You should try reviewing the document on an iPad.

Show resolved Hide resolved articles/ros2_threat_model.md Outdated
Show resolved Hide resolved articles/ros2_threat_model.md Outdated
Show resolved Hide resolved articles/ros2_threat_model.md Outdated
<td class="success">Y</td>
<td class="success">Y</td>
<td>A developer buildling a cloud service connected to the robot or an
analyst who has been granted access to robot data.</td>

This comment has been minimized.

Copy link
@kyrofa

kyrofa Mar 25, 2019

There may be two actors lumped into one here, which would be fine except that I question whether or not one is considered a super user. For example, this might be exactly the same as the "Robot Developer" but someone who happens to be remote, in which case, definitely a super user. However, an analyst who has been granted access to the data but nothing more feels like another actor that isn't a super user (they can't affect the behavior of the robot), but also isn't a normal end-user. The set of assets covered by each role is very different. Should the data analyst be an actor of its own?

Show resolved Hide resolved articles/ros2_threat_model.md
<td>
<ul>
<li>Multiple long-range communication transport layers should be used
when possible (e.g. cellular and WiFi)</li>

This comment has been minimized.

Copy link
@kyrofa

kyrofa Mar 25, 2019

And the system should be designed to "fail closed." If the system isn't being commanded to do something or detects that it's lost communication, it might be a good time to halt.

Show resolved Hide resolved articles/ros2_threat_model.md Outdated
Show resolved Hide resolved articles/ros2_threat_model.md Outdated
Show resolved Hide resolved articles/ros2_threat_model.md Outdated
Show resolved Hide resolved articles/ros2_threat_model.md Outdated

kyrofa and others added some commits Mar 27, 2019

Fix typos and minor rephrasing
Co-Authored-By: thomas-moulard <thomas.moulard@gmail.com>
@tfoote

This comment has been minimized.

Copy link
Contributor

tfoote commented Mar 27, 2019

With the first round of comments resolved and the structure laid out. This has been marked as a draft. Please open new PRs to discuss specific areas of the document so we can have deeper threaded discussions on this document.

@tfoote tfoote merged commit 6e0dfbe into ros2:gh-pages Mar 27, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@tfoote tfoote removed the in progress label Mar 27, 2019

@thomas-moulard thomas-moulard added this to Done in AWS Robotics Apr 2, 2019

@thomas-moulard thomas-moulard deleted the thomas-moulard:b-ros2-672 branch Apr 5, 2019

@olaldiko olaldiko referenced this pull request Apr 8, 2019

Open

MARA Threat Model #228

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.