Skip to content

Commit e4d2c7c

Browse files
geoffw0geoffw0
committed
C++: Rewrite so that we look for additional evidence.
1 parent 123889a commit e4d2c7c

File tree

4 files changed

+76
-47
lines changed

4 files changed

+76
-47
lines changed

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

Lines changed: 54 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -13,52 +13,72 @@
1313
import cpp
1414
import semmle.code.cpp.security.Encryption
1515

16-
abstract class InsecureCryptoSpec extends Locatable {
17-
abstract string description();
16+
/**
17+
* A function which may relate to an insecure encryption algorithm.
18+
*/
19+
Function getAnInsecureEncryptionFunction() {
20+
(
21+
isInsecureEncryption(result.getName()) or
22+
isInsecureEncryption(result.getAParameter().getName())
23+
) and
24+
exists(result.getACallToThisFunction())
1825
}
1926

20-
Function getAnInsecureFunction() {
21-
isInsecureEncryption(result.getName()) and
27+
/**
28+
* A function with additional evidence it is related to encryption.
29+
*/
30+
Function getAdditionalEvidenceFunction() {
31+
(
32+
isEncryptionAdditionalEvidence(result.getName()) or
33+
isEncryptionAdditionalEvidence(result.getAParameter().getName())
34+
) and
2235
exists(result.getACallToThisFunction())
2336
}
2437

25-
class InsecureFunctionCall extends InsecureCryptoSpec, FunctionCall {
26-
InsecureFunctionCall() {
27-
// the function name suggests it relates to an insecure crypto algorithm.
28-
this.getTarget() = getAnInsecureFunction()
29-
}
30-
31-
override string description() { result = "function call" }
32-
33-
override string toString() { result = FunctionCall.super.toString() }
34-
35-
override Location getLocation() { result = FunctionCall.super.getLocation() }
38+
/**
39+
* A macro which may relate to an insecure encryption algorithm.
40+
*/
41+
Macro getAnInsecureEncryptionMacro() {
42+
isInsecureEncryption(result.getName()) and
43+
exists(result.getAnInvocation())
3644
}
3745

38-
Macro getAnInsecureMacro() {
39-
isInsecureEncryption(result.getName()) and
46+
/**
47+
* A macro with additional evidence it is related to encryption.
48+
*/
49+
Macro getAdditionalEvidenceMacro() {
50+
isEncryptionAdditionalEvidence(result.getName()) and
4051
exists(result.getAnInvocation())
4152
}
4253

43-
class InsecureMacroSpec extends InsecureCryptoSpec, MacroInvocation {
44-
InsecureMacroSpec() {
45-
// the macro name suggests it relates to an insecure crypto algorithm.
46-
this.getMacro() = getAnInsecureMacro() and
47-
// the macro invocation generates something.
48-
exists(this.getAGeneratedElement().(ControlFlowNode)) and
49-
// exclude expressions controlling ifs/switches (as they may not be used).
50-
not any(IfStmt c).getCondition().getAChild*() = this.getAGeneratedElement() and
51-
not any(SwitchCase c).getExpr().getAChild*() = this.getAGeneratedElement() and
52-
// exclude expressions in array initializers (as they may not be used).
53-
not any(AggregateLiteral i).getAChild*() = this.getAGeneratedElement()
54+
/**
55+
* A function call we have a high confidence is related to use of an insecure
56+
* encryption algorithm.
57+
*/
58+
class InsecureFunctionCall extends FunctionCall {
59+
InsecureFunctionCall() {
60+
// find use of an insecure algorithm name
61+
(
62+
getTarget() = getAnInsecureEncryptionFunction()
63+
or
64+
exists(MacroInvocation mi |
65+
mi.getAGeneratedElement() = this.getAChild*() and
66+
mi.getMacro() = getAnInsecureEncryptionMacro()
67+
)
68+
) and
69+
// find additional evidence that this function is related to encryption.
70+
(
71+
getTarget() = getAdditionalEvidenceFunction()
72+
or
73+
exists(MacroInvocation mi |
74+
mi.getAGeneratedElement() = this.getAChild*() and
75+
mi.getMacro() = getAdditionalEvidenceMacro()
76+
)
77+
)
5478
}
5579

56-
override string description() { result = "macro invocation" }
57-
58-
override string toString() { result = MacroInvocation.super.toString() }
59-
60-
override Location getLocation() { result = MacroInvocation.super.getLocation() }
80+
string description() { result = "function call" }
6181
}
6282

63-
from InsecureCryptoSpec c
83+
from InsecureFunctionCall c
6484
select c, "This " + c.description() + " specifies a broken or weak cryptographic algorithm."

‎cpp/ql/src/semmle/code/cpp/security/Encryption.qll

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,17 @@ predicate isInsecureEncryption(string name) {
6060
not name.toUpperCase().regexpMatch(".*TRIPLE.*")
6161
}
6262

63+
/**
64+
* Holds if there is additional evidence that `name` looks like it might be
65+
* related to operations with an encyption algorithm, besides the name of a
66+
* specific algorithm. This can be used in conjuction with
67+
* `isInsecureEncryption` to produce a stronger heuristic.
68+
*/
69+
bindingset[name]
70+
predicate isEncryptionAdditionalEvidence(string name) {
71+
name.toUpperCase().regexpMatch(".*(CRYPT|CODE|CODING|CBC|KEY).*")
72+
}
73+
6374
/**
6475
* Gets a regular expression for matching strings that look like they
6576
* contain an algorithm that is known to be secure.

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

Lines changed: 9 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,14 @@
11
| test2.cpp:49:4:49:24 | call to my_des_implementation | This function call specifies a broken or weak cryptographic algorithm. |
2-
| test2.cpp:62:33:62:40 | ALGO_DES | This macro invocation specifies a broken or weak cryptographic algorithm. |
2+
| test2.cpp:62:2:62:12 | call to encrypt_bad | This function call specifies a broken or weak cryptographic algorithm. |
33
| test2.cpp:124:4:124:24 | call to my_des_implementation | This function call specifies a broken or weak cryptographic algorithm. |
4-
| test2.cpp:172:28:172:35 | ALGO_DES | This macro invocation specifies a broken or weak cryptographic algorithm. |
5-
| test2.cpp:182:38:182:45 | ALGO_DES | This macro invocation specifies a broken or weak cryptographic algorithm. |
6-
| test.cpp:38:2:38:31 | ENCRYPT_WITH_DES(data,amount) | This macro invocation specifies a broken or weak cryptographic algorithm. |
7-
| test.cpp:39:2:39:31 | ENCRYPT_WITH_RC2(data,amount) | This macro invocation specifies a broken or weak cryptographic algorithm. |
8-
| test.cpp:51:2:51:32 | DES_DO_ENCRYPTION(data,amount) | This macro invocation specifies a broken or weak cryptographic algorithm. |
9-
| test.cpp:52:2:52:31 | RUN_DES_ENCODING(data,amount) | This macro invocation specifies a broken or weak cryptographic algorithm. |
10-
| test.cpp:53:2:53:25 | DES_ENCODE(data,amount) | This macro invocation specifies a broken or weak cryptographic algorithm. |
11-
| test.cpp:54:2:54:26 | DES_SET_KEY(data,amount) | This macro invocation specifies a broken or weak cryptographic algorithm. |
12-
| test.cpp:56:2:56:9 | DES(str) | This macro invocation specifies a broken or weak cryptographic algorithm. |
13-
| test.cpp:59:12:59:25 | SORT_ORDER_DES | This macro invocation specifies a broken or weak cryptographic algorithm. |
4+
| test2.cpp:172:2:172:26 | call to set_encryption_algorithm1 | This function call specifies a broken or weak cryptographic algorithm. |
5+
| test2.cpp:182:2:182:17 | call to encryption_with1 | This function call specifies a broken or weak cryptographic algorithm. |
6+
| test.cpp:38:2:38:31 | call to my_implementation1 | This function call specifies a broken or weak cryptographic algorithm. |
7+
| test.cpp:39:2:39:31 | call to my_implementation2 | This function call specifies a broken or weak cryptographic algorithm. |
8+
| test.cpp:51:2:51:32 | call to my_implementation1 | This function call specifies a broken or weak cryptographic algorithm. |
9+
| test.cpp:52:2:52:31 | call to my_implementation1 | This function call specifies a broken or weak cryptographic algorithm. |
10+
| test.cpp:53:2:53:25 | call to my_implementation1 | This function call specifies a broken or weak cryptographic algorithm. |
11+
| test.cpp:54:2:54:26 | call to my_implementation1 | This function call specifies a broken or weak cryptographic algorithm. |
1412
| test.cpp:88:2:88:11 | call to encryptDES | This function call specifies a broken or weak cryptographic algorithm. |
1513
| test.cpp:89:2:89:11 | call to encryptRC2 | This function call specifies a broken or weak cryptographic algorithm. |
1614
| test.cpp:101:2:101:15 | call to do_des_encrypt | This function call specifies a broken or weak cryptographic algorithm. |

‎cpp/ql/test/query-tests/Security/CWE/CWE-327/test.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -53,10 +53,10 @@ void test_macros(void *data, size_t amount, const char *str)
5353
DES_ENCODE(data, amount); // BAD
5454
DES_SET_KEY(data, amount); // BAD
5555

56-
DES(str); // GOOD (probably nothing to do with encryption) [FALSE POSITIVE]
56+
DES(str); // GOOD (probably nothing to do with encryption)
5757
DESMOND(str); // GOOD (probably nothing to do with encryption)
5858
ANODES(str); // GOOD (probably nothing to do with encryption)
59-
int ord = SORT_ORDER_DES; // GOOD (probably nothing to do with encryption) [FALSE POSITIVE]
59+
int ord = SORT_ORDER_DES; // GOOD (probably nothing to do with encryption)
6060
}
6161

6262
// --- simple encryption function calls ---

0 commit comments

Comments
 (0)