From f3b2ba50f0facf160a9ae357f5a6e1e0c4a43b88 Mon Sep 17 00:00:00 2001 From: Prateeksha Singh Date: Fri, 22 Jun 2018 03:51:33 +0530 Subject: [PATCH 01/10] [global-search] trim varchar columns --- frappe/database.py | 5 +++++ frappe/utils/global_search.py | 9 ++++++--- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/frappe/database.py b/frappe/database.py index b7c982ace2..ce8f2c0c8f 100644 --- a/frappe/database.py +++ b/frappe/database.py @@ -15,6 +15,7 @@ import frappe.model.meta from frappe.utils import now, get_datetime, cstr from frappe import _ from frappe.model.utils.link_count import flush_local_link_count +from frappe.model.db_schema import varchar_len from frappe.utils.background_jobs import execute_job, get_queue # imports - compatibility imports @@ -913,6 +914,10 @@ class Database: return s + def trim_varchar(self, s): + """Trim given string to the defined varchar length""" + return s[:int(varchar_len)] + def enqueue_jobs_after_commit(): if frappe.flags.enqueue_after_commit and len(frappe.flags.enqueue_after_commit) > 0: for job in frappe.flags.enqueue_after_commit: diff --git a/frappe/utils/global_search.py b/frappe/utils/global_search.py index 5811d78057..d9c83081cc 100644 --- a/frappe/utils/global_search.py +++ b/frappe/utils/global_search.py @@ -142,8 +142,8 @@ def rebuild_for_doctype(doctype): "name": frappe.db.escape(doc.name), "content": frappe.db.escape(' ||| '.join(content or '')), "published": published, - "title": frappe.db.escape(title or ''), - "route": frappe.db.escape(route or '') + "title": frappe.db.trim_varchar(frappe.db.escape(title or '')), + "route": frappe.db.trim_varchar(frappe.db.escape(route or '')) }) if all_contents: insert_values_for_multiple_docs(all_contents) @@ -257,9 +257,12 @@ def update_global_search(doc): if hasattr(doc, 'is_website_published') and doc.meta.allow_guest_to_view: published = 1 if doc.is_website_published() else 0 + title = frappe.db.trim_varchar(doc.get_title()) + route = frappe.db.trim_varchar(doc.get('route')) + frappe.flags.update_global_search.append( dict(doctype=doc.doctype, name=doc.name, content=' ||| '.join(content or ''), - published=published, title=doc.get_title(), route=doc.get('route'))) + published=published, title=title, route=route)) enqueue_global_search() From 76be772e4c9b385967340903e77229a639a8dac6 Mon Sep 17 00:00:00 2001 From: Prateeksha Singh Date: Fri, 22 Jun 2018 17:21:00 +0530 Subject: [PATCH 02/10] [global-search] trim strings directly --- frappe/database.py | 5 ----- frappe/utils/global_search.py | 9 +++++---- 2 files changed, 5 insertions(+), 9 deletions(-) diff --git a/frappe/database.py b/frappe/database.py index ce8f2c0c8f..b7c982ace2 100644 --- a/frappe/database.py +++ b/frappe/database.py @@ -15,7 +15,6 @@ import frappe.model.meta from frappe.utils import now, get_datetime, cstr from frappe import _ from frappe.model.utils.link_count import flush_local_link_count -from frappe.model.db_schema import varchar_len from frappe.utils.background_jobs import execute_job, get_queue # imports - compatibility imports @@ -914,10 +913,6 @@ class Database: return s - def trim_varchar(self, s): - """Trim given string to the defined varchar length""" - return s[:int(varchar_len)] - def enqueue_jobs_after_commit(): if frappe.flags.enqueue_after_commit and len(frappe.flags.enqueue_after_commit) > 0: for job in frappe.flags.enqueue_after_commit: diff --git a/frappe/utils/global_search.py b/frappe/utils/global_search.py index d9c83081cc..952c1dbac2 100644 --- a/frappe/utils/global_search.py +++ b/frappe/utils/global_search.py @@ -8,6 +8,7 @@ import re import redis from frappe.utils import cint, strip_html_tags from frappe.model.base_document import get_controller +from frappe.model.db_schema import varchar_len from six import text_type @@ -142,8 +143,8 @@ def rebuild_for_doctype(doctype): "name": frappe.db.escape(doc.name), "content": frappe.db.escape(' ||| '.join(content or '')), "published": published, - "title": frappe.db.trim_varchar(frappe.db.escape(title or '')), - "route": frappe.db.trim_varchar(frappe.db.escape(route or '')) + "title": frappe.db.escape(title or '')[:int(varchar_len)], + "route": frappe.db.escape(route or '')[:int(varchar_len)] }) if all_contents: insert_values_for_multiple_docs(all_contents) @@ -257,8 +258,8 @@ def update_global_search(doc): if hasattr(doc, 'is_website_published') and doc.meta.allow_guest_to_view: published = 1 if doc.is_website_published() else 0 - title = frappe.db.trim_varchar(doc.get_title()) - route = frappe.db.trim_varchar(doc.get('route')) + title = doc.get_title()[:int(varchar_len)] + route = doc.get('route')[:int(varchar_len)] frappe.flags.update_global_search.append( dict(doctype=doc.doctype, name=doc.name, content=' ||| '.join(content or ''), From cf1338bbcae2568f9630b7ab2bbb3ae5c9d13d8b Mon Sep 17 00:00:00 2001 From: Prateeksha Singh Date: Mon, 25 Jun 2018 09:18:31 +0530 Subject: [PATCH 03/10] [global-search] route check --- frappe/utils/global_search.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/frappe/utils/global_search.py b/frappe/utils/global_search.py index 952c1dbac2..a3d728cc2f 100644 --- a/frappe/utils/global_search.py +++ b/frappe/utils/global_search.py @@ -258,12 +258,10 @@ def update_global_search(doc): if hasattr(doc, 'is_website_published') and doc.meta.allow_guest_to_view: published = 1 if doc.is_website_published() else 0 - title = doc.get_title()[:int(varchar_len)] - route = doc.get('route')[:int(varchar_len)] - frappe.flags.update_global_search.append( dict(doctype=doc.doctype, name=doc.name, content=' ||| '.join(content or ''), - published=published, title=title, route=route)) + published=published, title=doc.get_title()[:int(varchar_len)], route=doc.get('route'))) + enqueue_global_search() From e9cdf322c6fbdc738d417ff2f13898ec171d86ce Mon Sep 17 00:00:00 2001 From: Saurabh Date: Mon, 25 Jun 2018 11:23:32 +0530 Subject: [PATCH 04/10] [security][fix] Sanitize search fields to avoid sql injection (#5713) * [security][fix] Sanitize search fields to avoid sql injection * Test Cases for Sanitizer * Test Cases fix * [fix] test case --- frappe/desk/search.py | 28 ++++++++++++++++++++++++++++ frappe/tests/test_search.py | 28 ++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+) create mode 100644 frappe/tests/test_search.py diff --git a/frappe/desk/search.py b/frappe/desk/search.py index 8527076afe..67e52be218 100644 --- a/frappe/desk/search.py +++ b/frappe/desk/search.py @@ -8,6 +8,31 @@ from frappe.utils import cstr, unique from frappe import _ from six import string_types + +def sanitize_searchfield(searchfield): + blacklisted_keywords = ['select', 'delete', 'drop', 'update', 'case', 'and', 'or', 'like'] + + def _raise_exception(): + frappe.throw(_('Invalid Search Field'), frappe.DataError) + + if len(searchfield) >= 3: + + # to avoid 1=1 + if '=' in searchfield: + _raise_exception() + + # in mysql -- is used for commenting the query + elif ' --' in searchfield: + _raise_exception() + + # to avoid and, or and like + elif any(' {0} '.format(keyword) in searchfield.split() for keyword in blacklisted_keywords): + _raise_exception() + + # to avoid select, delete, drop, update and case + elif any(keyword in searchfield.split() for keyword in blacklisted_keywords): + _raise_exception() + # this is called by the Link Field @frappe.whitelist() def search_link(doctype, txt, query=None, filters=None, page_length=20, searchfield=None): @@ -24,6 +49,9 @@ def search_widget(doctype, txt, query=None, searchfield=None, start=0, meta = frappe.get_meta(doctype) + if searchfield: + sanitize_searchfield(searchfield) + if not searchfield: searchfield = "name" diff --git a/frappe/tests/test_search.py b/frappe/tests/test_search.py new file mode 100644 index 0000000000..ee4dd3cc53 --- /dev/null +++ b/frappe/tests/test_search.py @@ -0,0 +1,28 @@ +# Copyright (c) 2015, Frappe Technologies Pvt. Ltd. and Contributors +# MIT License. See license.txt + +from __future__ import unicode_literals +import unittest +import frappe +from frappe.desk.search import search_link + + +class TestSearch(unittest.TestCase): + def test_search_field_sanitizer(self): + # pass + search_link('DocType', 'User', query=None, filters=None, page_length=20, searchfield='name') + result = frappe.response['results'][0] + self.assertTrue('User' in result['value']) + + #raise exception on injection + self.assertRaises(frappe.DataError, + search_link, 'DocType', 'Customer', query=None, filters=None, + page_length=20, searchfield='1=1') + + self.assertRaises(frappe.DataError, + search_link, 'DocType', 'Customer', query=None, filters=None, + page_length=20, searchfield='select * from tabSessions) --') + + self.assertRaises(frappe.DataError, + search_link, 'DocType', 'Customer', query=None, filters=None, + page_length=20, searchfield='name or (select * from tabSessions)') From 9e2e65f3054c52652b3491713f28d80b80efdfd3 Mon Sep 17 00:00:00 2001 From: Rushabh Mehta Date: Mon, 25 Jun 2018 16:00:01 +0530 Subject: [PATCH 05/10] [fix] varchar_len in global_search.py --- frappe/utils/global_search.py | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/frappe/utils/global_search.py b/frappe/utils/global_search.py index a3d728cc2f..0f640083f7 100644 --- a/frappe/utils/global_search.py +++ b/frappe/utils/global_search.py @@ -11,7 +11,6 @@ from frappe.model.base_document import get_controller from frappe.model.db_schema import varchar_len from six import text_type - def setup_global_search_table(): """ Creates __global_seach table @@ -20,17 +19,16 @@ def setup_global_search_table(): if not '__global_search' in frappe.db.get_tables(): frappe.db.sql('''create table __global_search( doctype varchar(100), - name varchar(140), - title varchar(140), + name varchar({varchar_len}), + title varchar({varchar_len}), content text, fulltext(content), - route varchar(140), + route varchar({varchar_len}), published int(1) not null default 0, unique `doctype_name` (doctype, name)) COLLATE=utf8mb4_unicode_ci ENGINE=MyISAM - CHARACTER SET=utf8mb4''') - + CHARACTER SET=utf8mb4'''.format(varchar_len=varchar_len)) def reset(): """ From e2b1ebe84cb848e79caef763594e45d734b86475 Mon Sep 17 00:00:00 2001 From: Saurabh Date: Wed, 27 Jun 2018 09:15:42 +0530 Subject: [PATCH 06/10] [Security][fix] To avoid possible sql injection via filters and or_filters parameters and tighten the field level checks (#5721) * [fix] sanitize filters and or_filters to avoid sql injection * add test cases for filter sanitizer * codacy fix * added test cases to test valid scenarios --- frappe/model/db_query.py | 21 ++++++++++----- frappe/tests/test_db_query.py | 49 ++++++++++++++++++++++++++++++++++- frappe/utils/data.py | 23 ++++++++++++++++ 3 files changed, 86 insertions(+), 7 deletions(-) diff --git a/frappe/model/db_query.py b/frappe/model/db_query.py index dd0b386570..9413980506 100644 --- a/frappe/model/db_query.py +++ b/frappe/model/db_query.py @@ -140,7 +140,7 @@ class DatabaseQuery(object): if self.or_conditions: args.conditions += (' or ' if args.conditions else "") + \ - ' or '.join(self.or_conditions) + ' or '.join(self.or_conditions) self.set_field_tables() @@ -190,7 +190,8 @@ class DatabaseQuery(object): As field contains `,` and mysql function `version()`, with the help of regex the system will filter out this field. ''' - regex = re.compile('^.*[,();].*') + + sub_query_regex = re.compile("^.*[,();].*") blacklisted_keywords = ['select', 'create', 'insert', 'delete', 'drop', 'update', 'case'] blacklisted_functions = ['concat', 'concat_ws', 'if', 'ifnull', 'nullif', 'coalesce', 'connection_id', 'current_user', 'database', 'last_insert_id', 'session_user', @@ -200,14 +201,22 @@ class DatabaseQuery(object): frappe.throw(_('Cannot use sub-query or function in fields'), frappe.DataError) for field in self.fields: - if regex.match(field): - if any(keyword in field.lower() for keyword in blacklisted_keywords): + if sub_query_regex.match(field): + if any(keyword in field.lower().split() for keyword in blacklisted_keywords): _raise_exception() - if any("{0}(".format(keyword) in field.lower() \ - for keyword in blacklisted_functions): + if any("({0}".format(keyword) in field.lower() for keyword in blacklisted_keywords): _raise_exception() + if any("{0}(".format(keyword) in field.lower() for keyword in blacklisted_functions): + _raise_exception() + + if re.compile("[a-zA-Z]+\s*'").match(field): + _raise_exception() + + if re.compile('[a-zA-Z]+\s*,').match(field): + _raise_exception() + def extract_tables(self): """extract tables from fields""" self.tables = ['`tab' + self.doctype + '`'] diff --git a/frappe/tests/test_db_query.py b/frappe/tests/test_db_query.py index 484a309534..1540703a01 100644 --- a/frappe/tests/test_db_query.py +++ b/frappe/tests/test_db_query.py @@ -117,6 +117,12 @@ class TestReportview(unittest.TestCase): fields=["name", "issingle, IF(issingle=1, (SELECT name from tabUser), count(*))"], limit_start=0, limit_page_length=1) + self.assertRaises(frappe.DataError, DatabaseQuery("DocType").execute, + fields=["name", "issingle ''"],limit_start=0, limit_page_length=1) + + self.assertRaises(frappe.DataError, DatabaseQuery("DocType").execute, + fields=["name", "issingle,'"],limit_start=0, limit_page_length=1) + data = DatabaseQuery("DocType").execute(fields=["name", "issingle", "count(name)"], limit_start=0, limit_page_length=1) self.assertTrue('count(name)' in data[0]) @@ -133,6 +139,47 @@ class TestReportview(unittest.TestCase): "datediff(modified, creation) as date_diff"], limit_start=0, limit_page_length=1) self.assertTrue('date_diff' in data[0]) + def test_filter_sanitizer(self): + self.assertRaises(frappe.DataError, DatabaseQuery("DocType").execute, + fields=["name"], filters={'istable,': 1}, limit_start=0, limit_page_length=1) + + self.assertRaises(frappe.DataError, DatabaseQuery("DocType").execute, + fields=["name"], filters={'editable_grid,': 1}, or_filters={'istable,': 1}, + limit_start=0, limit_page_length=1) + + self.assertRaises(frappe.DataError, DatabaseQuery("DocType").execute, + fields=["name"], filters={'editable_grid,': 1}, + or_filters=[['DocType', 'istable,', '=', 1]], + limit_start=0, limit_page_length=1) + + self.assertRaises(frappe.DataError, DatabaseQuery("DocType").execute, + fields=["name"], filters={'editable_grid,': 1}, + 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"], + filters={'editable_grid': 1}, or_filters=[['DocType', 'istable', '=', 1]], + limit_start=0, limit_page_length=1) + self.assertEquals('DocField', data[0]['name']) + + data = 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']) + + data = 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]) + + 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]) + def create_event(subject="_Test Event", starts_on=None): """ create a test event """ @@ -145,4 +192,4 @@ def create_event(subject="_Test Event", starts_on=None): "starts_on": get_datetime(starts_on), }).insert(ignore_permissions=True) - return event \ No newline at end of file + return event diff --git a/frappe/utils/data.py b/frappe/utils/data.py index 86fef0b61c..b0b70bd644 100644 --- a/frappe/utils/data.py +++ b/frappe/utils/data.py @@ -801,6 +801,8 @@ def get_filter(doctype, f): f = frappe._dict(doctype=f[0], fieldname=f[1], operator=f[2], value=f[3]) + sanitize_column(f.fieldname) + if not f.operator: # if operator is missing f.operator = "=" @@ -840,6 +842,27 @@ def make_filter_dict(filters): return _filter +def sanitize_column(column_name): + from frappe import _ + regex = re.compile("^.*[,'();].*") + blacklisted_keywords = ['select', 'create', 'insert', 'delete', 'drop', 'update', 'case', 'and', 'or'] + + def _raise_exception(): + frappe.throw(_("Invalid field name {0}").format(column_name), frappe.DataError) + + if 'ifnull' in column_name: + if regex.match(column_name): + # to avoid and, or + if any(' {0} '.format(keyword) in column_name.split() for keyword in blacklisted_keywords): + _raise_exception() + + # to avoid select, delete, drop, update and case + elif any(keyword in column_name.split() for keyword in blacklisted_keywords): + _raise_exception() + + elif regex.match(column_name): + _raise_exception() + def scrub_urls(html): html = expand_relative_urls(html) # encoding should be responsibility of the composer From f4313f6b12a2387c210abb5bbb38b80c33105399 Mon Sep 17 00:00:00 2001 From: Faris Ansari Date: Mon, 2 Jul 2018 11:34:08 +0530 Subject: [PATCH 07/10] Remove !important rules so that custom css in Print Format can override (#5762) --- frappe/public/css/bootstrap.css | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/frappe/public/css/bootstrap.css b/frappe/public/css/bootstrap.css index 3bcdd13eb9..c4db43f199 100644 --- a/frappe/public/css/bootstrap.css +++ b/frappe/public/css/bootstrap.css @@ -193,9 +193,9 @@ th { *, *:before, *:after { - color: #000 !important; + color: #000; + background: transparent; text-shadow: none !important; - background: transparent !important; -webkit-box-shadow: none !important; box-shadow: none !important; } @@ -254,11 +254,11 @@ th { } .table td, .table th { - background-color: #fff !important; + background-color: #fff; } .table-bordered th, .table-bordered td { - border: 1px solid #ddd !important; + border: 1px solid #ddd; } } @font-face { From 5c6b02515a195555436ffc371f8046a1f7486c94 Mon Sep 17 00:00:00 2001 From: Ameya Shenoy Date: Tue, 3 Jul 2018 14:55:21 +0530 Subject: [PATCH 08/10] regex fix (#5765) courtesy: @netchanpfaris initally the regex used to allow only digits 0-9, not it considers the entire number range --- frappe/model/naming.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/model/naming.py b/frappe/model/naming.py index d3be62e88e..6c909f133f 100644 --- a/frappe/model/naming.py +++ b/frappe/model/naming.py @@ -202,7 +202,7 @@ def append_number_if_name_exists(doctype, value, fieldname='name', separator='-' filters.update({fieldname: value}) exists = frappe.db.exists(doctype, filters) - regex = '^{value}{separator}[[:digit:]]$'.format(value=re.escape(value), separator=separator) + regex = '^{value}{separator}\d+$'.format(value=re.escape(value), separator=separator) if exists: last = frappe.db.sql("""select {fieldname} from `tab{doctype}` where {fieldname} regexp %s From 9aeb79b8748e6f9f9ec92dfac022b0b0e6faafde Mon Sep 17 00:00:00 2001 From: Saurabh Date: Thu, 5 Jul 2018 17:33:30 +0530 Subject: [PATCH 09/10] [fix][test-case] filter sanitizer --- frappe/tests/test_db_query.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/frappe/tests/test_db_query.py b/frappe/tests/test_db_query.py index 1540703a01..07d47ca722 100644 --- a/frappe/tests/test_db_query.py +++ b/frappe/tests/test_db_query.py @@ -158,7 +158,8 @@ class TestReportview(unittest.TestCase): limit_start=0, limit_page_length=1) data = DatabaseQuery("DocType").execute(fields=["name", "issingle"], - filters={'editable_grid': 1}, or_filters=[['DocType', 'istable', '=', 1]], + filters={'editable_grid': 1, 'module': 'Core'}, + or_filters=[['DocType', 'istable', '=', 1]], limit_start=0, limit_page_length=1) self.assertEquals('DocField', data[0]['name']) From aec04731db6ca30ed4ec302dac713c553a0883e4 Mon Sep 17 00:00:00 2001 From: Saurabh Date: Fri, 6 Jul 2018 13:56:04 +0600 Subject: [PATCH 10/10] bumped to version 10.1.38 --- frappe/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/__init__.py b/frappe/__init__.py index 85aa4e660a..2ee63ff47c 100644 --- a/frappe/__init__.py +++ b/frappe/__init__.py @@ -14,7 +14,7 @@ import os, sys, importlib, inspect, json from .exceptions import * from .utils.jinja import get_jenv, get_template, render_template, get_email_from_template -__version__ = '10.1.37' +__version__ = '10.1.38' __title__ = "Frappe Framework" local = Local()