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.

Sign up for GitHub

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

Jump to bottom

delete unused attributes #119

Closed
wants to merge 1 commit into from
Closed

delete unused attributes #119

wants to merge 1 commit into from

Conversation

nineinchnick
Copy link
Collaborator

@nineinchnick nineinchnick commented Oct 2, 2020 •
edited

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.

  • authenticatedWith
  • authenticationScheme
  • codeType
  • handlesCrashes
  • handlesInterruptions
  • hasWriteAccess
  • implementsCommunicationProtocol
  • isAdmin
  • isSQL
  • onAWS
  • onRDS
  • OS
  • providesConfidentiality
  • storesLogData
  • storesPII
  • storesSensitiveData
  • tracksExecutionFlow

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
Copy link
Collaborator Author

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.

  • storesLogData
  • storesPII
  • storesSensitiveData

This was referenced Oct 2, 2020
Open
Merged
@izar
Copy link
Owner

izar commented Dec 29, 2020

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
Copy link
Collaborator Author

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
Copy link
Owner

izar commented Dec 29, 2020

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
Copy link
Collaborator

colesmj commented Dec 29, 2020 via email

@nineinchnick
Copy link
Collaborator Author

@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
Copy link
Owner

izar commented Dec 30, 2020 via email

@nineinchnick
Copy link
Collaborator Author

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
Copy link
Owner

izar commented Dec 30, 2020 via email

@nineinchnick
Copy link
Collaborator Author

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
Copy link
Owner

izar commented Dec 30, 2020 via email

@nineinchnick
Copy link
Collaborator Author

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
Copy link
Owner

izar commented Dec 31, 2020 via email

@colesmj
Copy link
Collaborator

colesmj commented Dec 31, 2020 via email

@colesmj
Copy link
Collaborator

colesmj commented Dec 31, 2020 via email

@izar
Copy link
Owner

izar commented Dec 31, 2020 via email

@izar izar added this to the v1.1.3 milestone Apr 12, 2021
@izar izar closed this Apr 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@izar izar Awaiting requested review from izar izar is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v1.1.3
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@nineinchnick @izar @colesmj