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

Add feature to define findings manually in the model, similar to over… #180

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

Add feature to define findings manually in the model, similar to over… #180

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
82bc106
Add feature to define findings manually in the model, similar to over…
nozmore-vera Oct 2, 2021
9e0fce5
rename from 'custom' to 'manual' findings.
nozmore-vera Oct 5, 2021
1eaeded
Added source element to Findings, merged manual findings into main an…
nozmore-vera Oct 5, 2021
1640899
Removed duplicate element reference when assigning a manual finding t…
nozmore-vera Oct 5, 2021
389ee11
remove manual_findings attribute and re-use findings. Updated sample …
nozmore-vera Oct 5, 2021
05f6dfd
review comments. In addition to supporting Findings added to an eleme…
nozmore-vera Oct 5, 2021
34959b6
Code cleanup, removed unnecessary condition.
nozmore-vera Oct 12, 2021
f577e66
Merge branch 'master' into custom_findings
nozmore-vera Oct 31, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 64 additions & 14 deletions docs/pytm/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
<head>
<meta charset="utf-8">
<meta name="viewport" content="width=device-width, initial-scale=1, minimum-scale=1" />
<meta name="generator" content="pdoc 0.9.2" />
<meta name="generator" content="pdoc 0.10.0" />
<title>pytm API documentation</title>
<meta name="description" content="" />
<link rel="preload stylesheet" as="style" href="https://cdnjs.cloudflare.com/ajax/libs/10up-sanitize.css/11.0.1/sanitize.min.css" integrity="sha256-PK9q560IAAa6WVRRh76LtCaI8pjTJ2z11v0miyNNjrs=" crossorigin>
Expand Down Expand Up @@ -2134,17 +2134,17 @@ <h3>Instance variables</h3>
&#34;&#34;&#34;Represents a Finding - the element in question
and a description of the finding&#34;&#34;&#34;

