initial implementation of safe variables

okbob · okbob · commit 9d2e909d0598 · 2019-04-21T10:40:23.000+02:00
diff --git a/README.md b/README.md
@@ -297,6 +297,13 @@ should be redesigned or plpgsql_check should be disabled for this function.
 <i>A usage of plpgsql_check adds a small overhead (in enabled passive mode) and you should use
 it only in develop or preprod environments.</i>
 
+<aside class="warning">
+Attention: The SQL injection check can detect only some SQL injection vulnerabilities. This tool
+cannot be used for security audit! Some issues should not be detected. This check can raise false
+alarms too - probably when variable is sanitized by other command or when value is of some compose
+type.
+</aside>
+
 ## Dynamic SQL
 
 This module doesn't check queries that are assembled in runtime. It is not possible
diff --git a/src/check_expr.c b/src/check_expr.c
@@ -757,6 +757,28 @@ plpgsql_check_expr_as_rvalue(PLpgSQL_checkstate *cstate, PLpgSQL_expr *expr,
 		tupdesc = plpgsql_check_expr_get_desc(cstate, expr, use_element_type, expand, is_expression, &first_level_typoid);
 		is_immutable_null = is_const_null_expr(cstate, expr);
 
+		/* try to detect safe variables */
+		if (cstate->cinfo->security_warnings && targetdno != -1)
+		{
+			PLpgSQL_var *var = (PLpgSQL_var *) cstate->estate->datums[targetdno];
+
+			if (var->dtype == PLPGSQL_DTYPE_VAR)
+			{
+				bool	typispreferred;
+				char 	typcategory;
+
+				get_type_category_preferred(var->datatype->typoid, &typcategory, &typispreferred);
+				if (typcategory == 'S')
+				{
+					Node *node = plpgsql_check_expr_get_node(cstate, expr, false);
+					int		location;
+
+					if (!plpgsql_check_is_sql_injection_vulnerable(cstate, expr, node, &location))
+						cstate->safe_variables = bms_add_member(cstate->safe_variables, targetdno);
+				}
+			}
+		}
+
 		if (expected_typoid != InvalidOid && type_is_rowtype(expected_typoid) && first_level_typoid != InvalidOid)
 		{
 			/* simple error, scalar source to composite target */
diff --git a/src/check_function.c b/src/check_function.c
@@ -1009,7 +1009,6 @@ plpgsql_check_setup_cstate(PLpgSQL_checkstate *cstate,
 	cstate->fake_rtd = fake_rtd;
 
 	cstate->safe_variables = NULL;
-	cstate->string_variables = NULL;
 
 	cstate->stop_check = false;
 }
diff --git a/src/expr_walk.c b/src/expr_walk.c
@@ -21,6 +21,7 @@
 typedef struct 
 {
 	PLpgSQL_checkstate *cstate;
+	PLpgSQL_expr *expr;
 	char *query_str;
 } check_funcexpr_walker_params;
 
@@ -29,7 +30,8 @@ static int check_fmt_string(const char *fmt,
 							int location,
 							check_funcexpr_walker_params *wp,
 							bool *is_error,
-							int *unsafe_expr_location);
+							int *unsafe_expr_location,
+							bool no_error);
 
 /*
  * Send to ouput all not yet displayed relations and functions.
@@ -223,7 +225,7 @@ check_funcexpr_walker(Node *node, void *context)
 
 							wp = (check_funcexpr_walker_params *) context;
 
-							required_nargs = check_fmt_string(fmt, fexpr->args, c->location, wp, &is_error, NULL);
+							required_nargs = check_fmt_string(fmt, fexpr->args, c->location, wp, &is_error, NULL, false);
 							if (!is_error && required_nargs != -1)
 							{
 								if (required_nargs + 1 != list_length(fexpr->args))
@@ -251,6 +253,7 @@ plpgsql_check_funcexpr(PLpgSQL_checkstate *cstate, Query *query, char *query_str
 
 	wp.cstate = cstate;
 	wp.query_str = query_str;
+	wp.expr = NULL;
 
 	check_funcexpr_walker((Node *) query, &wp);
 }
@@ -480,15 +483,17 @@ text_format_parse_format(const char *start_ptr,
 	}
 
 /*
- * Returns number of rquired arguments or -1 when we cannot detect this number
+ * Returns number of rquired arguments or -1 when we cannot detect this number.
+ * When no_error is true, then this function doesn't raise a error or warning.
  */
 static int
 check_fmt_string(const char *fmt,
 				 List *args,
 				 int location,
 				 check_funcexpr_walker_params *wp,
 				 bool *is_error,
-				 int *unsafe_expr_location)
+				 int *unsafe_expr_location,
+				 bool no_error)
 {
 	const char	   *cp;
 	const char	   *end_ptr = fmt + strlen(fmt);
@@ -535,7 +540,7 @@ check_fmt_string(const char *fmt,
 			appendStringInfo(&sinfo,
 					"unrecognized format() type specifier \"%c\"", *cp);
 
-			if (wp)
+			if (!no_error)
 				plpgsql_check_put_error(wp->cstate,
 										ERRCODE_INVALID_PARAMETER_VALUE, 0,
 										sinfo.data,
@@ -578,7 +583,7 @@ check_fmt_string(const char *fmt,
 				{
 					Node *arg = list_nth(args, argn - 1);
 
-					if (plpgsql_check_is_sql_injection_vulnerable(arg, unsafe_expr_location))
+					if (plpgsql_check_is_sql_injection_vulnerable(wp->cstate, wp->expr, arg, unsafe_expr_location))
 					{
 						/* found vulnerability, stop */
 						*is_error = false;
@@ -740,10 +745,13 @@ plpgsql_check_qual_has_fishy_cast(PlannedStmt *plannedstmt, Plan *plan, Param **
 
 /*
  * Recursive iterate to deep and search extern params with typcategory "S", and check
- * if this value is sanitized.
+ * if this value is sanitized. Flag is_safe is true, when result is safe.
  */
 bool
-plpgsql_check_is_sql_injection_vulnerable(Node *node, int *location)
+plpgsql_check_is_sql_injection_vulnerable(PLpgSQL_checkstate *cstate,
+										  PLpgSQL_expr *expr,
+										  Node *node,
+										  int *location)
 {
 	if (IsA(node, FuncExpr))
 	{
@@ -755,7 +763,7 @@ plpgsql_check_is_sql_injection_vulnerable(Node *node, int *location)
 		{
 			Node *arg = lfirst(lc);
 
-			if (plpgsql_check_is_sql_injection_vulnerable(arg, location))
+			if (plpgsql_check_is_sql_injection_vulnerable(cstate, expr, arg, location))
 			{
 				is_vulnerable = true;
 				break;
@@ -793,10 +801,15 @@ plpgsql_check_is_sql_injection_vulnerable(Node *node, int *location)
 								if (c->consttype == TEXTOID && !c->constisnull)
 								{
 									char *fmt = TextDatumGetCString(c->constvalue);
+									check_funcexpr_walker_params wp;
 									bool	is_error;
 
+									wp.cstate = cstate;
+									wp.expr = expr;
+									wp.query_str = expr->query;
+
 									*location = -1;
-									check_fmt_string(fmt, fexpr->args, c->location, NULL, &is_error, location);
+									check_fmt_string(fmt, fexpr->args, c->location, &wp, &is_error, location, true);
 
 									/* only in this case, "format" function obviously sanitize parameters */
 									if (!is_error)
@@ -826,7 +839,7 @@ plpgsql_check_is_sql_injection_vulnerable(Node *node, int *location)
 		{
 			Node *arg = lfirst(lc);
 
-			if (plpgsql_check_is_sql_injection_vulnerable(arg, location))
+			if (plpgsql_check_is_sql_injection_vulnerable(cstate, expr, arg, location))
 			{
 				is_vulnerable = true;
 				break;
@@ -861,24 +874,48 @@ plpgsql_check_is_sql_injection_vulnerable(Node *node, int *location)
 	}
 	else if (IsA(node, NamedArgExpr))
 	{
-		return plpgsql_check_is_sql_injection_vulnerable((Node *) ((NamedArgExpr *) node)->arg, location);
+		return plpgsql_check_is_sql_injection_vulnerable(cstate, expr, (Node *) ((NamedArgExpr *) node)->arg, location);
 	}
 	else if (IsA(node, RelabelType))
 	{
-		return plpgsql_check_is_sql_injection_vulnerable((Node *) ((RelabelType *) node)->arg, location);
+		return plpgsql_check_is_sql_injection_vulnerable(cstate, expr, (Node *) ((RelabelType *) node)->arg, location);
 	}
 	else if (IsA(node, Param))
 	{
 		Param *p = (Param *) node;
 
-		if (p->paramkind == PARAM_EXTERN)
+		if (p->paramkind == PARAM_EXTERN || p->paramkind == PARAM_EXEC)
 		{
 			bool	typispreferred;
 			char 	typcategory;
 
 			get_type_category_preferred(p->paramtype, &typcategory, &typispreferred);
 			if (typcategory == 'S')
 			{
+				if (p->paramkind == PARAM_EXTERN && p->paramid > 0 && p->location != -1)
+				{
+					int			dno = p->paramid - 1;
+
+					/*
+					 * When paramid looks well and related datum is variable with same
+					 * type, then we can check, if this variable has sanitized content
+					 * already.
+					 */
+					if (expr && bms_is_member(dno, expr->paramnos))
+					{
+						PLpgSQL_var *var = (PLpgSQL_var *) cstate->estate->datums[dno];
+
+						if (var->dtype == PLPGSQL_DTYPE_VAR)
+						{
+							if (var->datatype->typoid == p->paramtype)
+							{
+								if (bms_is_member(dno, cstate->safe_variables))
+									return false;
+							}
+						}
+					}
+				}
+
 				*location = p->location;
 				return true;
 			}
diff --git a/src/plpgsql_check.h b/src/plpgsql_check.h
@@ -102,7 +102,6 @@ typedef struct PLpgSQL_checkstate
 	plpgsql_check_result_info *result_info;
 	plpgsql_check_info *cinfo;
 	Bitmapset	   *safe_variables;			/* track which variables are safe against sql injection */
-	Bitmapset	   *string_variables;		/* track which variables are possibly vulnerable to sql injection */
 	bool			stop_check;				/* true after error when fatal_errors option is active */
 } PLpgSQL_checkstate;
 
@@ -181,7 +180,7 @@ extern void plpgsql_check_detect_dependency(PLpgSQL_checkstate *cstate, Query *q
 extern bool plpgsql_check_has_rtable(Query *query);
 extern bool plpgsql_check_qual_has_fishy_cast(PlannedStmt *plannedstmt, Plan *plan, Param **param);
 extern void plpgsql_check_funcexpr(PLpgSQL_checkstate *cstate, Query *query, char *query_str);
-extern bool plpgsql_check_is_sql_injection_vulnerable(Node *node, int *location);
+extern bool plpgsql_check_is_sql_injection_vulnerable(PLpgSQL_checkstate *cstate, PLpgSQL_expr *expr, Node *node, int *location);
 
 /*
  * functions from check_expr.c
diff --git a/src/stmtwalk.c b/src/stmtwalk.c
@@ -1892,17 +1892,28 @@ check_dynamic_sql(PLpgSQL_checkstate *cstate,
 		 * but we can check sanitize parameters.
 		 */
 		if (cstate->cinfo->security_warnings &&
-			plpgsql_check_is_sql_injection_vulnerable(expr_node, &loc))
+			plpgsql_check_is_sql_injection_vulnerable(cstate, query, expr_node, &loc))
 		{
-			plpgsql_check_put_error(cstate,
-									0, 0,
-						"text type variable is not sanitized",
-						"The EXECUTE expression is SQL injection vulnerable.",
-						"Use quote_ident, quote_literal or format function to secure variable.",
-									PLPGSQL_CHECK_WARNING_SECURITY,
-									loc,
-									query->query,
-									NULL);
+			if (loc != -1)
+				plpgsql_check_put_error(cstate,
+										0, 0,
+							"text type variable is not sanitized",
+							"The EXECUTE expression is SQL injection vulnerable.",
+							"Use quote_ident, quote_literal or format function to secure variable.",
+										PLPGSQL_CHECK_WARNING_SECURITY,
+										loc,
+										query->query,
+										NULL);
+			else
+				plpgsql_check_put_error(cstate,
+										0, 0,
+							"the expression is not SQL injection safe",
+							"Cannot ensure so dynamic EXECUTE statement is SQL injection secure.",
+							"Use quote_ident, quote_literal or format function to secure variable.",
+										PLPGSQL_CHECK_WARNING_SECURITY,
+										-1,
+										query->query,
+										NULL);
 		}
 
 		/*

Original file line number	Diff line number	Diff line change
`@@ -1009,7 +1009,6 @@ plpgsql_check_setup_cstate(PLpgSQL_checkstate *cstate,`
`1009`	`1009`	`cstate->fake_rtd = fake_rtd;`
`1010`	`1010`
`1011`	`1011`	`cstate->safe_variables = NULL;`
`1012`		`- cstate->string_variables = NULL;`
`1013`	`1012`
`1014`	`1013`	`cstate->stop_check = false;`
`1015`	`1014`	`}`