From 4cee67da753bd001222aeefc1757428e9b8214f2 Mon Sep 17 00:00:00 2001 From: ihsinme <61293369+ihsinme@users.noreply.github.com> Date: Wed, 13 Jan 2021 14:17:21 +0300 Subject: [PATCH 01/18] Add files via upload --- .../CompilerRemovalOfCodeToClearBuffers.c | 35 +++++ .../CompilerRemovalOfCodeToClearBuffers.qhelp | 31 +++++ .../CompilerRemovalOfCodeToClearBuffers.ql | 121 ++++++++++++++++++ 3 files changed, 187 insertions(+) create mode 100644 cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.c create mode 100644 cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.qhelp create mode 100644 cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.ql diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.c b/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.c new file mode 100644 index 000000000000..ab1bae3ae219 --- /dev/null +++ b/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.c @@ -0,0 +1,35 @@ +// BAD: the memset call will probably be optimized. +void getPassword(void) { + char pwd[64]; + if (GetPassword(pwd, sizeof(pwd))) { + /* Checking of password, secure operations, etc. */ + } + memset(pwd, 0, sizeof(pwd)); +} +// GOOD: in this case the memset will not be optimized. +void getPassword(void) { + char pwd[64]; + + if (retrievePassword(pwd, sizeof(pwd))) { + /* Checking of password, secure operations, etc. */ + } + memset_s(pwd, 0, sizeof(pwd)); +} +// GOOD: in this case the memset will not be optimized. +void getPassword(void) { + char pwd[64]; + if (retrievePassword(pwd, sizeof(pwd))) { + /* Checking of password, secure operations, etc. */ + } + SecureZeroMemory(pwd, sizeof(pwd)); +} +// GOOD: in this case the memset will not be optimized. +void getPassword(void) { + char pwd[64]; + if (retrievePassword(pwd, sizeof(pwd))) { + /* Checking of password, secure operations, etc. */ + } +#pragma optimize("", off) + memset(pwd, 0, sizeof(pwd)); +#pragma optimize("", on) +} diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.qhelp b/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.qhelp new file mode 100644 index 000000000000..89b4189a1008 --- /dev/null +++ b/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.qhelp @@ -0,0 +1,31 @@ + + + +

Compiler optimization will exclude the cleaning of private information. +Using the memset function to clear private data as a final expression when working with a variable is potentially dangerous, since the compiler can optimize this call. +For some compilers, optimization is also possible when using calls to free memory after the memset function.

+ +

It is possible to miss detection of vulnerabilities if used to clear fields of structures or parts of a buffer.

+ +
+ + +

We recommend to use the RtlSecureZeroMemory or memset_s functions, or compilation flags that exclude optimization of memset calls (-fno-builtin-memset).

+ +
+ +

The following example demonstrates an erroneous and corrected use of the memset function.

