diff --git a/cpp/ql/src/Likely Bugs/Memory Management/MemsetMayBeDeleted-bad.c b/cpp/ql/src/Likely Bugs/Memory Management/MemsetMayBeDeleted-bad.c new file mode 100644 index 000000000000..514b23241c90 --- /dev/null +++ b/cpp/ql/src/Likely Bugs/Memory Management/MemsetMayBeDeleted-bad.c @@ -0,0 +1,4 @@ +char * password = malloc(PASSWORD_SIZE); +// ... read and check password +memset(password, 0, PASSWORD_SIZE); +free(password); diff --git a/cpp/ql/src/Likely Bugs/Memory Management/MemsetMayBeDeleted-good.c b/cpp/ql/src/Likely Bugs/Memory Management/MemsetMayBeDeleted-good.c new file mode 100644 index 000000000000..3d6628bf0603 --- /dev/null +++ b/cpp/ql/src/Likely Bugs/Memory Management/MemsetMayBeDeleted-good.c @@ -0,0 +1,4 @@ +char * password = malloc(PASSWORD_SIZE); +// ... read and check password +memset_s(password, PASSWORD_SIZE, 0, PASSWORD_SIZE); +free(password); diff --git a/cpp/ql/src/Likely Bugs/Memory Management/MemsetMayBeDeleted.qhelp b/cpp/ql/src/Likely Bugs/Memory Management/MemsetMayBeDeleted.qhelp new file mode 100644 index 000000000000..0f32810e5cfc --- /dev/null +++ b/cpp/ql/src/Likely Bugs/Memory Management/MemsetMayBeDeleted.qhelp @@ -0,0 +1,52 @@ + + + +

Calling memset, bzero, FillMemory or +ZeroMemory on a buffer in order to clear its contents may get +optimized away by the compiler if the buffer is not subsequently used. +This is not desirable behavior if the buffer contains sensitive data that +could be somehow retrieved by an attacker.

+ +
+ +

The best workaround is to use alternate platform-supplied functions that + will not get optimized away: memset_s, SecureZeroMemory, + bzero_explicit, and the like (if they are available). + Otherwise, passing the -fno-builtin-memset flag to the + GCC/Clang compiler is usually effective. Finally, the public-domain + secure_memzero routine (see below) can be helpful.

+ +
+ +

The following program fragment uses memset to scrub memory after + it is no longer used:

+ +

Because of dead store elimination (DSE), the memset may be removed + by the compiler (since the data being scrubbed is not subsequently used), resulting + in potentially sensitive data remaining in memory.

+ +

The best solution to this problem is to use the memset_s function + instead of memset:

