search

izar / pytm

A Pythonic framework for threat modeling
Other
891 stars 168 forks source link

delete unused attributes #119

Closed nineinchnick closed 3 years ago

nineinchnick commented 3 years ago

The following attributes are not used in any threat. Using them in models can create a false sense of security where no new findings would be reported.

Its pretty easy for anyone to extend pytm classes and add these back when referencing them in custom threats. I believe that extending pytm this way should be encouraged.

This is controversial but it's easier to remove unused attributes than try to figure out correct description for them. Less is more.

nineinchnick commented 3 years ago

These should be moved to the new Data class (without the stores prefix) with another new attr like isStored, to separate dataflows for select and insert/update queries.

izar commented 3 years ago

You're forgetting the dimension that an attribute, in and off itself, can serve as a documentation/clarification. Granted, they are not part of a threat right now but they can offer clarity. We need a better job at documentation and using enumerations for possible values where appropriate, but i think these attributes still have a place. About the stores* I agree that they have a place in Data, but i would say that it is equally important to be able to pinpoint when a store stores a given type of data - either as clarification of for rules that are more datastore oriented than data itself.

nineinchnick commented 3 years ago

You're forgetting the dimension that an attribute, in and off itself, can serve as a documentation/clarification. Granted, they are not part of a threat right now but they can offer clarity. We need a better job at documentation and using enumerations for possible values where appropriate, but i think these attributes still have a place.

I hold my argument that if these attrs are not documented and not used in any threats they only add confusion when starting out with pytm and building first models. The names are not obvious enough and users can waste lots of time trying to figure out the "correct" value only to later learn that this was not necessary.

You can still add custom attributes to any object and reference them in the template. Since you'd have to reference them in your custom template I don't see any value in providing any default "doc only" attributes.

I only see two options, either removing them or documenting them. If you want to do the latter, please open up a PR and I would remove documented attrs from this one.

About the stores* I agree that they have a place in Data, but i would say that it is equally important to be able to pinpoint when a store stores a given type of data - either as clarification of for rules that are more datastore oriented than data itself.

Marking a datastore as storing a particular kind of data only makes sense if there are any incoming dataflows that carry this kind of data. Even if we'd keep these stores* attributes, they should be populated automatically. I can add that in this PR if you agree.

izar commented 3 years ago

I hold my argument that if these attrs are not documented and not used in any threats they only add confusion when starting out with pytm and building first models. The names are not obvious enough and users can waste lots of time trying to figure out the "correct" value only to later learn that this was not necessary.

Totally agreed. We MUST have better docs, and we are ok to change names.

Marking a datastore as storing a particular kind of data only makes sense if there are any incoming dataflows that carry this kind of data.

Consider a project where many developers are creating the model. That exact discrepancy is something worth flagging out. We cannot assume that the same person creating the datastore is the one documenting the dataflows.

colesmj commented 3 years ago

There is a value to allowing users to set custom attributes, or to set attributes that make sense for an object or flow but that does not yet contribute to a threat. But perhaps this something we solve in v2, not currently.

~Matthew Coles~

On Tue, Dec 29, 2020, 14:14 Izar Tarandach notifications@github.com wrote:

I hold my argument that if these attrs are not documented and not used in any threats they only add confusion when starting out with pytm and building first models. The names are not obvious enough and users can waste lots of time trying to figure out the "correct" value only to later learn that this was not necessary.

Totally agreed. We MUST have better docs, and we are ok to change names.

Marking a datastore as storing a particular kind of data only makes sense if there are any incoming dataflows that carry this kind of data.

Consider a project where many developers are creating the model. That exact discrepancy is something worth flagging out. We cannot assume that the same person creating the datastore is the one documenting the dataflows.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/izar/pytm/pull/119#issuecomment-752210568, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJMQZ6WYZ5EHXVMMM52IJETSXITBRANCNFSM4SBVUZ5A .

nineinchnick commented 3 years ago

@izar @colesmj so what's the conclusion? Should we remove attrs that are not well defined or try to document them? Removing is easier, hence this PR.

izar commented 3 years ago

I'm for documenting rather than deleting.

On Wed, Dec 30, 2020 at 11:44 AM Jan Waś notifications@github.com wrote:

@izar https://github.com/izar @colesmj https://github.com/colesmj so what's the conclusion? Should we remove attrs that are not well defined or try to document them? Removing is easier, hence this PR.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/izar/pytm/pull/119#issuecomment-752686089, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAC2BAJ7PGQSEAZZVVR7WPLSXNKH7ANCNFSM4SBVUZ5A .

nineinchnick commented 3 years ago

I'm for documenting rather than deleting.

I can give a shot for the obvious ones like OS but:

  1. what about attrs that are completely ambiguous, like handlesCrashes? How does it handle crashes? What kind of crashes?
  2. how to document that some attrs are not used in any threats? In their docs or should we keep a separate list somewhere?
  3. should docs include example values? Like for OS should it be Linux, Windows, OSX or more specifically distros and versions?
