Multi threat libs #263

izar · 2025-02-20T20:01:23Z

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

…files

raphaelahrens · 2025-02-21T10:14:25Z

tests/output.json

@@ -1405,5 +1405,5 @@
    "name": "my test tm",
    "onDuplicates": "Action.NO_ACTION",
    "threatsExcluded": [],
-    "threatsFile": "pytm/threatlib/threats.json"
+    "threatsFile": "{'pytm/threatlib/threats.json'}"


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

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

Is this a bug?

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

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?

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

There appears to be only 1 check for nested. If nested is true, the behavior seems to be potentially undefined (if the code reaches the not nested elif).

Yes. I started to rewrite the code see #268, but it is difficult to understand what the intention here was.
It seems that the default is result[i.lstrip("_")] = value and the cases above only change the value of value.
So setting nested = True means that value will not be touched, except it is a Element, Data, or Threat. Or the name of the member is in ("levels", "sourceFiles", "assumptions").

The last one seems to be a fix for a specific class.

pytm/pytm.py

.gitignore

colesmj · 2025-03-28T17:04:55Z

README.md

-             [--exclude EXCLUDE] [--seq] [--list] [--describe DESCRIBE]
-             [--list-elements] [--json JSON] [--levels LEVELS [LEVELS ...]]
-             [--stale_days STALE_DAYS]
+usage: tm.py [-h] [--sqldump SQLDUMP] [--debug] [--dfd] [--report REPORT] [--exclude EXCLUDE] [--seq] [--list] [--colormap]


Are you adding --colormap in this PR also?

it should be there already, it is an old one. It appears on the currently checked code on the master branch.

oh this is the readme! let me fix that

pytm/pytm.py

colesmj · 2025-03-28T17:29:38Z

pytm/pytm.py

@@ -1129,6 +1129,21 @@ def _process(self):
        result = get_args()
        logging.basicConfig(level=logging.INFO, format="%(levelname)s: %(message)s")

+        # delaying loading of threats to accomodate multiple threat files in the
+        # command line
+        if result.threat_files:


Why do extra work on line 791 to load the default when you can just do it here? You will always reach this line at least once, correct?

i don't remember the details but there is a timing issue there. Something around things being used before loaded.

colesmj · 2025-03-28T17:33:40Z

tests/output.json

@@ -1405,5 +1405,5 @@
    "name": "my test tm",
    "onDuplicates": "Action.NO_ACTION",
    "threatsExcluded": [],
-    "threatsFile": "pytm/threatlib/threats.json"
+    "threatsFile": "{'pytm/threatlib/threats.json'}"


There appears to be only 1 check for nested. If nested is true, the behavior seems to be potentially undefined (if the code reaches the not nested elif).

colesmj · 2025-03-28T17:39:12Z

tests/test_pytmfunc.py

+        foo_file = f"{dir_path}/1.json"
+        bar_file = f"{dir_path}/2.json"
+        threat_files = [os.path.abspath(os.path.join(os.path.dirname(__file__), ".."))
+    + "/pytm/threatlib/threats.json", foo_file, bar_file]


Do you have a test for when the default is not included in threatsfile?

Hm. No. Gotta do one, you're right.

izar tarandach added 5 commits February 12, 2025 14:55

feat: Support multiple threat files in threat management system

e0b5c1a

fix: Correct threatsFile initialization to accept a list of strings

7be52ea

fix: Correct file opening in _add_threats to iterate over threat files

370ccb6

test: Update threat count assertion and add test for multiple threat …

2ec73a6

…files

Adding support for multi threat libraries following issue #261

544fa6f

izar requested a review from colesmj February 20, 2025 20:01

oops, these are needed for the tests

6da8601

raphaelahrens reviewed Feb 21, 2025

View reviewed changes

pytm/pytm.py Outdated Show resolved Hide resolved

izar tarandach and others added 2 commits February 21, 2025 08:55

better have some documentation as well...

b546efb

fixed wrong file in error message (thanks @raphaelahrens!)

9dc0f1f

raphaelahrens mentioned this pull request Mar 22, 2025

Fix to_serializable #268

Open

colesmj reviewed Mar 28, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multi threat libs #263

Multi threat libs #263

izar commented Feb 20, 2025

raphaelahrens Feb 21, 2025

izar Feb 21, 2025

raphaelahrens Feb 21, 2025

izar Feb 21, 2025

raphaelahrens Mar 11, 2025

izar Mar 21, 2025

colesmj Mar 28, 2025

raphaelahrens Mar 28, 2025

colesmj Mar 28, 2025

izar Apr 10, 2025

izar Apr 10, 2025

colesmj Mar 28, 2025

izar Apr 10, 2025

colesmj Mar 28, 2025

colesmj Mar 28, 2025

izar Apr 10, 2025

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

	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

Multi threat libs #263

Are you sure you want to change the base?

Multi threat libs #263

Conversation

izar commented Feb 20, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment