Whitelist lines ending in # nosec #121

omergunal · 2018-04-22T10:56:49Z

Issue #108
I built nosec_lines but im not sure how to send to find_triggers and find_vulnerabilities

Added args and nosec_lines

KevinHock · 2018-04-22T20:52:46Z

You can pass in nosec_lines to https://github.com/omergunal/pyt/blob/4cd3ea596e2ef0c834e206938af6963bbd08130d/pyt/__main__.py#L304 and https://github.com/omergunal/pyt/blob/4cd3ea596e2ef0c834e206938af6963bbd08130d/pyt/__main__.py#L246, you'll just have to move it up in the file.

omergunal · 2018-04-23T09:48:59Z

Can you check main.py and vulnerabilities.py ?

KevinHock · 2018-04-22T20:45:12Z

pyt/__main__.py

+        nosec_lines = set()
+    else:
+        file = open(path, "r")
+        lines = file.readlines()


Nice, I like this even more than .read()->.splitlines.

KevinHock · 2018-04-22T20:46:02Z

pyt/__main__.py

@@ -142,6 +142,9 @@ def parse_args(args):
                             '(only JSON-formatted files are accepted)',
                        type=str,
                        default=False)
+    parser.add_argument('-in', '--ignore-nosec',


I kind of like how Bandit does this a little more https://github.com/openstack/bandit/blob/master/bandit/cli/main.py#L230

i.e.

parser.add_argument( '--ignore-nosec', dest='ignore_nosec', action='store_true', help='do not skip lines with # nosec comments' )

looks really nice.

KevinHock · 2018-04-24T01:31:00Z

pyt/vulnerabilities.py

-        trigger_nodes.extend(iter(label_contains(node, trigger_words)))
+        if node.line_number not in nosec_lines:
+            trigger_nodes.extend(iter(label_contains(node, trigger_words)))
+        else:


The else: pass isn't needed

KevinHock · 2018-04-24T01:32:17Z

pyt/vulnerabilities.py

    with open(vulnerability_files.blackbox_mapping) as infile:
        blackbox_mapping = json.load(infile)
    for cfg in cfg_list:
+


Nit: I kind of liked how the newlines were in this function.

KevinHock · 2018-04-24T01:37:31Z

pyt/__main__.py

@@ -298,15 +301,38 @@ def main(command_line_args=sys.argv[1:]):

    analyse(cfg_list, analysis_type=analysis)

+    nosec_lines = set()


Can you move this code

if args.ignore_nosec: nosec_lines = set() else: file = open(path, "r") lines = file.readlines() nosec_lines = set( lineno for (lineno, line) in enumerate(lines, start=1) if '#nosec' in line or '# nosec' in line)

to near the top, so we take into account nosec_lines at both call-sites to find_vulnerabilities.
https://github.com/omergunal/pyt/blob/fb88051e1d988d5890127ef3aa6867adf0db07de/pyt/__main__.py#L240
is a good spot, the same way UImode is set once and then passed to both call-sites https://github.com/python-security/pyt/blob/master/pyt/__main__.py#L232-L236

edited argument, moved code place

omergunal · 2018-04-24T21:26:50Z

pyt/__main__.py

    cfg_list = list()
+    if args.ignore_nosec:


i moved the code this place and deleted nosec_lines = set() .Because its already created in if-else statement.

omergunal · 2018-04-24T21:27:25Z

pyt/__main__.py

@@ -235,7 +238,28 @@ def main(command_line_args=sys.argv[1:]):
    elif args.trim_reassigned_in:
        ui_mode = UImode.TRIM

+    path = os.path.normpath(args.filepath) 


Its necessary for learning filename. i moved to above the ignore_nosec argument

omergunal · 2018-04-24T21:44:27Z

@KevinHock can you check it again?

KevinHock · 2018-04-25T19:32:26Z

So there are just a few things left, vulnerabilities.py looks good.

Can you pass nosec_lines to analyse_repo and then also to the call to find_vulnerabilities inside of that function?
Can you delete the find_vulnerabilities call in the else statement of if args.nosec_lines?

unnecessary codes deleted, passed nosec_lines on analyse_repo

omergunal · 2018-04-25T22:13:49Z

Like that?

KevinHock

LGTM

KevinHock · 2018-04-25T22:44:11Z

Yup, shall I merge? 😀

omergunal · 2018-04-25T22:46:49Z

Why not :)