izar commented 3 years ago

Of course some can be deleted if we decide they're not useful :) For example, for handlesCrashes, i could imagine something like a named bit map ( https://docs.astropy.org/en/stable/api/astropy.nddata.bitmask.BitFlagNameMap.html#astropy.nddata.bitmask.BitFlagNameMap) with OS, Process, etc. to express at which level(s) the crash may be handled. That would also cover example values. Enumerations and bit masks would make things clearer IMHO.

On Wed, Dec 30, 2020 at 12:21 PM Jan Waś notifications@github.com wrote:

I'm for documenting rather than deleting.

I can give a shot for the obvious ones like OS but:

  1. what about attrs that are completely ambiguous, like handlesCrashes? How does it handle crashes? What kind of crashes?
  2. how to document that some attrs are not used in any threats? In their docs or should we keep a separate list somewhere?
  3. should docs include example values? Like for OS should it be Linux, Windows, OSX or more specifically distros and versions?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/izar/pytm/pull/119#issuecomment-752696629, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAC2BANQNKETZJXVZWBGF43SXNOSRANCNFSM4SBVUZ5A .

nineinchnick commented 3 years ago

Of course some can be deleted if we decide they're not useful :) For example, for handlesCrashes, i could imagine something like a named bit map ( https://docs.astropy.org/en/stable/api/astropy.nddata.bitmask.BitFlagNameMap.html#astropy.nddata.bitmask.BitFlagNameMap) with OS, Process, etc. to express at which level(s) the crash may be handled. That would also cover example values. Enumerations and bit masks would make things clearer IMHO.

Sorry but that's still very vague. pytm should not try to model every property of every possible system since this is impossible. We should only keep things that are in any way related to threats. That's why I suggest removing all the attributes mentioned in this PR. Including generic things like OS if we don't have OS specific threats.

izar commented 3 years ago

I do believe being able to say that a specific element handles crashes both at the OS level (with some kind of reboot watchdog for example) or at the application level (by running in a loop, for example) is important information to have. And yes, things will be vague until they're solidified either by documentation or added to a threat or... - it is the nature of the beast.

On Wed, Dec 30, 2020 at 12:37 PM Jan Waś notifications@github.com wrote:

Of course some can be deleted if we decide they're not useful :) For example, for handlesCrashes, i could imagine something like a named bit map ( https://docs.astropy.org/en/stable/api/astropy.nddata.bitmask.BitFlagNameMap.html#astropy.nddata.bitmask.BitFlagNameMap) with OS, Process, etc. to express at which level(s) the crash may be handled. That would also cover example values. Enumerations and bit masks would make things clearer IMHO.

Sorry but that's still very vague. pytm should not try to model every property of every possible system since this is impossible. We should only keep things that are in any way related to threats. That's why I suggest removing all the attributes mentioned in this PR. Including generic things like OS if we don't have OS specific threats.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/izar/pytm/pull/119#issuecomment-752701238, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAC2BANGS5QBIVCZIUJO7ADSXNQNJANCNFSM4SBVUZ5A .

nineinchnick commented 3 years ago

I do believe being able to say that a specific element handles crashes both at the OS level (with some kind of reboot watchdog for example) or at the application level (by running in a loop, for example) is important information to have. And yes, things will be vague until they're solidified either by documentation or added to a threat or... - it is the nature of the beast.

I've chosen a bad example since handlesCrashes just begs to have a threat added and I'm pretty sure we'll find some matching CWEs quickly. So I can sign up for handling this one just to keep things moving forward. But what about others? Can we go through all of them one by one? Should we take this to Slack?

izar commented 3 years ago

Sure thing, let's see what makes sense to keep and what to drop. We might even draw an informal policy on what and how to add going forward.

On Thu, Dec 31, 2020, 07:30 Jan Waś notifications@github.com wrote:

I do believe being able to say that a specific element handles crashes both at the OS level (with some kind of reboot watchdog for example) or at the application level (by running in a loop, for example) is important information to have. And yes, things will be vague until they're solidified either by documentation or added to a threat or... - it is the nature of the beast.

I've chosen a bad example since handlesCrashes just begs to have a threat added and I'm pretty sure we'll find some matching CWEs quickly. So I can sign up for handling this one just to keep things moving forward. But what about others? Can we go through all of them one by one? Should we take this to Slack?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/izar/pytm/pull/119#issuecomment-752946462, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAC2BAKME5RSCB3ZZC5AFTTSXRVGTANCNFSM4SBVUZ5A .

colesmj commented 3 years ago

I will say that we should be maintaining a data model. I started one in the wiki in GitHub we can leverage (it needs updating of course for all the work that has been done so far). Let me know if you want me to take this as an action.

~Matthew Coles~

On Thu, Dec 31, 2020, 08:52 Izar Tarandach notifications@github.com wrote:

