Skip to content

Commit 3939167

Browse files
author
Max Schaefer
committed
Include more details in the message for py/weak-cryptographic-algorithm.
Specifically, we add a link to the location where the cryptographic algorithm is configured, which can be far away from its use.
1 parent 28bedda commit 3939167

7 files changed

Lines changed: 89 additions & 42 deletions

File tree

‎python/ql/lib/semmle/python/frameworks/Cryptodome.qll‎

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,8 @@ private module CryptodomeModel {
128128
this = newCall.getReturn().getMember(methodName).getACall()
129129
}
130130

131+
override DataFlow::Node getInitialization() { result = newCall }
132+
131133
override Cryptography::CryptographicAlgorithm getAlgorithm() { result.matchesName(cipherName) }
132134

133135
override DataFlow::Node getAnInput() {
@@ -181,21 +183,23 @@ private module CryptodomeModel {
181183
class CryptodomeGenericSignatureOperation extends Cryptography::CryptographicOperation::Range,
182184
DataFlow::CallCfgNode
183185
{
186+
API::CallNode newCall;
184187
string methodName;
185188
string signatureName;
186189

187190
CryptodomeGenericSignatureOperation() {
188191
methodName in ["sign", "verify"] and
189-
this =
192+
newCall =
190193
API::moduleImport(["Crypto", "Cryptodome"])
191194
.getMember("Signature")
192195
.getMember(signatureName)
193196
.getMember("new")
194-
.getReturn()
195-
.getMember(methodName)
196-
.getACall()
197+
.getACall() and
198+
this = newCall.getReturn().getMember(methodName).getACall()
197199
}
198200

201+
override DataFlow::Node getInitialization() { result = newCall }
202+
199203
override Cryptography::CryptographicAlgorithm getAlgorithm() {
200204
result.matchesName(signatureName)
201205
}
@@ -221,19 +225,23 @@ private module CryptodomeModel {
221225
class CryptodomeGenericHashOperation extends Cryptography::CryptographicOperation::Range,
222226
DataFlow::CallCfgNode
223227
{
228+
API::CallNode newCall;
224229
string hashName;
225230

226231
CryptodomeGenericHashOperation() {
227232
exists(API::Node hashModule |
228233
hashModule =
229-
API::moduleImport(["Crypto", "Cryptodome"]).getMember("Hash").getMember(hashName)
234+
API::moduleImport(["Crypto", "Cryptodome"]).getMember("Hash").getMember(hashName) and
235+
newCall = hashModule.getMember("new").getACall()
230236
|
231-
this = hashModule.getMember("new").getACall()
237+
this = newCall
232238
or
233-
this = hashModule.getMember("new").getReturn().getMember("update").getACall()
239+
this = newCall.getReturn().getMember("update").getACall()
234240
)
235241
}
236242

243+
override DataFlow::Node getInitialization() { result = newCall }
244+
237245
override Cryptography::CryptographicAlgorithm getAlgorithm() { result.matchesName(hashName) }
238246

239247
override DataFlow::Node getAnInput() { result in [this.getArg(0), this.getArgByName("data")] }

‎python/ql/lib/semmle/python/frameworks/Cryptography.qll‎

Lines changed: 22 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -209,18 +209,18 @@ private module CryptographyModel {
209209
class CryptographyGenericCipherOperation extends Cryptography::CryptographicOperation::Range,
210210
DataFlow::MethodCallNode
211211
{
212+
API::CallNode init;
212213
string algorithmName;
213214
string modeName;
214215

215216
CryptographyGenericCipherOperation() {
216-
this =
217-
cipherInstance(algorithmName, modeName)
218-
.getMember(["decryptor", "encryptor"])
219-
.getReturn()
220-
.getMember(["update", "update_into"])
221-
.getACall()
217+
init =
218+
cipherInstance(algorithmName, modeName).getMember(["decryptor", "encryptor"]).getACall() and
219+
this = init.getReturn().getMember(["update", "update_into"]).getACall()
222220
}
223221

222+
override DataFlow::Node getInitialization() { result = init }
223+
224224
override Cryptography::CryptographicAlgorithm getAlgorithm() {
225225
result.matchesName(algorithmName)
226226
}
@@ -247,19 +247,17 @@ private module CryptographyModel {
247247
}
248248

249249
/** Gets a reference to a Hash instance using algorithm with `algorithmName`. */
250-
private API::Node hashInstance(string algorithmName) {
251-
exists(API::CallNode call | result = call.getReturn() |
252-
call =
253-
API::moduleImport("cryptography")
254-
.getMember("hazmat")
255-
.getMember("primitives")
256-
.getMember("hashes")
257-
.getMember("Hash")
258-
.getACall() and
259-
algorithmClassRef(algorithmName).getReturn().getAValueReachableFromSource() in [
260-
call.getArg(0), call.getArgByName("algorithm")
261-
]
262-
)
250+
private API::CallNode hashInstance(string algorithmName) {
251+
result =
252+
API::moduleImport("cryptography")
253+
.getMember("hazmat")
254+
.getMember("primitives")
255+
.getMember("hashes")
256+
.getMember("Hash")
257+
.getACall() and
258+
algorithmClassRef(algorithmName).getReturn().getAValueReachableFromSource() in [
259+
result.getArg(0), result.getArgByName("algorithm")
260+
]
263261
}
264262

265263
/**
@@ -268,12 +266,16 @@ private module CryptographyModel {
268266
class CryptographyGenericHashOperation extends Cryptography::CryptographicOperation::Range,
269267
DataFlow::MethodCallNode
270268
{
269+
API::CallNode init;
271270
string algorithmName;
272271

273272
CryptographyGenericHashOperation() {
274-
this = hashInstance(algorithmName).getMember("update").getACall()
273+
init = hashInstance(algorithmName) and
274+
this = init.getReturn().getMember("update").getACall()
275275
}
276276

277+
override DataFlow::Node getInitialization() { result = init }
278+
277279
override Cryptography::CryptographicAlgorithm getAlgorithm() {
278280
result.matchesName(algorithmName)
279281
}

‎python/ql/lib/semmle/python/frameworks/Rsa.qll‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,8 @@ private module Rsa {
3737
class RsaEncryptCall extends Cryptography::CryptographicOperation::Range, DataFlow::CallCfgNode {
3838
RsaEncryptCall() { this = API::moduleImport("rsa").getMember("encrypt").getACall() }
3939

40+
override DataFlow::Node getInitialization() { result = this }
41+
4042
override Cryptography::CryptographicAlgorithm getAlgorithm() { result.getName() = "RSA" }
4143

4244
override DataFlow::Node getAnInput() {
@@ -54,6 +56,8 @@ private module Rsa {
5456
class RsaDecryptCall extends Cryptography::CryptographicOperation::Range, DataFlow::CallCfgNode {
5557
RsaDecryptCall() { this = API::moduleImport("rsa").getMember("decrypt").getACall() }
5658

59+
override DataFlow::Node getInitialization() { result = this }
60+
5761
override Cryptography::CryptographicAlgorithm getAlgorithm() { result.getName() = "RSA" }
5862

5963
override DataFlow::Node getAnInput() { result in [this.getArg(0), this.getArgByName("crypto")] }
@@ -69,6 +73,8 @@ private module Rsa {
6973
class RsaSignCall extends Cryptography::CryptographicOperation::Range, DataFlow::CallCfgNode {
7074
RsaSignCall() { this = API::moduleImport("rsa").getMember("sign").getACall() }
7175

76+
override DataFlow::Node getInitialization() { result = this }
77+
7278
override Cryptography::CryptographicAlgorithm getAlgorithm() {
7379
// signature part
7480
result.getName() = "RSA"
@@ -96,6 +102,8 @@ private module Rsa {
96102
class RsaVerifyCall extends Cryptography::CryptographicOperation::Range, DataFlow::CallCfgNode {
97103
RsaVerifyCall() { this = API::moduleImport("rsa").getMember("verify").getACall() }
98104

105+
override DataFlow::Node getInitialization() { result = this }
106+
99107
override Cryptography::CryptographicAlgorithm getAlgorithm() {
100108
// note that technically there is also a hashing operation going on but we don't
101109
// know what algorithm is used up front, since it is encoded in the signature
@@ -121,6 +129,8 @@ private module Rsa {
121129
{
122130
RsaComputeHashCall() { this = API::moduleImport("rsa").getMember("compute_hash").getACall() }
123131

132+
override DataFlow::Node getInitialization() { result = this }
133+
124134
override Cryptography::CryptographicAlgorithm getAlgorithm() {
125135
exists(StrConst str, DataFlow::Node hashNameArg |
126136
hashNameArg in [this.getArg(1), this.getArgByName("method_name")] and
@@ -144,6 +154,8 @@ private module Rsa {
144154
class RsaSignHashCall extends Cryptography::CryptographicOperation::Range, DataFlow::CallCfgNode {
145155
RsaSignHashCall() { this = API::moduleImport("rsa").getMember("sign_hash").getACall() }
146156

157+
override DataFlow::Node getInitialization() { result = this }
158+
147159
override Cryptography::CryptographicAlgorithm getAlgorithm() { result.getName() = "RSA" }
148160

149161
override DataFlow::Node getAnInput() {

‎python/ql/lib/semmle/python/frameworks/Stdlib.qll‎

Lines changed: 26 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2747,6 +2747,8 @@ private module StdlibPrivate {
27472747
exists(this.getParameter(1, "data"))
27482748
}
27492749

2750+
override DataFlow::Node getInitialization() { result = this }
2751+
27502752
override Cryptography::CryptographicAlgorithm getAlgorithm() { result.matchesName(hashName) }
27512753

27522754
override DataFlow::Node getAnInput() { result = this.getParameter(1, "data").asSink() }
@@ -2758,12 +2760,16 @@ private module StdlibPrivate {
27582760
* A hashing operation by using the `update` method on the result of calling the `hashlib.new` function.
27592761
*/
27602762
class HashlibNewUpdateCall extends Cryptography::CryptographicOperation::Range, API::CallNode {
2763+
API::CallNode init;
27612764
string hashName;
27622765

27632766
HashlibNewUpdateCall() {
2764-
this = hashlibNewCall(hashName).getReturn().getMember("update").getACall()
2767+
init = hashlibNewCall(hashName) and
2768+
this = init.getReturn().getMember("update").getACall()
27652769
}
27662770

2771+
override DataFlow::Node getInitialization() { result = init }
2772+
27672773
override Cryptography::CryptographicAlgorithm getAlgorithm() { result.matchesName(hashName) }
27682774

27692775
override DataFlow::Node getAnInput() { result = this.getArg(0) }
@@ -2802,7 +2808,14 @@ private module StdlibPrivate {
28022808
* (such as `hashlib.md5`), by calling its' `update` method.
28032809
*/
28042810
class HashlibHashClassUpdateCall extends HashlibGenericHashOperation {
2805-
HashlibHashClassUpdateCall() { this = hashClass.getReturn().getMember("update").getACall() }
2811+
API::CallNode init;
2812+
2813+
HashlibHashClassUpdateCall() {
2814+
init = hashClass.getACall() and
2815+
this = hashClass.getReturn().getMember("update").getACall()
2816+
}
2817+
2818+
override DataFlow::Node getInitialization() { result = init }
28062819

28072820
override DataFlow::Node getAnInput() { result = this.getArg(0) }
28082821
}
@@ -2819,6 +2832,8 @@ private module StdlibPrivate {
28192832
exists([this.getArg(0), this.getArgByName("string")])
28202833
}
28212834

2835+
override DataFlow::Node getInitialization() { result = this }
2836+
28222837
override DataFlow::Node getAnInput() {
28232838
result = this.getArg(0)
28242839
or
@@ -2865,6 +2880,8 @@ private module StdlibPrivate {
28652880
exists(this.getParameter(1, "msg").asSink())
28662881
}
28672882

2883+
override DataFlow::Node getInitialization() { result = this }
2884+
28682885
override API::Node getDigestArg() { result = digestArg }
28692886

28702887
override DataFlow::Node getAnInput() { result = this.getParameter(1, "msg").asSink() }
@@ -2876,12 +2893,16 @@ private module StdlibPrivate {
28762893
* See https://docs.python.org/3.11/library/hmac.html#hmac.HMAC.update
28772894
*/
28782895
class HmacUpdateCall extends HmacCryptographicOperation {
2896+
API::CallNode init;
28792897
API::Node digestArg;
28802898

28812899
HmacUpdateCall() {
2882-
this = getHmacConstructorCall(digestArg).getReturn().getMember("update").getACall()
2900+
init = getHmacConstructorCall(digestArg) and
2901+
this = init.getReturn().getMember("update").getACall()
28832902
}
28842903

2904+
override DataFlow::Node getInitialization() { result = init }
2905+
28852906
override API::Node getDigestArg() { result = digestArg }
28862907

28872908
override DataFlow::Node getAnInput() { result = this.getParameter(0, "msg").asSink() }
@@ -2895,6 +2916,8 @@ private module StdlibPrivate {
28952916
class HmacDigestCall extends HmacCryptographicOperation {
28962917
HmacDigestCall() { this = API::moduleImport("hmac").getMember("digest").getACall() }
28972918

2919+
override DataFlow::Node getInitialization() { result = this }
2920+
28982921
override API::Node getDigestArg() { result = this.getParameter(2, "digest") }
28992922

29002923
override DataFlow::Node getAnInput() { result = this.getParameter(1, "msg").asSink() }

‎python/ql/lib/semmle/python/internal/ConceptsShared.qll‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,9 @@ module Cryptography {
4040
/** Gets the algorithm used, if it matches a known `CryptographicAlgorithm`. */
4141
CryptographicAlgorithm getAlgorithm() { result = super.getAlgorithm() }
4242

43+
/** Gets the data-flow node where the cryptographic algorithm used in this operation is configured. */
44+
DataFlow::Node getInitialization() { result = super.getInitialization() }
45+
4346
/** Gets an input the algorithm is used on, for example the plain text input to be encrypted. */
4447
DataFlow::Node getAnInput() { result = super.getAnInput() }
4548

@@ -65,6 +68,9 @@ module Cryptography {
6568
* extend `CryptographicOperation` instead.
6669
*/
6770
abstract class Range extends DataFlow::Node {
71+
/** Gets the data-flow node where the cryptographic algorithm used in this operation is configured. */
72+
abstract DataFlow::Node getInitialization();
73+
6874
/** Gets the algorithm used, if it matches a known `CryptographicAlgorithm`. */
6975
abstract CryptographicAlgorithm getAlgorithm();
7076

‎python/ql/src/Security/CWE-327/BrokenCryptoAlgorithm.ql‎

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -13,18 +13,14 @@
1313
import python
1414
import semmle.python.Concepts
1515

16-
from
17-
Cryptography::CryptographicOperation operation, Cryptography::CryptographicAlgorithm algorithm,
18-
string msgPrefix
16+
from Cryptography::CryptographicOperation operation, string msgPrefix
1917
where
20-
algorithm = operation.getAlgorithm() and
2118
// `Cryptography::HashingAlgorithm` and `Cryptography::PasswordHashingAlgorithm` are
2219
// handled by `py/weak-sensitive-data-hashing`
23-
algorithm instanceof Cryptography::EncryptionAlgorithm and
24-
(
20+
exists(Cryptography::EncryptionAlgorithm algorithm | algorithm = operation.getAlgorithm() |
2521
algorithm.isWeak() and
26-
msgPrefix = "The cryptographic algorithm " + operation.getAlgorithm().getName()
22+
msgPrefix = "The cryptographic algorithm " + algorithm.getName()
2723
)
2824
or
2925
operation.getBlockMode().isWeak() and msgPrefix = "The block mode " + operation.getBlockMode()
30-
select operation, msgPrefix + " is broken or weak, and should not be used."
26+
select operation, msgPrefix + " (configured $@) is broken or weak, and should not be used.", operation.getInitialization(), "here"

‎python/ql/test/query-tests/Security/CWE-327-BrokenCryptoAlgorithm/BrokenCryptoAlgorithm.expected‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
| test_cryptodome.py:11:13:11:42 | ControlFlowNode for Attribute() | The cryptographic algorithm ARC4 is broken or weak, and should not be used. |
2-
| test_cryptodome.py:16:13:16:42 | ControlFlowNode for Attribute() | The block mode ECB is broken or weak, and should not be used. |
3-
| test_cryptography.py:13:13:13:44 | ControlFlowNode for Attribute() | The cryptographic algorithm ARC4 is broken or weak, and should not be used. |
4-
| test_cryptography.py:22:13:22:58 | ControlFlowNode for Attribute() | The block mode ECB is broken or weak, and should not be used. |
1+
| test_cryptodome.py:11:13:11:42 | ControlFlowNode for Attribute() | The cryptographic algorithm ARC4 (configured $@) is broken or weak, and should not be used. | test_cryptodome.py:10:10:10:22 | ControlFlowNode for Attribute() | here |
2+
| test_cryptodome.py:16:13:16:42 | ControlFlowNode for Attribute() | The block mode ECB (configured $@) is broken or weak, and should not be used. | test_cryptodome.py:15:10:15:35 | ControlFlowNode for Attribute() | here |
3+
| test_cryptography.py:13:13:13:44 | ControlFlowNode for Attribute() | The cryptographic algorithm ARC4 (configured $@) is broken or weak, and should not be used. | test_cryptography.py:12:13:12:30 | ControlFlowNode for Attribute() | here |
4+
| test_cryptography.py:22:13:22:58 | ControlFlowNode for Attribute() | The block mode ECB (configured $@) is broken or weak, and should not be used. | test_cryptography.py:21:13:21:30 | ControlFlowNode for Attribute() | here |

0 commit comments

Comments
 (0)