+ + +
+ + +
  • + CERT C Coding Standard: + MSC06-C. Beware of compiler optimizations. +
  • + +
    +
    diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.ql b/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.ql new file mode 100644 index 000000000000..fd53478cce6a --- /dev/null +++ b/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.ql @@ -0,0 +1,121 @@ +/** + * @name Compiler Removal Of Code To Clear Buffers + * @description --Using the memset function to clear private data as a final expression when working with a variable is potentially dangerous because the compiler can optimize this call. + * --For some compilers, optimization is also possible when using calls to free memory after the memset function. + * --To clear it, you need to use the RtlSecureZeroMemory or memset_s functions, or compilation flags that exclude optimization of memset calls (-fno-builtin-memset). + * @kind problem + * @id cpp/compiler-removal-of-code-to-clear-buffers + * @problem.severity warning + * @precision medium + * @tags security + * external/cwe/cwe-14 + */ + +import cpp +import semmle.code.cpp.dataflow.DataFlow + +/** + * A call to `memset` , for some local variable. + */ +class CompilerRemovaMemset extends FunctionCall { + CompilerRemovaMemset() { + this.getTarget().hasName("memset") and + exists(DataFlow::Node source, DataFlow::Node sink, LocalVariable isv, Expr exp | + DataFlow::localFlow(source, sink) and + this.getArgument(0) = isv.getAnAccess() and + source.asExpr() = exp and + exp.getLocation().getEndLine() < this.getArgument(0).getLocation().getStartLine() and + sink.asExpr() = this.getArgument(0) + ) + } + + predicate isExistsAllocForThisVariable() { + exists(FunctionCall alloc, Variable v | + alloc = v.getAnAssignedValue() and + this.getArgument(0) = v.getAnAccess() and + alloc.getASuccessor+() = this + ) + } + + predicate isExistsFreeForThisVariable() { + exists(FunctionCall free, Variable v | + free instanceof DeallocationExpr and + this.getArgument(0) = v.getAnAccess() and + free.getArgument(0) = v.getAnAccess() and + this.getASuccessor+() = free + ) + } + + predicate isExistsCallWithThisVariableExcludingDeallocationCalls() { + exists(FunctionCall fc, Variable v | + not fc instanceof DeallocationExpr and + this.getArgument(0) = v.getAnAccess() and + fc.getAnArgument() = v.getAnAccess() and + this.getASuccessor+() = fc + ) + } + + predicate isVariableUseAfterMemsetExcludingCalls() { + exists(DataFlow::Node source, DataFlow::Node sink, LocalVariable isv, Expr exp | + DataFlow::localFlow(source, sink) and + this.getArgument(0) = isv.getAnAccess() and + source.asExpr() = isv.getAnAccess() and + exp.getLocation().getStartLine() > this.getArgument(2).getLocation().getEndLine() and + not exp.getParent() instanceof FunctionCall and + sink.asExpr() = exp + ) + } + + predicate isVariableUseBoundWithArgumentFunction() { + exists(DataFlow::Node source, DataFlow::Node sink, LocalVariable isv, Parameter p, Expr exp | + DataFlow::localFlow(source, sink) and + this.getArgument(0) = isv.getAnAccess() and + this.getEnclosingFunction().getAParameter() = p and + exp.getAChild*() = p.getAnAccess() and + source.asExpr() = exp and + sink.asExpr() = isv.getAnAccess() + ) + } + + predicate isVariableUseBoundWithGlobalVariable() { + exists( + DataFlow::Node source, DataFlow::Node sink, LocalVariable isv, GlobalVariable gv, Expr exp + | + DataFlow::localFlow(source, sink) and + this.getArgument(0) = isv.getAnAccess() and + exp.getAChild*() = gv.getAnAccess() and + source.asExpr() = exp and + sink.asExpr() = isv.getAnAccess() + ) + } + + predicate isExistsCompilationFlagsBlockingRemoval() { + exists(Compilation c | + c.getAFileCompiled() = this.getFile() and + c.getAnArgument() = "-fno-builtin-memset" + ) + } + + predicate isUseVCCompilation() { + exists(Compilation c | + c.getAFileCompiled() = this.getFile() and + ( + c.getArgument(2).toString().matches("%gcc%") or + c.getArgument(2).toString().matches("%g++%") or + c.getArgument(2).toString().matches("%clang%") or + c.getArgument(2).toString() = "--force-recompute" + ) + ) + } +} + +from CompilerRemovaMemset fc +where + not (fc.isExistsAllocForThisVariable() and not fc.isExistsFreeForThisVariable()) and + not (fc.isExistsFreeForThisVariable() and not fc.isUseVCCompilation()) and + not fc.isVariableUseAfterMemsetExcludingCalls() and + not fc.isExistsCallWithThisVariableExcludingDeallocationCalls() and + not fc.isVariableUseBoundWithArgumentFunction() and + not fc.isVariableUseBoundWithGlobalVariable() and + not fc.isExistsCompilationFlagsBlockingRemoval() +select fc.getArgument(0), "this variable will not be cleared" From 3ad45f28c90ceb429d53e7245ab7627f36b677d8 Mon Sep 17 00:00:00 2001 From: ihsinme <61293369+ihsinme@users.noreply.github.com> Date: Wed, 13 Jan 2021 14:18:54 +0300 Subject: [PATCH 02/18] Add files via upload --- ...mpilerRemovalOfCodeToClearBuffers.expected | 3 + .../CompilerRemovalOfCodeToClearBuffers.qlref | 1 + .../Security/CWE/CWE-14/semmle/tests/test.c | 201 ++++++++++++++++++ 3 files changed, 205 insertions(+) create mode 100644 cpp/ql/test/experimental/query-tests/Security/CWE/CWE-14/semmle/tests/CompilerRemovalOfCodeToClearBuffers.expected create mode 100644 cpp/ql/test/experimental/query-tests/Security/CWE/CWE-14/semmle/tests/CompilerRemovalOfCodeToClearBuffers.qlref create mode 100644 cpp/ql/test/experimental/query-tests/Security/CWE/CWE-14/semmle/tests/test.c diff --git a/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-14/semmle/tests/CompilerRemovalOfCodeToClearBuffers.expected b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-14/semmle/tests/CompilerRemovalOfCodeToClearBuffers.expected new file mode 100644 index 000000000000..fbb5a3f15526 --- /dev/null +++ b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-14/semmle/tests/CompilerRemovalOfCodeToClearBuffers.expected @@ -0,0 +1,3 @@ +| test.c:13:9:13:13 | buff1 | this variable will not be cleared | +| test.c:35:9:35:13 | buff1 | this variable will not be cleared | +| test.c:43:9:43:13 | buff1 | this variable will not be cleared | diff --git a/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-14/semmle/tests/CompilerRemovalOfCodeToClearBuffers.qlref b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-14/semmle/tests/CompilerRemovalOfCodeToClearBuffers.qlref new file mode 100644 index 000000000000..61d2a29b1264 --- /dev/null +++ b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-14/semmle/tests/CompilerRemovalOfCodeToClearBuffers.qlref @@ -0,0 +1 @@ +experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.ql diff --git a/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-14/semmle/tests/test.c b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-14/semmle/tests/test.c new file mode 100644 index 000000000000..221072330c3f --- /dev/null +++ b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-14/semmle/tests/test.c @@ -0,0 +1,201 @@ +struct buffers +{ + unsigned char buff1[50]; + unsigned char *buff2; +} globalBuff1,*globalBuff2; + +unsigned char * globalBuff; +void badFunc0_0(){ + unsigned char buff1[12]; + int i; + for(i=0;i<12;i++) + buff1[i]=13; + memset(buff1,12,12); +} +void nobadFunc0_0(){ + unsigned char buff1[12]; + memset(buff1,12,12); +} +void nobadFunc0_1(){ + unsigned char buff1[12]; + int i; + memset(buff1,12,12); + for(i=0;i<12;i++) + buff1[i]=13; + free(buff1); +} +void nobadFunc1_0(){ + unsigned char * buff1; + buff1 = (unsigned char *) malloc(12); + memset(buff1,12,12); +} +void badFunc1_0(){ + unsigned char * buff1; + buff1 = (unsigned char *) malloc(12); + memset(buff1,12,12); + free(buff1); +} +void badFunc1_1(){ + unsigned char buff1[12]; + int i; + for(i=0;i<12;i++) + buff1[i]=13; + memset(buff1,12,12); + free(buff1); +} +void nobadFunc2_0_0(){ + unsigned char buff1[12]; + int i; + for(i=0;i<12;i++) + buff1[i]=13; + memset(buff1,12,12); + printf(buff1); +} + +void nobadFunc2_0_1(){ + unsigned char buff1[12]; + int i; + for(i=0;i<12;i++) + buff1[i]=13; + memset(buff1,12,12); + printf(buff1+3); +} + +void nobadFunc2_0_2(){ + unsigned char buff1[12]; + int i; + for(i=0;i<12;i++) + buff1[i]=13; + memset(buff1,12,12); + printf(*buff1); +} + +void nobadFunc2_0_3(){ + unsigned char buff1[12]; + int i; + for(i=0;i<12;i++) + buff1[i]=13; + memset(buff1,12,12); + printf(*(buff1+3)); +} +unsigned char * nobadFunc2_0_4(){ + unsigned char buff1[12]; + int i; + for(i=0;i<12;i++) + buff1[i]=13; + memset(buff1,12,12); + return buff1; +} + +unsigned char * nobadFunc2_0_5(){ + unsigned char buff1[12]; + int i; + for(i=0;i<12;i++) + buff1[i]=13; + memset(buff1,12,12); + return buff1+3; +} +unsigned char nobadFunc2_0_6(){ + unsigned char buff1[12]; + int i; + for(i=0;i<12;i++) + buff1[i]=13; + memset(buff1,12,12); + return *buff1; +} + +unsigned char nobadFunc2_0_7(){ + unsigned char buff1[12]; + int i; + for(i=0;i<12;i++) + buff1[i]=13; + memset(buff1,12,12); + return *(buff1+3); +} +void nobadFunc2_1_0(){ + unsigned char buff1[12]; + int i; + for(i=0;i<12;i++) + buff1[i]=13; + memset(buff1,12,12); + if(*buff1==0) + printf("123123"); +} +void nobadFunc2_1_1(){ + unsigned char buff1[12]; + int i; + for(i=0;i<12;i++) + buff1[i]=13; + memset(buff1,12,12); + if(*(buff1+3)==0) + printf("123123"); +} +void nobadFunc2_1_2(){ + unsigned char buff1[12]; + int i; + for(i=0;i<12;i++) + buff1[i]=13; + memset(buff1,12,12); + buff1[2]=5; +} +void nobadFunc3_0(unsigned char * buffAll){ + unsigned char * buff1 = buffAll; + memset(buff1,12,12); +} +void nobadFunc3_1(unsigned char * buffAll){ + unsigned char * buff1 = buffAll+3; + memset(buff1,12,12); +} +void nobadFunc3_2(struct buffers buffAll){ + unsigned char * buff1 = buffAll.buff1; + memset(buff1,12,12); +} +void nobadFunc3_3(struct buffers buffAll){ + unsigned char * buff1 = buffAll.buff2; + memset(buff1,12,12); +} +void nobadFunc3_4(struct buffers buffAll){ + unsigned char * buff1 = buffAll.buff2+3; + memset(buff1,12,12); +} +void nobadFunc3_5(struct buffers * buffAll){ + unsigned char * buff1 = buffAll->buff1; + memset(buff1,12,12); +} +void nobadFunc3_6(struct buffers *buffAll){ + unsigned char * buff1 = buffAll->buff2; + memset(buff1,12,12); +} +void nobadFunc4(){ + unsigned char * buff1 = globalBuff; + memset(buff1,12,12); +} +void nobadFunc4_0(){ + unsigned char * buff1 = globalBuff; + memset(buff1,12,12); +} +void nobadFunc4_1(){ + unsigned char * buff1 = globalBuff+3; + memset(buff1,12,12); +} +void nobadFunc4_2(){ + unsigned char * buff1 = globalBuff1.buff1; + memset(buff1,12,12); +} +void nobadFunc4_3(){ + unsigned char * buff1 = globalBuff1.buff2; + memset(buff1,12,12); +} +void nobadFunc4_4(){ + unsigned char * buff1 = globalBuff1.buff2+3; + memset(buff1,12,12); +} +void nobadFunc4_5(){ + unsigned char * buff1 = globalBuff2->buff1; + memset(buff1,12,12); +} +void nobadFunc4_6(){ + unsigned char * buff1 = globalBuff2->buff2; + memset(buff1,12,12); +} + From 1c4610c722952ad005beab3ae292f5717be5b724 Mon Sep 17 00:00:00 2001 From: ihsinme <61293369+ihsinme@users.noreply.github.com> Date: Thu, 14 Jan 2021 17:15:36 +0300 Subject: [PATCH 03/18] Update cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.ql Co-authored-by: Mathias Vorreiter Pedersen --- .../CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.ql | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.ql b/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.ql index fd53478cce6a..6fb8ea383167 100644 --- a/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.ql +++ b/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.ql @@ -38,10 +38,9 @@ class CompilerRemovaMemset extends FunctionCall { } predicate isExistsFreeForThisVariable() { - exists(FunctionCall free, Variable v | - free instanceof DeallocationExpr and + exists(DeallocationExpr free, Variable v | this.getArgument(0) = v.getAnAccess() and - free.getArgument(0) = v.getAnAccess() and + free.getFreedExpr() = v.getAnAccess() and this.getASuccessor+() = free ) } From b26a90e1e66e67f939dc4e4159c142fc744b1d2d Mon Sep 17 00:00:00 2001 From: ihsinme <61293369+ihsinme@users.noreply.github.com> Date: Thu, 14 Jan 2021 17:15:58 +0300 Subject: [PATCH 04/18] Update cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.ql Co-authored-by: Mathias Vorreiter Pedersen --- .../CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.ql | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.ql b/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.ql index 6fb8ea383167..56b1719c96e9 100644 --- a/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.ql +++ b/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.ql @@ -1,8 +1,7 @@ /** * @name Compiler Removal Of Code To Clear Buffers - * @description --Using the memset function to clear private data as a final expression when working with a variable is potentially dangerous because the compiler can optimize this call. - * --For some compilers, optimization is also possible when using calls to free memory after the memset function. - * --To clear it, you need to use the RtlSecureZeroMemory or memset_s functions, or compilation flags that exclude optimization of memset calls (-fno-builtin-memset). + * @description Using memset the function to clear private data in a variable that has no subsequent use + * is potentially dangerous because the compiler can remove the call. * @kind problem * @id cpp/compiler-removal-of-code-to-clear-buffers * @problem.severity warning From 9e3b288f3374c6da4e8e2fb81ed40ee155642692 Mon Sep 17 00:00:00 2001 From: ihsinme <61293369+ihsinme@users.noreply.github.com> Date: Thu, 14 Jan 2021 17:16:21 +0300 Subject: [PATCH 05/18] Update cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.c Co-authored-by: Mathias Vorreiter Pedersen --- .../Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.c b/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.c index ab1bae3ae219..0c71d6e5dab2 100644 --- a/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.c +++ b/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.c @@ -1,4 +1,4 @@ -// BAD: the memset call will probably be optimized. +// BAD: the memset call will probably be removed. void getPassword(void) { char pwd[64]; if (GetPassword(pwd, sizeof(pwd))) { From 4631658e5e4f2bdd26e61e891a11140722bf2a42 Mon Sep 17 00:00:00 2001 From: ihsinme <61293369+ihsinme@users.noreply.github.com> Date: Thu, 14 Jan 2021 17:16:37 +0300 Subject: [PATCH 06/18] Update cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.c Co-authored-by: Mathias Vorreiter Pedersen --- .../Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.c b/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.c index 0c71d6e5dab2..ce9be522a3c2 100644 --- a/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.c +++ b/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.c @@ -6,7 +6,7 @@ void getPassword(void) { } memset(pwd, 0, sizeof(pwd)); } -// GOOD: in this case the memset will not be optimized. +// GOOD: in this case the memset will not be removed. void getPassword(void) { char pwd[64]; From 76b768f7e0f8e31937665cd92ca21e4eb7af906e Mon Sep 17 00:00:00 2001 From: ihsinme <61293369+ihsinme@users.noreply.github.com> Date: Thu, 14 Jan 2021 17:16:53 +0300 Subject: [PATCH 07/18] Update cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.c Co-authored-by: Mathias Vorreiter Pedersen --- .../Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.c b/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.c index ce9be522a3c2..3e92f5e4fa2a 100644 --- a/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.c +++ b/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.c @@ -15,7 +15,7 @@ void getPassword(void) { } memset_s(pwd, 0, sizeof(pwd)); } -// GOOD: in this case the memset will not be optimized. +// GOOD: in this case the memset will not be removed. void getPassword(void) { char pwd[64]; if (retrievePassword(pwd, sizeof(pwd))) { From 4ba4de3d41eecff5e8d30e9cfd65064bc585869e Mon Sep 17 00:00:00 2001 From: ihsinme <61293369+ihsinme@users.noreply.github.com> Date: Thu, 14 Jan 2021 17:17:08 +0300 Subject: [PATCH 08/18] Update cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.c Co-authored-by: Mathias Vorreiter Pedersen --- .../Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.c b/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.c index 3e92f5e4fa2a..14dd07a573ee 100644 --- a/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.c +++ b/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.c @@ -23,7 +23,7 @@ void getPassword(void) { } SecureZeroMemory(pwd, sizeof(pwd)); } -// GOOD: in this case the memset will not be optimized. +// GOOD: in this case the memset will not be removed. void getPassword(void) { char pwd[64]; if (retrievePassword(pwd, sizeof(pwd))) { From 0d0ea0c5e15140d97f170b38f227e549487ea7f9 Mon Sep 17 00:00:00 2001 From: ihsinme <61293369+ihsinme@users.noreply.github.com> Date: Thu, 14 Jan 2021 17:17:56 +0300 Subject: [PATCH 09/18] Update cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.ql Co-authored-by: Mathias Vorreiter Pedersen --- .../Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.ql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.ql b/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.ql index 56b1719c96e9..892350cecc2b 100644 --- a/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.ql +++ b/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.ql @@ -14,7 +14,7 @@ import cpp import semmle.code.cpp.dataflow.DataFlow /** - * A call to `memset` , for some local variable. + * A call to `memset` of the form `memset(ptr, value, num)`, for some local variable `ptr`. */ class CompilerRemovaMemset extends FunctionCall { CompilerRemovaMemset() { From 3e715ff52d0e70d87504d9d6fb6a633e61744f08 Mon Sep 17 00:00:00 2001 From: ihsinme <61293369+ihsinme@users.noreply.github.com> Date: Thu, 14 Jan 2021 17:19:23 +0300 Subject: [PATCH 10/18] Update cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.qhelp Co-authored-by: Mathias Vorreiter Pedersen --- .../CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.qhelp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.qhelp b/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.qhelp index 89b4189a1008..d7d670ac9bfd 100644 --- a/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.qhelp +++ b/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.qhelp @@ -4,7 +4,7 @@

    Compiler optimization will exclude the cleaning of private information. -Using the memset function to clear private data as a final expression when working with a variable is potentially dangerous, since the compiler can optimize this call. +Using the memset function to clear private data in a variable that has no subsequent use is potentially dangerous, since the compiler can remove the call. For some compilers, optimization is also possible when using calls to free memory after the memset function.

    It is possible to miss detection of vulnerabilities if used to clear fields of structures or parts of a buffer.

    From 7f5e5fcb99b309c296b3d3140b5b9b3cd0eb7490 Mon Sep 17 00:00:00 2001 From: ihsinme <61293369+ihsinme@users.noreply.github.com> Date: Thu, 14 Jan 2021 17:19:57 +0300 Subject: [PATCH 11/18] Update cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.qhelp Co-authored-by: Mathias Vorreiter Pedersen --- .../CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.qhelp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.qhelp b/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.qhelp index d7d670ac9bfd..a739c12ef8ba 100644 --- a/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.qhelp +++ b/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.qhelp @@ -12,7 +12,7 @@ For some compilers, optimization is also possible when using calls to free memor
    -

    We recommend to use the RtlSecureZeroMemory or memset_s functions, or compilation flags that exclude optimization of memset calls (-fno-builtin-memset).

    +

    We recommend to use the RtlSecureZeroMemory or memset_s functions, or compilation flags that exclude optimization of memset calls (e.g. -fno-builtin-memset).

    From cd0d2a569259a13290ecd5cb2de873470bff4cc3 Mon Sep 17 00:00:00 2001 From: ihsinme <61293369+ihsinme@users.noreply.github.com> Date: Thu, 14 Jan 2021 17:21:19 +0300 Subject: [PATCH 12/18] Update cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.ql Co-authored-by: Mathias Vorreiter Pedersen --- .../CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.ql | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.ql b/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.ql index 892350cecc2b..ca0240ad218c 100644 --- a/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.ql +++ b/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.ql @@ -98,10 +98,10 @@ class CompilerRemovaMemset extends FunctionCall { exists(Compilation c | c.getAFileCompiled() = this.getFile() and ( - c.getArgument(2).toString().matches("%gcc%") or - c.getArgument(2).toString().matches("%g++%") or - c.getArgument(2).toString().matches("%clang%") or - c.getArgument(2).toString() = "--force-recompute" + c.getArgument(2).matches("%gcc%") or + c.getArgument(2).matches("%g++%") or + c.getArgument(2).matches("%clang%") or + c.getArgument(2) = "--force-recompute" ) ) } From 10ab1d9b549b01e65b6a00535952009280d8b96e Mon Sep 17 00:00:00 2001 From: ihsinme <61293369+ihsinme@users.noreply.github.com> Date: Thu, 14 Jan 2021 17:24:49 +0300 Subject: [PATCH 13/18] Update CompilerRemovalOfCodeToClearBuffers.ql --- .../Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.ql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.ql b/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.ql index ca0240ad218c..3f6e47ee891f 100644 --- a/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.ql +++ b/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.ql @@ -29,7 +29,7 @@ class CompilerRemovaMemset extends FunctionCall { } predicate isExistsAllocForThisVariable() { - exists(FunctionCall alloc, Variable v | + exists(AllocationExpr alloc, Variable v | alloc = v.getAnAssignedValue() and this.getArgument(0) = v.getAnAccess() and alloc.getASuccessor+() = this From 805352945ede85170b1dc14fcd72bf45c41196e3 Mon Sep 17 00:00:00 2001 From: ihsinme <61293369+ihsinme@users.noreply.github.com> Date: Thu, 14 Jan 2021 17:27:58 +0300 Subject: [PATCH 14/18] Update CompilerRemovalOfCodeToClearBuffers.ql --- .../Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.ql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.ql b/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.ql index 3f6e47ee891f..96ecc30a95ef 100644 --- a/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.ql +++ b/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.ql @@ -18,7 +18,7 @@ import semmle.code.cpp.dataflow.DataFlow */ class CompilerRemovaMemset extends FunctionCall { CompilerRemovaMemset() { - this.getTarget().hasName("memset") and + this.getTarget().hasGlobalOrStdName("memset") and exists(DataFlow::Node source, DataFlow::Node sink, LocalVariable isv, Expr exp | DataFlow::localFlow(source, sink) and this.getArgument(0) = isv.getAnAccess() and From dcbae8b22bf77d68139e0897e561fb69daacec71 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Fri, 15 Jan 2021 19:47:09 +0100 Subject: [PATCH 15/18] Fix code tag. --- .../CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.qhelp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.qhelp b/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.qhelp index a739c12ef8ba..df0ed151d8f3 100644 --- a/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.qhelp +++ b/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.qhelp @@ -5,7 +5,7 @@

    Compiler optimization will exclude the cleaning of private information. Using the memset function to clear private data in a variable that has no subsequent use is potentially dangerous, since the compiler can remove the call. -For some compilers, optimization is also possible when using calls to free memory after the memset function.

    +For some compilers, optimization is also possible when using calls to free memory after the memset function.

    It is possible to miss detection of vulnerabilities if used to clear fields of structures or parts of a buffer.

    From 4c9de4574a9169f51428dbc02b280b216cbdb5c7 Mon Sep 17 00:00:00 2001 From: ihsinme <61293369+ihsinme@users.noreply.github.com> Date: Wed, 20 Jan 2021 16:24:43 +0300 Subject: [PATCH 16/18] Update CompilerRemovalOfCodeToClearBuffers.ql --- .../Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.ql | 3 +++ 1 file changed, 3 insertions(+) diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.ql b/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.ql index 96ecc30a95ef..b0c412df4cca 100644 --- a/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.ql +++ b/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.ql @@ -12,6 +12,7 @@ import cpp import semmle.code.cpp.dataflow.DataFlow +import semmle.code.cpp.dataflow.StackAddress /** * A call to `memset` of the form `memset(ptr, value, num)`, for some local variable `ptr`. @@ -34,6 +35,8 @@ class CompilerRemovaMemset extends FunctionCall { this.getArgument(0) = v.getAnAccess() and alloc.getASuccessor+() = this ) + or + not stackPointerFlowsToUse(this.getArgument(0), _, _, _) } predicate isExistsFreeForThisVariable() { From 9c53e39394939674b49750af8ce28c2c7d680ccb Mon Sep 17 00:00:00 2001 From: ihsinme <61293369+ihsinme@users.noreply.github.com> Date: Thu, 21 Jan 2021 16:52:00 +0300 Subject: [PATCH 17/18] Update CompilerRemovalOfCodeToClearBuffers.ql --- .../CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.ql | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.ql b/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.ql index b0c412df4cca..435d2f8f96aa 100644 --- a/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.ql +++ b/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.ql @@ -23,7 +23,12 @@ class CompilerRemovaMemset extends FunctionCall { exists(DataFlow::Node source, DataFlow::Node sink, LocalVariable isv, Expr exp | DataFlow::localFlow(source, sink) and this.getArgument(0) = isv.getAnAccess() and - source.asExpr() = exp and + ( + source.asExpr() = exp + or + // handle the case where exp is defined by an address being passed into some function. + source.asDefiningArgument() = exp + ) and exp.getLocation().getEndLine() < this.getArgument(0).getLocation().getStartLine() and sink.asExpr() = this.getArgument(0) ) From 416aa49d99c55bf141509d9ff275a10f70dffb87 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Tue, 26 Jan 2021 17:24:03 +0100 Subject: [PATCH 18/18] C++: Capitalize alert message. --- .../CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.ql | 2 +- .../tests/CompilerRemovalOfCodeToClearBuffers.expected | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.ql b/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.ql index 435d2f8f96aa..db09de9430df 100644 --- a/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.ql +++ b/cpp/ql/src/experimental/Security/CWE/CWE-14/CompilerRemovalOfCodeToClearBuffers.ql @@ -124,4 +124,4 @@ where not fc.isVariableUseBoundWithArgumentFunction() and not fc.isVariableUseBoundWithGlobalVariable() and not fc.isExistsCompilationFlagsBlockingRemoval() -select fc.getArgument(0), "this variable will not be cleared" +select fc.getArgument(0), "This variable will not be cleared." diff --git a/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-14/semmle/tests/CompilerRemovalOfCodeToClearBuffers.expected b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-14/semmle/tests/CompilerRemovalOfCodeToClearBuffers.expected index fbb5a3f15526..8d98d94ff467 100644 --- a/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-14/semmle/tests/CompilerRemovalOfCodeToClearBuffers.expected +++ b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-14/semmle/tests/CompilerRemovalOfCodeToClearBuffers.expected @@ -1,3 +1,3 @@ -| test.c:13:9:13:13 | buff1 | this variable will not be cleared | -| test.c:35:9:35:13 | buff1 | this variable will not be cleared | -| test.c:43:9:43:13 | buff1 | this variable will not be cleared | +| test.c:13:9:13:13 | buff1 | This variable will not be cleared. | +| test.c:35:9:35:13 | buff1 | This variable will not be cleared. | +| test.c:43:9:43:13 | buff1 | This variable will not be cleared. |