Securely convert and return a user selected file

Question

I designed a Python3 webservice which gets a file name, looks for this file in a local folder, converts it using an external software, then sends the converted file to the user.

For this example, the input and output file types are respectively PNG and JPEG, but they're actually proprietary formats.

Here is my code:

#!/usr/bin/python3
# -*- coding: utf-8 -*

from http.server import BaseHTTPRequestHandler, HTTPServer
import os
import os.path as op
import subprocess as sub
from urllib.parse import parse_qs, urlparse

PORT_NUMBER = 8080
PNG_TO_JPG_CALL = '/usr/share/png-to-jpg'
PNG_FOLDER = '/home/foo/png_files'

class RequestHandler(BaseHTTPRequestHandler):
    # Display a text to the user.
    def display_msg(self, text):
        print(text)
        self.send_header('Content-type', 'text/plain; charset=utf-8')
        self.send_header('Content-Disposition', 'inline')
        self.end_headers()
        self.wfile.write(bytes(text, 'UTF-8'))

    # Send a file to the user (open the Download File dialog box).
    def send_file(self, file_path, jpg_name):
        self.send_header('Content-type', 'image/jpeg')
        self.send_header('Content-Disposition', 'attachment; filename="%s"' % jpg_name)
        self.end_headers()
        with open(file_path, 'rb') as f:
            self.wfile.write(f.read())

    # Check for GET requests
    def do_GET(self):
        self.send_response(200)

        # Get URL arguments
        url_args = parse_qs(urlparse(self.path).query, keep_blank_values=True)

        # Check for the `id` attribute
        if 'id' not in url_args:
            self.display_msg('USAGE: [...]/?id=some_file.jpg')
            return

        target = url_args['id'][0]

        # Check if the requested file have the good extension
        if target[-4:] != '.jpg':
            self.display_msg('TARGET SHOULD BE A PNG FILE')
            return

        # build the png file path (change the extension)
        png_path = op.join(PNG_FOLDER, op.splitext(target)[0] + '.png')
        print('Looking for %s' % png_path)

        # Check if the requested is present on the disk
        if not op.isfile(png_path):
            self.display_msg('PNG FILE NOT FOUND')
            return

        # call the converter with the requested file, print the output
        p = sub.Popen([PNG_TO_JPG_CALL, png_path], stdout=sub.PIPE, stderr=sub.PIPE)
        output, errors = p.communicate()
        if output:
            print('output: %s' % output.decode('utf-8'))

        # Check for errors outputs
        if errors:
            self.display_msg('CONVERSION ERROR: png-to-jpg says:\n' + errors.decode('utf-8'))
            return

        # Build the converted file path
        jpg_name = op.splitext(target)[0] + '.jpg'
        jpg_path = op.join(os.getcwd(), jpg_name)

        # Check if the converted file is present on the disk
        if not op.isfile(jpg_path):
            self.display_msg('CONVERSION ERROR: Can not locate the ouput directory.')
            return

        # returns the converted file then remove it
        self.send_file(jpg_path, jpg_name)
        os.remove(jpg_path)

# Launch the web server
try:
    server = HTTPServer(('', PORT_NUMBER), RequestHandler)
    print('Web server started. Please go to 127.0.0.1:%i.' % PORT_NUMBER)
    server.serve_forever()
except KeyboardInterrupt:
    print('Interrupted by the user - shutting down the web server.')
    server.socket.close()

Is this secure?

Answer 1

This script is insecure because there are a few small issues. And such issues can often be combined to produce bigger issues, so it's important to be wary about them.

First, you're not restricting the path of the image to PNG_FOLDER. An attacker can ask for ?id=../../etc/passwd. It will fail because you check that the id ends with '.jpg'. But what if the user sends ?id=../../etc/passwd,some_file.jpg? Did you check that png-to-jpg is not going to try to split on a comma? A simple fix: get a list of all possible file names first and only allow an id which is in the list.

Second, you're sending the full png-to-jpg answer. I don't know how this utility works, but an attacker can probably gain useful information just by looking at the output. He will know that he should look at png-to-jpg vulnerabilites. More generally, any information sent to an attacker is another thing that can be exploited.

Third (this is not necessarily an issue), but it's possible to list all files by simply trying all combinations and see the output. It's even possible to see what's already converted and what's not with timing attacks.

I'm not saying that fixing these issues will make your code fully secure, but it will be a step forward.