+ + +
+ + + +
  • MITRE: + + CWE-14.
  • +
  • Public domain: + + secure_memzero implementation.
  • +
  • Blog: + + Zero'ing memory, compiler optimizations and memset_s.
  • +
  • USENIX 2017 Paper: + + Dead Store Elimination (Still) Considered Harmful.
  • + +
    +
    diff --git a/cpp/ql/src/Likely Bugs/Memory Management/MemsetMayBeDeleted.ql b/cpp/ql/src/Likely Bugs/Memory Management/MemsetMayBeDeleted.ql new file mode 100644 index 000000000000..b08213c320ae --- /dev/null +++ b/cpp/ql/src/Likely Bugs/Memory Management/MemsetMayBeDeleted.ql @@ -0,0 +1,41 @@ +/** + * @name Call to `memset` may be deleted + * @description Calling `memset` or `bzero` on a buffer in order to clear its contents may get optimized away + * by the compiler if the buffer is not subsequently used. This is not desirable + * behavior if the buffer contains sensitive data that could be exploited by an attacker. + * @kind problem + * @problem.severity recommendation + * @precision high + * @id cpp/memset-may-be-deleted + * @tags security + * external/cwe/cwe-14 + */ + +import cpp +import semmle.code.cpp.ir.IR +import semmle.code.cpp.ir.ValueNumbering +import semmle.code.cpp.models.implementations.Memset + +class MemsetCallInstruction extends CallInstruction { + MemsetCallInstruction() { this.getStaticCallTarget() instanceof MemsetFunction } +} + +Instruction getAUseInstruction(Instruction insn) { result = insn.getAUse().getUse() } + +predicate pointsIntoStack(Instruction instr) { + instr.(VariableAddressInstruction).getIRVariable() instanceof IRAutomaticVariable + or + pointsIntoStack(instr.(CopyInstruction).getSourceValue()) + or + pointsIntoStack(instr.(ConvertInstruction).getUnary()) +} + +from MemsetCallInstruction memset, SizedBufferMustWriteSideEffectInstruction sei +where + sei.getPrimaryInstruction() = memset and + // The first argument to memset must reside on the stack + pointsIntoStack(valueNumber(memset.getPositionalArgument(0)).getAnInstruction()) and + // The result of memset may not be subsequently used + forall(Instruction use | use = getAUseInstruction+(sei) | use instanceof ChiInstruction) +select memset, + "Call to " + memset.getStaticCallTarget().getName() + " may be deleted by the compiler." diff --git a/cpp/ql/test/query-tests/Likely Bugs/Memory Management/MemsetMayBeDeleted/MemsetMayBeDeleted.c b/cpp/ql/test/query-tests/Likely Bugs/Memory Management/MemsetMayBeDeleted/MemsetMayBeDeleted.c new file mode 100644 index 000000000000..671b7d6589c2 --- /dev/null +++ b/cpp/ql/test/query-tests/Likely Bugs/Memory Management/MemsetMayBeDeleted/MemsetMayBeDeleted.c @@ -0,0 +1,140 @@ +typedef unsigned long long size_t; +void *memset(void *s, int c, unsigned long n); +void *__builtin_memset(void *s, int c, unsigned long n); +typedef int errno_t; +typedef unsigned int rsize_t; +errno_t memset_s(void *dest, rsize_t destsz, int ch, rsize_t count); +char *strcpy(char *dest, const char *src); + +extern void use_pw(char *pw); + +#define PW_SIZE 32 + +// x86-64 gcc 9.2: deleted +// x86-64 clang 9.0.0: deleted +// x64 msvc v19.14 (WINE): deleted +int func1(void) { + char pw1[PW_SIZE]; + use_pw(pw1); + memset(pw1, 0, PW_SIZE); // BAD + return 0; +} + +// x86-64 gcc 9.2: deleted +// x86-64 clang 9.0.0: deleted +// x64 msvc v19.14 (WINE): not deleted +int func1a(void) { + char pw1a[PW_SIZE]; + use_pw(pw1a); + __builtin_memset(pw1a, 0, PW_SIZE); // BAD + return 0; +} + +// x86-64 gcc 9.2: deleted +// x86-64 clang 9.0.0: deleted +// x64 msvc v19.14 (WINE): deleted +char *func1b(void) { + char pw1b[PW_SIZE]; + use_pw(pw1b); + memset(pw1b, 0, PW_SIZE); // BAD + pw1b[0] = pw1b[3] = 'a'; + return 0; +} + +// x86-64 gcc 9.2: not deleted +// x86-64 clang 9.0.0: not deleted +// x64 msvc v19.14 (WINE): not deleted +int func1c(char pw1c[PW_SIZE]) { + use_pw(pw1c); + memset(pw1c, 0, PW_SIZE); // GOOD + return 0; +} + +// x86-64 gcc 9.2: not deleted +// x86-64 clang 9.0.0: not deleted +// x64 msvc v19.14 (WINE): not deleted +char pw1d[PW_SIZE]; +int func1d() { + use_pw(pw1d); + memset(pw1d, 0, PW_SIZE); // GOOD + return 0; +} +// x86-64 gcc 9.2: not deleted +// x86-64 clang 9.0.0: not deleted +// x64 msvc v19.14 (WINE): not deleted +char *func2(void) { + char pw2[PW_SIZE]; + use_pw(pw2); + memset(pw2, 1, PW_SIZE); // BAD + return pw2; +} + +// x86-64 gcc 9.2: deleted +// x86-64 clang 9.0.0: deleted +// x64 msvc v19.14 (WINE): partially deleted +int func3(void) { + char pw3[PW_SIZE]; + use_pw(pw3); + memset(pw3, 4, PW_SIZE); // BAD [NOT DETECTED] + return pw3[2]; +} + +// x86-64 gcc 9.2: deleted +// x86-64 clang 9.0.0: deleted +// x64 msvc v19.14 (WINE): not deleted +int func4(void) { + char pw1a[PW_SIZE]; + use_pw(pw1a); + __builtin_memset(pw1a + 3, 0, PW_SIZE - 3); // BAD [NOT DETECTED] + return 0; +} + +// x86-64 gcc 9.2: deleted +// x86-64 clang 9.0.0: deleted +// x64 msvc v19.14 (WINE): not deleted +int func6(void) { + char pw1a[PW_SIZE]; + use_pw(pw1a); + __builtin_memset(&pw1a[3], 0, PW_SIZE - 3); // BAD [NOT DETECTED] + return pw1a[2]; +} + +// x86-64 gcc 9.2: deleted +// x86-64 clang 9.0.0: deleted +// x64 msvc v19.14 (WINE): not deleted +int func5(void) { + char pw1a[PW_SIZE]; + use_pw(pw1a); + __builtin_memset(pw1a + 3, 0, PW_SIZE - 4); // GOOD + return pw1a[4]; +} + +// x86-64 gcc 9.2: deleted +// x86-64 clang 9.0.0: deleted +// x64 msvc v19.14 (WINE): not deleted +int func7(void) { + char pw1a[PW_SIZE]; + use_pw(pw1a); + __builtin_memset(&pw1a[3], 0, PW_SIZE - 5); // BAD [NOT DETECTED] + return 0; +} + +// x86-64 gcc 9.2: not deleted +// x86-64 clang 9.0.0: not deleted +// x64 msvc v19.14 (WINE): not deleted +int func8(void) { + char pw1a[PW_SIZE]; + use_pw(pw1a); + __builtin_memset(pw1a + pw1a[3], 0, PW_SIZE - 4); // GOOD + return pw1a[4]; +} + +// x86-64 gcc 9.2: not deleted +// x86-64 clang 9.0.0: not deleted +// x64 msvc v19.14 (WINE): not deleted +char *func9(void) { + char pw1[PW_SIZE]; + use_pw(pw1); + memset(pw1, 0, PW_SIZE); // BAD + return 0; +} diff --git a/cpp/ql/test/query-tests/Likely Bugs/Memory Management/MemsetMayBeDeleted/MemsetMayBeDeleted.cpp b/cpp/ql/test/query-tests/Likely Bugs/Memory Management/MemsetMayBeDeleted/MemsetMayBeDeleted.cpp new file mode 100644 index 000000000000..ab2420b41019 --- /dev/null +++ b/cpp/ql/test/query-tests/Likely Bugs/Memory Management/MemsetMayBeDeleted/MemsetMayBeDeleted.cpp @@ -0,0 +1,184 @@ +extern "C" { + typedef unsigned long long size_t; + void *memset(void *s, int c, unsigned long n); + void *__builtin_memset(void *s, int c, unsigned long n); + typedef int errno_t; + typedef unsigned int rsize_t; + errno_t memset_s(void *dest, rsize_t destsz, int ch, rsize_t count); + char *strcpy(char *dest, const char *src); + void *memcpy(void *dest, const void *src, unsigned long n); + void *malloc(unsigned long size); + void free(void *ptr); + extern void use_pw(char *pw); +} + +#define PW_SIZE 32 + +struct mem { + int a; + char b[PW_SIZE]; + int c; +}; + +// x86-64 gcc 9.2: not deleted +// x86-64 clang 9.0.0: not deleted +// x64 msvc v19.22: not deleted +void func(char buff[128], unsigned long long sz) { + memset(buff, 0, PW_SIZE); // GOOD +} + +// x86-64 gcc 9.2: not deleted +// x86-64 clang 9.0.0: not deleted +// x64 msvc v19.22: not deleted +char *func2(char buff[128], unsigned long long sz) { + memset(buff, 0, PW_SIZE); // GOOD + return buff; +} + +// x86-64 gcc 9.2: deleted +// x86-64 clang 9.0.0: deleted +// x64 msvc v19.22: deleted +void func3(unsigned long long sz) { + char buff[128]; + memset(buff, 0, PW_SIZE); // BAD +} + +// x86-64 gcc 9.2: deleted +// x86-64 clang 9.0.0: deleted +// x64 msvc v19.22: deleted +void func4(unsigned long long sz) { + char buff[128]; + memset(buff, 0, PW_SIZE); // BAD [NOT DETECTED] + strcpy(buff, "Hello"); +} + +// x86-64 gcc 9.2: deleted +// x86-64 clang 9.0.0: deleted +// x64 msvc v19.22: deleted +void func5(unsigned long long sz) { + char buff[128]; + memset(buff, 0, PW_SIZE); // BAD [NOT DETECTED] + if (sz > 5) { + strcpy(buff, "Hello"); + } +} + +// x86-64 gcc 9.2: deleted +// x86-64 clang 9.0.0: deleted +// x64 msvc v19.22: deleted +void func6(unsigned long long sz) { + struct mem m; + memset(&m, 0, PW_SIZE); // BAD +} + +// x86-64 gcc 9.2: deleted +// x86-64 clang 9.0.0: deleted +// x64 msvc v19.22: deleted +void func7(unsigned long long sz) { + struct mem m; + memset(&m, 0, PW_SIZE); // BAD + m.a = 15; +} + +// x86-64 gcc 9.2: deleted +// x86-64 clang 9.0.0: deleted +// x64 msvc v19.22: not deleted +void func8(unsigned long long sz) { + struct mem *m = (struct mem *)malloc(sizeof(struct mem)); + memset(m, 0, PW_SIZE); // BAD [NOT DETECTED] +} + +// x86-64 gcc 9.2: deleted +// x86-64 clang 9.0.0: deleted +// x64 msvc v19.22: not deleted +void func9(unsigned long long sz) { + struct mem *m = (struct mem *)malloc(sizeof(struct mem)); + memset(m, 0, PW_SIZE); // BAD [NOT DETECTED] + free(m); +} + +// x86-64 gcc 9.2: deleted +// x86-64 clang 9.0.0: deleted +// x64 msvc v19.22: not deleted +void func10(unsigned long long sz) { + struct mem *m = (struct mem *)malloc(sizeof(struct mem)); + memset(m, 0, PW_SIZE); // BAD [NOT DETECTED] + m->a = sz; + m->c = m->a + 1; +} + +// x86-64 gcc 9.2: deleted +// x86-64 clang 9.0.0: deleted +// x64 msvc v19.22: not deleted +void func11(unsigned long long sz) { + struct mem *m = (struct mem *)malloc(sizeof(struct mem)); + ::memset(m, 0, PW_SIZE); // BAD [NOT DETECTED] + if (sz > 5) { + strcpy(m->b, "Hello"); + } +} + +// x86-64 gcc 9.2: not deleted +// x86-64 clang 9.0.0: not deleted +// x64 msvc v19.22: not deleted +int func12(unsigned long long sz) { + struct mem *m = (struct mem *)malloc(sizeof(struct mem)); + memset(m, 0, sz); // GOOD + return m->c; +} + +int funcN1() { + char pw[PW_SIZE]; + char *pw_ptr = pw; + memset(pw, 0, PW_SIZE); // GOOD + use_pw(pw_ptr); + return 0; +} + +char pw_global[PW_SIZE]; +int funcN2() { + use_pw(pw_global); + memset(pw_global, 0, PW_SIZE); // GOOD + return 0; +} + +int funcN3(unsigned long long sz) { + struct mem m; + memset(&m, 0, sizeof(m)); // GOOD + return m.a; +} + +void funcN(int num) { + char pw[PW_SIZE]; + int i; + + for (i = 0; i < num; i++) + { + use_pw(pw); + memset(pw, 0, PW_SIZE); // GOOD + } +} + +class MyClass +{ +public: + void set(int _x) { + x = _x; + } + + int get() + { + return x; + } + + void clear1() { + memset(&x, 0, sizeof(x)); // GOOD + } + + void clear2() { + memset(&(this->x), 0, sizeof(this->x)); // GOOD + } + +private: + int x; +}; diff --git a/cpp/ql/test/query-tests/Likely Bugs/Memory Management/MemsetMayBeDeleted/MemsetMayBeDeleted.expected b/cpp/ql/test/query-tests/Likely Bugs/Memory Management/MemsetMayBeDeleted/MemsetMayBeDeleted.expected new file mode 100644 index 000000000000..6ea51d59cfb0 --- /dev/null +++ b/cpp/ql/test/query-tests/Likely Bugs/Memory Management/MemsetMayBeDeleted/MemsetMayBeDeleted.expected @@ -0,0 +1,8 @@ +| MemsetMayBeDeleted.c:19:2:19:7 | Call: call to memset | Call to memset may be deleted by the compiler. | +| MemsetMayBeDeleted.c:29:2:29:17 | Call: call to __builtin_memset | Call to __builtin_memset may be deleted by the compiler. | +| MemsetMayBeDeleted.c:39:2:39:7 | Call: call to memset | Call to memset may be deleted by the compiler. | +| MemsetMayBeDeleted.c:68:2:68:7 | Call: call to memset | Call to memset may be deleted by the compiler. | +| MemsetMayBeDeleted.c:138:2:138:7 | Call: call to memset | Call to memset may be deleted by the compiler. | +| MemsetMayBeDeleted.cpp:43:5:43:10 | Call: call to memset | Call to memset may be deleted by the compiler. | +| MemsetMayBeDeleted.cpp:71:5:71:10 | Call: call to memset | Call to memset may be deleted by the compiler. | +| MemsetMayBeDeleted.cpp:79:5:79:10 | Call: call to memset | Call to memset may be deleted by the compiler. | diff --git a/cpp/ql/test/query-tests/Likely Bugs/Memory Management/MemsetMayBeDeleted/MemsetMayBeDeleted.qlref b/cpp/ql/test/query-tests/Likely Bugs/Memory Management/MemsetMayBeDeleted/MemsetMayBeDeleted.qlref new file mode 100644 index 000000000000..d0690449964f --- /dev/null +++ b/cpp/ql/test/query-tests/Likely Bugs/Memory Management/MemsetMayBeDeleted/MemsetMayBeDeleted.qlref @@ -0,0 +1 @@ +Likely Bugs/Memory Management/MemsetMayBeDeleted.ql