Skip to content

Commit 43d9d2c

Browse files
RasmusWLRasmusWL
authored
Merge pull request #14603 from github/max-schaefer/broken-crypto-algorithm-link
JavaScript/Python/Ruby: Improve alert message for `*/weak-cryptographic-algorithm`.
2 parents 4ae35d1 + 104700f commit 43d9d2c

File tree

17 files changed

+231
-116
lines changed

17 files changed

+231
-116
lines changed

‎javascript/ql/lib/semmle/javascript/frameworks/CryptoLibraries.qll

+51-19
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,7 @@ private module AsmCrypto {
5151
private class Apply extends CryptographicOperation::Range instanceof DataFlow::CallNode {
5252
DataFlow::Node input;
5353
CryptographicAlgorithm algorithm; // non-functional
54+
DataFlow::PropRead algorithmSelection;
5455
private string algorithmName;
5556
private string methodName;
5657

@@ -68,11 +69,14 @@ private module AsmCrypto {
6869
exists(DataFlow::SourceNode asmCrypto |
6970
asmCrypto = DataFlow::globalVarRef("asmCrypto") and
7071
algorithm.matchesName(algorithmName) and
71-
this = asmCrypto.getAPropertyRead(algorithmName).getAMemberCall(methodName) and
72+
algorithmSelection = asmCrypto.getAPropertyRead(algorithmName) and
73+
this = algorithmSelection.getAMemberCall(methodName) and
7274
input = this.getArgument(0)
7375
)
7476
}
7577

78+
override DataFlow::Node getInitialization() { result = algorithmSelection }
79+
7680
override DataFlow::Node getAnInput() { result = input }
7781

7882
override CryptographicAlgorithm getAlgorithm() { result = algorithm }
@@ -103,6 +107,7 @@ private module BrowserIdCrypto {
103107

104108
private class Apply extends CryptographicOperation::Range instanceof DataFlow::MethodCallNode {
105109
CryptographicAlgorithm algorithm; // non-functional
110+
DataFlow::CallNode keygen;
106111

107112
Apply() {
108113
/*
@@ -122,8 +127,7 @@ private module BrowserIdCrypto {
122127
*/
123128

124129
exists(
125-
DataFlow::SourceNode mod, DataFlow::Node algorithmNameNode, DataFlow::CallNode keygen,
126-
DataFlow::FunctionNode callback
130+
DataFlow::SourceNode mod, DataFlow::Node algorithmNameNode, DataFlow::FunctionNode callback
127131
|
128132
mod = DataFlow::moduleImport("browserid-crypto") and
129133
keygen = mod.getAMemberCall("generateKeypair") and
@@ -134,6 +138,8 @@ private module BrowserIdCrypto {
134138
)
135139
}
136140

141+
override DataFlow::Node getInitialization() { result = keygen }
142+
137143
override DataFlow::Node getAnInput() { result = super.getArgument(0) }
138144

139145
override CryptographicAlgorithm getAlgorithm() { result = algorithm }
@@ -239,6 +245,8 @@ private module NodeJSCrypto {
239245

240246
Apply() { this = instantiation.getAMethodCall(any(string m | m = "update" or m = "write")) }
241247

248+
override DataFlow::Node getInitialization() { result = instantiation }
249+
242250
override DataFlow::Node getAnInput() { result = super.getArgument(0) }
243251

244252
override CryptographicAlgorithm getAlgorithm() { result = instantiation.getAlgorithm() }
@@ -324,7 +332,9 @@ private module CryptoJS {
324332
)
325333
}
326334

327-
private API::CallNode getEncryptionApplication(API::Node input, CryptographicAlgorithm algorithm) {
335+
private API::CallNode getEncryptionApplication(
336+
API::Node input, API::Node algorithmNode, CryptographicAlgorithm algorithm
337+
) {
328338
/*
329339
* ```
330340
* var CryptoJS = require("crypto-js");
@@ -338,11 +348,14 @@ private module CryptoJS {
338348
* Also matches where `CryptoJS.<algorithmName>` has been replaced by `require("crypto-js/<algorithmName>")`
339349
*/
340350

341-
result = getAlgorithmNode(algorithm).getMember("encrypt").getACall() and
351+
algorithmNode = getAlgorithmNode(algorithm) and
352+
result = algorithmNode.getMember("encrypt").getACall() and
342353
input = result.getParameter(0)
343354
}
344355

345-
private API::CallNode getDirectApplication(API::Node input, CryptographicAlgorithm algorithm) {
356+
private API::CallNode getDirectApplication(
357+
API::Node input, API::Node algorithmNode, CryptographicAlgorithm algorithm
358+
) {
346359
/*
347360
* ```
348361
* var CryptoJS = require("crypto-js");
@@ -357,7 +370,8 @@ private module CryptoJS {
357370
* Also matches where `CryptoJS.<algorithmName>` has been replaced by `require("crypto-js/<algorithmName>")`
358371
*/
359372

360-
result = getAlgorithmNode(algorithm).getACall() and
373+
algorithmNode = getAlgorithmNode(algorithm) and
374+
result = algorithmNode.getACall() and
361375
input = result.getParameter(0)
362376
}
363377

@@ -389,18 +403,23 @@ private module CryptoJS {
389403
private class Apply extends CryptographicOperation::Range instanceof API::CallNode {
390404
API::Node input;
391405
CryptographicAlgorithm algorithm; // non-functional
406+
DataFlow::Node instantiation;
392407

393408
Apply() {
394-
this = getEncryptionApplication(input, algorithm)
395-
or
396-
this = getDirectApplication(input, algorithm)
397-
or
398-
exists(InstantiatedAlgorithm instantiation |
399-
this = getUpdatedApplication(input, instantiation) and
400-
algorithm = instantiation.getAlgorithm()
409+
exists(API::Node algorithmNode |
410+
this = getEncryptionApplication(input, algorithmNode, algorithm)
411+
or
412+
this = getDirectApplication(input, algorithmNode, algorithm)
413+
|
414+
instantiation = algorithmNode.asSource()
401415
)
416+
or
417+
this = getUpdatedApplication(input, instantiation) and
418+
algorithm = instantiation.(InstantiatedAlgorithm).getAlgorithm()
402419
}
403420

421+
override DataFlow::Node getInitialization() { result = instantiation }
422+
404423
override DataFlow::Node getAnInput() { result = input.asSink() }
405424

406425
override CryptographicAlgorithm getAlgorithm() { result = algorithm }
@@ -504,6 +523,8 @@ private module TweetNaCl {
504523
)
505524
}
506525

526+
override DataFlow::Node getInitialization() { result = this }
527+
507528
override DataFlow::Node getAnInput() { result = input }
508529

509530
override CryptographicAlgorithm getAlgorithm() { result = algorithm }
@@ -539,6 +560,7 @@ private module HashJs {
539560
private class Apply extends CryptographicOperation::Range instanceof DataFlow::CallNode {
540561
DataFlow::Node input;
541562
CryptographicAlgorithm algorithm; // non-functional
563+
DataFlow::CallNode init;
542564

543565
Apply() {
544566
/*
@@ -554,10 +576,13 @@ private module HashJs {
554576
* Also matches where `hash.<algorithmName>()` has been replaced by a more specific require a la `require("hash.js/lib/hash/sha/512")`
555577
*/
556578

557-
this = getAlgorithmNode(algorithm).getAMemberCall("update") and
579+
init = getAlgorithmNode(algorithm) and
580+
this = init.getAMemberCall("update") and
558581
input = super.getArgument(0)
559582
}
560583

584+
override DataFlow::Node getInitialization() { result = init }
585+
561586
override DataFlow::Node getAnInput() { result = input }
562587

563588
override CryptographicAlgorithm getAlgorithm() { result = algorithm }
@@ -653,6 +678,8 @@ private module Forge {
653678
algorithm = cipher.getAlgorithm()
654679
}
655680

681+
override DataFlow::Node getInitialization() { result = cipher }
682+
656683
override DataFlow::Node getAnInput() { result = input }
657684

658685
override CryptographicAlgorithm getAlgorithm() { result = algorithm }
@@ -715,6 +742,8 @@ private module Md5 {
715742
super.getArgument(0) = input
716743
}
717744

745+
override DataFlow::Node getInitialization() { result = this }
746+
718747
override DataFlow::Node getAnInput() { result = input }
719748

720749
override CryptographicAlgorithm getAlgorithm() { result = algorithm }
@@ -731,17 +760,18 @@ private module Bcrypt {
731760
private class Apply extends CryptographicOperation::Range instanceof DataFlow::CallNode {
732761
DataFlow::Node input;
733762
CryptographicAlgorithm algorithm;
763+
API::Node init;
734764

735765
Apply() {
736766
// `require("bcrypt").hash(password);` with minor naming variations
737767
algorithm.matchesName("BCRYPT") and
738-
this =
739-
API::moduleImport(["bcrypt", "bcryptjs", "bcrypt-nodejs"])
740-
.getMember(["hash", "hashSync"])
741-
.getACall() and
768+
init = API::moduleImport(["bcrypt", "bcryptjs", "bcrypt-nodejs"]) and
769+
this = init.getMember(["hash", "hashSync"]).getACall() and
742770
super.getArgument(0) = input
743771
}
744772

773+
override DataFlow::Node getInitialization() { result = init.asSource() }
774+
745775
override DataFlow::Node getAnInput() { result = input }
746776

747777
override CryptographicAlgorithm getAlgorithm() { result = algorithm }
@@ -769,6 +799,8 @@ private module Hasha {
769799
)
770800
}
771801

802+
override DataFlow::Node getInitialization() { result = this }
803+
772804
override DataFlow::Node getAnInput() { result = input }
773805

774806
override CryptographicAlgorithm getAlgorithm() { result = algorithm }

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

+6
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

‎javascript/ql/lib/semmle/javascript/security/dataflow/BrokenCryptoAlgorithmCustomizations.qll

+14-9
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,10 @@ module BrokenCryptoAlgorithm {
1919
/**
2020
* A data flow sink for sensitive information in broken or weak cryptographic algorithms.
2121
*/
22-
abstract class Sink extends DataFlow::Node { }
22+
abstract class Sink extends DataFlow::Node {
23+
/** Gets the data-flow node where the cryptographic algorithm used in this operation is configured. */
24+
abstract DataFlow::Node getInitialization();
25+
}
2326

2427
/**
2528
* A sanitizer for sensitive information in broken or weak cryptographic algorithms.
@@ -38,15 +41,17 @@ module BrokenCryptoAlgorithm {
3841
* An expression used by a broken or weak cryptographic algorithm.
3942
*/
4043
class WeakCryptographicOperationSink extends Sink {
44+
CryptographicOperation application;
45+
4146
WeakCryptographicOperationSink() {
42-
exists(CryptographicOperation application |
43-
(
44-
application.getAlgorithm().isWeak()
45-
or
46-
application.getBlockMode().isWeak()
47-
) and
48-
this = application.getAnInput()
49-
)
47+
(
48+
application.getAlgorithm().isWeak()
49+
or
50+
application.getBlockMode().isWeak()
51+
) and
52+
this = application.getAnInput()
5053
}
54+
55+
override DataFlow::Node getInitialization() { result = application.getInitialization() }
5156
}
5257
}

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

+9-4
Original file line numberDiff line numberDiff line change
@@ -16,9 +16,14 @@ import semmle.javascript.security.dataflow.BrokenCryptoAlgorithmQuery
1616
import semmle.javascript.security.SensitiveActions
1717
import DataFlow::PathGraph
1818

19-
from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
19+
from
20+
Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink, Source sourceNode,
21+
Sink sinkNode
2022
where
2123
cfg.hasFlowPath(source, sink) and
22-
not source.getNode() instanceof CleartextPasswordExpr // flagged by js/insufficient-password-hash
23-
select sink.getNode(), source, sink, "A broken or weak cryptographic algorithm depends on $@.",
24-
source.getNode(), "sensitive data from " + source.getNode().(Source).describe()
24+
sourceNode = source.getNode() and
25+
sinkNode = sink.getNode() and
26+
not sourceNode instanceof CleartextPasswordExpr // flagged by js/insufficient-password-hash
27+
select sinkNode, source, sink, "$@ depends on $@.", sinkNode.getInitialization(),
28+
"A broken or weak cryptographic algorithm", sourceNode,
29+
"sensitive data from " + sourceNode.describe()

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

+5-5
Original file line numberDiff line numberDiff line change
@@ -26,8 +26,8 @@ edges
2626
| tst.js:19:17:19:24 | password | tst.js:19:17:19:24 | password |
2727
| tst.js:22:21:22:30 | secretText | tst.js:22:21:22:30 | secretText |
2828
#select
29-
| tst.js:11:17:11:26 | secretText | tst.js:3:18:3:24 | trusted | tst.js:11:17:11:26 | secretText | A broken or weak cryptographic algorithm depends on $@. | tst.js:3:18:3:24 | trusted | sensitive data from an access to trusted |
30-
| tst.js:11:17:11:26 | secretText | tst.js:11:17:11:26 | secretText | tst.js:11:17:11:26 | secretText | A broken or weak cryptographic algorithm depends on $@. | tst.js:11:17:11:26 | secretText | sensitive data from an access to secretText |
31-
| tst.js:17:17:17:25 | o.trusted | tst.js:17:17:17:25 | o.trusted | tst.js:17:17:17:25 | o.trusted | A broken or weak cryptographic algorithm depends on $@. | tst.js:17:17:17:25 | o.trusted | sensitive data from an access to trusted |
32-
| tst.js:22:21:22:30 | secretText | tst.js:3:18:3:24 | trusted | tst.js:22:21:22:30 | secretText | A broken or weak cryptographic algorithm depends on $@. | tst.js:3:18:3:24 | trusted | sensitive data from an access to trusted |
33-
| tst.js:22:21:22:30 | secretText | tst.js:22:21:22:30 | secretText | tst.js:22:21:22:30 | secretText | A broken or weak cryptographic algorithm depends on $@. | tst.js:22:21:22:30 | secretText | sensitive data from an access to secretText |
29+
| tst.js:11:17:11:26 | secretText | tst.js:3:18:3:24 | trusted | tst.js:11:17:11:26 | secretText | $@ depends on $@. | tst.js:5:19:5:49 | crypto. ... ', key) | A broken or weak cryptographic algorithm | tst.js:3:18:3:24 | trusted | sensitive data from an access to trusted |
30+
| tst.js:11:17:11:26 | secretText | tst.js:11:17:11:26 | secretText | tst.js:11:17:11:26 | secretText | $@ depends on $@. | tst.js:5:19:5:49 | crypto. ... ', key) | A broken or weak cryptographic algorithm | tst.js:11:17:11:26 | secretText | sensitive data from an access to secretText |
31+
| tst.js:17:17:17:25 | o.trusted | tst.js:17:17:17:25 | o.trusted | tst.js:17:17:17:25 | o.trusted | $@ depends on $@. | tst.js:5:19:5:49 | crypto. ... ', key) | A broken or weak cryptographic algorithm | tst.js:17:17:17:25 | o.trusted | sensitive data from an access to trusted |
32+
| tst.js:22:21:22:30 | secretText | tst.js:3:18:3:24 | trusted | tst.js:22:21:22:30 | secretText | $@ depends on $@. | tst.js:21:22:21:60 | crypto. ... ', key) | A broken or weak cryptographic algorithm | tst.js:3:18:3:24 | trusted | sensitive data from an access to trusted |
33+
| tst.js:22:21:22:30 | secretText | tst.js:22:21:22:30 | secretText | tst.js:22:21:22:30 | secretText | $@ depends on $@. | tst.js:21:22:21:60 | crypto. ... ', key) | A broken or weak cryptographic algorithm | tst.js:22:21:22:30 | secretText | sensitive data from an access to secretText |

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

+15-7
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")] }

0 commit comments

Comments
 (0)