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

-r Recursive option #129

Merged
merged 29 commits into from
Jun 23, 2018
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
2ebe595
new args
omergunal May 10, 2018
ef3a21d
added recursive args
omergunal May 11, 2018
38be6e2
Update __main__.py
omergunal May 11, 2018
759f632
Update __main__.py
omergunal May 11, 2018
ed38dbb
Created discover_files() function
omergunal May 12, 2018
fcf4638
Merge branch 'master' into patch-4
omergunal Jun 6, 2018
3ac883c
added recursive option
omergunal Jun 6, 2018
e246104
discover_files
omergunal Jun 6, 2018
2cbac72
added recursive, targets
omergunal Jun 7, 2018
7875c82
update discover_files()
omergunal Jun 7, 2018
ca0b2d7
removed file_list
omergunal Jun 7, 2018
d9db9dd
"targets" must be required
omergunal Jun 10, 2018
c35ae81
created loop for discover_files()
omergunal Jun 10, 2018
9c54d8c
new params
omergunal Jun 10, 2018
40c0f8f
Update __main__.py
omergunal Jun 16, 2018
42759f0
Update __main__.py
omergunal Jun 16, 2018
5931faf
Merge branch 'master' into patch-4
omergunal Jun 16, 2018
5546c3d
changed func. and added baseline
omergunal Jun 19, 2018
8d1d805
new parameters for discover_files
omergunal Jun 19, 2018
35b8001
test_valid_args_but_no_targets()
omergunal Jun 20, 2018
2e4d07a
edited expected values
omergunal Jun 20, 2018
0c6b082
changed vulnerabilities list location
omergunal Jun 20, 2018
ae84a44
Update usage_test.py
omergunal Jun 20, 2018
1944b4a
Update usage_test.py
omergunal Jun 20, 2018
ba3d438
changed location of "recursive control"
omergunal Jun 21, 2018
6a25e25
Update usage.py
omergunal Jun 22, 2018
f42d283
de-dent some lines
omergunal Jun 22, 2018
c7b2f73
test_no_args
omergunal Jun 22, 2018
2afc177
test_no_args passed
omergunal Jun 22, 2018
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
134 changes: 80 additions & 54 deletions pyt/__main__.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,25 @@
)


def main(command_line_args=sys.argv[1:]): # noqa: C901
def discover_files(targets, excluded_files, recursive=False):
included_files = list()
excluded_list = excluded_files.split(",")

for target in targets:
if os.path.isdir(target):
if recursive:
Copy link
Collaborator

Choose a reason for hiding this comment

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

So having if recursive: here it will make it so that if you don't have -r then you won't search directories.

You can change it to:

def discover_files(targets, excluded_files, recursive=False):
    included_files = list()
    excluded_list = excluded_files.split(",")

    for target in targets:
        if os.path.isdir(target):
            for root, dirs, files in os.walk(target):
                for f in files:
                    fullpath = os.path.join(root, f)
                    if os.path.splitext(fullpath)[1] == '.py' and fullpath.split("/")[-1] not in excluded_list:
                        included_files.append(fullpath)
                if not recursive:
                    break
        else:
            if target not in excluded_list:
                included_files.append(target)
    return included_files

for root, dirs, files in os.walk(target):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this line, for root, dirs, files in os.walk(target): is indented one more level than it has to be.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i will try to do better for returning "included_files"

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: You can de-dent from line 38 to line 44

for f in files:
fullpath = os.path.join(root, f)
if os.path.splitext(fullpath)[1] == '.py' and fullpath.split("/")[-1] not in excluded_list:
included_files.append(fullpath)
else:
if target not in excluded_list:
included_files.append(targets[0])
Copy link
Collaborator

Choose a reason for hiding this comment

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

So if targets is a list of files, e.g. python -m pyt examples/vulnerable_code/command_injection.py examples/vulnerable_code/XSS.py, then discover_files will return the first file N times. (Where N is the len of targets.)

return included_files


def main(command_line_args=sys.argv[1:]):
args = parse_args(command_line_args)

ui_mode = UImode.NORMAL
Expand All @@ -39,66 +57,74 @@ def main(command_line_args=sys.argv[1:]): # noqa: C901
elif args.trim_reassigned_in:
ui_mode = UImode.TRIM

path = os.path.normpath(args.filepath)
files = discover_files(
args.targets,
args.excluded_paths,
args.recursive
)
vulnerabilities = list()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

i created list before loop as you said. the same problem about last item is still continue.
a

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, That's odd, I'll checkout/test your code later today to try and find the issue. 👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's really weird, I'll look more in-depth on Monday 👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

I checked out your branch and it was append vs. extend

for path in files:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Before this for loop, you can have a vulnerabilities = list(), and then do

vulnerabilities.append(find_vulnerabilities(
    cfg_list,
    ui_mode,
    args.blackbox_mapping_file,
    args.trigger_word_file,
    nosec_lines
))

