This repository has been archived by the owner on Jan 5, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 125
Go: add pgx sqli query #607
Open
japroc
wants to merge
3
commits into
github:main
Choose a base branch
from
japroc:pgx-sql-injection
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+257
−0
Open
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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) | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,43 @@ | ||
<!DOCTYPE qhelp PUBLIC | ||
"-//Semmle//qhelp//EN" | ||
"qhelp.dtd"> | ||
<qhelp> | ||
|
||
<overview> | ||
<p> | ||
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. | ||
</p> | ||
</overview> | ||
|
||
<recommendation> | ||
<p> | ||
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. | ||
</p> | ||
</recommendation> | ||
|
||
<example> | ||
<p> | ||
In the following example, assume the function <code>handler</code> is an HTTP request handler in a | ||
web application, whose parameter <code>req</code> contains the request object: | ||
</p> | ||
<sample src="SqlInjection.go"/> | ||
<p> | ||
The handler constructs an SQL query involving user input taken from the request object unsafely | ||
using <code>fmt.Sprintf</code> to embed a request parameter directly into the query string | ||
<code>q</code>. 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. | ||
</p> | ||
<p> | ||
Instead, the untrusted query parameter should be safely embedded using placeholder parameters: | ||
</p> | ||
<sample src="SqlInjectionGood.go" /> | ||
</example> | ||
|
||
<references> | ||
<li>Wikipedia: <a href="https://en.wikipedia.org/wiki/SQL_injection">SQL injection</a>.</li> | ||
</references> | ||
</qhelp> |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 experimental.frameworks.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 Pgx::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" |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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"]) | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,159 @@ | ||
/** | ||
* 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 Pgx { | ||
/** 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) | ||
) | ||
} | ||
} | ||
} |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for a set literal with only one element in it. The same applies in many other places.