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

Multi threat libs #263

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Multi threat libs #263

wants to merge 8 commits into from

Conversation

izar
Copy link
Collaborator

@izar izar commented Feb 20, 2025

Adds capability to select which threat lib to use on the command line:

  • nothing in the command line: uses the default library
  • --threat-files foo.json : uses the threats in foo.json only
  • --threat-files foo.json default : uses the threats in foo.json and the default ones

@izar izar requested a review from colesmj February 20, 2025 20:01
@@ -1405,5 +1405,5 @@
"name": "my test tm",
"onDuplicates": "Action.NO_ACTION",
"threatsExcluded": [],
"threatsFile": "pytm/threatlib/threats.json"
"threatsFile": "{'pytm/threatlib/threats.json'}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the output a string and not a list of strings?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

that's what varStrings with only one value put out...

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a bug?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

in the immortal words of just about everybody..."works for me"!

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok I had a quick look and the issue is that the default to_serializable is used.

pytm/pytm/pytm.py

Lines 2015 to 2018 in 9dc0f1f

@singledispatch
def to_serializable(val):
"""Used by default."""
return str(val)

This is just the same as

json.dumps(str(set(["./a.json"])))

The issue is created by

pytm/pytm/pytm.py

Line 2023 in 9dc0f1f

return serialize(obj, nested=True)

Because of this check for not nested in serialize().

pytm/pytm/pytm.py

Line 2064 in 9dc0f1f

not nested

pytm/pytm/pytm.py

Lines 2035 to 2070 in 9dc0f1f

def serialize(obj, nested=False):
"""Used if *obj* is an instance of TM, Element, Threat or Finding."""
klass = obj.__class__
result = {}
if isinstance(obj, (Actor, Asset)):
result["__class__"] = klass.__name__
for i in dir(obj):
if (
i.startswith("__")
or callable(getattr(klass, i, {}))
or (
isinstance(obj, TM)
and i in ("_sf", "_duplicate_ignored_attrs", "_threats")
)
or (isinstance(obj, Element) and i in ("_is_drawn", "uuid"))
or (isinstance(obj, Finding) and i == "element")
):
continue
value = getattr(obj, i)
if isinstance(obj, TM) and i == "_elements":
value = [e for e in value if isinstance(e, (Actor, Asset))]
if value is not None:
if isinstance(value, (Element, Data)):
value = value.name
elif isinstance(obj, Threat) and i == "target":
value = [v.__name__ for v in value]
elif i in ("levels", "sourceFiles", "assumptions"):
value = list(value)
elif (
not nested
and not isinstance(value, str)
and isinstance(value, Iterable)
):
value = [v.id if isinstance(v, Finding) else v.name for v in value]
result[i.lstrip("_")] = value
return result

This whole serialize function is a bit overloaded with special handling of member variables and might require a rewrite.
All the checks seem to be for specific classes which are all handle in the same function.
Also why is there the check for nested?
This is basically equivalent to checking if the class is TM.

A quick fix would be something like if instance(obj, TM) and i == "threatsFile" but oh boy that is not nice.

Should this be a new issue?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sounds like something to be addressed. @nineinchnick , you there?

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.

2 participants