Skip to content

Commit e0f74a5

Browse files
author
Porcupiney Hairs
committed
Include suggested changes from review.
1 parent bd1ddc1 commit e0f74a5

File tree

4 files changed

+52
-38
lines changed

4 files changed

+52
-38
lines changed

‎go/ql/src/experimental/CWE-321/HardcodedKeys.qhelp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@
1818
<recommendation>
1919

2020
<p>
21-
Generating a crytograhically secure secret key during application initialization and using this generated key for future JWT signing requests can prevent this vulnerability.
21+
Generating a cryptograhically secure secret key during application initialization and using this generated key for future JWT signing requests can prevent this vulnerability.
2222
</p>
2323

2424
</recommendation>

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

Lines changed: 23 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -65,26 +65,20 @@ module HardcodedKeys {
6565
"github.com/form3tech-oss/jwt-go", "github.com/ory/fosite/token/jwt"
6666
]
6767
|
68-
(
69-
exists(DataFlow::MethodCallNode m |
70-
// Models the `SignedString` method
71-
// `func (t *Token) SignedString(key interface{}) (string, error)`
72-
m.getTarget().hasQualifiedName(pkg, "Token", "SignedString")
73-
|
74-
this = m.getArgument(0)
75-
)
68+
exists(DataFlow::MethodCallNode m |
69+
// Models the `SignedString` method
70+
// `func (t *Token) SignedString(key interface{}) (string, error)`
71+
m.getTarget().hasQualifiedName(pkg, "Token", "SignedString") and
72+
this = m.getArgument(0)
7673
or
77-
exists(DataFlow::MethodCallNode m |
78-
// Model the `Sign` method of the `SigningMethod` interface
79-
// type SigningMethod interface {
80-
// Verify(signingString, signature string, key interface{}) error
81-
// Sign(signingString string, key interface{}) (string, error)
82-
// Alg() string
83-
// }
84-
m.getTarget().hasQualifiedName(pkg, "SigningMethod", "Sign")
85-
|
86-
this = m.getArgument(1)
87-
)
74+
// Model the `Sign` method of the `SigningMethod` interface
75+
// type SigningMethod interface {
76+
// Verify(signingString, signature string, key interface{}) error
77+
// Sign(signingString string, key interface{}) (string, error)
78+
// Alg() string
79+
// }
80+
m.getTarget().hasQualifiedName(pkg, "SigningMethod", "Sign") and
81+
this = m.getArgument(1)
8882
)
8983
)
9084
}
@@ -240,12 +234,21 @@ module HardcodedKeys {
240234
// }
241235
exists(DataFlow::CallNode randread, DataFlow::Node rand, DataFlow::ElementReadNode r |
242236
randread.getTarget().hasQualifiedName("crypto/rand", "Read") and
243-
TaintTracking::localTaint(randread.getArgument(0).getAPredecessor*().getASuccessor*(), rand) and
237+
TaintTracking::localTaint(any(DataFlow::PostUpdateNode pun |
238+
pun.getPreUpdateNode() = randread.getArgument(0)
239+
), rand) and
244240
(
241+
// Flow through a ModExpr if any of the operands are tainted.
242+
// For ex, in the case shown above,
243+
// `bytes[i] = characters[x%byte(len(characters))]`
244+
// given x is cryptographically secure random number,
245+
// we can assume that `bytes` is random and cryptographically secure.
245246
exists(ModExpr e | e.getAnOperand() = rand.asExpr() |
246247
r.reads(this, e.getGlobalValueNumber().getANode())
247248
)
248249
or
250+
// This is an alternative case where the code uses `x` directly instead
251+
// `bytes[i] = characters[x]`
249252
r.reads(this.getAPredecessor*(), rand)
250253
)
251254
)

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

Lines changed: 27 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -14,12 +14,16 @@ edges
1414
| main.go:80:16:80:21 | "key6" : string | main.go:80:9:80:22 | type conversion : string |
1515
| main.go:89:10:89:23 | type conversion : string | main.go:91:66:91:69 | key2 |
1616
| main.go:89:17:89:22 | "key7" : string | main.go:89:10:89:23 | type conversion : string |
17-
| main.go:97:9:97:22 | type conversion : string | main.go:103:30:103:32 | key |
17+
| main.go:97:9:97:22 | type conversion : string | main.go:102:30:102:32 | key |
1818
| main.go:97:16:97:21 | "key8" : string | main.go:97:9:97:22 | type conversion : string |
19-
| main.go:107:15:107:28 | type conversion : string | main.go:108:16:108:24 | sharedKey |
20-
| main.go:107:22:107:27 | "key9" : string | main.go:107:15:107:28 | type conversion : string |
21-
| main.go:111:23:111:37 | type conversion : string | main.go:114:16:114:30 | sharedKeyglobal |
22-
| main.go:111:30:111:36 | "key10" : string | main.go:111:23:111:37 | type conversion : string |
19+
| main.go:106:15:106:28 | type conversion : string | main.go:107:16:107:24 | sharedKey |
20+
| main.go:106:22:106:27 | "key9" : string | main.go:106:15:106:28 | type conversion : string |
21+
| main.go:110:23:110:37 | type conversion : string | main.go:113:16:113:30 | sharedKeyglobal |
22+
| main.go:110:30:110:36 | "key10" : string | main.go:110:23:110:37 | type conversion : string |
23+
| sanitizer.go:17:9:17:21 | type conversion : string | sanitizer.go:18:44:18:46 | key |
24+
| 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 |
2327
nodes
2428
| HardcodedKeysBad.go:11:18:11:38 | type conversion : string | semmle.label | type conversion : string |
2529
| HardcodedKeysBad.go:11:25:11:37 | "AllYourBase" : string | semmle.label | "AllYourBase" : string |
@@ -46,13 +50,19 @@ nodes
4650
| main.go:91:66:91:69 | key2 | semmle.label | key2 |
4751
| main.go:97:9:97:22 | type conversion : string | semmle.label | type conversion : string |
4852
| main.go:97:16:97:21 | "key8" : string | semmle.label | "key8" : string |
49-
| main.go:103:30:103:32 | key | semmle.label | key |
50-
| main.go:107:15:107:28 | type conversion : string | semmle.label | type conversion : string |
51-
| main.go:107:22:107:27 | "key9" : string | semmle.label | "key9" : string |
52-
| main.go:108:16:108:24 | sharedKey | semmle.label | sharedKey |
53-
| main.go:111:23:111:37 | type conversion : string | semmle.label | type conversion : string |
54-
| main.go:111:30:111:36 | "key10" : string | semmle.label | "key10" : string |
55-
| main.go:114:16:114:30 | sharedKeyglobal | semmle.label | sharedKeyglobal |
53+
| main.go:102:30:102:32 | key | semmle.label | key |
54+
| main.go:106:15:106:28 | type conversion : string | semmle.label | type conversion : string |
55+
| main.go:106:22:106:27 | "key9" : string | semmle.label | "key9" : string |
56+
| main.go:107:16:107:24 | sharedKey | semmle.label | sharedKey |
57+
| main.go:110:23:110:37 | type conversion : string | semmle.label | type conversion : string |
58+
| main.go:110:30:110:36 | "key10" : string | semmle.label | "key10" : string |
59+
| main.go:113:16:113:30 | sharedKeyglobal | semmle.label | sharedKeyglobal |
60+
| sanitizer.go:17:9:17:21 | type conversion : string | semmle.label | type conversion : string |
61+
| sanitizer.go:17:16:17:20 | `key` : string | semmle.label | `key` : string |
62+
| 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 |
5666
subpaths
5767
#select
5868
| 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 |
@@ -63,6 +73,8 @@ subpaths
6373
| main.go:74:15:74:18 | key2 | main.go:69:17:69:22 | "key5" : string | main.go:74:15:74:18 | key2 | $@ is used to sign a JWT token. | main.go:69:17:69:22 | "key5" | Hardcoded String |
6474
| main.go:84:41:84:43 | key | main.go:80:16:80:21 | "key6" : string | main.go:84:41:84:43 | key | $@ is used to sign a JWT token. | main.go:80:16:80:21 | "key6" | Hardcoded String |
6575
| main.go:91:66:91:69 | key2 | main.go:89:17:89:22 | "key7" : string | main.go:91:66:91:69 | key2 | $@ is used to sign a JWT token. | main.go:89:17:89:22 | "key7" | Hardcoded String |
66-
| main.go:103:30:103:32 | key | main.go:97:16:97:21 | "key8" : string | main.go:103:30:103:32 | key | $@ is used to sign a JWT token. | main.go:97:16:97:21 | "key8" | Hardcoded String |
67-
| main.go:108:16:108:24 | sharedKey | main.go:107:22:107:27 | "key9" : string | main.go:108:16:108:24 | sharedKey | $@ is used to sign a JWT token. | main.go:107:22:107:27 | "key9" | Hardcoded String |
68-
| main.go:114:16:114:30 | sharedKeyglobal | main.go:111:30:111:36 | "key10" : string | main.go:114:16:114:30 | sharedKeyglobal | $@ is used to sign a JWT token. | main.go:111:30:111:36 | "key10" | Hardcoded String |
76+
| main.go:102:30:102:32 | key | main.go:97:16:97:21 | "key8" : string | main.go:102:30:102:32 | key | $@ is used to sign a JWT token. | main.go:97:16:97:21 | "key8" | Hardcoded String |
77+
| 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 |
78+
| 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 |
79+
| 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/main.go

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ package main
77
//go:generate depstubber -vendor github.com/lestrrat/go-jwx/jwk "" New
88
//go:generate depstubber -vendor github.com/square/go-jose/v3 Recipient NewEncrypter,NewSigner
99
//go:generate depstubber -vendor gopkg.in/square/go-jose.v2 Recipient NewEncrypter,NewSigner
10-
////go:generate depstubber -vendor github.com/cristalhq/jwt/v3 Signer NewSignerHS,HS256
10+
//go:generate depstubber -vendor github.com/cristalhq/jwt/v3 Signer NewSignerHS,HS256
1111

1212
import (
1313
"time"
@@ -98,7 +98,6 @@ func go_kit() interface{} {
9898

9999
mapClaims = gjwt.MapClaims{"user": "go-kit"}
100100
)
101-
// e := func(ctx context.Context, i interface{}) (interface{}, error) { return ctx, nil }
102101

103102
return gokit.NewSigner(kid, key, nil, mapClaims) // BAD
104103
}

0 commit comments

Comments
 (0)