Merge branch 'main' into inline-taint-tests

Author: RasmusWL

.github/workflows/close-stale.yml

@@ -15,16 +15,16 @@ jobs:
     - uses: actions/stale@v3
       with:
         repo-token: ${{ secrets.GITHUB_TOKEN }}
-        stale-issue-message: 'This issue is stale because it has been open 14 days with no activity. Comment or remove the `stale` label in order to avoid having this issue closed in 7 days.'
+        stale-issue-message: 'This issue is stale because it has been open 14 days with no activity. Comment or remove the `Stale` label in order to avoid having this issue closed in 7 days.'
         close-issue-message: 'This issue was closed because it has been inactive for 7 days.'
         days-before-stale: 14
         days-before-close: 7
-        only-labels: question
-        
+        only-labels: awaiting-response
+
         # do not mark PRs as stale
         days-before-pr-stale: -1
         days-before-pr-close: -1
-        
+
         # Uncomment for dry-run
         # debug-only: true
         # operations-per-run: 1000

cpp/change-notes/2021-04-13-arithmetic-queries.md

@@ -0,0 +1,2 @@
+lgtm
+* The queries cpp/tainted-arithmetic, cpp/uncontrolled-arithmetic, and cpp/arithmetic-with-extreme-values have been improved to produce fewer false positives.

cpp/ql/src/Likely Bugs/Memory Management/ReturnStackAllocatedMemory.ql

@@ -13,6 +13,7 @@
 
 import cpp
 import semmle.code.cpp.dataflow.EscapesTree
+import semmle.code.cpp.models.interfaces.PointerWrapper
 import semmle.code.cpp.dataflow.DataFlow
 
 /**
@@ -39,6 +40,10 @@ predicate hasNontrivialConversion(Expr e) {
     e instanceof ParenthesisExpr
   )
   or
+  // A smart pointer can be stack-allocated while the data it points to is heap-allocated.
+  // So we exclude such "conversions" from this predicate.
+  e = any(PointerWrapper wrapper).getAnUnwrapperFunction().getACallToThisFunction()
+  or
   hasNontrivialConversion(e.getConversion())
 }
 

cpp/ql/src/experimental/Security/CWE/CWE-788/AccessOfMemoryLocationAfterEndOfBufferUsingStrncat.ql

@@ -11,54 +11,32 @@
  */
 
 import cpp
+import semmle.code.cpp.models.implementations.Strcat
 import semmle.code.cpp.valuenumbering.GlobalValueNumbering
 
 /**
- * A call to `strncat` of the form `strncat(buff, str, someExpr - strlen(buf))`, for some expression `someExpr` equal to `sizeof(buff)`.
+ * Holds if `call` is a call to `strncat` such that `sizeArg` and `destArg` are the size and
+ * destination arguments, respectively.
  */
