From 33092c9255b756f0140ab0f433af9137012b8eb9 Mon Sep 17 00:00:00 2001 From: Dave Bartolomeo Date: Fri, 25 Oct 2019 15:03:56 -0700 Subject: [PATCH] C++: Insecure memset --- .../cpp/models/implementations/Strcpy.qll | 23 ++- .../ir/memset/MemsetMayBeDeleted.c | 140 +++++++++++++ .../ir/memset/MemsetMayBeDeleted.cpp | 184 ++++++++++++++++++ .../ir/memset/MemsetMayBeDeleted.ql | 54 +++++ 4 files changed, 400 insertions(+), 1 deletion(-) create mode 100644 cpp/ql/test/library-tests/ir/memset/MemsetMayBeDeleted.c create mode 100644 cpp/ql/test/library-tests/ir/memset/MemsetMayBeDeleted.cpp create mode 100644 cpp/ql/test/library-tests/ir/memset/MemsetMayBeDeleted.ql diff --git a/cpp/ql/src/semmle/code/cpp/models/implementations/Strcpy.qll b/cpp/ql/src/semmle/code/cpp/models/implementations/Strcpy.qll index d309d32df54..90197d7c047 100644 --- a/cpp/ql/src/semmle/code/cpp/models/implementations/Strcpy.qll +++ b/cpp/ql/src/semmle/code/cpp/models/implementations/Strcpy.qll @@ -1,11 +1,14 @@ +import semmle.code.cpp.models.interfaces.Alias import semmle.code.cpp.models.interfaces.ArrayFunction import semmle.code.cpp.models.interfaces.DataFlow +import semmle.code.cpp.models.interfaces.SideEffect import semmle.code.cpp.models.interfaces.Taint /** * The standard function `strcpy` and its wide, sized, and Microsoft variants. */ -class StrcpyFunction extends ArrayFunction, DataFlowFunction, TaintFunction { +class StrcpyFunction extends ArrayFunction, DataFlowFunction, TaintFunction, AliasFunction, + SideEffectFunction { StrcpyFunction() { this.hasName("strcpy") or this.hasName("_mbscpy") or @@ -86,4 +89,22 @@ class StrcpyFunction extends ArrayFunction, DataFlowFunction, TaintFunction { output.isReturnValueDeref() ) } + + override predicate hasOnlySpecificReadSideEffects() { any() } + + override predicate hasOnlySpecificWriteSideEffects() { any() } + + override predicate hasSpecificReadSideEffect(ParameterIndex i, boolean buffer) { + i = 1 and buffer = true + } + + override predicate hasSpecificWriteSideEffect(ParameterIndex i, boolean buffer, boolean mustWrite) { + i = 0 and buffer = true and mustWrite = true + } + + override predicate parameterNeverEscapes(int index) { index = 1 } + + override predicate parameterEscapesOnlyViaReturn(int index) { index = 0 } + + override predicate parameterIsAlwaysReturned(int index) { index = 0 } } diff --git a/cpp/ql/test/library-tests/ir/memset/MemsetMayBeDeleted.c b/cpp/ql/test/library-tests/ir/memset/MemsetMayBeDeleted.c new file mode 100644 index 00000000000..671b7d6589c --- /dev/null +++ b/cpp/ql/test/library-tests/ir/memset/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/library-tests/ir/memset/MemsetMayBeDeleted.cpp b/cpp/ql/test/library-tests/ir/memset/MemsetMayBeDeleted.cpp new file mode 100644 index 00000000000..ab2420b4101 --- /dev/null +++ b/cpp/ql/test/library-tests/ir/memset/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/library-tests/ir/memset/MemsetMayBeDeleted.ql b/cpp/ql/test/library-tests/ir/memset/MemsetMayBeDeleted.ql new file mode 100644 index 00000000000..101c5a14307 --- /dev/null +++ b/cpp/ql/test/library-tests/ir/memset/MemsetMayBeDeleted.ql @@ -0,0 +1,54 @@ +/** + * @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 said 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 } +} + +private predicate isAlwaysLive(Instruction instr) { + instr instanceof ReturnValueInstruction or + instr instanceof ThrowValueInstruction or + instr instanceof AliasedUseInstruction +} + +private predicate isLive(Instruction instr) { + isAlwaysLive(instr) or + isLive(instr.getAUse().getUse()) or + isLive(instr.(SideEffectInstruction).getPrimaryInstruction()) +} + +predicate pointsIntoStack(Instruction instr) { + instr.(VariableAddressInstruction).getIRVariable() instanceof IRAutomaticVariable + or + pointsIntoStack(instr.(CopyInstruction).getSourceValue()) + or + pointsIntoStack(instr.(ConvertInstruction).getUnary()) + or + pointsIntoStack(instr.(PointerArithmeticInstruction).getLeft()) +} + +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) + not isLive(sei) +select memset, + "Call to " + memset.getStaticCallTarget().getName() + " may be deleted by the compiler."