From 049d51cdf17b06168b4fe7672be8ce01fff0edd2 Mon Sep 17 00:00:00 2001 From: Saurabh Date: Tue, 28 Feb 2017 17:59:03 +0530 Subject: [PATCH 1/7] [security][fix] fixed order by and group by clause vulnerability for sql injection --- frappe/model/db_query.py | 34 ++++++++++++++++++++++------------ 1 file changed, 22 insertions(+), 12 deletions(-) diff --git a/frappe/model/db_query.py b/frappe/model/db_query.py index d64eef4976..2233f51e2a 100644 --- a/frappe/model/db_query.py +++ b/frappe/model/db_query.py @@ -136,11 +136,12 @@ class DatabaseQuery(object): self.set_field_tables() args.fields = ', '.join(self.fields) - - self.set_order_by(args) - self.check_sort_by_table(args.order_by) + meta = frappe.get_meta(self.doctype) + self.set_order_by(args, meta) + self.validate_order_by_and_group_by_params(args.order_by, meta) args.order_by = args.order_by and (" order by " + args.order_by) or "" + self.validate_order_by_and_group_by_params(self.group_by, meta) args.group_by = self.group_by and (" group by " + self.group_by) or "" return args @@ -441,8 +442,7 @@ class DatabaseQuery(object): query = query.replace('%(key)s', 'name') return frappe.db.sql(query, as_dict = (not self.as_list)) - def set_order_by(self, args): - meta = frappe.get_meta(self.doctype) + def set_order_by(self, args, meta): if self.order_by: args.order_by = self.order_by else: @@ -475,13 +475,23 @@ class DatabaseQuery(object): if meta.is_submittable: args.order_by = "`tab{0}`.docstatus asc, {1}".format(self.doctype, args.order_by) - def check_sort_by_table(self, order_by): - if "." in order_by: - tbl = order_by.split('.')[0] - if tbl not in self.tables: - if tbl.startswith('`'): - tbl = tbl[4:-1] - frappe.throw(_("Please select atleast 1 column from {0} to sort").format(tbl)) + def validate_order_by_and_group_by_params(self, parameters, meta): + """ + Clause cases: + 1. check for . to split table and columns and check for `tab prefix + 2. elif check field in meta + """ + for field in parameters.split(","): + if "." in field and field.startswith("`tab"): + tbl = field.split('.')[0] + if tbl not in self.tables: + if tbl.startswith('`'): + tbl = tbl[4:-1] + frappe.throw(_("Please select atleast 1 column from {0} to sort").format(tbl)) + else: + field = field.strip().split(' ')[0] + if field not in [f.fieldname for f in meta.fields]: + frappe.throw(_("{0} invalid field in clause").format(field)) def add_limit(self): if self.limit_page_length: From acb660322d8ab3e8f269b85c56299e2ebdf0f304 Mon Sep 17 00:00:00 2001 From: Saurabh Date: Tue, 28 Feb 2017 19:15:39 +0530 Subject: [PATCH 2/7] [fix] trim whitespaces --- frappe/model/db_query.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/frappe/model/db_query.py b/frappe/model/db_query.py index 2233f51e2a..d075f29e83 100644 --- a/frappe/model/db_query.py +++ b/frappe/model/db_query.py @@ -481,9 +481,12 @@ class DatabaseQuery(object): 1. check for . to split table and columns and check for `tab prefix 2. elif check field in meta """ + if not parameters: + return + for field in parameters.split(","): - if "." in field and field.startswith("`tab"): - tbl = field.split('.')[0] + if "." in field and field.strip().startswith("`tab"): + tbl = field.strip().split('.')[0] if tbl not in self.tables: if tbl.startswith('`'): tbl = tbl[4:-1] From 25bce97acda7272c1b4976c71c5438c4af7d0fb8 Mon Sep 17 00:00:00 2001 From: Nabin Hait Date: Thu, 2 Mar 2017 12:57:49 +0530 Subject: [PATCH 3/7] Update db_query.py --- frappe/model/db_query.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frappe/model/db_query.py b/frappe/model/db_query.py index d075f29e83..11a0859cbe 100644 --- a/frappe/model/db_query.py +++ b/frappe/model/db_query.py @@ -490,11 +490,11 @@ class DatabaseQuery(object): if tbl not in self.tables: if tbl.startswith('`'): tbl = tbl[4:-1] - frappe.throw(_("Please select atleast 1 column from {0} to sort").format(tbl)) + frappe.throw(_("Please select atleast 1 column from {0} to sort/group").format(tbl)) else: field = field.strip().split(' ')[0] if field not in [f.fieldname for f in meta.fields]: - frappe.throw(_("{0} invalid field in clause").format(field)) + frappe.throw(_("Invalid field used to sort/group: {0}").format(field)) def add_limit(self): if self.limit_page_length: From 29d9abb7bd50fb8d06e3e7b46c0f470778c2427e Mon Sep 17 00:00:00 2001 From: Nabin Hait Date: Thu, 2 Mar 2017 13:01:41 +0530 Subject: [PATCH 4/7] Update db_query.py --- frappe/model/db_query.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frappe/model/db_query.py b/frappe/model/db_query.py index 11a0859cbe..06a0f5d0fb 100644 --- a/frappe/model/db_query.py +++ b/frappe/model/db_query.py @@ -10,7 +10,7 @@ import frappe.share import frappe.permissions from frappe.utils import flt, cint, getdate, get_datetime, get_time, make_filter_tuple, get_filter, add_to_date from frappe import _ -from frappe.model import optional_fields +from frappe.model import optional_fields, default_fields from frappe.model.utils.list_settings import get_list_settings, update_list_settings class DatabaseQuery(object): @@ -493,7 +493,7 @@ class DatabaseQuery(object): frappe.throw(_("Please select atleast 1 column from {0} to sort/group").format(tbl)) else: field = field.strip().split(' ')[0] - if field not in [f.fieldname for f in meta.fields]: + if field not in [f.fieldname for f in meta.fields] and field not in default_fields: frappe.throw(_("Invalid field used to sort/group: {0}").format(field)) def add_limit(self): From ab862b060c73393c369942c353fe8484bdcf2a11 Mon Sep 17 00:00:00 2001 From: mbauskar Date: Thu, 2 Mar 2017 16:39:22 +0530 Subject: [PATCH 5/7] [minor] filter dashboard fixes --- frappe/public/js/frappe/ui/listing.js | 2 +- frappe/public/js/frappe/views/reports/reportview.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/frappe/public/js/frappe/ui/listing.js b/frappe/public/js/frappe/ui/listing.js index 1790443efd..6a08015f6d 100644 --- a/frappe/public/js/frappe/ui/listing.js +++ b/frappe/public/js/frappe/ui/listing.js @@ -413,7 +413,7 @@ frappe.ui.Listing = Class.extend({ query += ' LIMIT ' + this.start + ',' + (this.page_length+1); return query }, - set_filter: function(parent, fieldname, label, no_run, no_duplicate) { + set_filter: function(fieldname, label, no_run, no_duplicate, parent) { var filter = this.filter_list.get_filter(fieldname); doctype = parent && this.doctype != parent? parent: this.doctype diff --git a/frappe/public/js/frappe/views/reports/reportview.js b/frappe/public/js/frappe/views/reports/reportview.js index 4b2f14161f..7eacd88f9b 100644 --- a/frappe/public/js/frappe/views/reports/reportview.js +++ b/frappe/public/js/frappe/views/reports/reportview.js @@ -350,7 +350,7 @@ frappe.views.ReportView = frappe.ui.Listing.extend({ }); if(!filter_set) { - this.set_filter(parent, fieldname, value); + this.set_filter(fieldname, value, false, false, parent); } else { var df = frappe.meta.get_docfield(parent, fieldname); if(df.fieldtype==='Link') { From b2b2df56aadcd5eb8aea6fe9565e4e113fa01d5e Mon Sep 17 00:00:00 2001 From: Saurabh Date: Thu, 2 Mar 2017 16:51:20 +0530 Subject: [PATCH 6/7] [fix] consider optional fields too while chekcing sql injection --- frappe/model/db_query.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/model/db_query.py b/frappe/model/db_query.py index 06a0f5d0fb..f7ea70c2db 100644 --- a/frappe/model/db_query.py +++ b/frappe/model/db_query.py @@ -493,7 +493,7 @@ class DatabaseQuery(object): frappe.throw(_("Please select atleast 1 column from {0} to sort/group").format(tbl)) else: field = field.strip().split(' ')[0] - if field not in [f.fieldname for f in meta.fields] and field not in default_fields: + if field not in [f.fieldname for f in meta.fields] and field not in (default_fields + optional_fields): frappe.throw(_("Invalid field used to sort/group: {0}").format(field)) def add_limit(self): From cefd90d0905895aec1fc9f369a1ff7a45508a50d Mon Sep 17 00:00:00 2001 From: Nabin Hait Date: Thu, 2 Mar 2017 17:29:25 +0600 Subject: [PATCH 7/7] bumped to version 7.2.25 --- frappe/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/__init__.py b/frappe/__init__.py index a717c7c6ed..097134b996 100644 --- a/frappe/__init__.py +++ b/frappe/__init__.py @@ -13,7 +13,7 @@ import os, sys, importlib, inspect, json from .exceptions import * from .utils.jinja import get_jenv, get_template, render_template -__version__ = '7.2.24' +__version__ = '7.2.25' __title__ = "Frappe Framework" local = Local()