-class WrongCallStrncat extends FunctionCall {
-  Expr leftsomeExpr;
-
-  WrongCallStrncat() {
-    this.getTarget().hasGlobalOrStdName("strncat") and
-    // the expression of the first argument in `strncat` and `strnlen` is identical
-    globalValueNumber(this.getArgument(0)) =
-      globalValueNumber(this.getArgument(2).(SubExpr).getRightOperand().(StrlenCall).getStringExpr()) and
-    // using a string constant often speaks of manually calculating the length of the required buffer.
-    (
-      not this.getArgument(1) instanceof StringLiteral and
-      not this.getArgument(1) instanceof CharLiteral
-    ) and
-    // for use in predicates
-    leftsomeExpr = this.getArgument(2).(SubExpr).getLeftOperand()
-  }
-
-  /**
-   * Holds if the left side of the expression `someExpr` equal to `sizeof(buf)`.
-   */
-  predicate isExpressionEqualSizeof() {
-    // the left side of the expression `someExpr` is `sizeof(buf)`.
-    globalValueNumber(this.getArgument(0)) =
-      globalValueNumber(leftsomeExpr.(SizeofExprOperator).getExprOperand())
-    or
-    // value of the left side of the expression `someExpr` equal  `sizeof(buf)` value, and `buf` is array.
-    leftsomeExpr.getValue().toInt() = this.getArgument(0).getType().getSize()
-  }
-
-  /**
-   * Holds if the left side of the expression `someExpr` equal to variable containing the length of the memory allocated for the buffer.
-   */
-  predicate isVariableEqualValueSizegBuffer() {
-    // the left side of expression `someExpr` is the variable that was used in the function of allocating memory for the buffer`.
-    exists(AllocationExpr alc |
-      leftsomeExpr.(VariableAccess).getTarget() =
-        alc.(FunctionCall).getArgument(0).(VariableAccess).getTarget()
-    )
-  }
+predicate interestringCallWithArgs(Call call, Expr sizeArg, Expr destArg) {
+  exists(StrcatFunction strcat |
+    strcat = call.getTarget() and
+    sizeArg = call.getArgument(strcat.getParamSize()) and
+    destArg = call.getArgument(strcat.getParamDest())
+  )
 }
 
-from WrongCallStrncat sc
+from FunctionCall call, Expr sizeArg, Expr destArg, SubExpr sub, int n
 where
-  sc.isExpressionEqualSizeof() or
-  sc.isVariableEqualValueSizegBuffer()
-select sc, "if the used buffer is full, writing out of the buffer is possible"
+  interestringCallWithArgs(call, sizeArg, destArg) and
+  // The destination buffer is an array of size n
+  destArg.getUnspecifiedType().(ArrayType).getSize() = n and
+  // The size argument is equivalent to a subtraction
+  globalValueNumber(sizeArg).getAnExpr() = sub and
+  // ... where the left side of the subtraction is the constant n
+  globalValueNumber(sub.getLeftOperand()).getAnExpr().getValue().toInt() = n and
+  // ... and the right side of the subtraction is a call to `strlen` where the argument is the
+  // destination buffer.
+  globalValueNumber(sub.getRightOperand()).getAnExpr().(StrlenCall).getStringExpr() =
+    globalValueNumber(destArg).getAnExpr()
+select call, "Possible out-of-bounds write due to incorrect size argument."

cpp/ql/src/semmle/code/cpp/models/implementations/SmartPointer.qll

@@ -18,20 +18,20 @@ private class UniqueOrSharedPtr extends Class, PointerWrapper {
 }
 
 /** Any function that unwraps a pointer wrapper class to reveal the underlying pointer. */
-private class PointerWrapperDataFlow extends DataFlowFunction {
-  PointerWrapperDataFlow() {
+private class PointerWrapperFlow extends TaintFunction, DataFlowFunction {
+  PointerWrapperFlow() {
     this = any(PointerWrapper wrapper).getAnUnwrapperFunction() and
     not this.getUnspecifiedType() instanceof ReferenceType
   }
 
-  override predicate hasDataFlow(FunctionInput input, FunctionOutput output) {
-    input.isQualifierAddress() and output.isReturnValue()
-    or
-    input.isQualifierObject() and output.isReturnValueDeref()
-    or
+  override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) {
     input.isReturnValueDeref() and
     output.isQualifierObject()
   }
+
+  override predicate hasDataFlow(FunctionInput input, FunctionOutput output) {
+    input.isQualifierObject() and output.isReturnValue()
+  }
 }
 
 /**

cpp/ql/src/semmle/code/cpp/security/Overflow.qll

@@ -5,6 +5,8 @@
 
 import cpp
 import semmle.code.cpp.controlflow.Dominance
+import semmle.code.cpp.rangeanalysis.SimpleRangeAnalysis
+import semmle.code.cpp.rangeanalysis.RangeAnalysisUtils
 
 /**
  * Holds if the value of `use` is guarded using `abs`.
@@ -94,9 +96,15 @@ predicate guardedGreater(Operation e, Expr use) {
 VariableAccess varUse(LocalScopeVariable v) { result = v.getAnAccess() }
 
 /**
- * Holds if `e` is not guarded against overflow by `use`.
+ * Holds if `e` potentially overflows and `use` is an operand of `e` that is not guarded.
  */
 predicate missingGuardAgainstOverflow(Operation e, VariableAccess use) {
+  (
+    convertedExprMightOverflowPositively(e)
+    or
+    // Ensure that the predicate holds when range analysis cannot determine an upper bound
+    upperBound(e.getFullyConverted()) = exprMaxVal(e.getFullyConverted())
+  ) and
   use = e.getAnOperand() and
   exists(LocalScopeVariable v | use.getTarget() = v |
     // overflow possible if large
@@ -115,9 +123,15 @@ predicate missingGuardAgainstOverflow(Operation e, VariableAccess use) {
 }
 
 /**
- * Holds if `e` is not guarded against underflow by `use`.
+ * Holds if `e` potentially underflows and `use` is an operand of `e` that is not guarded.
  */
 predicate missingGuardAgainstUnderflow(Operation e, VariableAccess use) {
+  (
+    convertedExprMightOverflowNegatively(e)
+    or
+    // Ensure that the predicate holds when range analysis cannot determine a lower bound
+    lowerBound(e.getFullyConverted()) = exprMinVal(e.getFullyConverted())
+  ) and
   use = e.getAnOperand() and
   exists(LocalScopeVariable v | use.getTarget() = v |
     // underflow possible if use is left operand and small

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-788/semmle/tests/AccessOfMemoryLocationAfterEndOfBufferUsingStrlen.expected

@@ -1,9 +1,9 @@
-| test.c:42:3:42:24 | ... = ... | potential unsafe or redundant assignment. |
-| test.c:43:3:43:40 | ... = ... | potential unsafe or redundant assignment. |
-| test.c:44:3:44:40 | ... = ... | potential unsafe or redundant assignment. |
-| test.c:45:3:45:44 | ... = ... | potential unsafe or redundant assignment. |
-| test.c:46:3:46:44 | ... = ... | potential unsafe or redundant assignment. |
-| test.c:47:3:47:48 | ... = ... | potential unsafe or redundant assignment. |
-| test.c:48:3:48:48 | ... = ... | potential unsafe or redundant assignment. |
-| test.c:49:3:49:50 | ... = ... | potential unsafe or redundant assignment. |
-| test.c:50:3:50:50 | ... = ... | potential unsafe or redundant assignment. |
+| test.c:54:3:54:24 | ... = ... | potential unsafe or redundant assignment. |
+| test.c:55:3:55:40 | ... = ... | potential unsafe or redundant assignment. |
+| test.c:56:3:56:44 | ... = ... | potential unsafe or redundant assignment. |
+| test.c:57:3:57:44 | ... = ... | potential unsafe or redundant assignment. |
+| test.c:58:3:58:48 | ... = ... | potential unsafe or redundant assignment. |
+| test.c:59:3:59:48 | ... = ... | potential unsafe or redundant assignment. |
+| test.c:60:3:60:52 | ... = ... | potential unsafe or redundant assignment. |
+| test.c:61:3:61:50 | ... = ... | potential unsafe or redundant assignment. |
+| test.c:62:3:62:54 | ... = ... | potential unsafe or redundant assignment. |

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-788/semmle/tests/AccessOfMemoryLocationAfterEndOfBufferUsingStrncat.expected

@@ -1,3 +1,5 @@
-| test.c:4:3:4:9 | call to strncat | if the used buffer is full, writing out of the buffer is possible |
-| test.c:11:3:11:9 | call to strncat | if the used buffer is full, writing out of the buffer is possible |
-| test.c:19:3:19:9 | call to strncat | if the used buffer is full, writing out of the buffer is possible |
+| test.c:8:3:8:9 | call to strncat | Possible out-of-bounds write due to incorrect size argument. |
+| test.c:9:3:9:9 | call to strncat | Possible out-of-bounds write due to incorrect size argument. |
+| test.c:17:3:17:9 | call to strncat | Possible out-of-bounds write due to incorrect size argument. |
+| test.c:18:3:18:9 | call to strncat | Possible out-of-bounds write due to incorrect size argument. |
+| test.c:46:3:46:9 | call to strncat | Possible out-of-bounds write due to incorrect size argument. |

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-788/semmle/tests/test.c

@@ -1,70 +1,84 @@
-void workFunction_0(char *s) {
+char * strncat(char*, const char*, unsigned);
+unsigned strlen(const char*);
+void* malloc(unsigned);
+
+void strncat_test1(char *s) {
   char buf[80];
-  strncat(buf, s, sizeof(buf)-strlen(buf)-1); // GOOD
-  strncat(buf, s, sizeof(buf)-strlen(buf));  // BAD
-  strncat(buf, "fix", sizeof(buf)-strlen(buf)); // BAD [NOT DETECTED] 
+  strncat(buf, s, sizeof(buf) - strlen(buf) - 1); // GOOD
+  strncat(buf, s, sizeof(buf) - strlen(buf));  // BAD
+  strncat(buf, "fix", sizeof(buf)-strlen(buf)); // BAD
 }
-void workFunction_1(char *s) {
+
 #define MAX_SIZE 80
+
+void strncat_test2(char *s) {
   char buf[MAX_SIZE];
-  strncat(buf, s, MAX_SIZE-strlen(buf)-1); // GOOD
-  strncat(buf, s, MAX_SIZE-strlen(buf));  // BAD
-  strncat(buf, "fix", MAX_SIZE-strlen(buf)); // BAD [NOT DETECTED]
+  strncat(buf, s, MAX_SIZE - strlen(buf) - 1); // GOOD
+  strncat(buf, s, MAX_SIZE - strlen(buf));  // BAD
+  strncat(buf, "fix", MAX_SIZE - strlen(buf)); // BAD
 }
-void workFunction_2_0(char *s) {
-  char * buf;
-  int len=80;
-  buf = (char *) malloc(len);
-  strncat(buf, s, len-strlen(buf)-1); // GOOD
-  strncat(buf, s, len-strlen(buf));  // BAD
-  strncat(buf, "fix", len-strlen(buf)); // BAD [NOT DETECTED]
+
+void strncat_test3(char *s) {
+  int len = 80;
+  char* buf = (char *) malloc(len);
+  strncat(buf, s, len - strlen(buf) - 1); // GOOD
+  strncat(buf, s, len - strlen(buf));  // BAD [NOT DETECTED]
+  strncat(buf, "fix", len - strlen(buf)); // BAD [NOT DETECTED]
 }
-void workFunction_2_1(char *s) {
-  char * buf;
-  int len=80;
-  buf = (char *) malloc(len+1);
-  strncat(buf, s, len-strlen(buf)-1); // GOOD
-  strncat(buf, s, len-strlen(buf));  // GOOD
+
+void strncat_test4(char *s) {
+  int len = 80;
+  char* buf = (char *) malloc(len + 1);
+  strncat(buf, s, len - strlen(buf) - 1); // GOOD
+  strncat(buf, s, len - strlen(buf)); // GOOD
 }
 
 struct buffers
 {
-    unsigned char buff1[50];
-    unsigned char *buff2;
+    unsigned char array[50];
+    unsigned char *pointer;
 } globalBuff1,*globalBuff2,globalBuff1_c,*globalBuff2_c;
 
+void strncat_test5(char* s, struct buffers* buffers) {
+  unsigned len_array = strlen(buffers->array);
+  unsigned max_size = sizeof(buffers->array);
+  unsigned free_size = max_size - len_array;
+  strncat(buffers->array, s, free_size); // BAD
+}
 
-void badFunc0(){
+void strlen_test1(){
   unsigned char buff1[12];
   struct buffers buffAll;
   struct buffers * buffAll1;
 
   buff1[strlen(buff1)]=0; // BAD
-  buffAll.buff1[strlen(buffAll.buff1)]=0; // BAD
-  buffAll.buff2[strlen(buffAll.buff2)]=0; // BAD
-  buffAll1->buff1[strlen(buffAll1->buff1)]=0; // BAD
-  buffAll1->buff2[strlen(buffAll1->buff2)]=0; // BAD
-  globalBuff1.buff1[strlen(globalBuff1.buff1)]=0; // BAD
-  globalBuff1.buff2[strlen(globalBuff1.buff2)]=0; // BAD
-  globalBuff2->buff1[strlen(globalBuff2->buff1)]=0; // BAD
-  globalBuff2->buff2[strlen(globalBuff2->buff2)]=0; // BAD
+  buffAll.array[strlen(buffAll.array)]=0; // BAD
+  buffAll.pointer[strlen(buffAll.pointer)]=0; // BAD
+  buffAll1->array[strlen(buffAll1->array)]=0; // BAD
+  buffAll1->pointer[strlen(buffAll1->pointer)]=0; // BAD
+  globalBuff1.array[strlen(globalBuff1.array)]=0; // BAD
+  globalBuff1.pointer[strlen(globalBuff1.pointer)]=0; // BAD
+  globalBuff2->array[strlen(globalBuff2->array)]=0; // BAD
+  globalBuff2->pointer[strlen(globalBuff2->pointer)]=0; // BAD
 }
-void noBadFunc0(){
+
+void strlen_test2(){
   unsigned char buff1[12],buff1_c[12];
   struct buffers buffAll,buffAll_c;
   struct buffers * buffAll1,*buffAll1_c;
 
   buff1[strlen(buff1_c)]=0; // GOOD
-  buffAll.buff1[strlen(buffAll_c.buff1)]=0; // GOOD
-  buffAll.buff2[strlen(buffAll.buff1)]=0; // GOOD
-  buffAll1->buff1[strlen(buffAll1_c->buff1)]=0; // GOOD
-  buffAll1->buff2[strlen(buffAll1->buff1)]=0; // GOOD
-  globalBuff1.buff1[strlen(globalBuff1_c.buff1)]=0; // GOOD
-  globalBuff1.buff2[strlen(globalBuff1.buff1)]=0; // GOOD
-  globalBuff2->buff1[strlen(globalBuff2_c->buff1)]=0; // GOOD
-  globalBuff2->buff2[strlen(globalBuff2->buff1)]=0; // GOOD
+  buffAll.array[strlen(buffAll_c.array)]=0; // GOOD
+  buffAll.pointer[strlen(buffAll.array)]=0; // GOOD
+  buffAll1->array[strlen(buffAll1_c->array)]=0; // GOOD
+  buffAll1->pointer[strlen(buffAll1->array)]=0; // GOOD
+  globalBuff1.array[strlen(globalBuff1_c.array)]=0; // GOOD
+  globalBuff1.pointer[strlen(globalBuff1.array)]=0; // GOOD
+  globalBuff2->array[strlen(globalBuff2_c->array)]=0; // GOOD
+  globalBuff2->pointer[strlen(globalBuff2->array)]=0; // GOOD
 }
-void goodFunc0(){
+
+void strlen_test3(){
   unsigned char buffer[12];
   int i;
   for(i = 0; i < 6; i++)