Sure thing, let's see what makes sense to keep and what to drop. We might even draw an informal policy on what and how to add going forward.

On Thu, Dec 31, 2020, 07:30 Jan Waś notifications@github.com wrote:

I do believe being able to say that a specific element handles crashes both at the OS level (with some kind of reboot watchdog for example) or at the application level (by running in a loop, for example) is important information to have. And yes, things will be vague until they're solidified either by documentation or added to a threat or... - it is the nature of the beast.

I've chosen a bad example since handlesCrashes just begs to have a threat added and I'm pretty sure we'll find some matching CWEs quickly. So I can sign up for handling this one just to keep things moving forward. But what about others? Can we go through all of them one by one? Should we take this to Slack?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/izar/pytm/pull/119#issuecomment-752946462, or unsubscribe < https://github.com/notifications/unsubscribe-auth/AAC2BAKME5RSCB3ZZC5AFTTSXRVGTANCNFSM4SBVUZ5A

.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/izar/pytm/pull/119#issuecomment-752963386, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJMQZ6WHINC7MQNBRZWFNV3SXR6YDANCNFSM4SBVUZ5A .

colesmj commented 3 years ago

FYI. I went ahead and I've started on the object model for 1.x release in the wiki based on the code and documentation within.

~Matthew Coles~

On Thu, Dec 31, 2020, 09:27 Matthew Coles coles.matthewj@gmail.com wrote:

I will say that we should be maintaining a data model. I started one in the wiki in GitHub we can leverage (it needs updating of course for all the work that has been done so far). Let me know if you want me to take this as an action.

~Matthew Coles~

On Thu, Dec 31, 2020, 08:52 Izar Tarandach notifications@github.com wrote:

Sure thing, let's see what makes sense to keep and what to drop. We might even draw an informal policy on what and how to add going forward.

On Thu, Dec 31, 2020, 07:30 Jan Waś notifications@github.com wrote:

I do believe being able to say that a specific element handles crashes both at the OS level (with some kind of reboot watchdog for example) or at the application level (by running in a loop, for example) is important information to have. And yes, things will be vague until they're solidified either by documentation or added to a threat or... - it is the nature of the beast.

I've chosen a bad example since handlesCrashes just begs to have a threat added and I'm pretty sure we'll find some matching CWEs quickly. So I can sign up for handling this one just to keep things moving forward. But what about others? Can we go through all of them one by one? Should we take this to Slack?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/izar/pytm/pull/119#issuecomment-752946462, or unsubscribe < https://github.com/notifications/unsubscribe-auth/AAC2BAKME5RSCB3ZZC5AFTTSXRVGTANCNFSM4SBVUZ5A

.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/izar/pytm/pull/119#issuecomment-752963386, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJMQZ6WHINC7MQNBRZWFNV3SXR6YDANCNFSM4SBVUZ5A .

izar commented 3 years ago

That's a great idea. We should use that as the base for documentation - the "source of truth" going forward.

On Thu, Dec 31, 2020, 10:10 colesmj notifications@github.com wrote:

FYI. I went ahead and I've started on the object model for 1.x release in the wiki based on the code and documentation within.

~Matthew Coles~

On Thu, Dec 31, 2020, 09:27 Matthew Coles coles.matthewj@gmail.com wrote:

I will say that we should be maintaining a data model. I started one in the wiki in GitHub we can leverage (it needs updating of course for all the work that has been done so far). Let me know if you want me to take this as an action.

~Matthew Coles~

On Thu, Dec 31, 2020, 08:52 Izar Tarandach notifications@github.com wrote:

Sure thing, let's see what makes sense to keep and what to drop. We might even draw an informal policy on what and how to add going forward.

On Thu, Dec 31, 2020, 07:30 Jan Waś notifications@github.com wrote:

I do believe being able to say that a specific element handles crashes both at the OS level (with some kind of reboot watchdog for example) or at the application level (by running in a loop, for example) is important information to have. And yes, things will be vague until they're solidified either by documentation or added to a threat or... - it is the nature of the beast.

I've chosen a bad example since handlesCrashes just begs to have a threat added and I'm pretty sure we'll find some matching CWEs quickly. So I can sign up for handling this one just to keep things moving forward. But what about others? Can we go through all of them one by one? Should we take this to Slack?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/izar/pytm/pull/119#issuecomment-752946462, or unsubscribe <

https://github.com/notifications/unsubscribe-auth/AAC2BAKME5RSCB3ZZC5AFTTSXRVGTANCNFSM4SBVUZ5A

.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/izar/pytm/pull/119#issuecomment-752963386, or unsubscribe < https://github.com/notifications/unsubscribe-auth/AJMQZ6WHINC7MQNBRZWFNV3SXR6YDANCNFSM4SBVUZ5A

.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/izar/pytm/pull/119#issuecomment-752984498, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAC2BAMN4PKZNJQURXAG2SLSXSH65ANCNFSM4SBVUZ5A .