KevinHock · 2018-04-25T22:56:26Z

Oh whoops, it seems like tests are failing https://travis-ci.org/python-security/pyt/builds/371295436?utm_source=github_status&utm_medium=notification
Can you make nosec_lines default to being an empty set e.g. nosec_lines=set()? You can also delete the test_no_args (tests.command_line_test.CommandLineTest) as it's annoying lol

Added empty nosec_lines for tests

omergunal · 2018-04-26T15:59:11Z

Looks like ok now.

KevinHock · 2018-04-26T17:10:40Z

Sorry by Can you make nosec_lines default to being an empty set e.g. nosec_lines=set()? I meant

Before:

def find_vulnerabilities(
    cfg_list,
    analysis_type,
    ui_mode,
    vulnerability_files,
    nosec_lines
):

After:

def find_vulnerabilities(
    cfg_list,
    analysis_type,
    ui_mode,
    vulnerability_files,
    nosec_lines=set()
):

So that you won't have to manually pass an empty set to every find_vulnerabilities call, it makes things cleaner.

omergunal · 2018-04-26T17:56:30Z

You are right, my bad :)
I changed it

omergunal · 2018-04-28T08:10:42Z

I think its ok now.

KevinHock

Almost there 👍

KevinHock · 2018-04-28T18:13:12Z

pyt/vulnerabilities.py

