Skip to content

Commit 1ef42a1

Browse files
author
Porcupiney Hairs
committed
Include suggested changes from review.
1 parent ae2bc1b commit 1ef42a1

File tree

3 files changed

+54
-58
lines changed

3 files changed

+54
-58
lines changed

‎go/ql/src/experimental/CWE-321/HardcodedKeysLib.qll

Lines changed: 52 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -182,57 +182,65 @@ module HardcodedKeys {
182182
FormattingSanitizer() { exists(Formatting::StringFormatCall s | s.getAResult() = this) }
183183
}
184184

185+
private string getRandIntFunctionName() {
186+
result =
187+
[
188+
"ExpFloat64", "Float32", "Float64", "Int", "Int31", "Int31n", "Int63", "Int63n", "Intn",
189+
"NormFloat64", "Uint32", "Uint64"
190+
]
191+
}
192+
193+
private DataFlow::CallNode getARandIntCall() {
194+
result.getTarget().hasQualifiedName("math/rand", getRandIntFunctionName()) or
195+
result.getTarget().(Method).hasQualifiedName("math/rand", "Rand", getRandIntFunctionName()) or
196+
result.getTarget().hasQualifiedName("crypto/rand", "Int")
197+
}
198+
199+
private DataFlow::CallNode getARandReadCall() {
200+
result.getTarget().hasQualifiedName("crypto/rand", "Read")
201+
}
202+
185203
/**
186204
* Mark any taint arising from a read on a tainted slice with a random index as a
187205
* sanitizer for all instances of the taint
188206
*/
189207
private class RandSliceSanitizer extends Sanitizer {
190208
RandSliceSanitizer() {
191-
// Sanitize flows like this:
192-
// func GenerateCryptoString(n int) (string, error) {
193-
// const chars = "123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz-"
194-
// ret := make([]byte, n)
195-
// for i := range ret {
196-
// num, err := crand.Int(crand.Reader, big.NewInt(int64(len(chars))))
197-
// if err != nil {
198-
// return "", err
199-
// }
200-
// ret[i] = chars[num.Int64()]
201-
// }
202-
// return string(ret), nil
203-
// }
204-
exists(
205-
DataFlow::CallNode randint, string name, DataFlow::ElementReadNode r, DataFlow::Node index
209+
exists(DataFlow::Node randomValue, DataFlow::Node index |
210+
// Sanitize flows like this:
211+
// func GenerateCryptoString(n int) (string, error) {
212+
// const chars = "123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz-"
213+
// ret := make([]byte, n)
214+
// for i := range ret {
215+
// num, err := crand.Int(crand.Reader, big.NewInt(int64(len(chars))))
216+
// if err != nil {
217+
// return "", err
218+
// }
219+
// ret[i] = chars[num.Int64()]
220+
// }
221+
// return string(ret), nil
222+
// }
223+
randomValue = getARandIntCall().getAResult()
224+
or
225+
// Sanitize flows like :
226+
// func GenerateRandomString(size int) string {
227+
// var bytes = make([]byte, size)
228+
// rand.Read(bytes)
229+
// for i, x := range bytes {
230+
// bytes[i] = characters[x%byte(len(characters))]
231+
// }
232+
// return string(bytes)
233+
// }
234+
randomValue =
235+
any(DataFlow::PostUpdateNode pun |
236+
pun.getPreUpdateNode() = getARandReadCall().getArgument(0)
237+
)
206238
|
239+
TaintTracking::localTaint(randomValue, index) and
207240
(
208-
randint.getTarget().hasQualifiedName("math/rand", name) or
209-
randint.getTarget().(Method).hasQualifiedName("math/rand", "Rand", name) or
210-
randint.getTarget().hasQualifiedName("crypto/rand", "Int")
211-
) and
212-
name =
213-
[
214-
"ExpFloat64", "Float32", "Float64", "Int", "Int31", "Int31n", "Int63", "Int63n", "Intn",
215-
"NormFloat64", "Uint32", "Uint64"
216-
] and
217-
TaintTracking::localTaint(randint.getAResult(), index) and
218-
r.reads(this, index)
219-
)
220-
or
221-
// Sanitize flows like :
222-
// func GenerateRandomString(size int) string {
223-
// var bytes = make([]byte, size)
224-
// rand.Read(bytes)
225-
// for i, x := range bytes {
226-
// bytes[i] = characters[x%byte(len(characters))]
227-
// }
228-
// return string(bytes)
229-
// }
230-
exists(DataFlow::CallNode randread, DataFlow::Node rand |
231-
randread.getTarget().hasQualifiedName("crypto/rand", "Read") and
232-
TaintTracking::localTaint(any(DataFlow::PostUpdateNode pun |
233-
pun.getPreUpdateNode() = randread.getArgument(0)
234-
), rand) and
235-
this.(DataFlow::ElementReadNode).reads(_, rand)
241+
this.(DataFlow::ElementReadNode).reads(_, randomValue) or
242+
any(DataFlow::ElementReadNode r).reads(this, index)
243+
)
236244
)
237245
}
238246
}
@@ -250,7 +258,7 @@ module HardcodedKeys {
250258
}
251259

