1
\$\begingroup\$

I wrote this code to to perform the following:

  1. I will remove the "spaces" within the url.
  2. Return welcome message.
  3. Redirect the user to different url.
  4. Compute HMAC for the request.
  5. Return and revoke the user profile picture.

Would someone please help me in checking if my code contains any security issues that I should avoid?

import base64
import mimetypes
import os
import hashlib
import hmac
import requests

from django.core.urlresolvers import reverse
from django.http import HttpResponse
from django.shortcuts import redirect, render
from django.views.decorators.csrf import csrf_exempt

def ordenary(s):
     return s.strip().replace(' ', '').lower()

def form_of_message(request):
     env = {'message': request.GET.get('message', 'hello')}
     response = render(request, 'forms/message_form.html', env)
     response.set_cookie(key='message_rendered_at', value=time.time())
     return response

 def proxy(request):
     url = request.GET.get('url')
     return redirect(url)

 def compute_hmac_signature(message, key):
     key = bytes(key, 'UTF-8')
     message = bytes(message, 'UTF-8')

     digest = hmac.new(key, message, hashlib.sha1).hexdigest()
     return "sha1={}".format(str(digest))

 def user_pic(request):
     """A view that returns the user's avatar image"""

     base_path = os.path.join(os.path.dirname(__file__), '../../images/avatars')
     filename = request.GET.get('u')

     try:
         data = open(os.path.join(base_path, filename), 'rb').read()
     except IOError:
         return render(request, 'templates/avatar.html')

     return HttpResponse(data, content_type=mimetypes.guess_type(filename)[0])
\$\endgroup\$
  • 2
    \$\begingroup\$ This looks like a set of functions, but they are never invoked. Do you have a usage example with this? \$\endgroup\$ – Mast Feb 16 '19 at 20:05
1
\$\begingroup\$

Some comments, with security issues highlighted:

  • Why would you remove whitespace from URLs? It is perfectly valid for URLs to contain whitespace, unencoded or encoded as + or %20. If your backend code can't handle whitespace there are much bigger fish to fry. In general:
    • Blacklisting is more dangerous than whitelisting, because it's easy to omit some values.
    • Whitelisting parts of values (such as characters in a URL) is more dangerous than whitelisting entire values, because clever hackers often find a way to escape the sandboxing attempt by combining parts. For example, depending on how you pass values to your backend, %20 in a URL may or may not be replaced with a space character.
    • Blanket removal of parts of the input limits your application severely while providing very little security (because of the above). A better solution is to escape the input and test the escaping using previously insecure values. This escaping should be provided by the official libraries (such as SQL or shell escaping).
    • An even better solution is to let the library do the escaping for you, for example with parametric SQL queries, because now you don't need to remember to escape every value yourself, only to use parameters rather than string concatenation.
  • Why would you lowercase the URL? There's no security issue with uppercase characters that I've ever heard of. And again, limiting the URL space like this is a bad trade-off.
  • Redirecting to a URL without checking it against a strict whitelist first is a well-known security issue.
  • Similarly, not validating the username parameter is a massive issue - what if I specify something like u=../../etc/passwd?
  • Why would you implement your own HMAC generator? It's built right into the standard library, even in Python 2.
  • What is the purpose of the message_rendered_at cookie?
  • Guessing the MIME type indicates that someone could send a JavaScript, PDF or other potentially harmful file as their avatar. If this is shown to other people that could lead to remote code injection. This has already happened on many sites with Bitcoin mining scripts. At best you should instead convert all avatars to a single format (PNG is ideal for this) and serve that. If you really need some flexibility then allow a small set of well-known formats and change the file extension when saving it. Then use a dict to map file extension to MIME type.
  • ordenary is not an English word as far as I know. ordinary would not be a good function name either, because it gives the programmer no idea what it actually does. One thing I've found useful to determine better names for things is to use type hints and validate them using mypy with strict settings (not allowing Any type). It takes some getting used to, but doing this properly should tell you exactly what goes into and comes out of any function. A function which takes a Request to /foo and returns a Response, for example, could be a respond_to_foo_request.
  • Some of the imports are unused.
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.