Skip to content

Commit 47c8395

Browse files
owen-mcowen-mc
committed
Fix email injection sink that needs local flow
1 parent 869d12c commit 47c8395

File tree

3 files changed

+60
-11
lines changed

3 files changed

+60
-11
lines changed

‎go/ql/lib/semmle/go/frameworks/Email.qll‎

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,9 +26,14 @@ module EmailData {
2626
private class SmtpData extends Range {
2727
SmtpData() {
2828
// func (c *Client) Data() (io.WriteCloser, error)
29-
exists(Method data |
29+
exists(Method data, DataFlow::Node n |
3030
data.hasQualifiedName("net/smtp", "Client", "Data") and
31-
this.(DataFlow::SsaNode).getInit() = data.getACall().getResult(0)
31+
// Deal with cases like
32+
// w, _ := s.Data()
33+
// io.WriteString(w, source()) // $ Alert
34+
// w.Write(source()) // $ Alert
35+
DataFlow::localFlow(data.getACall().getResult(0), n) and
36+
this.(DataFlow::PostUpdateNode).getPreUpdateNode() = n
3237
)
3338
or
3439
// func SendMail(addr string, a Auth, from string, to []string, msg []byte) error
@@ -98,3 +103,18 @@ private class MultipartNewWriterModel extends TaintTracking::FunctionModel {
98103
input.isResult() and output.isParameter(0)
99104
}
100105
}
106+
// /**
107+
// * A taint model of the `Data` method of `Client` from `net/smtp`.
108+
// *
109+
// * If tainted data is written to the writer created by this method, the client
110+
// * should be considered tainted as well.
111+
// */
112+
// private class SmtpClientDataModel extends TaintTracking::FunctionModel, Method {
113+
// SmtpClientDataModel() {
114+
// // func (c *Client) Data() (io.WriteCloser, error)
115+
// this.hasQualifiedName("net/smtp", "Client", "Data")
116+
// }
117+
// override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) {
118+
// input.isResult(0) and output.isReceiver()
119+
// }
120+
// }

‎go/ql/test/query-tests/Security/CWE-640/EmailInjection.expected‎

Lines changed: 23 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
#select
22
| EmailBad.go:12:56:12:67 | type conversion | EmailBad.go:9:10:9:17 | selection of Header | EmailBad.go:12:56:12:67 | type conversion | Email content may contain $@. | EmailBad.go:9:10:9:17 | selection of Header | untrusted input |
33
| main.go:33:57:33:78 | type conversion | main.go:31:21:31:31 | call to Referer | main.go:33:57:33:78 | type conversion | Email content may contain $@. | main.go:31:21:31:31 | call to Referer | untrusted input |
4+
| main.go:43:18:43:22 | write [postupdate] | main.go:39:21:39:31 | call to Referer | main.go:43:18:43:22 | write [postupdate] | Email content may contain $@. | main.go:39:21:39:31 | call to Referer | untrusted input |
45
| main.go:54:46:54:59 | untrustedInput | main.go:48:21:48:31 | call to Referer | main.go:54:46:54:59 | untrustedInput | Email content may contain $@. | main.go:48:21:48:31 | call to Referer | untrusted input |
56
| main.go:55:52:55:65 | untrustedInput | main.go:48:21:48:31 | call to Referer | main.go:55:52:55:65 | untrustedInput | Email content may contain $@. | main.go:48:21:48:31 | call to Referer | untrusted input |
67
| main.go:65:16:65:22 | content | main.go:60:21:60:31 | call to Referer | main.go:65:16:65:22 | content | Email content may contain $@. | main.go:60:21:60:31 | call to Referer | untrusted input |
@@ -11,10 +12,14 @@
1112
| main.go:95:16:95:23 | content2 | main.go:84:21:84:31 | call to Referer | main.go:95:16:95:23 | content2 | Email content may contain $@. | main.go:84:21:84:31 | call to Referer | untrusted input |
1213
| main.go:124:57:124:65 | call to Bytes | main.go:113:21:113:31 | call to Referer | main.go:124:57:124:65 | call to Bytes | Email content may contain $@. | main.go:113:21:113:31 | call to Referer | untrusted input |
1314
| main.go:141:57:141:65 | call to Bytes | main.go:129:21:129:31 | call to Referer | main.go:141:57:141:65 | call to Bytes | Email content may contain $@. | main.go:129:21:129:31 | call to Referer | untrusted input |
15+
| main.go:151:3:151:3 | w [postupdate] | main.go:146:22:146:32 | call to Referer | main.go:151:3:151:3 | w [postupdate] | Email content may contain $@. | main.go:146:22:146:32 | call to Referer | untrusted input |
16+
| main.go:152:3:152:3 | w [postupdate] | main.go:147:22:147:32 | call to Referer | main.go:152:3:152:3 | w [postupdate] | Email content may contain $@. | main.go:147:22:147:32 | call to Referer | untrusted input |
1417
edges
15-
| EmailBad.go:9:10:9:17 | selection of Header | EmailBad.go:9:10:9:29 | call to Get | provenance | Src:MaD:1 MaD:7 |
18+
| EmailBad.go:9:10:9:17 | selection of Header | EmailBad.go:9:10:9:29 | call to Get | provenance | Src:MaD:1 MaD:8 |
1619
| EmailBad.go:9:10:9:29 | call to Get | EmailBad.go:12:56:12:67 | type conversion | provenance | |
1720
| main.go:31:21:31:31 | call to Referer | main.go:33:57:33:78 | type conversion | provenance | Src:MaD:2 |
21+
| main.go:39:21:39:31 | call to Referer | main.go:43:25:43:38 | untrustedInput | provenance | Src:MaD:2 |
22+
| main.go:43:25:43:38 | untrustedInput | main.go:43:18:43:22 | write [postupdate] | provenance | MaD:5 |
1823
| main.go:48:21:48:31 | call to Referer | main.go:54:46:54:59 | untrustedInput | provenance | Src:MaD:2 |
1924
| main.go:48:21:48:31 | call to Referer | main.go:55:52:55:65 | untrustedInput | provenance | Src:MaD:2 |
2025
| main.go:60:21:60:31 | call to Referer | main.go:62:47:62:60 | untrustedInput | provenance | Src:MaD:2 |
@@ -32,28 +37,36 @@ edges
3237
| main.go:113:21:113:31 | call to Referer | main.go:119:28:119:41 | untrustedInput | provenance | Src:MaD:2 |
3338
| main.go:116:29:116:30 | &... [postupdate] | main.go:124:57:124:57 | b | provenance | |
3439
| main.go:119:3:119:4 | mw [postupdate] | main.go:116:29:116:30 | &... [postupdate] | provenance | FunctionModel |
35-
| main.go:119:28:119:41 | untrustedInput | main.go:119:3:119:4 | mw [postupdate] | provenance | MaD:6 |
40+
| main.go:119:28:119:41 | untrustedInput | main.go:119:3:119:4 | mw [postupdate] | provenance | MaD:7 |
3641
| main.go:124:57:124:57 | b | main.go:124:57:124:65 | call to Bytes | provenance | MaD:3 |
3742
| main.go:129:21:129:31 | call to Referer | main.go:136:30:136:43 | untrustedInput | provenance | Src:MaD:2 |
3843
| main.go:132:29:132:30 | &... [postupdate] | main.go:141:57:141:57 | b | provenance | |
3944
| main.go:135:20:135:21 | mw [postupdate] | main.go:132:29:132:30 | &... [postupdate] | provenance | FunctionModel |
4045
| main.go:136:18:136:27 | formWriter [postupdate] | main.go:135:20:135:21 | mw [postupdate] | provenance | FunctionModel |
4146
| main.go:136:30:136:43 | untrustedInput | main.go:136:18:136:27 | formWriter [postupdate] | provenance | MaD:5 |
4247
| main.go:141:57:141:57 | b | main.go:141:57:141:65 | call to Bytes | provenance | MaD:3 |
48+
| main.go:146:22:146:32 | call to Referer | main.go:151:11:151:33 | type conversion | provenance | Src:MaD:2 |
49+
| main.go:147:22:147:32 | call to Referer | main.go:152:11:152:33 | type conversion | provenance | Src:MaD:2 |
50+
| main.go:151:11:151:33 | type conversion | main.go:151:3:151:3 | w [postupdate] | provenance | MaD:6 |
51+
| main.go:152:11:152:33 | type conversion | main.go:152:3:152:3 | w [postupdate] | provenance | MaD:6 |
4352
models
4453
| 1 | Source: net/http; Request; true; Header; ; ; ; remote; manual |
4554
| 2 | Source: net/http; Request; true; Referer; ; ; ReturnValue; remote; manual |
4655
| 3 | Summary: bytes; Buffer; true; Bytes; ; ; Argument[receiver]; ReturnValue; taint; manual |
4756
| 4 | Summary: github.com/sendgrid/sendgrid-go/helpers/mail; ; false; NewContent; ; ; Argument[1]; ReturnValue; taint; manual |
4857
| 5 | Summary: io; ; false; WriteString; ; ; Argument[1]; Argument[0]; taint; manual |
49-
| 6 | Summary: mime/multipart; Writer; true; WriteField; ; ; Argument[0..1]; Argument[receiver]; taint; manual |
50-
| 7 | Summary: net/http; Header; true; Get; ; ; Argument[receiver]; ReturnValue; taint; manual |
58+
| 6 | Summary: io; Writer; true; Write; ; ; Argument[0]; Argument[receiver]; taint; manual |
59+
| 7 | Summary: mime/multipart; Writer; true; WriteField; ; ; Argument[0..1]; Argument[receiver]; taint; manual |
60+
| 8 | Summary: net/http; Header; true; Get; ; ; Argument[receiver]; ReturnValue; taint; manual |
5161
nodes
5262
| EmailBad.go:9:10:9:17 | selection of Header | semmle.label | selection of Header |
5363
| EmailBad.go:9:10:9:29 | call to Get | semmle.label | call to Get |
5464
| EmailBad.go:12:56:12:67 | type conversion | semmle.label | type conversion |
5565
| main.go:31:21:31:31 | call to Referer | semmle.label | call to Referer |
5666
| main.go:33:57:33:78 | type conversion | semmle.label | type conversion |
67+
| main.go:39:21:39:31 | call to Referer | semmle.label | call to Referer |
68+
| main.go:43:18:43:22 | write [postupdate] | semmle.label | write [postupdate] |
69+
| main.go:43:25:43:38 | untrustedInput | semmle.label | untrustedInput |
5770
| main.go:48:21:48:31 | call to Referer | semmle.label | call to Referer |
5871
| main.go:54:46:54:59 | untrustedInput | semmle.label | untrustedInput |
5972
| main.go:55:52:55:65 | untrustedInput | semmle.label | untrustedInput |
@@ -85,7 +98,10 @@ nodes
8598
| main.go:136:30:136:43 | untrustedInput | semmle.label | untrustedInput |
8699
| main.go:141:57:141:57 | b | semmle.label | b |
87100
| main.go:141:57:141:65 | call to Bytes | semmle.label | call to Bytes |
101+
| main.go:146:22:146:32 | call to Referer | semmle.label | call to Referer |
102+
| main.go:147:22:147:32 | call to Referer | semmle.label | call to Referer |
103+
| main.go:151:3:151:3 | w [postupdate] | semmle.label | w [postupdate] |
104+
| main.go:151:11:151:33 | type conversion | semmle.label | type conversion |
105+
| main.go:152:3:152:3 | w [postupdate] | semmle.label | w [postupdate] |
106+
| main.go:152:11:152:33 | type conversion | semmle.label | type conversion |
88107
subpaths
89-
testFailures
90-
| main.go:39:33:39:43 | comment | Missing result: Source |
91-
| main.go:42:24:42:33 | comment | Missing result: Alert |

‎go/ql/test/query-tests/Security/CWE-640/main.go‎

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -39,8 +39,8 @@ func main() {
3939
untrustedInput := r.Referer() // $ Source
4040

4141
s, _ := smtp.Dial("test.test")
42-
write, _ := s.Data() // $ Alert
43-
io.WriteString(write, untrustedInput)
42+
write, _ := s.Data()
43+
io.WriteString(write, untrustedInput) // $ Alert
4444
})
4545

4646
// Not OK
@@ -141,6 +141,19 @@ func main() {
141141
smtp.SendMail("test.test", nil, "[email protected]", nil, b.Bytes()) // $ Alert
142142
})
143143

144+
// Not OK - check only one result when sink is Client.Data() from net/smtp
145+
{
146+
untrustedInput1 := r.Referer() // $ Source=s1
147+
untrustedInput2 := r.Referer() // $ Source=s2
148+
c, _ := smtp.Dial("mail.example.com:smtp")
149+
w, _ := c.Data()
150+
w.Write([]byte("safe text"))
151+
w.Write([]byte(untrustedInput1)) // $ Alert=s1
152+
w.Write([]byte(untrustedInput2)) // $ Alert=s2
153+
w.Close()
154+
c.Quit()
155+
}
156+
144157
log.Println(http.ListenAndServe(":80", nil))
145158

146159
}

0 commit comments

Comments
 (0)