izar / pytm

A Pythonic framework for threat modeling
Other
861 stars 161 forks source link

Add a way to exclude threats for specific Elements #244

Closed Dakes closed 3 weeks ago

Dakes commented 2 months ago

I have a situation where a lot of unrelated threats get detected. In this case the communication is happening via IPC, so I don't really need web relating threats, like PHP, XSS, XML etc. However, this is only true for some parts of the system. There is also a web server and frontend involved, where some of these might apply. So I can't use the global --exclude flag.

I could maybe modify the threats.json to check for a new custom property.

But I think a more general solution might be, either, to modify --exclude to take an optional boundary name as a filter, so the exclude only applies within the boundary.

Or, in my opinion, a better solution would be to add an Element specific exclude. Similar to the overrides property. For example I (and probably others as well) have created custom classes, that inherit from the pytm classes and then inherit from each other. So this would also be a convenient way for excludes to be applied to multiple Elements.

What do you think would fit better with pytm? Is the second solution fine, or does anyone have additional input?

If we decide on a specific implementation, I would be happy to create a pull request for the feature.

raphaelahrens commented 1 month ago

I would not recommend to use the exclude flag for this.

First it should be included in the model not in how you execute pytm. The main argument for me here is that the model should hold all information about the system.

I think what would be a more fitting place to put this information are assumptions about the system.. Currently assumptions in pytm are just a list of strings, for the whole model stored in the class TM. But a new attribute (assumptions) could be added to the Element class, which holds a list of class Assumption or strings for this element. An Assumption could then look like this

srv = Server("Not a PHP server")
srv.assumptions = [ Assumption("We are not using PHP", exclude="INP16")

Then the excluded threats could still be added add the end of the report for a review in an assumption section, but will not overwhelm the devs with useless findings. Further we have the chance to add a reason for the exclusion, not always is the decision of excluding so easy as in the described case above and having a form of documentation for an exclusion of a threat is valuable in my opinion. And sometimes system change and then it is good to remember why a threat was excluded back in the day.

For your case I would also like to discuss the option to change Dataflow to be less depended on IP/(TCP| UPD). Currently Dataflow assumes that there is a port number for the source and destination, the potential to use TLS and the potential for session tokens. But I guess this could be a separate issue.

Any way these are just my opinions.

Dakes commented 1 month ago

That sounds pretty good, thanks for the suggestion. I will try to implement is like this. 👍