Copy link
Contributor Author

@omergunal omergunal Jun 11, 2018

Choose a reason for hiding this comment

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

I think it seems bad
selection_102

there is no problem at the moment. why we should change like this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Does it find vulnerabilities in all the files, or just the last file (i.e. last iteration of the loop)? If I'm reading it write I think it might do e.g. vulnerabilites = kitmap_vulns...then finally vulnerabilities = a.py_vulns, and only report the last list.

Copy link
Collaborator

Choose a reason for hiding this comment

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

(As an aside, it seems strange it's not printing out the vulnerability info, and just seems to print the object.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, it taking last item on the list. have you idea for fix?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the fix is having a list outside of the for loop and adding the vulnerabilities of each file to it. The code from my first comment should do it, although now that I think about it it's probably extend and not append.

Copy link
Collaborator

Choose a reason for hiding this comment

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

^Ahh, this was it! 👍 Just change append to extend and it'll all work! :D

print(path)
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
)

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
)
if args.project_root:
directory = os.path.normpath(args.project_root)
else:
directory = os.path.dirname(path)
project_modules = get_modules(directory)
local_modules = get_directory_modules(directory)
tree = generate_ast(path)

if args.project_root:
directory = os.path.normpath(args.project_root)
else:
directory = os.path.dirname(path)
project_modules = get_modules(directory)
local_modules = get_directory_modules(directory)
cfg = make_cfg(
tree,
project_modules,
local_modules,
path
)
cfg_list = [cfg]

tree = generate_ast(path)

cfg = make_cfg(
tree,
project_modules,
local_modules,
path
)
cfg_list = [cfg]
framework_route_criteria = is_flask_route_function
if args.adaptor:
if args.adaptor.lower().startswith('e'):
framework_route_criteria = is_function
elif args.adaptor.lower().startswith('p'):
framework_route_criteria = is_function_without_leading_
elif args.adaptor.lower().startswith('d'):
framework_route_criteria = is_django_view_function
# Add all the route functions to the cfg_list
FrameworkAdaptor(
cfg_list,
project_modules,
local_modules,
framework_route_criteria
)
framework_route_criteria = is_flask_route_function
if args.adaptor:
if args.adaptor.lower().startswith('e'):
framework_route_criteria = is_function
elif args.adaptor.lower().startswith('p'):
framework_route_criteria = is_function_without_leading_
elif args.adaptor.lower().startswith('d'):
framework_route_criteria = is_django_view_function
# Add all the route functions to the cfg_list
FrameworkAdaptor(
cfg_list,
project_modules,
local_modules,
framework_route_criteria
)

initialize_constraint_table(cfg_list)
analyse(cfg_list)
vulnerabilities = find_vulnerabilities(
cfg_list,
ui_mode,
args.blackbox_mapping_file,
args.trigger_word_file,
nosec_lines
)
initialize_constraint_table(cfg_list)
analyse(cfg_list)
vulnerabilities.extend(find_vulnerabilities(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

er

Copy link
Collaborator

Choose a reason for hiding this comment

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

Look good to you?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, there are no vulnerability in a.py b.py and c.py but it printing from xss.py

Copy link
Collaborator

Choose a reason for hiding this comment

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

I didn't figure out the bug yet, gonna look more tomorrow 😁 This is harder than expected to track down

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, i fixed it. its about vulnerabilities = list() location

cfg_list,
ui_mode,
args.blackbox_mapping_file,
args.trigger_word_file,
nosec_lines
))

if args.baseline:
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can keep this, but just de-dent it so that we only trim once.

vulnerabilities = get_vulnerabilities_not_in_baseline(
vulnerabilities,
args.baseline
)
vulnerabilities = get_vulnerabilities_not_in_baseline(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you for de-denting the if args.baseline: You can de-dent the vulnerabilities = get_vulnerabilities_not_in_baseline( too, 👍

vulnerabilities,
args.baseline
)


Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: You can delete this newline

if args.json:
json.report(vulnerabilities, args.output_file)
Expand Down
20 changes: 15 additions & 5 deletions pyt/usage.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,8 @@ def valid_date(s):
def _add_required_group(parser):
required_group = parser.add_argument_group('required arguments')
required_group.add_argument(
'-f', '--filepath',
help='Path to the file that should be analysed.',
type=str
'targets', metavar='targets', type=str, nargs='*',
help='source file(s) or directory(s) to be tested'
)


Expand Down Expand Up @@ -91,6 +90,17 @@ def _add_optional_group(parser):
action='store_true',
help='do not skip lines with # nosec comments'
)
optional_group.add_argument(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe make it

    parser.add_argument(
        '-r', '--recursive', dest='recursive',
        action='store_true', help='find and process files in subdirectories'
    )

'-r', '--recursive', dest='recursive',
action='store_true', help='find and process files in subdirectories'
)
optional_group.add_argument(
'-x', '--exclude',
dest='excluded_paths',
action='store',
default='',
help='Separate files with commas'
)


