2 Duplication problems and a false-positive in a portion of django.nV output, among other things.

[KevinHock]

So I run python -m pyt -a E -f example/django.nV/taskManager/upload_controller.py -trim and out I get:

5 vulnerabilities found:
Vulnerability 1:
File: example/django.nV/taskManager/misc.py
 > User input at line 24, trigger word "Flask function URL parameter": 
    title
File: example/django.nV/taskManager/misc.py
 > reaches line 33, trigger word "system(": 
    ¤call_2 = ret_os.system('mv ' + uploaded_file.temporary_file_path() + ' ' + '%s/%s' % (upload_dir_path, title))

Vulnerability 2:
File: example/django.nV/taskManager/upload_controller.py
 > User input at line 11, trigger word "get(": 
    ¤call_3 = ret_request.POST.get('name', False)
Reassigned in: 
    File: example/django.nV/taskManager/upload_controller.py
     > Line 11: name = ¤call_3
    File: example/django.nV/taskManager/upload_controller.py
     > Line 12: temp_4_title = name
    File: example/django.nV/taskManager/misc.py
     > Line 24: title = temp_4_title
File: example/django.nV/taskManager/misc.py
 > reaches line 33, trigger word "system(": 
    ¤call_6 = ret_os.system('mv ' + uploaded_file.temporary_file_path() + ' ' + '%s/%s' % (upload_dir_path, title))

Vulnerability 3:
File: example/django.nV/taskManager/upload_controller.py
 > User input at line 3, trigger word "Flask function URL parameter": 
    request
Reassigned in: 
    File: example/django.nV/taskManager/upload_controller.py
     > Line 12: temp_4_uploaded_file = request.FILES['file']
    File: example/django.nV/taskManager/misc.py
     > Line 24: uploaded_file = temp_4_uploaded_file
File: example/django.nV/taskManager/misc.py
 > reaches line 33, trigger word "system(": 
    ¤call_6 = ret_os.system('mv ' + uploaded_file.temporary_file_path() + ' ' + '%s/%s' % (upload_dir_path, title))

Vulnerability 4:
File: example/django.nV/taskManager/upload_controller.py
 > User input at line 11, trigger word "get(": 
    ¤call_3 = ret_request.POST.get('name', False)
Reassigned in: 
    File: example/django.nV/taskManager/upload_controller.py
     > Line 12: temp_4_title = name
    File: example/django.nV/taskManager/misc.py
     > Line 24: title = temp_4_title
    File: example/django.nV/taskManager/misc.py
     > Line 41: ret_store_uploaded_file = '/static/taskManager/uploads/%s' % title
    File: example/django.nV/taskManager/upload_controller.py
     > Line 12: ¤call_4 = ret_store_uploaded_file
    File: example/django.nV/taskManager/upload_controller.py
     > Line 12: upload_path = ¤call_4
File: example/django.nV/taskManager/upload_controller.py
 > reaches line 16, trigger word "execute(": 
    ¤call_8 = ret_curs.execute('insert into taskManager_file ('name','path','project_id') values ('%s','%s',%s)' % (name, upload_path, project_id))

Vulnerability 5:
File: example/django.nV/taskManager/upload_controller.py
 > User input at line 3, trigger word "Flask function URL parameter": 
    request
Reassigned in: 
    File: example/django.nV/taskManager/upload_controller.py
     > Line 12: temp_4_title = name
    File: example/django.nV/taskManager/misc.py
     > Line 24: title = temp_4_title
    File: example/django.nV/taskManager/misc.py
     > Line 41: ret_store_uploaded_file = '/static/taskManager/uploads/%s' % title
    File: example/django.nV/taskManager/upload_controller.py
     > Line 12: ¤call_4 = ret_store_uploaded_file
    File: example/django.nV/taskManager/upload_controller.py
     > Line 12: upload_path = ¤call_4
File: example/django.nV/taskManager/upload_controller.py
 > reaches line 16, trigger word "execute(": 
    ¤call_8 = ret_curs.execute('insert into taskManager_file ('name','path','project_id') values ('%s','%s',%s)' % (name, upload_path, project_id))

There are many issues with this output.

(a) Vulnerability #1 should not be in the output, or at least, if you would argue it should be, you'd concede it's a good idea to give an option for vulnerabilities like it to not be in the output. When I say 'vulnerabilities like it' I mean, we ran it on a controller file, upload_controller.py which calls into misc.py, then we reported vulnerabilities as though we ran it on misc.py, resulting in a duplicate (vulnerabilities 1 and 2).

To solve this, maybe we should do something with self.filenames[-1] inside of interprocedural.py or just, at a higher level, grab the file from the -f output and skip any vulnerabilities that don't match it (note the File: example/django.nV/taskManager/misc.py in the output). The latter idea sounds cleaner and smoother.

(b) Vulnerability #3 is not unknown, although we know uploaded_file is tainted we don't have any idea if uploaded_file.temporary_file_path() is something that leads to a vulnerability.

To solve this, we somehow add the return value of uploaded_file.temporary_file_path() to blackbox_assignments. The .args list of the sink might include uploaded_file, so we'll need to change this as well when we're visiting BBorBInode arguments.

(c) Vulnerabilities #4 and #5 are the same vulnerability, stemming from the same line. (d) In the Vulnerability #5 output, it doesn't show the actual request.whatever line that led to the vulnerability.

Perhaps these can be solved with the same code, not sure.

(e) If you run it without -trim, and search through the output you'll see ret_render_to_response('taskManager/upload.html', 'form'form, ¤call_13) (from the original line render_to_response('taskManager/upload.html', {'form': form}, RequestContext(request))), so I take it I don't handle visual_args very well when they're dictionaries. A low-priority issue from where I stand though.

Another thing that I noticed, but I'm not going to implement, is https://github.com/python-security/pyt/issues/71

[amacfie]

Can you elaborate on how Vulnerability 3 would ideally be handled?

[amacfie]

@KevinHock defining non-propagating attributes is one idea, another unrelated one is refining sinks by specifying which specific arguments are sinks. Would these be desirable features?

[KevinHock]

Sorry for the lack of replies. Please see https://pyre-check.org/docs/pysa-basics.html for the project to contribute to 👍

[KevinHock]

That should work out-the-box 👍