Skip to content

Commit 05389bb

Browse files
MathiasVPMathiasVP
authored
Merge pull request #6099 from geoffw0/weak-crypto3
Further improvements to cpp/weak-cryptographic-algorithm
2 parents 565af1a + 05ed4ed commit 05389bb

File tree

5 files changed

+198
-25
lines changed

5 files changed

+198
-25
lines changed

‎cpp/change-notes/2021-06-21-weak-cryptographic-algorithm.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
lgtm,codescanning
2+
* The "Use of a broken or risky cryptographic algorithm" (`cpp/weak-cryptographic-algorithm`) query has been further improved to reduce false positives and its `@precision` increased to `high`.

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

Lines changed: 32 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
* @kind problem
66
* @problem.severity error
77
* @security-severity 7.5
8-
* @precision medium
8+
* @precision high
99
* @id cpp/weak-cryptographic-algorithm
1010
* @tags security
1111
* external/cwe/cwe-327
@@ -70,9 +70,12 @@ EnumConstant getAdditionalEvidenceEnumConst() { isEncryptionAdditionalEvidence(r
7070
predicate getInsecureEncryptionEvidence(FunctionCall fc, Element blame, string description) {
7171
// find use of an insecure algorithm name
7272
(
73-
fc.getTarget() = getAnInsecureEncryptionFunction() and
74-
blame = fc and
75-
description = "call to " + fc.getTarget().getName()
73+
exists(FunctionCall fc2 |
74+
fc.getAChild*() = fc2 and
75+
fc2.getTarget() = getAnInsecureEncryptionFunction() and
76+
blame = fc2 and
77+
description = "call to " + fc.getTarget().getName()
78+
)
7679
or
7780
exists(MacroInvocation mi |
7881
(
@@ -93,7 +96,10 @@ predicate getInsecureEncryptionEvidence(FunctionCall fc, Element blame, string d
9396
) and
9497
// find additional evidence that this function is related to encryption.
9598
(
96-
fc.getTarget() = getAnAdditionalEvidenceFunction()
99+
exists(FunctionCall fc2 |
100+
fc.getAChild*() = fc2 and
101+
fc2.getTarget() = getAnAdditionalEvidenceFunction()
102+
)
97103
or
98104
exists(MacroInvocation mi |
99105
(
@@ -107,6 +113,27 @@ predicate getInsecureEncryptionEvidence(FunctionCall fc, Element blame, string d
107113
ec = fc.getAnArgument() and
108114
ec.getTarget() = getAdditionalEvidenceEnumConst()
109115
)
116+
) and
117+
// exclude calls from templates as this is rarely the right place to flag an
118+
// issue
119+
not fc.isFromTemplateInstantiation(_) and
120+
(
121+
// the function should have an input that looks like a non-constant buffer
122+
exists(Expr e |
123+
fc.getAnArgument() = e and
124+
(
125+
e.getUnspecifiedType() instanceof PointerType or
126+
e.getUnspecifiedType() instanceof ReferenceType or
127+
e.getUnspecifiedType() instanceof ArrayType
128+
) and
129+
not e.getType().isDeeplyConstBelow() and
130+
not e.isConstant()
131+
)
132+
or
133+
// or be a non-const member function of an object
134+
fc.getTarget() instanceof MemberFunction and
135+
not fc.getTarget() instanceof ConstMemberFunction and
136+
not fc.getTarget().isStatic()
110137
)
111138
}
112139

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

Lines changed: 12 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,16 @@
11
| test2.cpp:49:4:49:24 | call to my_des_implementation | This file makes use of a broken or weak cryptographic algorithm (specified by $@). | test2.cpp:49:4:49:24 | call to my_des_implementation | call to my_des_implementation |
2-
| test2.cpp:49:4:49:24 | call to my_des_implementation | This file makes use of a broken or weak cryptographic algorithm (specified by $@). | test2.cpp:62:33:62:40 | ALGO_DES | invocation of macro ALGO_DES |
3-
| test2.cpp:49:4:49:24 | call to my_des_implementation | This file makes use of a broken or weak cryptographic algorithm (specified by $@). | test2.cpp:124:4:124:24 | call to my_des_implementation | call to my_des_implementation |
4-
| test2.cpp:49:4:49:24 | call to my_des_implementation | This file makes use of a broken or weak cryptographic algorithm (specified by $@). | test2.cpp:144:27:144:29 | DES | access of enum constant DES |
5-
| test2.cpp:49:4:49:24 | call to my_des_implementation | This file makes use of a broken or weak cryptographic algorithm (specified by $@). | test2.cpp:172:28:172:35 | ALGO_DES | invocation of macro ALGO_DES |
6-
| test2.cpp:49:4:49:24 | call to my_des_implementation | This file makes use of a broken or weak cryptographic algorithm (specified by $@). | test2.cpp:175:28:175:34 | USE_DES | access of enum constant USE_DES |
7-
| test2.cpp:49:4:49:24 | call to my_des_implementation | This file makes use of a broken or weak cryptographic algorithm (specified by $@). | test2.cpp:182:38:182:45 | ALGO_DES | invocation of macro ALGO_DES |
8-
| test2.cpp:49:4:49:24 | call to my_des_implementation | This file makes use of a broken or weak cryptographic algorithm (specified by $@). | test2.cpp:185:38:185:44 | USE_DES | access of enum constant USE_DES |
9-
| test2.cpp:49:4:49:24 | call to my_des_implementation | This file makes use of a broken or weak cryptographic algorithm (specified by $@). | test2.cpp:238:2:238:20 | call to encrypt | call to encrypt |
10-
| test2.cpp:49:4:49:24 | call to my_des_implementation | This file makes use of a broken or weak cryptographic algorithm (specified by $@). | test2.cpp:245:5:245:11 | call to encrypt | call to encrypt |
2+
| test2.cpp:49:4:49:24 | call to my_des_implementation | This file makes use of a broken or weak cryptographic algorithm (specified by $@). | test2.cpp:64:33:64:40 | ALGO_DES | invocation of macro ALGO_DES |
3+
| test2.cpp:49:4:49:24 | call to my_des_implementation | This file makes use of a broken or weak cryptographic algorithm (specified by $@). | test2.cpp:66:31:66:38 | ALGO_DES | invocation of macro ALGO_DES |
4+
| test2.cpp:49:4:49:24 | call to my_des_implementation | This file makes use of a broken or weak cryptographic algorithm (specified by $@). | test2.cpp:128:4:128:24 | call to my_des_implementation | call to my_des_implementation |
5+
| test2.cpp:49:4:49:24 | call to my_des_implementation | This file makes use of a broken or weak cryptographic algorithm (specified by $@). | test2.cpp:148:27:148:29 | DES | access of enum constant DES |
6+
| test2.cpp:49:4:49:24 | call to my_des_implementation | This file makes use of a broken or weak cryptographic algorithm (specified by $@). | test2.cpp:186:38:186:45 | ALGO_DES | invocation of macro ALGO_DES |
7+
| test2.cpp:49:4:49:24 | call to my_des_implementation | This file makes use of a broken or weak cryptographic algorithm (specified by $@). | test2.cpp:189:38:189:44 | USE_DES | access of enum constant USE_DES |
8+
| test2.cpp:49:4:49:24 | call to my_des_implementation | This file makes use of a broken or weak cryptographic algorithm (specified by $@). | test2.cpp:242:2:242:20 | call to encrypt | call to encrypt |
9+
| test2.cpp:49:4:49:24 | call to my_des_implementation | This file makes use of a broken or weak cryptographic algorithm (specified by $@). | test2.cpp:249:5:249:11 | call to encrypt | call to encrypt |
10+
| test2.cpp:49:4:49:24 | call to my_des_implementation | This file makes use of a broken or weak cryptographic algorithm (specified by $@). | test2.cpp:304:20:304:37 | call to desEncryptor | call to desEncryptor |
11+
| test2.cpp:49:4:49:24 | call to my_des_implementation | This file makes use of a broken or weak cryptographic algorithm (specified by $@). | test2.cpp:308:5:308:19 | call to doDesEncryption | call to doDesEncryption |
12+
| test2.cpp:49:4:49:24 | call to my_des_implementation | This file makes use of a broken or weak cryptographic algorithm (specified by $@). | test2.cpp:309:9:309:23 | call to doDesEncryption | call to doDesEncryption |
13+
| test2.cpp:49:4:49:24 | call to my_des_implementation | This file makes use of a broken or weak cryptographic algorithm (specified by $@). | test2.cpp:403:26:403:45 | call to getEncryptionNameDES | call to doEncryption |
1114
| test.cpp:38:2:38:31 | ENCRYPT_WITH_DES(data,amount) | This file makes use of a broken or weak cryptographic algorithm (specified by $@). | test.cpp:38:2:38:31 | ENCRYPT_WITH_DES(data,amount) | invocation of macro ENCRYPT_WITH_DES |
1215
| test.cpp:38:2:38:31 | ENCRYPT_WITH_DES(data,amount) | This file makes use of a broken or weak cryptographic algorithm (specified by $@). | test.cpp:39:2:39:31 | ENCRYPT_WITH_RC2(data,amount) | invocation of macro ENCRYPT_WITH_RC2 |
1316
| test.cpp:38:2:38:31 | ENCRYPT_WITH_DES(data,amount) | This file makes use of a broken or weak cryptographic algorithm (specified by $@). | test.cpp:41:2:41:32 | ENCRYPT_WITH_3DES(data,amount) | invocation of macro ENCRYPT_WITH_3DES |
@@ -21,5 +24,3 @@
2124
| test.cpp:38:2:38:31 | ENCRYPT_WITH_DES(data,amount) | This file makes use of a broken or weak cryptographic algorithm (specified by $@). | test.cpp:91:2:91:12 | call to encrypt3DES | call to encrypt3DES |
2225
| test.cpp:38:2:38:31 | ENCRYPT_WITH_DES(data,amount) | This file makes use of a broken or weak cryptographic algorithm (specified by $@). | test.cpp:92:2:92:17 | call to encryptTripleDES | call to encryptTripleDES |
2326
| test.cpp:38:2:38:31 | ENCRYPT_WITH_DES(data,amount) | This file makes use of a broken or weak cryptographic algorithm (specified by $@). | test.cpp:101:2:101:15 | call to do_des_encrypt | call to do_des_encrypt |
24-
| test.cpp:38:2:38:31 | ENCRYPT_WITH_DES(data,amount) | This file makes use of a broken or weak cryptographic algorithm (specified by $@). | test.cpp:102:2:102:12 | call to DES_Set_Key | call to DES_Set_Key |
25-
| test.cpp:38:2:38:31 | ENCRYPT_WITH_DES(data,amount) | This file makes use of a broken or weak cryptographic algorithm (specified by $@). | test.cpp:121:2:121:24 | INIT_ENCRYPT_WITH_DES() | invocation of macro INIT_ENCRYPT_WITH_DES |

‎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
@@ -99,7 +99,7 @@ void test_functions(void *data, size_t amount, const char *str)
9999
DoDESEncryption(data, amount); // BAD [NOT DETECTED]
100100
encryptDes(data, amount); // BAD [NOT DETECTED]
101101
do_des_encrypt(data, amount); // BAD
102-
DES_Set_Key(str); // BAD
102+
DES_Set_Key(str); // BAD [NOT DETECTED]
103103
DESSetKey(str); // BAD [NOT DETECTED]
104104

105105
Des(); // GOOD (probably nothing to do with encryption)
@@ -118,7 +118,7 @@ void my_implementation8();
118118

119119
void test_macros2()
120120
{
121-
INIT_ENCRYPT_WITH_DES(); // BAD
121+
INIT_ENCRYPT_WITH_DES(); // BAD [NOT DETECTED]
122122
INIT_ENCRYPT_WITH_AES(); // GOOD (good algorithm)
123123

124124
// ...

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

Lines changed: 150 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -58,8 +58,12 @@ void encrypt_bad(char *data, size_t amount, keytype key, int algo)
5858

5959
void do_encrypts(char *data, size_t amount, keytype key)
6060
{
61+
char data2[128];
62+
6163
encrypt_good(data, amount, key, ALGO_AES); // GOOD
6264
encrypt_bad(data, amount, key, ALGO_DES); // BAD
65+
encrypt_good(data2, 128, key, ALGO_AES); // GOOD
66+
encrypt_bad(data2, 128, key, ALGO_DES); // BAD
6367
}
6468

6569
// --- more involved CPP-style example ---
@@ -169,10 +173,10 @@ const char *get_algorithm3();
169173

170174
void do_unseen_encrypts(char *data, size_t amount, keytype key)
171175
{
172-
set_encryption_algorithm1(ALGO_DES); // BAD
176+
set_encryption_algorithm1(ALGO_DES); // BAD [NOT DETECTED]
173177
set_encryption_algorithm1(ALGO_AES); // GOOD
174178

175-
set_encryption_algorithm2(USE_DES); // BAD
179+
set_encryption_algorithm2(USE_DES); // BAD [NOT DETECTED]
176180
set_encryption_algorithm2(USE_AES); // GOOD
177181

178182
set_encryption_algorithm3("DES"); // BAD [NOT DETECTED]
@@ -208,32 +212,32 @@ void do_unseen_encrypts(char *data, size_t amount, keytype key)
208212
class desEncrypt
209213
{
210214
public:
211-
static void encrypt(const char *data);
215+
static void encrypt(char *data);
212216
static void doSomethingElse();
213217
};
214218

215219
class aes256Encrypt
216220
{
217221
public:
218-
static void encrypt(const char *data);
222+
static void encrypt(char *data);
219223
static void doSomethingElse();
220224
};
221225

222226
class desCipher
223227
{
224228
public:
225-
void encrypt(const char *data);
229+
void encrypt(char *data);
226230
void doSomethingElse();
227231
};
228232

229233
class aesCipher
230234
{
231235
public:
232-
void encrypt(const char *data);
236+
void encrypt(char *data);
233237
void doSomethingElse();
234238
};
235239

236-
void do_classes(const char *data)
240+
void do_classes(char *data)
237241
{
238242
desEncrypt::encrypt(data); // BAD
239243
aes256Encrypt::encrypt(data); // GOOD
@@ -260,3 +264,142 @@ void do_fn_ptr(char *data, size_t amount, keytype key)
260264
impl = &my_aes_implementation; // GOOD
261265
impl(data, amount, key);
262266
}
267+
268+
// --- template classes ---
269+
270+
class desEncryptor
271+
{
272+
public:
273+
desEncryptor();
274+
275+
void doDesEncryption(char *data);
276+
};
277+
278+
template <class C>
279+
class container
280+
{
281+
public:
282+
container() {
283+
obj = new C(); // GOOD
284+
}
285+
286+
~container() {
287+
delete obj;
288+
}
289+
290+
C *obj;
291+
};
292+
293+
template <class C>
294+
class templateDesEncryptor
295+
{
296+
public:
297+
templateDesEncryptor();
298+
299+
void doDesEncryption(C &data);
300+
};
301+
302+
void do_template_classes(char *data)
303+
{
304+
desEncryptor *p = new desEncryptor(); // BAD
305+
container<desEncryptor> c; // BAD [NOT DETECTED]
306+
templateDesEncryptor<char *> t; // BAD [NOT DETECTED]
307+
308+
p->doDesEncryption(data); // BAD
309+
c.obj->doDesEncryption(data); // BAD
310+
t.doDesEncryption(data); // BAD [NOT DETECTED]
311+
}
312+
313+
// --- assert ---
314+
315+
int assertFunc(const char *file, int line);
316+
#define assert(_cond) ((_cond) || assertFunc(__FILE__, __LINE__))
317+
318+
struct algorithmInfo;
319+
320+
const algorithmInfo *getEncryptionAlgorithmInfo(int algo);
321+
322+
void test_assert(int algo, algorithmInfo *algoInfo)
323+
{
324+
assert(algo != ALGO_DES); // GOOD
325+
assert(algoInfo != getEncryptionAlgorithmInfo(ALGO_DES)); // GOOD
326+
327+
// ...
328+
}
329+
330+
// --- string comparisons ---
331+
332+
int strcmp(const char *s1, const char *s2);
333+
void abort(void);
334+
335+
#define ENCRYPTION_DES_NAME "DES"
336+
#define ENCRYPTION_AES_NAME "AES"
337+
338+
void test_string_comparisons1(const char *algo_name)
339+
{
340+
if (strcmp(algo_name, ENCRYPTION_DES_NAME) == 0) // GOOD
341+
{
342+
abort();
343+
}
344+
if (strcmp(algo_name, ENCRYPTION_AES_NAME) == 0) // GOOD
345+
{
346+
// ...
347+
}
348+
}
349+
350+
const char *getEncryptionNameDES()
351+
{
352+
return "DES";
353+
}
354+
355+
const char *getEncryptionNameAES()
356+
{
357+
return "AES";
358+
}
359+
360+
void test_string_comparisons2(const char *algo_name)
361+
{
362+
if (strcmp(algo_name, getEncryptionNameDES()) == 0) // GOOD
363+
{
364+
abort();
365+
}
366+
if (strcmp(algo_name, getEncryptionNameAES()) == 0) // GOOD
367+
{
368+
// ...
369+
}
370+
}
371+
372+
const char *getEncryptionName(int algo)
373+
{
374+
switch (algo)
375+
{
376+
case ALGO_DES:
377+
return getEncryptionNameDES(); // GOOD
378+
case ALGO_AES:
379+
return getEncryptionNameAES(); // GOOD
380+
default:
381+
abort();
382+
}
383+
}
384+
385+
void test_string_comparisons3(const char *algo_name)
386+
{
387+
if (strcmp(algo_name, getEncryptionName(ALGO_DES)) == 0) // GOOD
388+
{
389+
abort();
390+
}
391+
if (strcmp(algo_name, getEncryptionName(ALGO_AES)) == 0) // GOOD
392+
{
393+
// ...
394+
}
395+
}
396+
397+
// --- function call in a function call ---
398+
399+
void doEncryption(char *data, size_t len, const char *algorithmName);
400+
401+
void test_fn_in_fn(char *data, size_t len)
402+
{
403+
doEncryption(data, len, getEncryptionNameDES()); // BAD
404+
doEncryption(data, len, getEncryptionNameAES()); // GOOD
405+
}

0 commit comments

Comments
 (0)