From 18f57e4f024452ac8801b873181c5d6418c86305 Mon Sep 17 00:00:00 2001 From: Evgenii Protsenko Date: Mon, 15 Nov 2021 21:30:47 +0300 Subject: [PATCH 1/3] go: add pgx sqli query --- ql/src/experimental/CWE-089/Pgx.qll | 154 ++++++++++++++++++ .../experimental/CWE-089/PgxSqlInjection.go | 14 ++ .../CWE-089/PgxSqlInjection.qhelp | 43 +++++ .../experimental/CWE-089/PgxSqlInjection.ql | 29 ++++ .../CWE-089/PgxSqlInjectionGood.go | 12 ++ 5 files changed, 252 insertions(+) create mode 100644 ql/src/experimental/CWE-089/Pgx.qll create mode 100644 ql/src/experimental/CWE-089/PgxSqlInjection.go create mode 100644 ql/src/experimental/CWE-089/PgxSqlInjection.qhelp create mode 100644 ql/src/experimental/CWE-089/PgxSqlInjection.ql create mode 100644 ql/src/experimental/CWE-089/PgxSqlInjectionGood.go diff --git a/ql/src/experimental/CWE-089/Pgx.qll b/ql/src/experimental/CWE-089/Pgx.qll new file mode 100644 index 000000000..d694e074c --- /dev/null +++ b/ql/src/experimental/CWE-089/Pgx.qll @@ -0,0 +1,154 @@ +import go + +module Japroc { + /** Gets the package name for Jackc. */ + string pgx3PackagePath() { result = "github.com/jackc/pgx" } + + string pgxStdlibPackagePath() { result = package(["github.com/jackc/pgx"], "stdlib") } + + string pgxPackagePath() { + result = package(["github.com/jackc/pgx"], "") and result != pgx3PackagePath() + } + + string pgxAnyPackagePath() { result = package(["github.com/jackc/pgx"], "") } + + string pgxPoolPackagePath() { result = package(["github.com/jackc/pgx"], "pgxpool") } + + string pgconnPackagePath() { result = package(["github.com/jackc/pgconn"], "") } + + predicate batchQueries(string pkg, string tp, string m, int arg) { + pkg = pgxAnyPackagePath() and + tp = "Batch" and + (m = "Queue" and arg = 0) + } + + predicate stdlibQueries(string pkg, string tp, string m, int arg) { + pkg = pgxStdlibPackagePath() and + tp = "Conn" and + ( + m = "Prepare" and arg = 0 + or + m = "PrepareContext" and arg = 1 + or + m = "Exec" and arg = 0 + or + m = "ExecContext" and arg = 1 + or + m = "Query" and arg = 0 + or + m = "QueryContext" and arg = 1 + ) + } + + predicate v3Queries(string pkg, string tp, string m, int arg) { + pkg = pgx3PackagePath() and + tp = ["Tx", "Conn", "ConnPool", "ReplicationConn"] and + ( + m = "Exec" and arg = 0 + or + m = "ExecEx" and arg = 1 + or + m = "Prepare" and arg = 1 + or + m = "PrepareEx" and arg = 2 + or + m = "Query" and arg = 0 + or + m = "QueryEx" and arg = 1 + or + m = "QueryRow" and arg = 0 + or + m = "QueryRowEx" and arg = 1 + ) + } + + predicate v4Queries(string pkg, string tp, string m, int arg) { + pkg = pgxPackagePath() and + tp = ["Conn", "Tx"] and + ( + m = "Prepare" and arg = 2 + or + m = "Exec" and arg = 1 + or + m = "Query" and arg = 1 + or + m = "QueryRow" and arg = 1 + or + m = "QueryFunc" and arg = 1 + ) + } + + predicate pgxpoolConnPoolQueries(string pkg, string tp, string m, int arg) { + pkg = pgxPoolPackagePath() and + tp = ["Conn", "Pool"] and + ( + m = "Exec" and arg = 1 + or + m = "Query" and arg = 1 + or + m = "QueryRow" and arg = 1 + or + m = "QueryFunc" and arg = 1 + ) + } + + predicate pgxpoolTxQueries(string pkg, string tp, string m, int arg) { + pkg = pgxPoolPackagePath() and + tp = ["Tx"] and + ( + m = "Exec" and arg = 1 + or + m = "Query" and arg = 1 + or + m = "QueryRow" and arg = 1 + or + m = "QueryFunc" and arg = 1 + or + m = "Prepare" and arg = 2 + ) + } + + predicate pgconnQueries(string pkg, string tp, string m, int arg) { + pkg = pgconnPackagePath() and + tp = "PgConn" and + ( + m = "Exec" and arg = 1 + or + m = "ExecParams" and arg = 1 + or + m = "CopyTo" and arg = 2 + or + m = "CopyFrom" and arg = 2 + or + m = "Prepare" and arg = 2 + ) + } + + predicate pgxQueries(string pkg, string tp, string m, int arg) { + pgconnQueries(pkg, tp, m, arg) + or + pgxpoolTxQueries(pkg, tp, m, arg) + or + pgxpoolConnPoolQueries(pkg, tp, m, arg) + or + v3Queries(pkg, tp, m, arg) + or + v4Queries(pkg, tp, m, arg) + or + stdlibQueries(pkg, tp, m, arg) + or + batchQueries(pkg, tp, m, arg) + } + + /** A model for sinks of github.com/jackc/pgx. */ + class QueryString extends DataFlow::Node { + // class QueryString extends SQL::QueryString { + QueryString() { + exists(Method meth, string pkg, string tp, string m, int arg | + pgxQueries(pkg, tp, m, arg) and + meth.hasQualifiedName(pkg, tp, m) and + this = meth.getACall().getArgument(arg) + ) + } + } +} diff --git a/ql/src/experimental/CWE-089/PgxSqlInjection.go b/ql/src/experimental/CWE-089/PgxSqlInjection.go new file mode 100644 index 000000000..81ff70ad9 --- /dev/null +++ b/ql/src/experimental/CWE-089/PgxSqlInjection.go @@ -0,0 +1,14 @@ +package main + +import ( + "fmt" + "net/http" + + "github.com/jackc/pgx/v4" +) + +func handler(conn *pgx.Conn, req *http.Request) { + q := fmt.Sprintf("SELECT ITEM,PRICE FROM PRODUCT WHERE ITEM_CATEGORY='%s' ORDER BY PRICE", + req.URL.Query()["category"]) + conn.Query(q) +} diff --git a/ql/src/experimental/CWE-089/PgxSqlInjection.qhelp b/ql/src/experimental/CWE-089/PgxSqlInjection.qhelp new file mode 100644 index 000000000..d78e33837 --- /dev/null +++ b/ql/src/experimental/CWE-089/PgxSqlInjection.qhelp @@ -0,0 +1,43 @@ + + + + +

+If a database query is built from user-provided data without sufficient sanitization, +a malicious user may be able to run commands that exfiltrate, tamper with, +or destroy data stored in the database. +

+ + + +

+Most database connector libraries offer a way of safely embedding untrusted data into a query by +means of query parameters or prepared statements. Use these features rather than building queries +by string concatenation. +

+ + + +

+In the following example, assume the function handler is an HTTP request handler in a +web application, whose parameter req contains the request object: +

+ +

+The handler constructs an SQL query involving user input taken from the request object unsafely +using fmt.Sprintf to embed a request parameter directly into the query string +q. The parameter may include quote characters, allowing a malicious user to terminate +the string literal into which the parameter is embedded and add arbitrary SQL code after it. +

+

+Instead, the untrusted query parameter should be safely embedded using placeholder parameters: +

+ + + + +
  • Wikipedia: SQL injection.
    • +     +     diff --git a/ql/src/experimental/CWE-089/PgxSqlInjection.ql b/ql/src/experimental/CWE-089/PgxSqlInjection.ql new file mode 100644 index 000000000..6c907ec79 --- /dev/null +++ b/ql/src/experimental/CWE-089/PgxSqlInjection.ql @@ -0,0 +1,29 @@ +/** + * @name Database pgx query built from user-controlled sources + * @description Building a database pgx query from user-controlled sources is vulnerable to insertion of + * malicious code by the user. + * @kind path-problem + * @problem.severity error + * @security-severity 8.8 + * @precision high + * @id go/pgx-sql-injection + * @tags security + * external/cwe/cwe-089 + */ + +import go +import Pgx +import DataFlow::PathGraph + +class PgxSqlInjectionConfiguration extends TaintTracking::Configuration { + PgxSqlInjectionConfiguration() { this = "PgxSqlInjectionConfiguration" } + + override predicate isSource(DataFlow::Node source) { source instanceof UntrustedFlowSource } + + override predicate isSink(DataFlow::Node sink) { sink instanceof Japroc::QueryString } +} + +from PgxSqlInjectionConfiguration cfg, DataFlow::PathNode source, DataFlow::PathNode sink +where cfg.hasFlowPath(source, sink) +select sink.getNode(), source, sink, "This query depends on $@.", source.getNode(), + "a user-provided value" diff --git a/ql/src/experimental/CWE-089/PgxSqlInjectionGood.go b/ql/src/experimental/CWE-089/PgxSqlInjectionGood.go new file mode 100644 index 000000000..ea2dc3ca9 --- /dev/null +++ b/ql/src/experimental/CWE-089/PgxSqlInjectionGood.go @@ -0,0 +1,12 @@ +package main + +import ( + "net/http" + + "github.com/jackc/pgx/v4" +) + +func handlerGood(conn *pgx.Conn, req *http.Request) { + q := "SELECT ITEM,PRICE FROM PRODUCT WHERE ITEM_CATEGORY='?' ORDER BY PRICE" + conn.Query(q, req.URL.Query()["category"]) +} From 797185cbb6d0d7a0c28507509b792a856422a084 Mon Sep 17 00:00:00 2001 From: Evgenii Protsenko Date: Mon, 15 Nov 2021 21:43:11 +0300 Subject: [PATCH 2/3] Move Pgx.qll to frameworks folder --- ql/src/experimental/CWE-089/PgxSqlInjection.ql | 4 ++-- ql/src/experimental/{CWE-089 => frameworks}/Pgx.qll | 7 ++++++- 2 files changed, 8 insertions(+), 3 deletions(-) rename ql/src/experimental/{CWE-089 => frameworks}/Pgx.qll (95%) diff --git a/ql/src/experimental/CWE-089/PgxSqlInjection.ql b/ql/src/experimental/CWE-089/PgxSqlInjection.ql index 6c907ec79..7e6bf15a9 100644 --- a/ql/src/experimental/CWE-089/PgxSqlInjection.ql +++ b/ql/src/experimental/CWE-089/PgxSqlInjection.ql @@ -12,7 +12,7 @@ */ import go -import Pgx +import experimental.frameworks.Pgx import DataFlow::PathGraph class PgxSqlInjectionConfiguration extends TaintTracking::Configuration { @@ -20,7 +20,7 @@ class PgxSqlInjectionConfiguration extends TaintTracking::Configuration { override predicate isSource(DataFlow::Node source) { source instanceof UntrustedFlowSource } - override predicate isSink(DataFlow::Node sink) { sink instanceof Japroc::QueryString } + override predicate isSink(DataFlow::Node sink) { sink instanceof Pgx::QueryString } } from PgxSqlInjectionConfiguration cfg, DataFlow::PathNode source, DataFlow::PathNode sink diff --git a/ql/src/experimental/CWE-089/Pgx.qll b/ql/src/experimental/frameworks/Pgx.qll similarity index 95% rename from ql/src/experimental/CWE-089/Pgx.qll rename to ql/src/experimental/frameworks/Pgx.qll index d694e074c..1be4cf38b 100644 --- a/ql/src/experimental/CWE-089/Pgx.qll +++ b/ql/src/experimental/frameworks/Pgx.qll @@ -1,6 +1,11 @@ +/** + * Provides predicates and class for working with sql injection sinks + * from the `github.com/jackc/pgx` and `github.com/jackc/pgconn` packages. + */ + import go -module Japroc { +module Pgx { /** Gets the package name for Jackc. */ string pgx3PackagePath() { result = "github.com/jackc/pgx" } From 47346129f578e0e5e738c87c59b53370c642adb3 Mon Sep 17 00:00:00 2001 From: Evgenii Protsenko Date: Fri, 3 Dec 2021 15:40:54 +0300 Subject: [PATCH 3/3] replace list of 1 elem with a static string --- ql/src/experimental/frameworks/Pgx.qll | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/ql/src/experimental/frameworks/Pgx.qll b/ql/src/experimental/frameworks/Pgx.qll index 1be4cf38b..adcdbacbe 100644 --- a/ql/src/experimental/frameworks/Pgx.qll +++ b/ql/src/experimental/frameworks/Pgx.qll @@ -9,17 +9,17 @@ module Pgx { /** Gets the package name for Jackc. */ string pgx3PackagePath() { result = "github.com/jackc/pgx" } - string pgxStdlibPackagePath() { result = package(["github.com/jackc/pgx"], "stdlib") } + string pgxStdlibPackagePath() { result = package("github.com/jackc/pgx", "stdlib") } string pgxPackagePath() { - result = package(["github.com/jackc/pgx"], "") and result != pgx3PackagePath() + result = package("github.com/jackc/pgx", "") and result != pgx3PackagePath() } - string pgxAnyPackagePath() { result = package(["github.com/jackc/pgx"], "") } + string pgxAnyPackagePath() { result = package("github.com/jackc/pgx", "") } - string pgxPoolPackagePath() { result = package(["github.com/jackc/pgx"], "pgxpool") } + string pgxPoolPackagePath() { result = package("github.com/jackc/pgx", "pgxpool") } - string pgconnPackagePath() { result = package(["github.com/jackc/pgconn"], "") } + string pgconnPackagePath() { result = package("github.com/jackc/pgconn", "") } predicate batchQueries(string pkg, string tp, string m, int arg) { pkg = pgxAnyPackagePath() and