@@ -170,7 +171,8 @@ def append_node_if_reassigned(

 def find_triggers(
    nodes,
-    trigger_words
+    trigger_words,
+    nosec_lines = set()


Can you please do nosec_lines=set() instead of nosec_lines = set()? I thought find_vulnerabilities was the only function that needed this, but you're right since we do call find_triggers once from a test here

pyt/tests/vulnerabilities_test.py

Line 96 in a3b9951

l = vulnerabilities.find_triggers(XSS1.nodes, trigger_words)

maybe since we just call it once from tests we can pass in the set in that test.

You can see some of these in the https://codeclimate.com/github/python-security/pyt/pull/121 output.

KevinHock · 2018-04-28T18:14:46Z

pyt/vulnerabilities.py

@@ -466,7 +469,8 @@ def find_vulnerabilities_in_cfg(
    lattice,
    ui_mode,
    blackbox_mapping,
-    vulnerabilities_list
+    vulnerabilities_list,
+    nosec_lines = set()


So this shouldn't be necessary, (to make it default to empty set), we can just leave it as nosec_lines. This is b/c we only call find_vulnerabilities from tests, not this function.

KevinHock · 2018-04-28T18:16:00Z

tests/vulnerabilities_across_files_test.py

@@ -15,7 +15,6 @@
 from pyt.reaching_definitions_taint import ReachingDefinitionsTaintAnalysis
 from pyt.vulnerabilities import find_vulnerabilities

-


We normally leave 2 lines between the imports and other code.

KevinHock · 2018-04-28T18:33:57Z

tests/vulnerabilities_test.py

@@ -23,7 +23,6 @@
 from pyt.node_types import Node
 from pyt.reaching_definitions_taint import ReachingDefinitionsTaintAnalysis

-


Same here, sorry.

KevinHock · 2018-04-28T18:34:56Z

pyt/vulnerabilities.py

@@ -73,7 +73,8 @@ def identify_triggers(
    cfg,
    sources,
    sinks,
-    lattice
+    lattice,
+    nosec_lines=set()


So identify_triggers( is never called in tests, so I don't think a default arg is needed.

KevinHock

LGTM

KevinHock · 2018-04-28T18:38:00Z

Thanks @omergunal!

omergunal · 2018-04-28T18:39:29Z

You are welcome 👍

nosec_lines for python-security#108's issue

4cd3ea5

Added args and nosec_lines

omergunal added 3 commits April 23, 2018 12:33

nosec_lines

ae9e8e9

added nosec_lines parameter

1735db7

Update vulnerabilities.py

fb88051

KevinHock self-requested a review April 23, 2018 20:34

KevinHock requested changes Apr 24, 2018

View reviewed changes

omergunal added 2 commits April 25, 2018 00:21

Code edited

d0e8ef0

edited argument, moved code place

unnecessary codes deleted

8569aa4

omergunal commented Apr 24, 2018

View reviewed changes

passed nosec_lines on analyse_repo

e8cddd0

unnecessary codes deleted, passed nosec_lines on analyse_repo

KevinHock approved these changes Apr 25, 2018

View reviewed changes

omergunal added 5 commits April 26, 2018 18:40

Added nosec_lines

3d5867b

Added empty nosec_lines for tests

added empty nosec lines for tests

1502ee7

added nosec_lines

bed2f77

added empty nosec_lines for tests

69d0193

Added ignore-nosec argument

3cb5186

omergunal added 3 commits April 26, 2018 20:51

Update vulnerabilities_test.py

ec6d23a

unnecessary codes removed

6f09912

added default nosec_lines

9c4dea6

omergunal added 2 commits April 27, 2018 11:40

Update vulnerabilities.py

f4ebbff

Update vulnerabilities.py

7c872a0

KevinHock requested changes Apr 28, 2018

View reviewed changes

omergunal added 3 commits April 28, 2018 21:26

removed spaces

b943310

new line between imports and codes

175c235

Update __main__.py

c79161c

KevinHock reviewed Apr 28, 2018

View reviewed changes

omergunal added 2 commits April 28, 2018 21:35

new line between imports and codes

ed13514

removed set() from nosec_lines

0928700

KevinHock approved these changes Apr 28, 2018

View reviewed changes

KevinHock merged commit 76379d4 into python-security:master Apr 28, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Whitelist lines ending in # nosec #121

Whitelist lines ending in # nosec #121

omergunal commented Apr 22, 2018

KevinHock commented Apr 22, 2018

omergunal commented Apr 23, 2018

KevinHock Apr 22, 2018

KevinHock Apr 22, 2018

KevinHock Apr 24, 2018

KevinHock Apr 24, 2018

KevinHock Apr 24, 2018

KevinHock Apr 24, 2018

omergunal Apr 24, 2018

omergunal Apr 24, 2018 •

edited

Loading

omergunal commented Apr 24, 2018

KevinHock commented Apr 25, 2018

omergunal commented Apr 25, 2018

KevinHock left a comment

KevinHock commented Apr 25, 2018

omergunal commented Apr 25, 2018

KevinHock commented Apr 25, 2018 •

edited

Loading

omergunal commented Apr 26, 2018

KevinHock commented Apr 26, 2018 •

edited

Loading

omergunal commented Apr 26, 2018 •

edited

Loading

omergunal commented Apr 28, 2018

KevinHock left a comment

KevinHock Apr 28, 2018

KevinHock Apr 28, 2018

KevinHock Apr 28, 2018

KevinHock Apr 28, 2018

KevinHock Apr 28, 2018

KevinHock Apr 28, 2018

KevinHock left a comment

KevinHock commented Apr 28, 2018

omergunal commented Apr 28, 2018

		@@ -298,15 +301,38 @@ def main(command_line_args=sys.argv[1:]):

		analyse(cfg_list, analysis_type=analysis)

		nosec_lines = set()

		@@ -15,7 +15,6 @@
		from pyt.reaching_definitions_taint import ReachingDefinitionsTaintAnalysis
		from pyt.vulnerabilities import find_vulnerabilities

		@@ -23,7 +23,6 @@
		from pyt.node_types import Node
		from pyt.reaching_definitions_taint import ReachingDefinitionsTaintAnalysis

Whitelist lines ending in # nosec #121

Whitelist lines ending in # nosec #121

Conversation

omergunal commented Apr 22, 2018

KevinHock commented Apr 22, 2018

omergunal commented Apr 23, 2018

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

omergunal Apr 24, 2018 • edited Loading

Choose a reason for hiding this comment

omergunal commented Apr 24, 2018

KevinHock commented Apr 25, 2018

omergunal commented Apr 25, 2018

KevinHock left a comment

Choose a reason for hiding this comment

KevinHock commented Apr 25, 2018

omergunal commented Apr 25, 2018

KevinHock commented Apr 25, 2018 • edited Loading

omergunal commented Apr 26, 2018

KevinHock commented Apr 26, 2018 • edited Loading

omergunal commented Apr 26, 2018 • edited Loading

omergunal commented Apr 28, 2018

KevinHock left a 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

KevinHock left a comment

Choose a reason for hiding this comment

KevinHock commented Apr 28, 2018

omergunal commented Apr 28, 2018

omergunal Apr 24, 2018 •

edited

Loading

KevinHock commented Apr 25, 2018 •

edited

Loading

KevinHock commented Apr 26, 2018 •

edited

Loading

omergunal commented Apr 26, 2018 •

edited

Loading