Repairman2

taskManager/settings.py

@@ -25,10 +25,10 @@
 
 # A5: Security Misconfiguration
 # SECURITY WARNING: don't run with debug turned on in production!
-DEBUG = True
-TEMPLATE_DEBUG = True
+DEBUG = False
+TEMPLATE_DEBUG = False
 
-ALLOWED_HOSTS = []
+ALLOWED_HOSTS = ['127.0.0.1']
 
 # Application definition
 

taskManager/views.py

@@ -117,7 +117,7 @@ def manage_groups(request):
 
         user_list = User.objects.order_by('date_joined')
 
-        if request.method == 'POST':
+        if request.method == 'POST' and user.has_perm('can_change_group'):
 
             post_data = request.POST.dict()
 
@@ -171,30 +171,34 @@ def upload(request, project_id):
 
     if request.method == 'POST':
 
-        proj = Project.objects.get(pk=project_id)
-        form = ProjectFileForm(request.POST, request.FILES)
+        user = request.user
 
-        if form.is_valid():
-            name = request.POST.get('name', False)
-            upload_path = store_uploaded_file(name, request.FILES['file'])
+        if user.is_authenticated():
 
-            #A1 - Injection (SQLi)
-            curs = connection.cursor()
-            curs.execute(
-                "insert into taskManager_file ('name','path','project_id') values ('%s','%s',%s)" %
-                (name, upload_path, project_id))
+            proj = Project.objects.get(pk=project_id)
+            form = ProjectFileForm(request.POST, request.FILES)
 
-            # file = File(
-            #name = name,
-            #path = upload_path,
-            # project = proj)
+            if form.is_valid():
+                name = request.POST.get('name', False)
+                upload_path = store_uploaded_file(name, request.FILES['file'])
 
