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

MARA Threat Model #228

Merged
merged 26 commits into from
May 1, 2019
Merged

Conversation

olaldiko
Copy link
Contributor

@olaldiko olaldiko commented Apr 8, 2019

This adds an extension to the original Threat Model proposed by @thomas-moulard started on #218 .

Based on the work done by the Amazon Robotics team, we have developed a threat model for an industrial cobot, the MARA by Acutronics Robotics.

We also have some additions on attack vectors and assets in the main threat table.

The objective of this threat model extension is to create a replicable base for creating threat models for different robots on industrial environments.

Work done by @o-olalde, @XabierPB and @borkenerice and myself (@olaldiko) as part of Alias Robotics.

@tfoote tfoote added the in review Waiting for review (Kanban column) label Apr 8, 2019
@olaldiko olaldiko changed the title Mara Threat Model. Initial Version MARA Threat Model Apr 8, 2019
@vmayoral
Copy link
Member

vmayoral commented Apr 8, 2019

Maybe it's worth sharing that Acutronic Robotics has been funding this work for the last few months and encouraged a release aligned with @thomas-moulard's (and the rest of his colleagues).

I will go through the PR tonight soon and will try contributing with what's been our experience from "the other side". @LanderU and @ahcorde, feel free to jump in and add your views as well.

@thomas-moulard
Copy link
Contributor

Awesome! 👍 I'll start reviewing this but before that, would you mind making sure that the new files are passing the Markdown linter: mdl.

gem install --user-install mdl / mdl ./myfile.md

I had to ignore the MD033 forbidding inline HTML but everything else seems like things which can be fixed.

@olaldiko
Copy link
Contributor Author

olaldiko commented Apr 8, 2019

Thanks @thomas-moulard! Sure, we'll get on it ASAP!

