From 081b17fbe89f49acf6bd33a96d6845f1b9160eb7 Mon Sep 17 00:00:00 2001 From: Saurabh Date: Wed, 11 Jul 2018 11:04:51 +0530 Subject: [PATCH 1/7] =?UTF-8?q?[security][fix]=20tighten=20criteria=20to?= =?UTF-8?q?=20prevent=20sql=20injection=20in=20search-f=E2=80=A6=20(#5800)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * [security][fix] tighten criteria to prevent sql injection in search-fields * test cases --- frappe/desk/search.py | 24 ++++++++++++++++++------ frappe/tests/test_search.py | 12 ++++++++++++ 2 files changed, 30 insertions(+), 6 deletions(-) diff --git a/frappe/desk/search.py b/frappe/desk/search.py index 67e52be218..f0c47316f0 100644 --- a/frappe/desk/search.py +++ b/frappe/desk/search.py @@ -7,31 +7,43 @@ import frappe, json from frappe.utils import cstr, unique from frappe import _ from six import string_types +import re def sanitize_searchfield(searchfield): blacklisted_keywords = ['select', 'delete', 'drop', 'update', 'case', 'and', 'or', 'like'] - def _raise_exception(): - frappe.throw(_('Invalid Search Field'), frappe.DataError) + def _raise_exception(searchfield): + frappe.throw(_('Invalid Search Field {0}').format(searchfield), frappe.DataError) + + if len(searchfield) == 1: + # do not allow special characters to pass as searchfields + regex = re.compile('^.*[=;*,\'"$\-+%#@()_].*') + if regex.match(searchfield): + _raise_exception(searchfield) if len(searchfield) >= 3: # to avoid 1=1 if '=' in searchfield: - _raise_exception() + _raise_exception(searchfield) # in mysql -- is used for commenting the query elif ' --' in searchfield: - _raise_exception() + _raise_exception(searchfield) # to avoid and, or and like elif any(' {0} '.format(keyword) in searchfield.split() for keyword in blacklisted_keywords): - _raise_exception() + _raise_exception(searchfield) # to avoid select, delete, drop, update and case elif any(keyword in searchfield.split() for keyword in blacklisted_keywords): - _raise_exception() + _raise_exception(searchfield) + + else: + regex = re.compile('^.*[=;*,\'"$\-+%#@()].*') + if any(regex.match(f) for f in searchfield.split()): + _raise_exception(searchfield) # this is called by the Link Field @frappe.whitelist() diff --git a/frappe/tests/test_search.py b/frappe/tests/test_search.py index ee4dd3cc53..9f1891753e 100644 --- a/frappe/tests/test_search.py +++ b/frappe/tests/test_search.py @@ -26,3 +26,15 @@ class TestSearch(unittest.TestCase): self.assertRaises(frappe.DataError, search_link, 'DocType', 'Customer', query=None, filters=None, page_length=20, searchfield='name or (select * from tabSessions)') + + self.assertRaises(frappe.DataError, + search_link, 'DocType', 'Customer', query=None, filters=None, + page_length=20, searchfield='*') + + self.assertRaises(frappe.DataError, + search_link, 'DocType', 'Customer', query=None, filters=None, + page_length=20, searchfield=';') + + self.assertRaises(frappe.DataError, + search_link, 'DocType', 'Customer', query=None, filters=None, + page_length=20, searchfield=';') From 6f84e922f8e64627d90565d88b914c311de1fac5 Mon Sep 17 00:00:00 2001 From: rohitwaghchaure Date: Wed, 11 Jul 2018 11:14:05 +0530 Subject: [PATCH 2/7] [Fix] Brute force security (#5785) * [Fix] Brute force security * Added patch and change the error message * Added test case --- frappe/auth.py | 50 ++++++- .../doctype/activity_log/test_activity_log.py | 45 ++++++- .../system_settings/system_settings.json | 123 +++++++++++++++++- frappe/exceptions.py | 2 +- frappe/patches.txt | 1 + .../patches/v10_0/set_default_locking_time.py | 8 ++ frappe/utils/password.py | 6 + 7 files changed, 229 insertions(+), 6 deletions(-) create mode 100644 frappe/patches/v10_0/set_default_locking_time.py diff --git a/frappe/auth.py b/frappe/auth.py index 40694bbfef..629f27ac69 100644 --- a/frappe/auth.py +++ b/frappe/auth.py @@ -8,13 +8,13 @@ from frappe import _ import frappe import frappe.database import frappe.utils -from frappe.utils import cint +from frappe.utils import cint, flt, get_datetime, datetime import frappe.utils.user from frappe import conf from frappe.sessions import Session, clear_sessions, delete_session from frappe.modules.patch_handler import check_session_stopped from frappe.translate import get_lang_code -from frappe.utils.password import check_password +from frappe.utils.password import check_password, delete_login_failed_cache from frappe.core.doctype.activity_log.activity_log import add_authentication_log from frappe.utils.background_jobs import enqueue from frappe.twofactor import (should_run_2fa, authenticate_for_2factor, @@ -207,6 +207,10 @@ class LoginManager: def check_if_enabled(self, user): """raise exception if user not enabled""" + doc = frappe.get_doc("System Settings") + if doc.allow_consecutive_login_attempts > 0: + check_consecutive_login_attempts(user, doc) + if user=='Administrator': return if not cint(frappe.db.get_value('User', user, 'enabled')): self.fail('User disabled or missing', user=user) @@ -217,6 +221,7 @@ class LoginManager: # returns user in correct case return check_password(user, pwd) except frappe.AuthenticationError: + self.update_invalid_login(user) self.fail('Incorrect password', user=user) def fail(self, message, user=None): @@ -227,6 +232,15 @@ class LoginManager: frappe.db.commit() raise frappe.AuthenticationError + def update_invalid_login(self, user): + last_login_tried = get_last_tried_login_data(user) + + failed_count = 0 + if last_login_tried > get_datetime(): + failed_count = get_login_failed_count(user) + + frappe.cache().hset('login_failed_count', user, failed_count + 1) + def run_trigger(self, event='on_login'): for method in frappe.get_hooks().get(event, []): frappe.call(frappe.get_attr(method), login_manager=self) @@ -335,3 +349,35 @@ def get_website_user_home_page(user): return '/' + home_page.strip('/') else: return '/me' + +def get_last_tried_login_data(user, get_last_login=False): + locked_account_time = frappe.cache().hget('locked_account_time', user) + if get_last_login and locked_account_time: + return locked_account_time + + last_login_tried = frappe.cache().hget('last_login_tried', user) + if not last_login_tried or last_login_tried < get_datetime(): + last_login_tried = get_datetime() + datetime.timedelta(seconds=60) + + frappe.cache().hset('last_login_tried', user, last_login_tried) + + return last_login_tried + +def get_login_failed_count(user): + return cint(frappe.cache().hget('login_failed_count', user)) or 0 + +def check_consecutive_login_attempts(user, doc): + login_failed_count = get_login_failed_count(user) + last_login_tried = (get_last_tried_login_data(user, True) + + datetime.timedelta(seconds=doc.allow_login_after_fail)) + + if login_failed_count >= cint(doc.allow_consecutive_login_attempts): + locked_account_time = frappe.cache().hget('locked_account_time', user) + if not locked_account_time: + frappe.cache().hset('locked_account_time', user, get_datetime()) + + if last_login_tried > get_datetime(): + frappe.throw(_("Your account has been locked and will resume after {0} seconds") + .format(doc.allow_login_after_fail), frappe.SecurityException) + else: + delete_login_failed_cache(user) \ No newline at end of file diff --git a/frappe/core/doctype/activity_log/test_activity_log.py b/frappe/core/doctype/activity_log/test_activity_log.py index e2f749ff04..895dde01f6 100644 --- a/frappe/core/doctype/activity_log/test_activity_log.py +++ b/frappe/core/doctype/activity_log/test_activity_log.py @@ -5,10 +5,11 @@ from __future__ import unicode_literals import frappe import unittest +import time +from frappe.auth import LoginManager, CookieManager class TestActivityLog(unittest.TestCase): def test_activity_log(self): - from frappe.auth import LoginManager, CookieManager # test user login log frappe.local.form_dict = frappe._dict({ @@ -44,4 +45,44 @@ class TestActivityLog(unittest.TestCase): name = names[0] auth_log = frappe.get_doc('Activity Log', name) - return auth_log \ No newline at end of file + return auth_log + + def test_brute_security(self): + update_system_settings({ + 'allow_consecutive_login_attempts': 3, + 'allow_login_after_fail': 5 + }) + + frappe.local.form_dict = frappe._dict({ + 'cmd': 'login', + 'sid': 'Guest', + 'pwd': 'admin', + 'usr': 'Administrator' + }) + + frappe.local.cookie_manager = CookieManager() + frappe.local.login_manager = LoginManager() + + auth_log = self.get_auth_log() + self.assertEquals(auth_log.status, 'Success') + + # test user logout log + frappe.local.login_manager.logout() + auth_log = self.get_auth_log(operation='Logout') + self.assertEquals(auth_log.status, 'Success') + + # test invalid login + frappe.form_dict.update({ 'pwd': 'password' }) + self.assertRaises(frappe.AuthenticationError, LoginManager) + self.assertRaises(frappe.AuthenticationError, LoginManager) + self.assertRaises(frappe.AuthenticationError, LoginManager) + self.assertRaises(frappe.SecurityException, LoginManager) + time.sleep(5) + self.assertRaises(frappe.AuthenticationError, LoginManager) + + frappe.local.form_dict = frappe._dict() + +def update_system_settings(args): + doc = frappe.get_doc('System Settings') + doc.update(args) + doc.save() diff --git a/frappe/core/doctype/system_settings/system_settings.json b/frappe/core/doctype/system_settings/system_settings.json index 092e9440b1..537a2d6869 100644 --- a/frappe/core/doctype/system_settings/system_settings.json +++ b/frappe/core/doctype/system_settings/system_settings.json @@ -958,6 +958,127 @@ "set_only_once": 0, "unique": 0 }, + { + "allow_bulk_edit": 0, + "allow_on_submit": 0, + "bold": 0, + "collapsible": 1, + "columns": 0, + "fieldname": "brute_force_security", + "fieldtype": "Section Break", + "hidden": 0, + "ignore_user_permissions": 0, + "ignore_xss_filter": 0, + "in_filter": 0, + "in_global_search": 0, + "in_list_view": 0, + "in_standard_filter": 0, + "label": "Brute Force Security", + "length": 0, + "no_copy": 0, + "permlevel": 0, + "precision": "", + "print_hide": 0, + "print_hide_if_no_value": 0, + "read_only": 0, + "remember_last_selected_value": 0, + "report_hide": 0, + "reqd": 0, + "search_index": 0, + "set_only_once": 0, + "unique": 0 + }, + { + "allow_bulk_edit": 0, + "allow_on_submit": 0, + "bold": 0, + "collapsible": 0, + "columns": 0, + "fieldname": "allow_consecutive_login_attempts", + "fieldtype": "Int", + "hidden": 0, + "ignore_user_permissions": 0, + "ignore_xss_filter": 0, + "in_filter": 0, + "in_global_search": 0, + "in_list_view": 0, + "in_standard_filter": 0, + "label": "Allow Consecutive Login Attempts ", + "length": 0, + "no_copy": 0, + "permlevel": 0, + "precision": "", + "print_hide": 0, + "print_hide_if_no_value": 0, + "read_only": 0, + "remember_last_selected_value": 0, + "report_hide": 0, + "reqd": 0, + "search_index": 0, + "set_only_once": 0, + "unique": 0 + }, + { + "allow_bulk_edit": 0, + "allow_on_submit": 0, + "bold": 0, + "collapsible": 0, + "columns": 0, + "fieldname": "column_break_34", + "fieldtype": "Column Break", + "hidden": 0, + "ignore_user_permissions": 0, + "ignore_xss_filter": 0, + "in_filter": 0, + "in_global_search": 0, + "in_list_view": 0, + "in_standard_filter": 0, + "length": 0, + "no_copy": 0, + "permlevel": 0, + "precision": "", + "print_hide": 0, + "print_hide_if_no_value": 0, + "read_only": 0, + "remember_last_selected_value": 0, + "report_hide": 0, + "reqd": 0, + "search_index": 0, + "set_only_once": 0, + "unique": 0 + }, + { + "allow_bulk_edit": 0, + "allow_on_submit": 0, + "bold": 0, + "collapsible": 0, + "columns": 0, + "default": "60", + "description": "In seconds", + "fieldname": "allow_login_after_fail", + "fieldtype": "Int", + "hidden": 0, + "ignore_user_permissions": 0, + "ignore_xss_filter": 0, + "in_filter": 0, + "in_global_search": 0, + "in_list_view": 0, + "in_standard_filter": 0, + "label": "Allow Login After Fail", + "length": 0, + "no_copy": 0, + "permlevel": 0, + "precision": "", + "print_hide": 0, + "print_hide_if_no_value": 0, + "read_only": 0, + "remember_last_selected_value": 0, + "report_hide": 0, + "reqd": 0, + "search_index": 0, + "set_only_once": 0, + "unique": 0 + }, { "allow_bulk_edit": 0, "allow_on_submit": 0, @@ -1311,7 +1432,7 @@ "issingle": 1, "istable": 0, "max_attachments": 0, - "modified": "2017-10-15 20:29:46.700707", + "modified": "2018-07-06 16:33:49.222058", "modified_by": "Administrator", "module": "Core", "name": "System Settings", diff --git a/frappe/exceptions.py b/frappe/exceptions.py index de37c7e481..3a8548a6e0 100644 --- a/frappe/exceptions.py +++ b/frappe/exceptions.py @@ -83,4 +83,4 @@ class ImplicitCommitError(ValidationError): pass class RetryBackgroundJobError(Exception): pass class DocumentLockedError(ValidationError): pass class CircularLinkingError(ValidationError): pass - +class SecurityException(Exception): pass diff --git a/frappe/patches.txt b/frappe/patches.txt index de06c712d3..3c8884b52b 100644 --- a/frappe/patches.txt +++ b/frappe/patches.txt @@ -203,3 +203,4 @@ execute:frappe.delete_doc('Page', 'data-import-tool', ignore_missing=True) frappe.patches.v10_0.reload_countries_and_currencies frappe.patches.v10_0.set_no_copy_to_workflow_state frappe.patches.v10_0.increase_single_table_column_length +frappe.patches.v10_0.set_default_locking_time \ No newline at end of file diff --git a/frappe/patches/v10_0/set_default_locking_time.py b/frappe/patches/v10_0/set_default_locking_time.py new file mode 100644 index 0000000000..045fa0e3fa --- /dev/null +++ b/frappe/patches/v10_0/set_default_locking_time.py @@ -0,0 +1,8 @@ +# Copyright (c) 2015, Frappe Technologies Pvt. Ltd. and Contributors +# MIT License. See license.txt + +import frappe + +def execute(): + frappe.reload_doc("core", "doctype", "system_settings") + frappe.db.set_value('System Settings', None, "allow_login_after_fail", 60) \ No newline at end of file diff --git a/frappe/utils/password.py b/frappe/utils/password.py index 24ab2d58e3..1246a64ba9 100644 --- a/frappe/utils/password.py +++ b/frappe/utils/password.py @@ -44,9 +44,15 @@ def check_password(user, pwd, doctype='User', fieldname='password'): # lettercase agnostic user = auth[0].name + delete_login_failed_cache(user) return user +def delete_login_failed_cache(user): + frappe.cache().hdel('last_login_tried', user) + frappe.cache().hdel('login_failed_count', user) + frappe.cache().hdel('locked_account_time', user) + def update_password(user, pwd, doctype='User', fieldname='password', logout_all_sessions=False): ''' Update the password for the User From 7293311f72c0975b6f95e299d59944e616cde760 Mon Sep 17 00:00:00 2001 From: rohitwaghchaure Date: Wed, 11 Jul 2018 11:56:01 +0530 Subject: [PATCH 3/7] [Fix] Handle the OverflowError exception in the in_words method (#5769) --- frappe/utils/data.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/frappe/utils/data.py b/frappe/utils/data.py index b0b70bd644..e407446a1d 100644 --- a/frappe/utils/data.py +++ b/frappe/utils/data.py @@ -542,6 +542,8 @@ def in_words(integer, in_million=True): ret = num2words(integer, lang=locale) except NotImplementedError: ret = num2words(integer, lang='en') + except OverflowError: + ret = num2words(integer, lang='en') return ret.replace('-', ' ') def is_html(text): From b40d6f9a7122b35847bf80dd1b487cd3b2af4154 Mon Sep 17 00:00:00 2001 From: Saurabh Date: Wed, 11 Jul 2018 11:02:27 +0530 Subject: [PATCH 4/7] test case fixes for db query engin --- frappe/tests/test_db_query.py | 32 +++++++++++++++----------------- 1 file changed, 15 insertions(+), 17 deletions(-) diff --git a/frappe/tests/test_db_query.py b/frappe/tests/test_db_query.py index 07d47ca722..28b608f08e 100644 --- a/frappe/tests/test_db_query.py +++ b/frappe/tests/test_db_query.py @@ -157,29 +157,27 @@ class TestReportview(unittest.TestCase): or_filters=[['DocType', 'istable', '=', 1], ['DocType', 'beta and 1=1', '=', 0]], limit_start=0, limit_page_length=1) - data = DatabaseQuery("DocType").execute(fields=["name", "issingle"], + out = DatabaseQuery("DocType").execute(fields=["name"], filters={'editable_grid': 1, 'module': 'Core'}, - or_filters=[['DocType', 'istable', '=', 1]], - limit_start=0, limit_page_length=1) - self.assertEquals('DocField', data[0]['name']) + or_filters=[['DocType', 'istable', '=', 1]], order_by='creation') + self.assertTrue('DocField' in [d['name'] for d in out]) - data = DatabaseQuery("DocType").execute(fields=["name"], + out = DatabaseQuery("DocType").execute(fields=["name"], filters={'issingle': 1}, or_filters=[['DocType', 'module', '=', 'Core']], - limit_start=0, limit_page_length=1) - self.assertEquals('User Permission for Page and Report', data[0]['name']) + order_by='creation') + self.assertTrue('User Permission for Page and Report' in [d['name'] for d in out]) - data = DatabaseQuery("DocType").execute(fields=["name"], + out = DatabaseQuery("DocType").execute(fields=["name"], filters={'track_changes': 1, 'module': 'Core'}, - limit_start=0, limit_page_length=4) - self.assertEquals(['File', 'Version', 'Data Import', 'Domain Settings'], - [d['name'] for d in data]) + order_by='creation') + self.assertTrue('File' in [d['name'] for d in out]) - data = DatabaseQuery("DocType").execute(fields=["name"], - filters=[['DocType', 'ifnull(track_changes, 0)', '=', 0], - ['DocType', 'module', '=', 'Core']], - limit_start=0, limit_page_length=4) - self.assertEquals(['DocField', 'User Permission for Page and Report', 'SMS Settings', 'Block Module'], - [d['name'] for d in data]) + out = DatabaseQuery("DocType").execute(fields=["name"], + filters=[ + ['DocType', 'ifnull(track_changes, 0)', '=', 0], + ['DocType', 'module', '=', 'Core'] + ], order_by='creation') + self.assertTrue('DefaultValue' in [d['name'] for d in out]) def create_event(subject="_Test Event", starts_on=None): """ create a test event """ From 2f3fabc4f8badf2caf9879577a40fdc07413ac33 Mon Sep 17 00:00:00 2001 From: rohitwaghchaure Date: Thu, 12 Jul 2018 16:29:59 +0530 Subject: [PATCH 5/7] [Fix] Disable Show rows with zero values checkbox, still showin zero rows in the print (#5817) --- frappe/public/js/frappe/views/reports/grid_report.js | 4 ++-- frappe/public/js/lib/microtemplate.js | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/frappe/public/js/frappe/views/reports/grid_report.js b/frappe/public/js/frappe/views/reports/grid_report.js index da8c3a9bf4..958634c8ac 100644 --- a/frappe/public/js/frappe/views/reports/grid_report.js +++ b/frappe/public/js/frappe/views/reports/grid_report.js @@ -146,6 +146,7 @@ frappe.views.GridReport = Class.extend({ }, setup_filters: function() { var me = this; + $.each(me.filter_inputs, function(i, v) { var opts = v.get(0).opts; if(opts.fieldtype == "Select" && in_list(me.doctypes, opts.link)) { @@ -175,7 +176,7 @@ frappe.views.GridReport = Class.extend({ this.page.add_menu_item(__("Print"), function() { frappe.ui.get_print_settings(false, function(print_settings) { - frappe.render_grid({grid: me.grid, title: me.page.title, print_settings: print_settings }); + frappe.render_grid({grid: me.grid, title: me.page.title, print_settings: print_settings, report: me}); }); }, true); @@ -381,7 +382,6 @@ frappe.views.GridReport = Class.extend({ background-color: #eee; '>") .appendTo(this.wrapper); this.id = frappe.dom.set_unique_id(this.grid_wrapper.get(0)); - // zero-value check $('
\