element = varElement(None, required=True, doc=&#34;Element this finding applies to&#34;)
element = varElement(None, required=False, doc=&#34;Element this finding applies to&#34;)
target = varString(&#34;&#34;, doc=&#34;Name of the element this finding applies to&#34;)
description = varString(&#34;&#34;, required=True, doc=&#34;Threat description&#34;)
details = varString(&#34;&#34;, required=True, doc=&#34;Threat details&#34;)
severity = varString(&#34;&#34;, required=True, doc=&#34;Threat severity&#34;)
mitigations = varString(&#34;&#34;, required=True, doc=&#34;Threat mitigations&#34;)
example = varString(&#34;&#34;, required=True, doc=&#34;Threat example&#34;)
severity = varString(&#34;&#34;, required=False, doc=&#34;Threat severity&#34;)
mitigations = varString(&#34;&#34;, required=False, doc=&#34;Threat mitigations&#34;)
example = varString(&#34;&#34;, required=False, doc=&#34;Threat example&#34;)
id = varString(&#34;&#34;, required=True, doc=&#34;Finding ID&#34;)
threat_id = varString(&#34;&#34;, required=True, doc=&#34;Threat ID&#34;)
references = varString(&#34;&#34;, required=True, doc=&#34;Threat references&#34;)
condition = varString(&#34;&#34;, required=True, doc=&#34;Threat condition&#34;)
threat_id = varString(&#34;&#34;, required=False, doc=&#34;Threat ID&#34;)
references = varString(&#34;&#34;, required=False, doc=&#34;Threat references&#34;)
condition = varString(&#34;&#34;, required=False, doc=&#34;Threat condition&#34;)
response = varString(
&#34;&#34;,
required=False,
Expand All @@ -2157,6 +2157,7 @@ <h3>Instance variables</h3>
&#34;&#34;&#34;,
)
cvss = varString(&#34;&#34;, required=False, doc=&#34;The CVSS score and/or vector&#34;)
source = varString(&#34;manual&#34;, required=False, doc=&#34;The source of the Finding.&#34;)

def __init__(
self,
Expand Down Expand Up @@ -2218,7 +2219,7 @@ <h3>Instance variables</h3>
)

def __str__(self):
return f&#34;{self.target}: {self.description}\n{self.details}\n{self.severity}&#34;</code></pre>
return f&#34;&#39;{self.target}&#39;: {self.description}\n{self.details}\n{self.severity}&#34;</code></pre>
</details>
<h3>Instance variables</h3>
<dl>
Expand Down Expand Up @@ -2403,6 +2404,22 @@ <h3>Instance variables</h3>
return self.data.get(instance, self.default)</code></pre>
</details>
</dd>
<dt id="pytm.Finding.source"><code class="name">var <span class="ident">source</span></code></dt>
<dd>
<div class="desc"><p>The source of the Finding.</p></div>
<details class="source">
<summary>
<span>Expand source code</span>
</summary>
<pre><code class="python">def __get__(self, instance, owner):
# when x.d is called we get here
# instance = x
# owner = type(x)
if instance is None:
return self
return self.data.get(instance, self.default)</code></pre>
</details>
</dd>
<dt id="pytm.Finding.target"><code class="name">var <span class="ident">target</span></code></dt>
<dd>
<div class="desc"><p>Name of the element this finding applies to</p></div>
Expand Down Expand Up @@ -3587,6 +3604,16 @@ <h3>Class variables</h3>
if not e.inScope:
continue

if (len(e.findings) &gt; 0):

for f in e.findings:
finding_count += 1
f._safeset(&#34;id&#34;, str(finding_count))
f._safeset(&#34;element&#34;, e)
f._safeset(&#34;target&#34;, e.name)
findings.append(f)
elements[e].append(f)

override_ids = set(f.threat_id for f in e.overrides)
# if element is a dataflow filter out overrides from source and sink
# because they will be always applied there anyway
Expand All @@ -3602,12 +3629,15 @@ <h3>Class variables</h3>
continue

finding_count += 1
f = Finding(e, id=str(finding_count), threat=t)
f = Finding(e, id=str(finding_count), threat=t, source=&#34;pytm&#34;)
logger.debug(f&#34;new finding: {f}&#34;)
findings.append(f)
elements[e].append(f)

self.findings = findings
for e, findings in elements.items():
e.findings = findings
e._safeset(&#34;findings&#34;, findings)
#e.findings = findings

def check(self):
if self.description is None:
Expand Down Expand Up @@ -3839,6 +3869,9 @@ <h3>Class variables</h3>
if result.describe is not None:
_describe_classes(result.describe.split())

if result.list_elements:
_list_elements()

if result.list is True:
[print(&#34;{} - {}&#34;.format(t.id, t.description)) for t in TM._threats]

Expand Down Expand Up @@ -4160,6 +4193,9 @@ <h3>Methods</h3>
if result.describe is not None:
_describe_classes(result.describe.split())

if result.list_elements:
_list_elements()

if result.list is True:
[print(&#34;{} - {}&#34;.format(t.id, t.description)) for t in TM._threats]

Expand Down Expand Up @@ -4215,6 +4251,16 @@ <h3>Methods</h3>
if not e.inScope:
continue

if (len(e.findings) &gt; 0):

for f in e.findings:
finding_count += 1
f._safeset(&#34;id&#34;, str(finding_count))
f._safeset(&#34;element&#34;, e)
f._safeset(&#34;target&#34;, e.name)
findings.append(f)
elements[e].append(f)

override_ids = set(f.threat_id for f in e.overrides)
# if element is a dataflow filter out overrides from source and sink
# because they will be always applied there anyway
Expand All @@ -4230,12 +4276,15 @@ <h3>Methods</h3>
continue

finding_count += 1
f = Finding(e, id=str(finding_count), threat=t)
f = Finding(e, id=str(finding_count), threat=t, source=&#34;pytm&#34;)
logger.debug(f&#34;new finding: {f}&#34;)
findings.append(f)
elements[e].append(f)

self.findings = findings
for e, findings in elements.items():
e.findings = findings</code></pre>
e._safeset(&#34;findings&#34;, findings)
#e.findings = findings</code></pre>
</details>
</dd>
<dt id="pytm.TM.sqlDump"><code class="name flex">
Expand Down Expand Up @@ -4673,6 +4722,7 @@ <h4><code><a title="pytm.Finding" href="#pytm.Finding">Finding</a></code></h4>
<li><code><a title="pytm.Finding.references" href="#pytm.Finding.references">references</a></code></li>
<li><code><a title="pytm.Finding.response" href="#pytm.Finding.response">response</a></code></li>
<li><code><a title="pytm.Finding.severity" href="#pytm.Finding.severity">severity</a></code></li>
<li><code><a title="pytm.Finding.source" href="#pytm.Finding.source">source</a></code></li>
<li><code><a title="pytm.Finding.target" href="#pytm.Finding.target">target</a></code></li>
<li><code><a title="pytm.Finding.threat_id" href="#pytm.Finding.threat_id">threat_id</a></code></li>
</ul>
Expand Down Expand Up @@ -4805,7 +4855,7 @@ <h4><code><a title="pytm.Threat" href="#pytm.Threat">Threat</a></code></h4>
</nav>
</main>
<footer id="footer">
<p>Generated by <a href="https://pdoc3.github.io/pdoc"><cite>pdoc</cite> 0.9.2</a>.</p>
<p>Generated by <a href="https://pdoc3.github.io/pdoc" title="pdoc: Python API documentation generator"><cite>pdoc</cite> 0.10.0</a>.</p>
</footer>
</body>
</html>
3 changes: 3 additions & 0 deletions docs/template.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,11 @@ Name|Description|Classification
<p>{{item.mitigations}}</p>
<h6>References</h6>
<p>{{item.references}}</p>
<h6>Source</h6>
<p>{{item.source}}</p>
&nbsp;
&nbsp;
&emsp;
</details>
}|

45 changes: 32 additions & 13 deletions pytm/pytm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,12 +59,18 @@ def __set__(self, instance, value):
# called when x.d = val
# instance = x
# value = val

if instance in self.data:
raise ValueError(
"cannot overwrite {}.{} value with {}, already set to {}".format(
instance, self.__class__.__name__, value, self.data[instance]
if (not isinstance(instance, Finding)
or (isinstance(instance, Finding) and isinstance(self,varString) and self.data[instance] != "invalid")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm against using special values like "invalid". Looks like we should not use this var* type for Findings? What's exactly the purpose of these conditions?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

invalid was already there, I didn't remove it. It set when a Finding is created without and element, I dont think it ever happened before because the element was required but actually gets used now. The conditions are used to workaround the 'overwrite' protection. I wanted to narrow down the specific occurances where overwrite would be allowed and is only for Finding's element and target attributes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we can avoid setting the findings two times? Can you locate the place where it's being set the second time?

Copy link
Collaborator Author

@nozmore nozmore Oct 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

ok, cleaned things up. Instead of using setting the value to 'Element("invalid")' I set it to None and avoided the double set on the findings element and target attributes.

or (isinstance(instance, Finding) and isinstance(self,varElement) and self.data[instance].name != "invalid")
):
raise ValueError(
"cannot overwrite {}.{} value with {}, already set to {}".format(
instance, self.__class__.__name__, value, self.data[instance]
)
)
)

self.data[instance] = value
if self.onSet is not None:
self.onSet(instance, value)
Expand Down Expand Up @@ -603,17 +609,17 @@ class Finding:
"""Represents a Finding - the element in question
and a description of the finding"""

element = varElement(None, required=True, doc="Element this finding applies to")
element = varElement(None, required=False, doc="Element this finding applies to")
target = varString("", doc="Name of the element this finding applies to")
description = varString("", required=True, doc="Threat description")
details = varString("", required=True, doc="Threat details")
severity = varString("", required=True, doc="Threat severity")
mitigations = varString("", required=True, doc="Threat mitigations")
example = varString("", required=True, doc="Threat example")
severity = varString("", required=False, doc="Threat severity")
mitigations = varString("", required=False, doc="Threat mitigations")
example = varString("", required=False, doc="Threat example")
id = varString("", required=True, doc="Finding ID")
threat_id = varString("", required=True, doc="Threat ID")
references = varString("", required=True, doc="Threat references")
condition = varString("", required=True, doc="Threat condition")
threat_id = varString("", required=False, doc="Threat ID")
references = varString("", required=False, doc="Threat references")
condition = varString("", required=False, doc="Threat condition")
response = varString(
"",
required=False,
Expand All @@ -626,6 +632,7 @@ class Finding:
""",
)
cvss = varString("", required=False, doc="The CVSS score and/or vector")
source = varString("manual", required=False, doc="The source of the Finding.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

The name source on its own is too vague. Can the doc mention that it's automatically populated with pytm for findings matched from the threat library? It'll be the second possible value next to the default value, and it would help users to figure out the meaning of this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure. I'll add some better text. I could remove the logic around source altogether and we just add it to every element in the threatlib.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's fine to have that logic, it's better to have this field only in Finding than both in here and in Threat


def __init__(
self,
Expand Down Expand Up @@ -759,6 +766,16 @@ def resolve(self):
if not e.inScope:
continue

if (len(e.findings) > 0):

for f in e.findings:
finding_count += 1
f._safeset("id", str(finding_count))
f._safeset("element", e)
f._safeset("target", e.name)
findings.append(f)
elements[e].append(f)

override_ids = set(f.threat_id for f in e.overrides)
# if element is a dataflow filter out overrides from source and sink
# because they will be always applied there anyway
Expand All @@ -774,13 +791,14 @@ def resolve(self):
continue

finding_count += 1
f = Finding(e, id=str(finding_count), threat=t)
f = Finding(e, id=str(finding_count), threat=t, source="pytm")
logger.debug(f"new finding: {f}")
findings.append(f)
elements[e].append(f)

self.findings = findings
for e, findings in elements.items():
e.findings = findings
e._safeset("findings", findings)

def check(self):
if self.description is None:
Expand Down Expand Up @@ -1854,6 +1872,7 @@ def encode_threat_data(obj):
"threat_id",
"references",
"condition",
"source",
]

if type(obj) is Finding or (len(obj) != 0 and type(obj[0]) is Finding):
Expand Down
2 changes: 1 addition & 1 deletion tests/output.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -801,4 +801,4 @@
"onDuplicates": "Action.NO_ACTION",
"threatsExcluded": [],
"threatsFile": "pytm/threatlib/threats.json"
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please don't remove end of line at end of file. Configure your text editor not to do that automatically.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I replaced the json using the produced json during the tests. I'm editing in vi so nothing is removed automatically on my end. If the space is important we could look to add it to the generated file.

32 changes: 32 additions & 0 deletions tm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
Datastore,
Lambda,
Server,
Finding,
)

tm = TM("my test tm")
Expand All @@ -33,6 +34,13 @@
web.encodesOutput = True
web.authorizesSource = False
web.sourceFiles = ["pytm/json.py", "docs/template.md"]
web.findings = [
Finding(
description = "foo_description_3",
details = "foo_details",
),
]


db = Datastore("SQL Database")
db.OS = "CentOS"
Expand Down Expand Up @@ -108,6 +116,30 @@
my_lambda_to_db.protocol = "MySQL"
my_lambda_to_db.dstPort = 3306
my_lambda_to_db.data = clear_op
my_lambda_to_db.findings = [
Finding(
description = "foo_description_1",
details = "foo_details",
severity = "HIGH",
mitigations = "foo_mitigations",
example = "foo_example",
threat_id = "foo_threat_id_1",
references = "foo_references",
response = "accepted",
),
Finding(
description = "foo_description_2",
details = "foo_details",
severity = "HIGH",
mitigations = "foo_mitigations",
example = "foo_example",
threat_id = "foo_threat_id_2",
references = "foo_references",
response = "accepted",
source = "Product Security"
)

]

userIdToken = Data(
name="User ID Token",
Expand Down