def _add_print_group(parser):
Expand All @@ -110,8 +120,8 @@ def _add_print_group(parser):


def _check_required_and_mutually_exclusive_args(parser, args):
if args.filepath is None:
parser.error('The -f/--filepath argument is required')
if args.targets is None:
parser.error('The targets argument is required')


def parse_args(args):
Expand Down
10 changes: 6 additions & 4 deletions tests/main_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,17 +5,18 @@


class MainTest(BaseTestCase):
@mock.patch('pyt.__main__.discover_files')
@mock.patch('pyt.__main__.parse_args')
@mock.patch('pyt.__main__.find_vulnerabilities')
@mock.patch('pyt.__main__.text')
def test_text_output(self, mock_text, mock_find_vulnerabilities, mock_parse_args):
def test_text_output(self, mock_text, mock_find_vulnerabilities, mock_parse_args, mock_discover_files):
mock_find_vulnerabilities.return_value = 'stuff'
example_file = 'examples/vulnerable_code/inter_command_injection.py'
output_file = 'mocked_outfile'

mock_discover_files.return_value = [example_file]
mock_parse_args.return_value = mock.Mock(
autospec=True,
filepath=example_file,
project_root=None,
baseline=None,
json=None,
Expand All @@ -32,17 +33,18 @@ def test_text_output(self, mock_text, mock_find_vulnerabilities, mock_parse_args
mock_parse_args.return_value.output_file
)

@mock.patch('pyt.__main__.discover_files')
@mock.patch('pyt.__main__.parse_args')
@mock.patch('pyt.__main__.find_vulnerabilities')
@mock.patch('pyt.__main__.json')
def test_json_output(self, mock_json, mock_find_vulnerabilities, mock_parse_args):
def test_json_output(self, mock_json, mock_find_vulnerabilities, mock_parse_args, mock_discover_files):
mock_find_vulnerabilities.return_value = 'stuff'
example_file = 'examples/vulnerable_code/inter_command_injection.py'
output_file = 'mocked_outfile'

mock_discover_files.return_value = [example_file]
mock_parse_args.return_value = mock.Mock(
autospec=True,
filepath=example_file,
project_root=None,
baseline=None,
json=True,
Expand Down
17 changes: 10 additions & 7 deletions tests/usage_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,14 @@ def test_no_args(self):

self.maxDiff = None

EXPECTED = """usage: python -m pyt [-h] [-f FILEPATH] [-a ADAPTOR] [-pr PROJECT_ROOT]
EXPECTED = """usage: python -m pyt [-h] [-a ADAPTOR] [-pr PROJECT_ROOT]
[-b BASELINE_JSON_FILE] [-j] [-m BLACKBOX_MAPPING_FILE]
[-t TRIGGER_WORD_FILE] [-o OUTPUT_FILE] [--ignore-nosec]
[-trim] [-i]
[-r] [-x EXCLUDED_PATHS] [-trim] [-i]
[targets [targets ...]]

required arguments:
-f FILEPATH, --filepath FILEPATH
Path to the file that should be analysed.
targets source file(s) or directory(s) to be tested

optional arguments:
-a ADAPTOR, --adaptor ADAPTOR
Expand All @@ -52,6 +52,9 @@ def test_no_args(self):
-o OUTPUT_FILE, --output OUTPUT_FILE
write report to filename
--ignore-nosec do not skip lines with # nosec comments
-r, --recursive find and process files in subdirectories
-x EXCLUDED_PATHS, --exclude EXCLUDED_PATHS
Separate files with commas

print arguments:
-trim, --trim-reassigned-in
Expand All @@ -62,7 +65,7 @@ def test_no_args(self):

self.assertEqual(stdout.getvalue(), EXPECTED)

def test_valid_args_but_no_filepath(self):
'''def test_valid_args_but_no_filepath(self):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

filepath option removed

Copy link
Collaborator

Choose a reason for hiding this comment

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

👍 You can change this to match something along the lines of targets argument is required

with self.assertRaises(SystemExit):
with capture_sys_output() as (_, stderr):
parse_args(['-j'])
Expand All @@ -73,7 +76,7 @@ def test_valid_args_but_no_filepath(self):
[-trim] [-i]
python -m pyt: error: The -f/--filepath argument is required\n"""

self.assertEqual(stderr.getvalue(), EXPECTED)
self.assertEqual(stderr.getvalue(), EXPECTED)'''

# def test_using_both_mutually_exclusive_args(self):
# with self.assertRaises(SystemExit):
Expand All @@ -89,7 +92,7 @@ def test_valid_args_but_no_filepath(self):

def test_normal_usage(self):
with capture_sys_output() as (stdout, stderr):
parse_args(['-f', 'foo.py'])
parse_args(['foo.py'])

self.assertEqual(stdout.getvalue(), '')
self.assertEqual(stderr.getvalue(), '')