@@ -45,19 +52,78 @@ th {

</div>

## Table of Contents

- [{{ page.title }}](#pagetitle)
Copy link
Contributor

Choose a reason for hiding this comment

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

{:toc} ?

Choose a reason for hiding this comment

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

The toc should appear automatically with that tag when running it with jekyll?

Copy link
Contributor

@thomas-moulard thomas-moulard Apr 9, 2019

Choose a reason for hiding this comment

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

I realized that I had this tag and it's not doing what I expect it to do. I guess it's fine to keep that manual for this PR but we should probably try to automate this somehow.

Copy link

@XabierPB XabierPB Apr 9, 2019

Choose a reason for hiding this comment

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

We found out that putting a "* TOC" just above {:toc} generates a visible table of contents. See example below. You could just try! Visually it generates the toc on the right side of the page (using Jekyll)

Suggested change
- [{{ page.title }}](#pagetitle)
* TOC
{:toc}
- [{{ page.title }}](#pagetitle)

Copy link
Member

Choose a reason for hiding this comment

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

@olaldiko will you accept this? If so, maybe we should remove the TOC generated manually below?

@@ -186,6 +252,10 @@ attack).
<td>Physical Safety</td>
<td>The robotic system must not harm its users or environment.</td>
</tr>
<tr>
<td>Robot Integrity</td>
<td>The robotic system must not damage itself.</td>
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have a strong opinion there, but compared to expanding the scope of Physical Safety, do you see cases where those two columns values won't be identical?

Copy link
Member

Choose a reason for hiding this comment

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

+1

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

[Diagram Source (draw.io)](ros2_threat_model/attack_tree_physical.xml)

The next diagram shows the infrastructure affected on a possible attack based on a compromise of a physical communication port.
Copy link
Contributor

Choose a reason for hiding this comment

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

affected by?

Copy link

Choose a reason for hiding this comment

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

To me it sounds better "the affected infrastructure". Opinions?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, SGTM

articles/ros2_threat_model.md Outdated Show resolved Hide resolved
@thomas-moulard
Copy link
Contributor

@olaldiko this is great, thanks for sending this! Do you think we should align and use RVSS for all of the doc instead of DREAD?

XabierPB and others added 7 commits April 8, 2019 21:34
Good catch from @thomas-moulard! I don't think it needs further discussion ;)
Just reorder references alphabetically.
Typos: Just correct Maximun and Minimun
@olaldiko
Copy link
Contributor Author

olaldiko commented Apr 9, 2019

@olaldiko this is great, thanks for sending this! Do you think we should align and use RVSS for all of the doc instead of DREAD?

While it would be nice, in this case RVSS is not meant for measuring risks, but actual vulnerabilities. It would be the equivalent of the CVSS for robotics.
However, I think that it would be interesting to evaluate each of the robots with the RSF as well to define weak points and improvements. What do you think, @thomas-moulard?

Just reorder references alphabetically.
@thomas-moulard
Copy link
Contributor

@olaldiko That SGTM, not sure if we will be able to follow-up on that for TB3 right now but it seems better to have only on standard and make it evolve as/if needed. We can explain that in another section "How to add your robot to this threat model? You should evaluate your threats using RSF, etc.". If you would like to add this section in this PR that'd be great, otherwise it can done later by either you or us.

@olaldiko
Copy link
Contributor Author

Sure @thomas-moulard, that SGTM too. I will start working on the new section!

Copy link
Contributor

@thomas-moulard thomas-moulard left a comment

Choose a reason for hiding this comment

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

SGTM, thanks! We can merge this and iterate on the doc as needed IMHO.

articles/ros2_threat_model.md Outdated Show resolved Hide resolved
@thomas-moulard
Copy link
Contributor

@olaldiko Let's rebase this and we can merge the doc 🚢

@olaldiko
Copy link
Contributor Author

Good! @thomas-moulard, @tfoote, what would be the procedure here? Shall we rewrite history on the PR itself or the rebase is performed when merging into gh-pages branch?

Co-Authored-By: olaldiko <gorka.olalde@gmail.com>
@thomas-moulard
Copy link
Contributor

I'll let @tfoote answer this one. I did rebase and pushed with git push --force-with-lease but I know not everyone agrees this is the "right thing to do".

@vmayoral
Copy link
Member

@tfoote and @thomas-moulard can we merge this please?

We are preparing a few additional contributions that should ship soon after this one.

@vmayoral
Copy link
Member

@olaldiko are you guys fine with it?

@olaldiko
Copy link
Contributor Author

olaldiko commented Apr 26, 2019 via email

@thomas-moulard
Copy link
Contributor

@olaldiko Can you rebase the doc? I will merge it once it's done.

@vmayoral
Copy link
Member

vmayoral commented Apr 26, 2019

Thanks a lot for reacting @thomas-moulard, great!

@olaldiko, happy let me know if you need help rebasing the file.

@olaldiko
Copy link
Contributor Author

@thomas-moulard, I think that we have successfully rebased the commits now. Please, check and let me know if any changes need to be done!

@thomas-moulard
Copy link
Contributor

thomas-moulard commented Apr 30, 2019

@olaldiko Could you check again? I can't merge because there are still conflicts in this PR:

Rebasing the commits of this branch on top of the base branch cannot be performed automatically due to conflicts encountered while reapplying the individual commits from the head branch.

@olaldiko
Copy link
Contributor Author

olaldiko commented May 1, 2019

@thomas-moulard , I have been doing several tests on my own fork, and I think that the best strategy here would be to squash and merge into the base branch instead of rebasing. This way the changes will show up as a single commit on the gh-pages branch. I think that this was done in #218 and others as well, as each PR shows up as a single commit on base. No conflicts arise following this strategy. Could you please check and tell me your thoughts?
Thanks!

@thomas-moulard thomas-moulard merged commit 4d127ad into ros2:gh-pages May 1, 2019
@tfoote tfoote removed the in review Waiting for review (Kanban column) label May 1, 2019
@dirk-thomas
Copy link
Member

@thomas-moulard This document doesn't follow the style guide for markdown (https://index.ros.org/doc/ros2/Contributing/Developer-Guide/#markdown-restructured-text-docblocks). Please update the doc to follow the one-sentence-per-line rule. See https://github.com/ros2/design/pull/236/files#diff-60aa2b3022e7728b236f31c650141eceL3544 for a case why the current state is not desired.

@thomas-moulard
Copy link
Contributor

Thanks Dirk for checking, I assigned to myself #237 and I'll do that by EOW.

@thomas-moulard
Copy link
Contributor

#238 should fix style issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants