Skip to content

Commit ee64ea5

Browse files
smowtonsmowton
authored
Merge pull request #12901 from porcupineyhairs/goDsn
Go: Add query to detect DSN Injection.
2 parents 8206734 + b6c2db6 commit ee64ea5

File tree

11 files changed

+264
-0
lines changed

11 files changed

+264
-0
lines changed

‎go/ql/src/experimental/CWE-134/DsnBad.go‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
2+
func bad() interface{} {
3+
name := os.Args[1:]
4+
// This is bad. `name` can be something like `test?allowAllFiles=true&` which will allow an attacker to access local files.
5+
dbDSN := fmt.Sprintf("%s:%s@tcp(%s:%d)/%s?charset=utf8", "username", "password", "127.0.0.1", 3306, name)
6+
db, _ := sql.Open("mysql", dbDSN)
7+
return db
8+
}

‎go/ql/src/experimental/CWE-134/DsnGood.go‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
func good() (interface{}, error) {
2+
name := os.Args[1]
3+
hasBadChar, _ := regexp.MatchString(".*[?].*", name)
4+
5+
if hasBadChar {
6+
return nil, errors.New("Bad input")
7+
}
8+
9+
dbDSN := fmt.Sprintf("%s:%s@tcp(%s:%d)/%s?charset=utf8", "username", "password", "127.0.0.1", 3306, name)
10+
db, _ := sql.Open("mysql", dbDSN)
11+
return db, nil
12+
}

‎go/ql/src/experimental/CWE-134/DsnInjection.qhelp‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
2+
3+
<qhelp>
4+
<overview>
5+
<p>If a Data-Source Name (DSN) is built using untrusted user input without proper sanitization,
6+
the system may be vulnerable to DSN injection vulnerabilities.</p>
7+
</overview>
8+
9+
<recommendation>
10+
<p>If user input must be included in a DSN, additional steps should be taken to sanitize
11+
untrusted data, such as checking for special characters included in user input.</p>
12+
</recommendation>
13+
14+
<example>
15+
<p>In the following examples, the code accepts the db name from the user,
16+
which it then uses to build a DSN string.</p>
17+
18+
<p>The following example uses the unsanitized user input directly
19+
in the process of constructing a DSN name.
20+
A malicious user could provide special characters to change the meaning of this string, and
21+
carry out unexpected database operations.</p>
22+
23+
<sample src="DsnBad.go" />
24+
25+
<p>In the following example, the input provided by the user is sanitized before it is included
26+
in the DSN string.
27+
This ensures the meaning of the DSN string cannot be changed by a malicious user.</p>
28+
29+
<sample src="DsnGood.go" />
30+
</example>
31+
32+
<references>
33+
<li>
34+
CVE-2022-3023: <a href="https://nvd.nist.gov/vuln/detail/CVE-2022-3023/">Data Source Name Injection in pingcap/tidb.</a>
35+
</li>
36+
37+
</references>
38+
</qhelp>

‎go/ql/src/experimental/CWE-134/DsnInjection.ql‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
/**
2+
* @name SQL Data-source URI built from user-controlled sources
3+
* @description Building an SQL data-source URI from untrusted sources can allow attacker to compromise security
4+
* @kind path-problem
5+
* @problem.severity error
6+
* @id go/dsn-injection
7+
* @tags security
8+
* experimental
9+
* external/cwe/cwe-134
10+
*/
11+
12+
import go
13+
import DataFlow::PathGraph
14+
import DsnInjectionCustomizations
15+
16+
/** An untrusted flow source taken as a source for the `DsnInjection` taint-flow configuration. */
17+
private class UntrustedFlowAsSource extends Source instanceof UntrustedFlowSource { }
18+
19+
from DsnInjection cfg, DataFlow::PathNode source, DataFlow::PathNode sink
20+
where cfg.hasFlowPath(source, sink)
21+
select sink.getNode(), source, sink, "This query depends on a $@.", source.getNode(),
22+
"user-provided value"

‎go/ql/src/experimental/CWE-134/DsnInjectionCustomizations.qll‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
/** Provides a taint-tracking model to reason about Data-Source name injection vulnerabilities. */
2+
3+
import go
4+
import DataFlow::PathGraph
5+
import semmle.go.dataflow.barrierguardutil.RegexpCheck
6+
7+
/** A source for `DsnInjection` taint-flow configuration. */
8+
abstract class Source extends DataFlow::Node { }
9+
10+
/** A taint-tracking configuration to reason about Data Source Name injection vulnerabilities. */
11+
class DsnInjection extends TaintTracking::Configuration {
12+
DsnInjection() { this = "DsnInjection" }
13+
14+
override predicate isSource(DataFlow::Node node) { node instanceof Source }
15+
16+
override predicate isSink(DataFlow::Node node) {
17+
exists(Function f | f.hasQualifiedName("database/sql", "Open") |
18+
node = f.getACall().getArgument(1)
19+
)
20+
}
21+
22+
override predicate isSanitizer(DataFlow::Node node) { node instanceof RegexpCheckBarrier }
23+
}
24+
25+
/** A model of a function which decodes or unmarshals a tainted input, propagating taint from any argument to either the method receiver or return value. */
26+
private class DecodeFunctionModel extends TaintTracking::FunctionModel {
27+
DecodeFunctionModel() {
28+
// This matches any function with a name like `Decode`,`Unmarshal` or `Parse`.
29+
// This is done to allow taints stored in encoded forms, such as in toml or json to flow freely.
30+
this.getName().regexpMatch("(?i).*(parse|decode|unmarshal).*")
31+
}
32+
33+
override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) {
34+
input.isParameter(_) and
35+
(output.isResult(0) or output.isReceiver())
36+
}
37+
}
38+
39+
/** A model of `flag.Parse`, propagating tainted input passed via CLI flags to `Parse`'s result. */
40+
private class FlagSetFunctionModel extends TaintTracking::FunctionModel {
41+
FlagSetFunctionModel() { this.hasQualifiedName("flag", "Parse") }
42+
43+
override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) {
44+
input.isParameter(0) and output.isResult()
45+
}
46+
}

‎go/ql/src/experimental/CWE-134/DsnInjectionLocal.ql‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
/**
2+
* @name SQL Data-source URI built from local user-controlled sources
3+
* @description Building an SQL data-source URI from untrusted sources can allow attacker to compromise security
4+
* @kind path-problem
5+
* @problem.severity error
6+
* @id go/dsn-injection-local
7+
* @tags security
8+
* experimental
9+
* external/cwe/cwe-134
10+
*/
11+
12+
import go
13+
import DataFlow::PathGraph
14+
import DsnInjectionCustomizations
15+
16+
/** An argument passed via the command line taken as a source for the `DsnInjection` taint-flow configuration. */
17+
private class OsArgsSource extends Source {
18+
OsArgsSource() { this = any(Variable c | c.hasQualifiedName("os", "Args")).getARead() }
19+
}
20+
21+
from DsnInjection cfg, DataFlow::PathNode source, DataFlow::PathNode sink
22+
where cfg.hasFlowPath(source, sink)
23+
select sink.getNode(), source, sink, "This query depends on a $@.", source.getNode(),
24+
"user-provided value"

‎go/ql/test/experimental/CWE-134/Dsn.go‎

Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,77 @@
1+
package main
2+
3+
import (
4+
"database/sql"
5+
"errors"
6+
"fmt"
7+
"net/http"
8+
"os"
9+
"regexp"
10+
)
11+
12+
func good() (interface{}, error) {
13+
name := os.Args[1]
14+
hasBadChar, _ := regexp.MatchString(".*[?].*", name)
15+
16+
if hasBadChar {
17+
return nil, errors.New("bad input")
18+
}
19+
20+
dbDSN := fmt.Sprintf("%s:%s@tcp(%s:%d)/%s?charset=utf8", "username", "password", "127.0.0.1", 3306, name)
21+
db, _ := sql.Open("mysql", dbDSN)
22+
return db, nil
23+
}
24+
25+
func bad() interface{} {
26+
name2 := os.Args[1:]
27+
// This is bad. `name` can be something like `test?allowAllFiles=true&` which will allow an attacker to access local files.
28+
dbDSN := fmt.Sprintf("%s:%s@tcp(%s:%d)/%s?charset=utf8", "username", "password", "127.0.0.1", 3306, name2[0])
29+
db, _ := sql.Open("mysql", dbDSN)
30+
return db
31+
}
32+
33+
func good2(w http.ResponseWriter, req *http.Request) (interface{}, error) {
34+
name := req.FormValue("name")
35+
hasBadChar, _ := regexp.MatchString(".*[?].*", name)
36+
37+
if hasBadChar {
38+
return nil, errors.New("bad input")
39+
}
40+
41+
dbDSN := fmt.Sprintf("%s:%s@tcp(%s:%d)/%s?charset=utf8", "username", "password", "127.0.0.1", 3306, name)
42+
db, _ := sql.Open("mysql", dbDSN)
43+
return db, nil
44+
}
45+
46+
func bad2(w http.ResponseWriter, req *http.Request) interface{} {
47+
name := req.FormValue("name")
48+
// This is bad. `name` can be something like `test?allowAllFiles=true&` which will allow an attacker to access local files.
49+
dbDSN := fmt.Sprintf("%s:%s@tcp(%s:%d)/%s?charset=utf8", "username", "password", "127.0.0.1", 3306, name)
50+
db, _ := sql.Open("mysql", dbDSN)
51+
return db
52+
}
53+
54+
type Config struct {
55+
dsn string
56+
}
57+
58+
func NewConfig() *Config { return &Config{dsn: ""} }
59+
func (Config) Parse([]string) error { return nil }
60+
61+
func RegexFuncModelTest(w http.ResponseWriter, req *http.Request) (interface{}, error) {
62+
cfg := NewConfig()
63+
err := cfg.Parse(os.Args[1:]) // This is bad. `name` can be something like `test?allowAllFiles=true&` which will allow an attacker to access local files.
64+
if err != nil {
65+
return nil, err
66+
}
67+
dbDSN := fmt.Sprintf("%s:%s@tcp(%s:%d)/%s?charset=utf8", "username", "password", "127.0.0.1", 3306, cfg.dsn)
68+
db, _ := sql.Open("mysql", dbDSN)
69+
return db, nil
70+
}
71+
72+
func main() {
73+
bad2(nil, nil)
74+
good()
75+
bad()
76+
good2(nil, nil)
77+
}

‎go/ql/test/experimental/CWE-134/DsnInjection.expected‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
edges
2+
| Dsn.go:47:10:47:30 | call to FormValue | Dsn.go:50:29:50:33 | dbDSN |
3+
nodes
4+
| Dsn.go:47:10:47:30 | call to FormValue | semmle.label | call to FormValue |
5+
| Dsn.go:50:29:50:33 | dbDSN | semmle.label | dbDSN |
6+
subpaths
7+
#select
8+
| Dsn.go:50:29:50:33 | dbDSN | Dsn.go:47:10:47:30 | call to FormValue | Dsn.go:50:29:50:33 | dbDSN | This query depends on a $@. | Dsn.go:47:10:47:30 | call to FormValue | user-provided value |

‎go/ql/test/experimental/CWE-134/DsnInjection.qlref‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
experimental/CWE-134/DsnInjection.ql

‎go/ql/test/experimental/CWE-134/DsnInjectionLocal.expected‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
edges
2+
| Dsn.go:26:11:26:17 | selection of Args | Dsn.go:29:29:29:33 | dbDSN |
3+
| Dsn.go:62:2:62:4 | definition of cfg [pointer] | Dsn.go:63:9:63:11 | cfg [pointer] |
4+
| Dsn.go:62:2:62:4 | definition of cfg [pointer] | Dsn.go:67:102:67:104 | cfg [pointer] |
5+
| Dsn.go:63:9:63:11 | cfg [pointer] | Dsn.go:63:9:63:11 | implicit dereference |
6+
| Dsn.go:63:9:63:11 | implicit dereference | Dsn.go:62:2:62:4 | definition of cfg [pointer] |
7+
| Dsn.go:63:9:63:11 | implicit dereference | Dsn.go:63:9:63:11 | implicit dereference |
8+
| Dsn.go:63:9:63:11 | implicit dereference | Dsn.go:68:29:68:33 | dbDSN |
9+
| Dsn.go:63:19:63:25 | selection of Args | Dsn.go:63:9:63:11 | implicit dereference |
10+
| Dsn.go:63:19:63:25 | selection of Args | Dsn.go:68:29:68:33 | dbDSN |
11+
| Dsn.go:67:102:67:104 | cfg [pointer] | Dsn.go:67:102:67:104 | implicit dereference |
12+
| Dsn.go:67:102:67:104 | implicit dereference | Dsn.go:63:9:63:11 | implicit dereference |
13+
| Dsn.go:67:102:67:104 | implicit dereference | Dsn.go:68:29:68:33 | dbDSN |
14+
nodes
15+
| Dsn.go:26:11:26:17 | selection of Args | semmle.label | selection of Args |
16+
| Dsn.go:29:29:29:33 | dbDSN | semmle.label | dbDSN |
17+
| Dsn.go:62:2:62:4 | definition of cfg [pointer] | semmle.label | definition of cfg [pointer] |
18+
| Dsn.go:63:9:63:11 | cfg [pointer] | semmle.label | cfg [pointer] |
19+
| Dsn.go:63:9:63:11 | implicit dereference | semmle.label | implicit dereference |
20+
| Dsn.go:63:19:63:25 | selection of Args | semmle.label | selection of Args |
21+
| Dsn.go:67:102:67:104 | cfg [pointer] | semmle.label | cfg [pointer] |
22+
| Dsn.go:67:102:67:104 | implicit dereference | semmle.label | implicit dereference |
23+
| Dsn.go:68:29:68:33 | dbDSN | semmle.label | dbDSN |
24+
subpaths
25+
#select
26+
| Dsn.go:29:29:29:33 | dbDSN | Dsn.go:26:11:26:17 | selection of Args | Dsn.go:29:29:29:33 | dbDSN | This query depends on a $@. | Dsn.go:26:11:26:17 | selection of Args | user-provided value |
27+
| Dsn.go:68:29:68:33 | dbDSN | Dsn.go:63:19:63:25 | selection of Args | Dsn.go:68:29:68:33 | dbDSN | This query depends on a $@. | Dsn.go:63:19:63:25 | selection of Args | user-provided value |

0 commit comments

Comments
 (0)