Injection

Description

Injection vulnerabilities are a class of vulnerability where attackers can cause malicious code to be exected on the server. Often times, untrusted data that is unproperly validated can end up in commands sent to a database, shell, or other parsers. Since the malicious data will appear in-stream with the intended code, it will execute with the same privileges as the original operation.

SQL Injection is a class of injection where queries given to a SQL database are modified by an attacker in order to perform unintended operations or retrieve sensitive information. For example, let's assume we use the following query to check if a user exists and is has an access_level of 1. 

SELECT 1 FROM USER WHERE user_id = " + user_id + " AND access_level = 1
However, if the user_id is not properly sanitized, the attacker can inject SQL code (where data should be) and bypass an ID check. If the attacker could pass 1 OR 1=1 as the user_id, they would transform the query into the following: 
SELECT 1 FROM USER WHERE user_id = 1 OR 1=1 AND access_level = 1
This transforms the query entirely. While it originally checked if the given user had an access_level of 1, it now checks if any user has an access_level of 1. Unfortunately, SQL injections can be far more malicious than simply bypassing a permissions check, which is already bad enough. Consider the following, where the user_id is passed as 1; DROP TABLE user; --. Our simple query then becomes: 
SELECT 1 FROM USER WHERE user_id = 1; DROP TABLE user; -- AND access_level = 1
The attacker has now successfully dropped our entire users table!

Bug

In the TaskManager app, there are two types of file uploads - profile pictures, and files related to projects. One type, the project files, use the following view function to store said files. 
def upload(request, project_id):
    if request.method == 'POST':

        proj = Project.objects.get(pk = project_id)
        form = ProjectFileForm(request.POST, request.FILES)

        if form.is_valid():
            name = request.POST.get('name', False)
            upload_path = store_uploaded_file(name, request.FILES['file'])

            curs = connection.cursor()
            curs.execute("insert into taskManager_file ('name','path','project_id') values ('%s','%s',%s)"%(name,upload_path,project_id))

            return redirect('/taskManager/' + project_id + '/', {'new_file_added':True})
        else:
            form = ProjectFileForm()
    else:
        form = ProjectFileForm()
    return render_to_response('taskManager/upload.html', {'form': form}, RequestContext(request))
This code uses the curs.execute() function directly, without any validation on the fields passed. If the user were to submit a file with the filename: 
testPic',(select password from auth_user where username='admin'),8);--
The function would execute the following query: 
INSERT INTO taskManager_file ('name','path','project_id') VALUES ('testPic',(SELECT password FROM auth_user WHERE username='admin'),8);--,...
Notice the embedded query that runs and would reveal the password of the admin user!

Furthermore, the file processing function store_uploaded_file() contains shell injection. It works by moving the uploaded file that are stored as temporary files using os.system() The problem is that user input in the form of the file title is directly inserted into the command sent to the shell, allowing for other commands to be injected.

Solution

Django's ORM provides us with very powerful tools to validate input and make database updates securely. Rather than using a the curs.execute() function directly on the database, we should use the ORM to create a new file object. 
file = File(
name = name,
path = upload_path,
project = proj)
file.save()
This will insure that the arguments passed are properly escaped. Users will still be able to create files with strange names, but they won't be able to perform SQL injection attacks using those names anymore.

In general, you should avoid using direct functions in Django's ORM (like curs.execute) and instead stick to common ORM methods. These will provide common sense filtering and escaping in their query generation, providing strong security-by-default for developer code. Further ORM features are just more icing on the cake.

User input should never be directly passed to Python's os.system() function call. In this instance, file names should be randomly generated strings; there is no need to allow users to specify on-disk filenames, since they will never encounter these. The injection opportunity can be entirely avoided by removing any user input from the shell command. Another approach would be to use os.rename() with escaping and avoid access to arbitrary shell commands.

On top of these fixes, you may want to run checks using django's Form class for validation and to provide more immediate feedback.

Hint

The TaskManager devs seem to have just thrown file uploads in at the last minute...

Single quotes are a great way to find vanilla SQLi. Try them in different form fields within the app. And how are uploaded files processed?