-            # file.save()
+                #A1 - Injection (SQLi)
+                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()
+                # file = File(
+                #name = name,
+                #path = upload_path,
+                # project = proj)
+
+                # file.save()
+
+                return redirect('/taskManager/' + project_id +
+                                '/', {'new_file_added': True})
+            else:
+                form = ProjectFileForm()
     else:
         form = ProjectFileForm()
     return render_to_response(
@@ -707,7 +711,6 @@ def profile(request):
 # A8: Cross Site Request Forgery (CSRF)
 
 
-@csrf_exempt
 def profile_by_id(request, user_id):
     user = User.objects.get(pk=user_id)
 
@@ -736,7 +739,7 @@ def profile_by_id(request, user_id):
 
 # A8: Cross Site Request Forgery (CSRF)
 
-@csrf_exempt
+
 def reset_password(request):
 
     if request.method == 'POST':
@@ -776,7 +779,7 @@ def reset_password(request):
 
 # Vuln: Username Enumeration
 
-@csrf_exempt
+
 def forgot_password(request):
 
     if request.method == 'POST':
@@ -810,7 +813,7 @@ def forgot_password(request):
 
 # A8: Cross Site Request Forgery (CSRF)
 
-@csrf_exempt
+
 def change_password(request):
 
     if request.method == 'POST':

vulnerability-report.md

@@ -0,0 +1,188 @@
+# Vulnerability Report
+
+Reviewer 1: Ben Shields
+
+Reviewer 2: Conor Clary
+
+Date: Feb 5, 2016
+
+Reviewing nVisium Task Manager
+
+
+**A1 - Injection:**
+
+Since the upload function takes in a request argument, doing some validation on the request.user.is_authenticated, request.user.username, and request.user.id objects would probably be pretty helpful. Alienation and cleansing of the entered data would also be helpful. Parsing it for terms, especially SQL query calls (FROM, SELECT, DROP, *, TABLE) would also be helpful to securing the form.
+
+Or you could just do what the solution says and instantiate a new File class instead of using the curs.execute method at all. 
+
+The site seems really secure seeing as I couldn't even figure out how to upload a task until about 30 minutes in.
+
+```Python
+def upload(request, project_id):
+
+    bad_words = ['FROM', 'SELECT', 'DROP', '*', 'TABLE']
+
+    if request.method == 'POST' and request.user.is_authenticated:
+
+        proj = Project.objects.get(pk=project_id)
+        form = ProjectFileForm(request.POST, request.FILES)
+
+        if form.is_valid() and request.user:
+            name = request.POST.get('name', False)
+            upload_path = store_uploaded_file(name, request.FILES['file'])
+
+            #A1 - Injection (SQLi)
+
+            check = name.split()
+            for item in check:
+                if item in bad_words:
+                    return HTTPResponseNotFound("You killed me, Jim! Actually you tried to SQL inject us, but we caught you.")
+
+            curs = connection.cursor()
+            curs.execute(
+                "insert into taskManager_file ('name','path','project_id') values ('%s','%s',%s)" %
+                (name, upload_path, project_id))
+
+            # file = File(
+            #name = name,
+            #path = upload_path,
+            # project = proj)
+
+            # file.save()
+
+            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))
+```
+
+**A2 - Broken Auth:**
+
+Would never have guessed this one. It makes sense though. Seems like a Django responsibility and maybe should be highlighted in the docs. Better to explicitly include what you need than explicitly exclude what you do not. 
+
+```
+class UserForm(forms.ModelForm):
+    """ User registration form """
+
+    class Meta:
+        model = User
+        # exclude = ['groups', 'user_permissions', 'last_login', 'date_joined', 'is_active']
+
+        fields = ['username', 'first_name', 'last_name', 'email', 'password']
+```
+**A3 - XSS:**
+I assume this has something to do with implementing the {% csrf_token %} within the html and enabling csrf in the settings. 
+Not really sure how to exploit these attacks really, but I don't know why one would put the csrf_exempt wrapper on something as sensitive as login, reset password, profile by id, etc. Those have been removed.
+
+**A4 - Insecure DOR:**
+
+Put in authorization and permission checks on sensitive functions, especially anything that deletes. Again, maybe this seems so simple because you already taught us this, but why would a developer assume their users are anything but idiots or attackers? I get that being lazy is generally a good quality to have, but leaving out security is not lazy, it's dumb and could end up costing the developer their job/client.
+
+Check for permissions on sensitive functions.
+
+Exposure: Many instances of Insecure Direct Object References. You can navigate to functions that should really be protected accessing a variety of routes.
+
+/taskManager/profile/profile_id : access and edit existing user profiles, including admin (id=1)
+
+/taskManager/downloadprofilepic/profile_id : view profile pictures
+
+/taskManager/dashboard/ : access and edit any project and/or task
+
+/taskManager/project_id/upload/ : access a project and upload a file
+
+/taskManager/project_id/task_id : access projects and edit them
+
+/taskManager/project_id/task_edit/task_id : access specific tasks and edit them
+
+Repair:
+
+This is an example repair for the upload file to project view.
+
+def upload(request, project_id):
+
+    if request.method == 'POST':
+
+        user = request.user
+
+        if user.is_authenticated():
+
+            etc.
+The user must be authenticated to successfully post. However, the user can still navigate to this page by url.
+
+**A5 - Security Misconfiguration:**
+
+Exposure:
+Security Misconfiguration can occur when development aspects of the application remain in production levels. Debug being left set to True in a production level application is an example of Security Misconfiguration.
+
+Repair:
+In settings.py:
+
+```python
+DEBUG = False
+TEMPLATE_DEBUG = False
+
+ALLOWED_HOSTS = ['127.0.0.1:8000']
+```
+
+**A6 - Sensitive Data Exposure:**
+
+Exposure:
+Sensitive data include passwords, addresses, phone numbers, social security numbers, credit card numbers, etc. Often, sensitive data is exposed in transit if it is not properly encrypted. MD5 is not a very strong hashing algorithm: https://en.wikipedia.org/wiki/MD5.
+
+Repair:
+django.contrib.auth.hashers.py has a bunch of other hashing algorithms available.
+
+in settings.py:
+
+```python
+PASSWORD_HASHERS = ['django.contrib.auth.hashers.PBKDF2PasswordHasher']
+```
+
+**A7 - Missing Function Level Access Control:**
+
+Exposure:
+In manage_groups view, a check is performed for authentication ie to see if the user is logged in correctly. However, access is never checked for authorization. Does the user have the right permissions to perform this action?
+
+Repair:
+Make sure to authorize in addition to authenticating.
+
+```python
+def manage_groups(request):
+
+    user = request.user
+
+    if user.is_authenticated():
+
+        user_list = User.objects.order_by('date_joined')
+
+        if request.method == 'POST' and user.has_perm('can_change_group'):
+            etc.
+
+```
+
+**A4 - Cross-Site Request Forgery:**
+
+Exposure:
+Several views are CSRF exempt (employ the @csrf-exempt decorator). Django does not check validity for these views.
+
+Repair:
+Remove the decorators.
+
+```python
+def profile_by_id(request, user_id):
+    etc.
+
+def reset_password(request):
+    etc.
+
+def forgot_password(request):
+    etc.
+
+def change_password(request):
+    etc.
+
+```