From 1938f178351fd046346c0f99845855a5c6a09345 Mon Sep 17 00:00:00 2001 From: Ted Callahan Date: Fri, 3 Feb 2017 16:09:38 -0800 Subject: [PATCH 1/5] Added vulnerability report. --- vulnerabilityreport.md | 47 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) create mode 100644 vulnerabilityreport.md diff --git a/vulnerabilityreport.md b/vulnerabilityreport.md new file mode 100644 index 00000000..7528e82c --- /dev/null +++ b/vulnerabilityreport.md @@ -0,0 +1,47 @@ +Vulnerability Report +Reviewer 1: Ted Callahan + +Reviewer 2: Rick Valenzuela + +Date: February 1st, 2017 + +Reviewing nVisium Task Manager + +Vulnerability 1 +Exposure +We found an instance of [vulnerability 1] by typing some relevant code into some vulnerable field OR by doing some edge-case thing. + +By exploiting [this vulnerability], we were able to retrieve XYZ attributes from the site / access to some unauthorized part of the site / something else valuable. + +Repair +problem_file1.py and problem_file2.py contained the vulnerability. We were able to fix the first with the following adjustment(s): + +problem_file1.py + +Some body of relevant code that solves our problem +problem_file2.py + + +Vulnerability 1 +Exposure +We found an instance of SQL Injection vulnerability in the file upload form (line 183 in views.py). + +By exploiting this problem, we are able to perform SQL injection and retrieve data or drop tables from the database. + +Repair +Rather than use a formatted SQL command with input from the form, the site should use the DJANGO ORM to create a File object instance for addition to the databse. + +Problem Code (Lines 182 - 185, views.py): +curs = connection.cursor() +curs.execute( + "insert into taskManager_file ('name','path','project_id') values ('%s','%s',%s)" % + (name, upload_path, project_id)) + +Solution: +file = File( +name = name, +path = upload_path, +project = proj) + +file.save() + From 6aba55c194eccf90d1e6d16e681f1f625a24e6ef Mon Sep 17 00:00:00 2001 From: Ted Callahan Date: Fri, 3 Feb 2017 17:11:13 -0800 Subject: [PATCH 2/5] vulnerability report completed for security issues 1 - 4 --- vulnerabilityreport.md | 89 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 89 insertions(+) diff --git a/vulnerabilityreport.md b/vulnerabilityreport.md index 7528e82c..5551858b 100644 --- a/vulnerabilityreport.md +++ b/vulnerabilityreport.md @@ -45,3 +45,92 @@ project = proj) file.save() +Vulnerability 2 +Exposure +Various upload functions call the store_uploaded_file() funtion at line 24 of msc.py. This function passes a given filename to the os.system function giving a user the potential to pass in shell commands as a filename. + +Repair +Do not allow the user to pass in a file name. Give the file a randomly generated name, there is no reason for a user to give a file a name for use in the application. + +Instead, create a File instance and save the file instance in the database. The user can give the File object a name, thus preventing any need for calling os.system or any commands that rely on command line commands. + + +Vulnerability 3 +Exposure +The User registration form (class UserForm) as defined in forms.py (lines 71 - 75) is insecure because it does not explicitly state which fields are allowed. A user could add fields for other attributes of the Django user model (such as is_superuser). + +By exploiting this problem, we are able to add form fields to the User registration form to edit user attributes on the Django model. + +Repair +Instead of a partial exclude list of fields, use the fields attribute in the Meta class to explicitly declare which fields should be in the form. + +Problem Code (line 75 in forms.py): +exclude = ['groups', 'user_permissions', 'last_login', 'date_joined', 'is_active'] + +Solution: +fields = ['username', 'email', 'first_name', 'last_name', 'password'] + + +Vulnerability 4 +Something to do with: +SESSION_ENGINE = "django.contrib.sessions.backends.signed_cookies" + + +Vulnerability 5 +Exposure +The template base_backend.html inserts the username with a safe flag. This allows the potential for a JS script as a username to be rendered to the page, causing it to be run because it will not be escaped. + +Repair +Instead of declaring safe output with the safe flag, do not do that. + +Problem Code (line 58 of base_backend.html): + {{user.username|safe}} + +Solution: + {{user.username}} + + +Vulnerabilty 6 +Exposure +Website is generally at-risk for Cross Site Scripting attacks. By turning on Django's SECURE_BROWSER_XSS_FILTER setting, Django will add X-XSS-Protection to all responses. The browser will then automatically block all XSS attacks it can detect. + +Repair +Set SECURE_BROWSER_XSS_FILTER = True in settings.py + + +Vulnerability 7 +Exposure +All CRUD operations are vulnerable to Insecure Direct Object Reference (IDOR). Basically, in order to prevent unauthorized users or users with inappropriate permissions, the site should require all proper credentials and permissions from an user before a CRUD operation is performed. + +For Class Based Views, use the LoginReqiuredMixin and PermissionRequiredMixin + +Repair +Apply @login_required and @permission_required decorators for any function in views.py that performs a CRUD operation. + +Problem Code functions listed from views.py: + - manage_products + - manage_groups + - upload + - task_create + - task_edit + - task_delete + - project_delete + - project_edit + - project_create + - note_create + - note_edit + - note_delete + +Solution: +### For Function Views: +from django.contrib.auth.decorators import permission_required, login_required + +@login_required +@permission_required +def function_view_with_CRUD(request): + + +### For Class Based Views: +from django.contrib.auth.mixins import PermissionRequiredMixin, LoginRequiredMixin + +class ClassBasedCRUDView(LoginRequiredMixin, PermissionRequiredMixin, ClassView): From dacc66db4bdf2ff2aa83af94303ae61c8a20199d Mon Sep 17 00:00:00 2001 From: Rick Valenzuela Date: Tue, 21 Feb 2017 19:11:41 -0800 Subject: [PATCH 3/5] backporting report info for assignment spec --- vulnerability-report.md | 75 ++++++++++++++++++++++ vulnerabilityreport.md | 136 ---------------------------------------- 2 files changed, 75 insertions(+), 136 deletions(-) create mode 100644 vulnerability-report.md delete mode 100644 vulnerabilityreport.md diff --git a/vulnerability-report.md b/vulnerability-report.md new file mode 100644 index 00000000..3ee8235c --- /dev/null +++ b/vulnerability-report.md @@ -0,0 +1,75 @@ +#Vulnerability Report +###Reviewer 1: Ted Callahan +###Reviewer 2: Rick Valenzuela + +###Date: February 1st, 2017 + +##Reviewing nVisium Task Manager + +##Vulnerability 1 +###Exposure +We found an instance of an SQL Injection vulnerability in the file upload form (line 183 in views.py). + +By exploiting this problem, we are able to perform SQL injection and retrieve data or drop tables from the database. + +###Repair +Rather than use a formatted SQL command with input from the form, the site should use the Django ORM to create a File object instance for addition to the databse. + +Problem Code (Lines 182 - 185, views.py): +``` +curs = connection.cursor() +curs.execute( + "insert into taskManager_file ('name','path','project_id') values ('%s','%s',%s)" % + (name, upload_path, project_id)) +``` + +###Solution: +``` +file = File( +name = name, +path = upload_path, +project = proj) + +file.save() +``` + +##Vulnerability 2 +###Exposure +Various upload functions call the store_uploaded_file() funtion at line 24 of misc.py. This function passes a given filename to the os.system function giving a user the potential to pass in shell commands as a filename. + +###Repair +Do not allow the user to pass in a filename. Give the file a randomly generated name; there is no reason for a user to give a file a name for use in the application. + +Instead, create a File instance and save the file instance in the database. The user can give the File object a name, thus preventing any need for calling os.system or any commands that rely on command line commands. + +###Solution: +Comment out or remove lines 14-17 of templates/taskManager/upload.html: +``` +
+ + +
+``` + +##Vulnerability 3 +###Exposure +The User registration form (class UserForm) as defined in forms.py (lines 71 - 75) is insecure because it does not explicitly state which fields are allowed. A user could add fields for other attributes of the Django user model (in this instance, is_superuser and is_staff). + +By exploiting this problem, we are able to add form fields to the User registration form to edit user attributes on the Django model. + +###Repair +Instead of a partial exclude list of fields, use the fields attribute in the Meta class to explicitly declare which fields should be in the form. + +Problem Code (line 75 in forms.py): +``` +exclude = ['groups', 'user_permissions', 'last_login', 'date_joined', 'is_active'] +``` + +###Solution: +``` +fields = ['username', 'email', 'first_name', 'last_name', 'password'] +``` + +##Vulnerability 4 +Something to do with: +SESSION_ENGINE = "django.contrib.sessions.backends.signed_cookies" \ No newline at end of file diff --git a/vulnerabilityreport.md b/vulnerabilityreport.md deleted file mode 100644 index 5551858b..00000000 --- a/vulnerabilityreport.md +++ /dev/null @@ -1,136 +0,0 @@ -Vulnerability Report -Reviewer 1: Ted Callahan - -Reviewer 2: Rick Valenzuela - -Date: February 1st, 2017 - -Reviewing nVisium Task Manager - -Vulnerability 1 -Exposure -We found an instance of [vulnerability 1] by typing some relevant code into some vulnerable field OR by doing some edge-case thing. - -By exploiting [this vulnerability], we were able to retrieve XYZ attributes from the site / access to some unauthorized part of the site / something else valuable. - -Repair -problem_file1.py and problem_file2.py contained the vulnerability. We were able to fix the first with the following adjustment(s): - -problem_file1.py - -Some body of relevant code that solves our problem -problem_file2.py - - -Vulnerability 1 -Exposure -We found an instance of SQL Injection vulnerability in the file upload form (line 183 in views.py). - -By exploiting this problem, we are able to perform SQL injection and retrieve data or drop tables from the database. - -Repair -Rather than use a formatted SQL command with input from the form, the site should use the DJANGO ORM to create a File object instance for addition to the databse. - -Problem Code (Lines 182 - 185, views.py): -curs = connection.cursor() -curs.execute( - "insert into taskManager_file ('name','path','project_id') values ('%s','%s',%s)" % - (name, upload_path, project_id)) - -Solution: -file = File( -name = name, -path = upload_path, -project = proj) - -file.save() - -Vulnerability 2 -Exposure -Various upload functions call the store_uploaded_file() funtion at line 24 of msc.py. This function passes a given filename to the os.system function giving a user the potential to pass in shell commands as a filename. - -Repair -Do not allow the user to pass in a file name. Give the file a randomly generated name, there is no reason for a user to give a file a name for use in the application. - -Instead, create a File instance and save the file instance in the database. The user can give the File object a name, thus preventing any need for calling os.system or any commands that rely on command line commands. - - -Vulnerability 3 -Exposure -The User registration form (class UserForm) as defined in forms.py (lines 71 - 75) is insecure because it does not explicitly state which fields are allowed. A user could add fields for other attributes of the Django user model (such as is_superuser). - -By exploiting this problem, we are able to add form fields to the User registration form to edit user attributes on the Django model. - -Repair -Instead of a partial exclude list of fields, use the fields attribute in the Meta class to explicitly declare which fields should be in the form. - -Problem Code (line 75 in forms.py): -exclude = ['groups', 'user_permissions', 'last_login', 'date_joined', 'is_active'] - -Solution: -fields = ['username', 'email', 'first_name', 'last_name', 'password'] - - -Vulnerability 4 -Something to do with: -SESSION_ENGINE = "django.contrib.sessions.backends.signed_cookies" - - -Vulnerability 5 -Exposure -The template base_backend.html inserts the username with a safe flag. This allows the potential for a JS script as a username to be rendered to the page, causing it to be run because it will not be escaped. - -Repair -Instead of declaring safe output with the safe flag, do not do that. - -Problem Code (line 58 of base_backend.html): - {{user.username|safe}} - -Solution: - {{user.username}} - - -Vulnerabilty 6 -Exposure -Website is generally at-risk for Cross Site Scripting attacks. By turning on Django's SECURE_BROWSER_XSS_FILTER setting, Django will add X-XSS-Protection to all responses. The browser will then automatically block all XSS attacks it can detect. - -Repair -Set SECURE_BROWSER_XSS_FILTER = True in settings.py - - -Vulnerability 7 -Exposure -All CRUD operations are vulnerable to Insecure Direct Object Reference (IDOR). Basically, in order to prevent unauthorized users or users with inappropriate permissions, the site should require all proper credentials and permissions from an user before a CRUD operation is performed. - -For Class Based Views, use the LoginReqiuredMixin and PermissionRequiredMixin - -Repair -Apply @login_required and @permission_required decorators for any function in views.py that performs a CRUD operation. - -Problem Code functions listed from views.py: - - manage_products - - manage_groups - - upload - - task_create - - task_edit - - task_delete - - project_delete - - project_edit - - project_create - - note_create - - note_edit - - note_delete - -Solution: -### For Function Views: -from django.contrib.auth.decorators import permission_required, login_required - -@login_required -@permission_required -def function_view_with_CRUD(request): - - -### For Class Based Views: -from django.contrib.auth.mixins import PermissionRequiredMixin, LoginRequiredMixin - -class ClassBasedCRUDView(LoginRequiredMixin, PermissionRequiredMixin, ClassView): From 01ec39b355f25430af6c480889468a6c14d64eb4 Mon Sep 17 00:00:00 2001 From: Rick Valenzuela Date: Tue, 21 Feb 2017 19:16:37 -0800 Subject: [PATCH 4/5] markdown formatting changes --- vulnerability-report.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/vulnerability-report.md b/vulnerability-report.md index 3ee8235c..95a2075b 100644 --- a/vulnerability-report.md +++ b/vulnerability-report.md @@ -1,8 +1,8 @@ #Vulnerability Report -###Reviewer 1: Ted Callahan -###Reviewer 2: Rick Valenzuela +Reviewer 1: Ted Callahan +Reviewer 2: Rick Valenzuela -###Date: February 1st, 2017 +Date: February 1st, 2017 ##Reviewing nVisium Task Manager @@ -72,4 +72,4 @@ fields = ['username', 'email', 'first_name', 'last_name', 'password'] ##Vulnerability 4 Something to do with: -SESSION_ENGINE = "django.contrib.sessions.backends.signed_cookies" \ No newline at end of file +SESSION_ENGINE = "django.contrib.sessions.backends.signed_cookies" From bce2a034320e1a276761ccd0341802230216314d Mon Sep 17 00:00:00 2001 From: Rick Valenzuela Date: Tue, 21 Feb 2017 19:57:28 -0800 Subject: [PATCH 5/5] wrote up A4, IDOR --- taskManager/views.py | 354 +++++++++++++++++++++------------------- vulnerability-report.md | 25 ++- 2 files changed, 205 insertions(+), 174 deletions(-) diff --git a/taskManager/views.py b/taskManager/views.py index 274de347..b81a4354 100644 --- a/taskManager/views.py +++ b/taskManager/views.py @@ -168,37 +168,38 @@ def manage_groups(request): def upload(request, project_id): + user = request.user + if user.is_authenticated(): + if request.method == 'POST': - if request.method == 'POST': - - proj = Project.objects.get(pk=project_id) - form = ProjectFileForm(request.POST, request.FILES) + 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']) + if form.is_valid(): + name = request.POST.get('name', False) + upload_path = store_uploaded_file(name, request.FILES['file']) - #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)) + #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)) - # file = File( - #name = name, - #path = upload_path, - # project = proj) + # file = File( + #name = name, + #path = upload_path, + # project = proj) - # file.save() + # file.save() - return redirect('/taskManager/' + project_id + - '/', {'new_file_added': True}) + return redirect('/taskManager/' + project_id + + '/', {'new_file_added': True}) + else: + form = ProjectFileForm() else: form = ProjectFileForm() - else: - form = ProjectFileForm() - return render_to_response( - 'taskManager/upload.html', {'form': form}, RequestContext(request)) + return render_to_response( + 'taskManager/upload.html', {'form': form}, RequestContext(request)) # A4: Insecure Direct Object Reference (IDOR) @@ -220,11 +221,13 @@ def download(request, file_id): def download_profile_pic(request, user_id): user = User.objects.get(pk=user_id) - filepath = user.userprofile.image - if len(filepath) > 1: - return redirect(filepath) - else: - return redirect('/static/taskManager/uploads/default.png') + if user.is_authenticated(): + + filepath = user.userprofile.image + if len(filepath) > 1: + return redirect(filepath) + else: + return redirect('/static/taskManager/uploads/default.png') #filename = user.get_full_name()+"."+filepath.split(".")[-1] # try: # abspath = open(filepath, 'rb') @@ -238,72 +241,76 @@ def download_profile_pic(request, user_id): def task_create(request, project_id): + user = request.user + if user.is_authenticated(): + if request.method == 'POST': - if request.method == 'POST': + proj = Project.objects.get(pk=project_id) - proj = Project.objects.get(pk=project_id) + text = request.POST.get('text', False) + task_title = request.POST.get('task_title', False) + now = timezone.now() + task_duedate = timezone.now() + datetime.timedelta(weeks=1) + if request.POST.get('task_duedate') != '': + task_duedate = datetime.datetime.fromtimestamp( + int(request.POST.get('task_duedate', False))) + + task = Task( + text=text, + title=task_title, + start_date=now, + due_date=task_duedate, + project=proj) - text = request.POST.get('text', False) - task_title = request.POST.get('task_title', False) - now = timezone.now() - task_duedate = timezone.now() + datetime.timedelta(weeks=1) - if request.POST.get('task_duedate') != '': - task_duedate = datetime.datetime.fromtimestamp( - int(request.POST.get('task_duedate', False))) - - task = Task( - text=text, - title=task_title, - start_date=now, - due_date=task_duedate, - project=proj) - - task.save() - task.users_assigned = [request.user] - - return redirect('/taskManager/' + project_id + - '/', {'new_task_added': True}) - else: - return render_to_response( - 'taskManager/task_create.html', {'proj_id': project_id}, RequestContext(request)) + task.save() + task.users_assigned = [request.user] + + return redirect('/taskManager/' + project_id + + '/', {'new_task_added': True}) + else: + return render_to_response( + 'taskManager/task_create.html', {'proj_id': project_id}, RequestContext(request)) # A4: Insecure Direct Object Reference (IDOR) def task_edit(request, project_id, task_id): + user = request.user + if user.is_authenticated(): + proj = Project.objects.get(pk=project_id) + task = Task.objects.get(pk=task_id) - proj = Project.objects.get(pk=project_id) - task = Task.objects.get(pk=task_id) - - if request.method == 'POST': + if request.method == 'POST': - if task.project == proj: + if task.project == proj: - text = request.POST.get('text', False) - task_title = request.POST.get('task_title', False) - task_completed = request.POST.get('task_completed', False) + text = request.POST.get('text', False) + task_title = request.POST.get('task_title', False) + task_completed = request.POST.get('task_completed', False) - task.title = task_title - task.text = text - task.completed = True if task_completed == "1" else False - task.save() + task.title = task_title + task.text = text + task.completed = True if task_completed == "1" else False + task.save() - return redirect('/taskManager/' + project_id + '/' + task_id) - else: - return render_to_response( - 'taskManager/task_edit.html', {'task': task}, RequestContext(request)) + return redirect('/taskManager/' + project_id + '/' + task_id) + else: + return render_to_response( + 'taskManager/task_edit.html', {'task': task}, RequestContext(request)) # A4: Insecure Direct Object Reference (IDOR) def task_delete(request, project_id, task_id): - proj = Project.objects.get(pk=project_id) - task = Task.objects.get(pk=task_id) - if proj is not None: - if task is not None and task.project == proj: - task.delete() + user = request.user + if user.is_authenticated(): + proj = Project.objects.get(pk=project_id) + task = Task.objects.get(pk=task_id) + if proj is not None: + if task is not None and task.project == proj: + task.delete() - return redirect('/taskManager/' + project_id + '/') + return redirect('/taskManager/' + project_id + '/') # A4: Insecure Direct Object Reference (IDOR) @@ -320,55 +327,57 @@ def task_complete(request, project_id, task_id): def project_create(request): + user = request.user + if user.is_authenticated(): + if request.method == 'POST': - if request.method == 'POST': - - title = request.POST.get('title', False) - text = request.POST.get('text', False) - project_priority = int(request.POST.get('project_priority', False)) - now = timezone.now() - project_duedate = timezone.make_aware(datetime.datetime.fromtimestamp( - int(request.POST.get('project_duedate', False)))) - - project = Project(title=title, - text=text, - priority=project_priority, - due_date=project_duedate, - start_date=now) - project.save() - project.users_assigned = [request.user.id] - - return redirect('/taskManager/', {'new_project_added': True}) - else: - return render_to_response( - 'taskManager/project_create.html', - {}, - RequestContext(request)) + title = request.POST.get('title', False) + text = request.POST.get('text', False) + project_priority = int(request.POST.get('project_priority', False)) + now = timezone.now() + project_duedate = timezone.make_aware(datetime.datetime.fromtimestamp( + int(request.POST.get('project_duedate', False)))) + + project = Project(title=title, + text=text, + priority=project_priority, + due_date=project_duedate, + start_date=now) + project.save() + project.users_assigned = [request.user.id] + + return redirect('/taskManager/', {'new_project_added': True}) + else: + return render_to_response( + 'taskManager/project_create.html', + {}, + RequestContext(request)) # A4: Insecure Direct Object Reference (IDOR) def project_edit(request, project_id): + user = request.user + if user.is_authenticated(): + proj = Project.objects.get(pk=project_id) - proj = Project.objects.get(pk=project_id) - - if request.method == 'POST': + if request.method == 'POST': - title = request.POST.get('title', False) - text = request.POST.get('text', False) - project_priority = int(request.POST.get('project_priority', False)) - project_duedate = datetime.datetime.fromtimestamp( - int(request.POST.get('project_duedate', False))) + title = request.POST.get('title', False) + text = request.POST.get('text', False) + project_priority = int(request.POST.get('project_priority', False)) + project_duedate = datetime.datetime.fromtimestamp( + int(request.POST.get('project_duedate', False))) - proj.title = title - proj.text = text - proj.priority = project_priority - proj.due_date = project_duedate - proj.save() + proj.title = title + proj.text = text + proj.priority = project_priority + proj.due_date = project_duedate + proj.save() - return redirect('/taskManager/' + project_id + '/') - else: - return render_to_response( - 'taskManager/project_edit.html', {'proj': proj}, RequestContext(request)) + return redirect('/taskManager/' + project_id + '/') + else: + return render_to_response( + 'taskManager/project_edit.html', {'proj': proj}, RequestContext(request)) # A4: Insecure Direct Object Reference (IDOR) @@ -509,85 +518,92 @@ def profile_view(request, user_id): def project_details(request, project_id): - proj = Project.objects.filter( - users_assigned=request.user.id, - pk=project_id) - if not proj: - messages.warning( - request, - 'You are not authorized to view this project') - return redirect('/taskManager/dashboard') - else: - proj = Project.objects.get(pk=project_id) - user_can_edit = request.user.has_perm('project_edit') + user = request.user + if user.is_authenticated(): + proj = Project.objects.filter( + users_assigned=request.user.id, + pk=project_id) + if not proj: + messages.warning( + request, + 'You are not authorized to view this project') + return redirect('/taskManager/dashboard') + else: + proj = Project.objects.get(pk=project_id) + user_can_edit = request.user.has_perm('project_edit') - return render(request, 'taskManager/project_details.html', - {'proj': proj, 'user_can_edit': user_can_edit}) + return render(request, 'taskManager/project_details.html', + {'proj': proj, 'user_can_edit': user_can_edit}) # A4: Insecure Direct Object Reference (IDOR) def note_create(request, project_id, task_id): - if request.method == 'POST': + user = request.user + if user.is_authenticated(): + if request.method == 'POST': - parent_task = Task.objects.get(pk=task_id) + parent_task = Task.objects.get(pk=task_id) - note_title = request.POST.get('note_title', False) - text = request.POST.get('text', False) + note_title = request.POST.get('note_title', False) + text = request.POST.get('text', False) - note = Notes( - title=note_title, - text=text, - user=request.user, - task=parent_task) + note = Notes( + title=note_title, + text=text, + user=request.user, + task=parent_task) - note.save() - return redirect('/taskManager/' + project_id + '/' + - task_id, {'new_note_added': True}) - else: - return render_to_response( - 'taskManager/note_create.html', {'task_id': task_id}, RequestContext(request)) + note.save() + return redirect('/taskManager/' + project_id + '/' + + task_id, {'new_note_added': True}) + else: + return render_to_response( + 'taskManager/note_create.html', {'task_id': task_id}, RequestContext(request)) # A4: Insecure Direct Object Reference (IDOR) def note_edit(request, project_id, task_id, note_id): + user = request.user + if user.is_authenticated(): + proj = Project.objects.get(pk=project_id) + task = Task.objects.get(pk=task_id) + note = Notes.objects.get(pk=note_id) - proj = Project.objects.get(pk=project_id) - task = Task.objects.get(pk=task_id) - note = Notes.objects.get(pk=note_id) - - if request.method == 'POST': + if request.method == 'POST': - if task.project == proj: + if task.project == proj: - if note.task == task: + if note.task == task: - text = request.POST.get('text', False) - note_title = request.POST.get('note_title', False) + text = request.POST.get('text', False) + note_title = request.POST.get('note_title', False) - note.title = note_title - note.text = text - note.save() + note.title = note_title + note.text = text + note.save() - return redirect('/taskManager/' + project_id + '/' + task_id) - else: - return render_to_response( - 'taskManager/note_edit.html', {'note': note}, RequestContext(request)) + return redirect('/taskManager/' + project_id + '/' + task_id) + else: + return render_to_response( + 'taskManager/note_edit.html', {'note': note}, RequestContext(request)) # A4: Insecure Direct Object Reference (IDOR) def note_delete(request, project_id, task_id, note_id): - proj = Project.objects.get(pk=project_id) - task = Task.objects.get(pk=task_id) - note = Notes.objects.get(pk=note_id) - if proj is not None: - if task is not None and task.project == proj: - if note is not None and note.task == task: - note.delete() + user = request.user + if user.is_authenticated(): + proj = Project.objects.get(pk=project_id) + task = Task.objects.get(pk=task_id) + note = Notes.objects.get(pk=note_id) + if proj is not None: + if task is not None and task.project == proj: + if note is not None and note.task == task: + note.delete() - return redirect('/taskManager/' + project_id + '/' + task_id) + return redirect('/taskManager/' + project_id + '/' + task_id) def task_details(request, project_id, task_id): @@ -701,7 +717,9 @@ def show_tutorial(request, vuln_id): def profile(request): - return render(request, 'taskManager/profile.html', {'user': request.user}) + user = request.user + if user.is_authenticated(): + return render(request, 'taskManager/profile.html', {'user': request.user}) # A4: Insecure Direct Object Reference (IDOR) # A8: Cross Site Request Forgery (CSRF) diff --git a/vulnerability-report.md b/vulnerability-report.md index 3ee8235c..20fcef37 100644 --- a/vulnerability-report.md +++ b/vulnerability-report.md @@ -6,7 +6,8 @@ ##Reviewing nVisium Task Manager -##Vulnerability 1 + +##Vulnerability A1: Injection ###Exposure We found an instance of an SQL Injection vulnerability in the file upload form (line 183 in views.py). @@ -33,7 +34,7 @@ project = proj) file.save() ``` -##Vulnerability 2 +##Vulnerability A2: Broken Authentication and Sesson Management ###Exposure Various upload functions call the store_uploaded_file() funtion at line 24 of misc.py. This function passes a given filename to the os.system function giving a user the potential to pass in shell commands as a filename. @@ -51,7 +52,7 @@ Comment out or remove lines 14-17 of templates/taskManager/upload.html: ``` -##Vulnerability 3 +##Vulnerability A3: Cross-Site Scripting ###Exposure The User registration form (class UserForm) as defined in forms.py (lines 71 - 75) is insecure because it does not explicitly state which fields are allowed. A user could add fields for other attributes of the Django user model (in this instance, is_superuser and is_staff). @@ -70,6 +71,18 @@ exclude = ['groups', 'user_permissions', 'last_login', 'date_joined', 'is_active fields = ['username', 'email', 'first_name', 'last_name', 'password'] ``` -##Vulnerability 4 -Something to do with: -SESSION_ENGINE = "django.contrib.sessions.backends.signed_cookies" \ No newline at end of file +##Vulnerability A4: Insecure Direct Object References +###Exposure +Users are not authenticated for pages that should require it; therefore, a user's data can be accessed, deleted or changed by others simply if another user feeds in the proper parameters to hit that page. Several views in views.py for a CRUD operation lacked an authentication check. These are the view functions that begin on lines: 112, 170, 221, 243, 277, 304, 329, 358, 520, 541, 567 and 719. + +By exploiting this problem, we were able to modify, delete and create tasks and files on another user's account. + +###Repair +At the start of each view function, add an authentication check. + +###Solution +add this to each function: +``` +user = request.user +if user.is_authenticated(): +```