From 2a77dcca518f621b7348dbd4f099216dbb2048e7 Mon Sep 17 00:00:00 2001 From: William Benjamin Shields Date: Mon, 6 Feb 2017 16:53:29 -0800 Subject: [PATCH 01/12] initial commit for vulnerability report. --- vulnerability-report.md | 63 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 63 insertions(+) create mode 100644 vulnerability-report.md diff --git a/vulnerability-report.md b/vulnerability-report.md new file mode 100644 index 00000000..8ff1df71 --- /dev/null +++ b/vulnerability-report.md @@ -0,0 +1,63 @@ +Vulnerability Report +Reviewer 1: Ben Shields + +Reviewer 2: Conor Clary + +Date: Feb 5, 2016 + +Reviewing nVisium Task Manager + + +**A1 - Injection:** +Exposure + +Repair + +**A2 - Broken Auth:** +Exposure + +Repair + +**A3 - XSS:** +Exposure + +Repair + +**A4 - Insecure DOR:** +Exposure + +Repair + + + + +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``` + +```Some body of relevant code that solves our problem``` +Vulnerability 2 +Exposure +We found an instance of [vulnerability 2] 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 + +```Some body of relevant code that solves our problem``` \ No newline at end of file From a0a1120472f8b2d3f5e22a485350a1531f1df24f Mon Sep 17 00:00:00 2001 From: William Benjamin Shields Date: Mon, 6 Feb 2017 17:15:54 -0800 Subject: [PATCH 02/12] a4 dor findings. --- vulnerability-report.md | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/vulnerability-report.md b/vulnerability-report.md index 8ff1df71..c037a8ee 100644 --- a/vulnerability-report.md +++ b/vulnerability-report.md @@ -25,6 +25,19 @@ Repair **A4 - Insecure DOR:** 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 From f3632e2f164df0da75b9dafd224def691fcf9f06 Mon Sep 17 00:00:00 2001 From: William Benjamin Shields Date: Mon, 6 Feb 2017 17:30:22 -0800 Subject: [PATCH 03/12] fix for upload view and report repair fo a4. --- taskManager/views.py | 42 ++++++++++++++++++++++------------------- vulnerability-report.md | 35 ++++++++-------------------------- 2 files changed, 31 insertions(+), 46 deletions(-) diff --git a/taskManager/views.py b/taskManager/views.py index 274de347..cdef9f2c 100644 --- a/taskManager/views.py +++ b/taskManager/views.py @@ -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( diff --git a/vulnerability-report.md b/vulnerability-report.md index c037a8ee..077adff7 100644 --- a/vulnerability-report.md +++ b/vulnerability-report.md @@ -41,36 +41,17 @@ Many instances of Insecure Direct Object References. You can navigate to functio Repair +This is an example repair for the upload file to project view. +```def upload(request, project_id): + if request.method == 'POST': -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``` - -```Some body of relevant code that solves our problem``` -Vulnerability 2 -Exposure -We found an instance of [vulnerability 2] 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): + user = request.user -problem_file1.py + if user.is_authenticated(): -```Some body of relevant code that solves our problem``` -problem_file2.py + etc. +``` -```Some body of relevant code that solves our problem``` \ No newline at end of file +The user must be authenticated to successfully post. However, the user can still navigate to this page by url. From e9ad80347d257ecdf387314082ef13e8dd45530c Mon Sep 17 00:00:00 2001 From: William Benjamin Shields Date: Mon, 6 Feb 2017 17:34:11 -0800 Subject: [PATCH 04/12] edit for code in report. --- vulnerability-report.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/vulnerability-report.md b/vulnerability-report.md index 077adff7..a8ba74bb 100644 --- a/vulnerability-report.md +++ b/vulnerability-report.md @@ -43,7 +43,8 @@ Repair This is an example repair for the upload file to project view. -```def upload(request, project_id): +``` +def upload(request, project_id): if request.method == 'POST': From 8fdc3a036df745a9c16afd3815f1d36d8fb536e9 Mon Sep 17 00:00:00 2001 From: William Benjamin Shields Date: Mon, 6 Feb 2017 17:35:41 -0800 Subject: [PATCH 05/12] python indicator for code block. --- vulnerability-report.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vulnerability-report.md b/vulnerability-report.md index a8ba74bb..a833d820 100644 --- a/vulnerability-report.md +++ b/vulnerability-report.md @@ -43,7 +43,7 @@ Repair This is an example repair for the upload file to project view. -``` +```python def upload(request, project_id): if request.method == 'POST': From ac20c85174af9db0ecec6270b36d0d1853efa733 Mon Sep 17 00:00:00 2001 From: William Benjamin Shields Date: Mon, 6 Feb 2017 18:06:40 -0800 Subject: [PATCH 06/12] a5 security misconfiguration fix and added to report. --- taskManager/settings.py | 4 ++-- vulnerability-report.md | 48 +++++++++++++++++++++++++++++++++++++++-- 2 files changed, 48 insertions(+), 4 deletions(-) diff --git a/taskManager/settings.py b/taskManager/settings.py index 17bb6978..c936af5c 100644 --- a/taskManager/settings.py +++ b/taskManager/settings.py @@ -25,8 +25,8 @@ # 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 = [] diff --git a/vulnerability-report.md b/vulnerability-report.md index a833d820..adc2d8bd 100644 --- a/vulnerability-report.md +++ b/vulnerability-report.md @@ -24,7 +24,8 @@ Exposure Repair **A4 - Insecure DOR:** -Exposure + +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) @@ -39,7 +40,7 @@ Many instances of Insecure Direct Object References. You can navigate to functio /taskManager/project_id/task_edit/task_id : access specific tasks and edit them -Repair +Repair: This is an example repair for the upload file to project view. @@ -56,3 +57,46 @@ def upload(request, project_id): ``` The user must be authenticated to successfully post. However, the user can still navigate to this page by url. + +**A5 - Security Misconfiguration:** + +Exposure: +Debug being left set to True in a production level application is an example of Security Misconfiguration. Other examples include + +Repair: +In settings.py: + +```python +DEBUG = False +TEMPLATE_DEBUG = False +``` + +**A6 - Sensitive Data Exposure:** + +Exposure + +Repair + +```python + +``` + +**A7 - Missing Function Level Access Control:** + +Exposure: + +Repair + +```python + +``` + +**A4 - Cross-Site Request Forgery:** + +Exposure + +Repair + +```python + +``` \ No newline at end of file From 8a4df10e97e62183b37a247e8d0a494e116a7e77 Mon Sep 17 00:00:00 2001 From: William Benjamin Shields Date: Mon, 6 Feb 2017 18:45:42 -0800 Subject: [PATCH 07/12] better hashing algorithm in settings.py, authorization in manage_group view, updated report for a6 and a7. --- taskManager/settings.py | 2 +- taskManager/views.py | 2 +- vulnerability-report.md | 28 +++++++++++++++++++++++----- 3 files changed, 25 insertions(+), 7 deletions(-) diff --git a/taskManager/settings.py b/taskManager/settings.py index c936af5c..af3b2edb 100644 --- a/taskManager/settings.py +++ b/taskManager/settings.py @@ -28,7 +28,7 @@ DEBUG = False TEMPLATE_DEBUG = False -ALLOWED_HOSTS = [] +ALLOWED_HOSTS = ['127.0.0.1'] # Application definition diff --git a/taskManager/views.py b/taskManager/views.py index cdef9f2c..21e3e8aa 100644 --- a/taskManager/views.py +++ b/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() diff --git a/vulnerability-report.md b/vulnerability-report.md index adc2d8bd..b91b7930 100644 --- a/vulnerability-report.md +++ b/vulnerability-report.md @@ -61,7 +61,7 @@ The user must be authenticated to successfully post. However, the user can still **A5 - Security Misconfiguration:** Exposure: -Debug being left set to True in a production level application is an example of Security Misconfiguration. Other examples include +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: @@ -69,25 +69,43 @@ In settings.py: ```python DEBUG = False TEMPLATE_DEBUG = False + +ALLOWED_HOSTS = ['127.0.0.1:8000'] ``` **A6 - Sensitive Data Exposure:** -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 +Repair: +django.contrib.auth.hashers.py has a bunch of other hashing algorithms available. -```python +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 +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. ``` From 0c1c24850ca2e4ec38a18f08cb8dc9d922aedbfb Mon Sep 17 00:00:00 2001 From: William Benjamin Shields Date: Mon, 6 Feb 2017 18:59:19 -0800 Subject: [PATCH 08/12] fixed csrf-exempt views. added a8 to report. --- taskManager/views.py | 7 +++---- vulnerability-report.md | 17 +++++++++++++++-- 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/taskManager/views.py b/taskManager/views.py index 21e3e8aa..63114108 100644 --- a/taskManager/views.py +++ b/taskManager/views.py @@ -711,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) @@ -740,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': @@ -780,7 +779,7 @@ def reset_password(request): # Vuln: Username Enumeration -@csrf_exempt + def forgot_password(request): if request.method == 'POST': @@ -814,7 +813,7 @@ def forgot_password(request): # A8: Cross Site Request Forgery (CSRF) -@csrf_exempt + def change_password(request): if request.method == 'POST': diff --git a/vulnerability-report.md b/vulnerability-report.md index b91b7930..f18266b5 100644 --- a/vulnerability-report.md +++ b/vulnerability-report.md @@ -111,10 +111,23 @@ def manage_groups(request): **A4 - Cross-Site Request Forgery:** -Exposure +Exposure: +Several views are CSRF exempt (employ the @csrf-exempt decorator). Django does not check validity for these views. -Repair +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. ``` \ No newline at end of file From afb678ac86c982b435a6ef44ada266fefdf2aef8 Mon Sep 17 00:00:00 2001 From: William Benjamin Shields Date: Mon, 6 Feb 2017 19:07:20 -0800 Subject: [PATCH 09/12] report formatting. --- vulnerability-report.md | 1 + 1 file changed, 1 insertion(+) diff --git a/vulnerability-report.md b/vulnerability-report.md index f18266b5..7ac17f34 100644 --- a/vulnerability-report.md +++ b/vulnerability-report.md @@ -1,4 +1,5 @@ Vulnerability Report + Reviewer 1: Ben Shields Reviewer 2: Conor Clary From b7083ce2beba779a945d73d6affee12774401d40 Mon Sep 17 00:00:00 2001 From: William Benjamin Shields Date: Mon, 6 Feb 2017 22:45:46 -0800 Subject: [PATCH 10/12] Update vulnerability-report.md --- vulnerability-report.md | 82 ++++++++++++++++++++++++++++++++++------- 1 file changed, 68 insertions(+), 14 deletions(-) diff --git a/vulnerability-report.md b/vulnerability-report.md index 7ac17f34..c10e6f9b 100644 --- a/vulnerability-report.md +++ b/vulnerability-report.md @@ -9,25 +9,82 @@ Date: Feb 5, 2016 Reviewing nVisium Task Manager -**A1 - Injection:** -Exposure +**A1 - Injection** -Repair +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:** -Exposure -Repair +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. -**A3 - XSS:** -Exposure +``` +class UserForm(forms.ModelForm): + """ User registration form """ -Repair + 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:** -Exposure: -Many instances of Insecure Direct Object References. You can navigate to functions that should really be protected accessing a variety of routes. +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) @@ -45,7 +102,6 @@ Repair: This is an example repair for the upload file to project view. -```python def upload(request, project_id): if request.method == 'POST': @@ -55,8 +111,6 @@ def upload(request, project_id): 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:** @@ -131,4 +185,4 @@ def forgot_password(request): def change_password(request): etc. -``` \ No newline at end of file +``` From a0a36e5bf75f3a2c2b13e21c2c6f2414838bdee3 Mon Sep 17 00:00:00 2001 From: William Benjamin Shields Date: Mon, 6 Feb 2017 22:46:01 -0800 Subject: [PATCH 11/12] Update vulnerability-report.md --- vulnerability-report.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vulnerability-report.md b/vulnerability-report.md index c10e6f9b..15e1ce12 100644 --- a/vulnerability-report.md +++ b/vulnerability-report.md @@ -9,7 +9,7 @@ Date: Feb 5, 2016 Reviewing nVisium Task Manager -**A1 - Injection** +**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. From 954659e156b87d26b9ce7bed6c5fac3bb86eef77 Mon Sep 17 00:00:00 2001 From: William Benjamin Shields Date: Mon, 6 Feb 2017 22:46:34 -0800 Subject: [PATCH 12/12] Update vulnerability-report.md --- vulnerability-report.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vulnerability-report.md b/vulnerability-report.md index 15e1ce12..9af44699 100644 --- a/vulnerability-report.md +++ b/vulnerability-report.md @@ -1,4 +1,4 @@ -Vulnerability Report +# Vulnerability Report Reviewer 1: Ben Shields