252260
/*
253-
* This is code is used to model taint flow through a binary operation such as a
261+
* Models taint flow through a binary operation such as a
254262
* modulo `%` operation or an addition `+` operation
255263
*/
256264

@@ -282,8 +290,6 @@ module HardcodedKeys {
282290

283291
override predicate isSanitizer(DataFlow::Node sanitizer) { sanitizer instanceof Sanitizer }
284292

285-
// override predicate isAdditionalTaintStep(DataFlow::Node prev, DataFlow::Node succ) {
286-
// }
287293
override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
288294
guard instanceof SanitizerGuard
289295
}

‎go/ql/test/experimental/CWE-321/HardcodedKeys.expected

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -22,8 +22,6 @@ edges
2222
| main.go:110:30:110:36 | "key10" : string | main.go:110:23:110:37 | type conversion : string |
2323
| sanitizer.go:17:9:17:21 | type conversion : string | sanitizer.go:18:44:18:46 | key |
2424
| sanitizer.go:17:16:17:20 | `key` : string | sanitizer.go:17:9:17:21 | type conversion : string |
25-
| sanitizer.go:80:10:80:14 | "asd" : string | sanitizer.go:99:2:99:24 | ... := ...[0] : string |
26-
| sanitizer.go:99:2:99:24 | ... := ...[0] : string | sanitizer.go:104:44:104:47 | key4 |
2725
nodes
2826
| HardcodedKeysBad.go:11:18:11:38 | type conversion : string | semmle.label | type conversion : string |
2927
| HardcodedKeysBad.go:11:25:11:37 | "AllYourBase" : string | semmle.label | "AllYourBase" : string |
@@ -60,9 +58,6 @@ nodes
6058
| sanitizer.go:17:9:17:21 | type conversion : string | semmle.label | type conversion : string |
6159
| sanitizer.go:17:16:17:20 | `key` : string | semmle.label | `key` : string |
6260
| sanitizer.go:18:44:18:46 | key | semmle.label | key |
63-
| sanitizer.go:80:10:80:14 | "asd" : string | semmle.label | "asd" : string |
64-
| sanitizer.go:99:2:99:24 | ... := ...[0] : string | semmle.label | ... := ...[0] : string |
65-
| sanitizer.go:104:44:104:47 | key4 | semmle.label | key4 |
6661
subpaths
6762
#select
6863
| HardcodedKeysBad.go:19:28:19:39 | mySigningKey | HardcodedKeysBad.go:11:25:11:37 | "AllYourBase" : string | HardcodedKeysBad.go:19:28:19:39 | mySigningKey | $@ is used to sign a JWT token. | HardcodedKeysBad.go:11:25:11:37 | "AllYourBase" | Hardcoded String |
@@ -77,4 +72,3 @@ subpaths
7772
| main.go:107:16:107:24 | sharedKey | main.go:106:22:106:27 | "key9" : string | main.go:107:16:107:24 | sharedKey | $@ is used to sign a JWT token. | main.go:106:22:106:27 | "key9" | Hardcoded String |
7873
| main.go:113:16:113:30 | sharedKeyglobal | main.go:110:30:110:36 | "key10" : string | main.go:113:16:113:30 | sharedKeyglobal | $@ is used to sign a JWT token. | main.go:110:30:110:36 | "key10" | Hardcoded String |
7974
| sanitizer.go:18:44:18:46 | key | sanitizer.go:17:16:17:20 | `key` : string | sanitizer.go:18:44:18:46 | key | $@ is used to sign a JWT token. | sanitizer.go:17:16:17:20 | `key` | Hardcoded String |
80-
| sanitizer.go:104:44:104:47 | key4 | sanitizer.go:80:10:80:14 | "asd" : string | sanitizer.go:104:44:104:47 | key4 | $@ is used to sign a JWT token. | sanitizer.go:80:10:80:14 | "asd" | Hardcoded String |

‎go/ql/test/experimental/CWE-321/sanitizer.go

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -73,12 +73,8 @@ func RandString(length int64) string {
7373
return string(result)
7474
}
7575
func genKey(size int) (string, error) {
76-
if size < 10 {
77-
err := errors.New("size too small")
78-
return "", err
79-
} else {
80-
return "asd", nil
81-
}
76+
err := errors.New("size too small")
77+
return "", err
8278
}
8379
func test1() {
8480
key := GenerateRandomString(32)

0 commit comments

Comments
 (0)