Merge remote-tracking branch 'upstream/master' into FalsySanitizer

Author: erik-krogh

change-notes/1.24/analysis-cpp.md

@@ -19,6 +19,7 @@ The following changes in version 1.24 affect C/C++ analysis in all applications.
 | Memory is never freed (`cpp/memory-never-freed`) | More true positive results | This query now identifies a wider variety of buffer allocations using the `semmle.code.cpp.models.interfaces.Allocation` library. |
 | Memory may not be freed (`cpp/memory-may-not-be-freed`) | More true positive results | This query now identifies a wider variety of buffer allocations using the `semmle.code.cpp.models.interfaces.Allocation` library. |
 | Missing return statement (`cpp/missing-return`) | Fewer false positive results | Functions containing `asm` statements are no longer highlighted by this query. |
+| No space for zero terminator (`cpp/no-space-for-terminator`) | More correct results | String arguments to formatting functions are now (usually) expected to be null terminated strings. |
 | Hard-coded Japanese era start date (`cpp/japanese-era/exact-era-date`) |  | This query is no longer run on LGTM. |
 | No space for zero terminator (`cpp/no-space-for-terminator`) | Fewer false positive results | This query has been modified to be more conservative when identifying which pointers point to null-terminated strings.  This approach produces fewer, more accurate results. |
 | Overloaded assignment does not return 'this' (`cpp/assignment-does-not-return-this`) | Fewer false positive results | This query no longer reports incorrect results in template classes. |

change-notes/1.24/analysis-csharp.md

@@ -27,7 +27,9 @@ The following changes in version 1.24 affect C# analysis in all applications.
 ## Changes to code extraction
 
 * Tuple expressions, for example `(int,bool)` in `default((int,bool))` are now extracted correctly.
-* Expression nullability flow state is extracted. 
+* Expression nullability flow state is extracted.
+* Implicitly typed `stackalloc` expressions are now extracted correctly.
+* The difference between `stackalloc` array creations and normal array creations is extracted.
 
 ## Changes to libraries
 
@@ -38,5 +40,6 @@ The following changes in version 1.24 affect C# analysis in all applications.
 * The taint tracking library now tracks flow through (implicit or explicit) conversion operator calls.
 * [Code contracts](https://docs.microsoft.com/en-us/dotnet/framework/debug-trace-profile/code-contracts) are now recognized, and are treated like any other assertion methods.
 * Expression nullability flow state is given by the predicates `Expr.hasNotNullFlowState()` and `Expr.hasMaybeNullFlowState()`.
+* `stackalloc` array creations are now represented by the QL class `Stackalloc`. Previously they were represented by the class `ArrayCreation`.
 
 ## Changes to autobuilder

cpp/ql/src/Security/CWE/CWE-131/NoSpaceForZeroTerminator.ql

@@ -22,16 +22,25 @@ import semmle.code.cpp.models.interfaces.Allocation
 predicate terminationProblem(AllocationExpr malloc, string msg) {
   // malloc(strlen(...))
   exists(StrlenCall strlen | DataFlow::localExprFlow(strlen, malloc.getSizeExpr())) and
-  // flows into a null-terminated string function
+  // flows to a call that implies this is a null-terminated string
   exists(ArrayFunction af, FunctionCall fc, int arg |
     DataFlow::localExprFlow(malloc, fc.getArgument(arg)) and
     fc.getTarget() = af and
     (
-      // null terminated string
+      // flows into null terminated string argument
       af.hasArrayWithNullTerminator(arg)
       or
-      // likely a null terminated string (such as `strcpy`, `strcat`)
+      // flows into likely null terminated string argument (such as `strcpy`, `strcat`)
       af.hasArrayWithUnknownSize(arg)
+      or
+      // flows into string argument to a formatting function (such as `printf`)
+      exists(int n, FormatLiteral fl |
+        fc.getArgument(arg) = fc.(FormattingFunctionCall).getConversionArgument(n) and
+        fl = fc.(FormattingFunctionCall).getFormat() and
+        fl.getConversionType(n) instanceof PointerType and // `%s`, `%ws` etc
+        not fl.getConversionType(n) instanceof VoidPointerType and // exclude: `%p`
+        not fl.hasPrecision(n) // exclude: `%.*s`
+      )
     )
   ) and
   msg = "This allocation does not include space to null-terminate the string."

cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll

@@ -365,10 +365,10 @@ private predicate modelFlow(Instruction iFrom, Instruction iTo) {
       modelOut.isReturnValueDeref() and
       iTo = call
       or
-      exists(WriteSideEffectInstruction outNode |
-        modelOut.isParameterDeref(outNode.getIndex()) and
+      exists(int index, WriteSideEffectInstruction outNode |
+        modelOut.isParameterDeref(index) and
         iTo = outNode and
-        outNode.getPrimaryInstruction() = call
+        outNode = getSideEffectFor(call, index)
       )
       // TODO: add write side effects for qualifiers
     ) and
@@ -380,8 +380,7 @@ private predicate modelFlow(Instruction iFrom, Instruction iTo) {
       or
       exists(int index, ReadSideEffectInstruction read |
         modelIn.isParameterDeref(index) and
-        read.getIndex() = index and
-        read.getPrimaryInstruction() = call and
+        read = getSideEffectFor(call, index) and
         iFrom = read.getSideEffectOperand().getAnyDef()
       )
       or
@@ -392,6 +391,18 @@ private predicate modelFlow(Instruction iFrom, Instruction iTo) {
   )
 }
 
+/**
+ * Holds if the result is a side effect for instruction `call` on argument
+ * index `argument`. This helper predicate makes it easy to join on both of
+ * these columns at once, avoiding pathological join orders in case the
+ * argument index should get joined first.
+ */
+pragma[noinline]
+SideEffectInstruction getSideEffectFor(CallInstruction call, int argument) {
+  call = result.getPrimaryInstruction() and
+  argument = result.(IndexedInstruction).getIndex()
+}
+
 /**
  * Holds if data flows from `source` to `sink` in zero or more local
  * (intra-procedural) steps.

cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/AliasedSSA.qll

@@ -220,9 +220,12 @@ class VariableMemoryLocation extends TVariableMemoryLocation, AllocationMemoryLo
   /**
    * Holds if this memory location covers the entire variable.
    */
-  final predicate coversEntireVariable() {
-    startBitOffset = 0 and
-    endBitOffset = var.getIRType().getByteSize() * 8
+  final predicate coversEntireVariable() { varIRTypeHasBitRange(startBitOffset, endBitOffset) }
+
+  pragma[noinline]
+  private predicate varIRTypeHasBitRange(int start, int end) {
+    start = 0 and
+    end = var.getIRType().getByteSize() * 8
   }
 }
 

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

@@ -94,6 +94,22 @@ abstract class FunctionWithWrappers extends Function {
     )
   }
 
+  /**
+   * Whether 'func' is a (possibly nested) wrapper function that feeds a parameter at the given index
+   * through to an interesting parameter of 'this' function.
+   *
+   * The 'cause' gives the name of 'this' interesting function and its relevant parameter
+   * at the end of the call chain.
+   *
+   * If there is more than one possible 'cause', a unique one is picked (by lexicographic order).
+   */
+  pragma[nomagic]
+  private string wrapperFunctionAnyDepthUnique(Function func, int paramIndex) {
+    result =
+      toCause(func, paramIndex) + ", which ends up calling " +
+        min(string targetCause | this.wrapperFunctionAnyDepth(func, paramIndex, targetCause))
+  }
+
   /**
    * Whether 'func' is a (possibly nested) wrapper function that feeds a parameter at the given index
    * through to an interesting parameter of 'this' function.
@@ -114,13 +130,7 @@ abstract class FunctionWithWrappers extends Function {
       )
     or
     not this.wrapperFunctionLimitedDepth(func, paramIndex, _, _) and
-    cause =
-      min(string targetCause, string possibleCause |
-        this.wrapperFunctionAnyDepth(func, paramIndex, targetCause) and
-        possibleCause = toCause(func, paramIndex) + ", which ends up calling " + targetCause
-      |
-        possibleCause
-      )
+    cause = wrapperFunctionAnyDepthUnique(func, paramIndex)
   }
 
   /**

cpp/ql/test/query-tests/Security/CWE/CWE-131/semmle/NoSpaceForZeroTerminator/NoSpaceForZeroTerminator.expected

@@ -5,6 +5,7 @@
 | test.c:49:20:49:25 | call to malloc | This allocation does not include space to null-terminate the string. |
 | test.cpp:24:35:24:40 | call to malloc | This allocation does not include space to null-terminate the string. |
 | test.cpp:45:28:45:33 | call to malloc | This allocation does not include space to null-terminate the string. |
+| test.cpp:55:28:55:33 | call to malloc | This allocation does not include space to null-terminate the string. |
 | test.cpp:63:28:63:33 | call to malloc | This allocation does not include space to null-terminate the string. |
 | test.cpp:71:28:71:33 | call to malloc | This allocation does not include space to null-terminate the string. |
 | test.cpp:79:28:79:33 | call to malloc | This allocation does not include space to null-terminate the string. |

cpp/ql/test/query-tests/Security/CWE/CWE-131/semmle/NoSpaceForZeroTerminator/test.cpp

@@ -51,7 +51,7 @@ void decode(char *dest, char *src);
 void wdecode(wchar_t *dest, wchar_t *src);
 
 void bad4(char *str) {
-    // BAD -- zero-termination proved by wprintf (as parameter) [NOT DETECTED]
+    // BAD -- zero-termination proved by wprintf (as parameter)
     char *buffer = (char *)malloc(strlen(str));
     decode(buffer, str);
     wprintf(L"%s", buffer);
@@ -107,3 +107,19 @@ void bad9(wchar_t *wstr) {
     wcscpy(wbuffer, wstr);
     delete wbuffer;
 }
+
+void good3(char *str) {
+    // GOOD -- zero-termination not required for this printf
+    char *buffer = (char *)malloc(strlen(str));
+    decode(buffer, str);
+    wprintf(L"%p", buffer);
+    free(buffer);
+}
+
+void good4(char *str) {
+    // GOOD -- zero-termination not required for this printf
+    char *buffer = (char *)malloc(strlen(str));
+    decode(buffer, str);
+    wprintf(L"%.*s", strlen(str), buffer);
+    free(buffer);
+}

csharp/extractor/Semmle.Extraction.CSharp/Entities/Expressions/ArrayCreation.cs

@@ -90,9 +90,29 @@ class StackAllocArrayCreation : ExplicitArrayCreation<StackAllocArrayCreationExp
 
         public override InitializerExpressionSyntax Initializer => Syntax.Initializer;
 
+        protected override void PopulateExpression(TextWriter trapFile)
+        {
+            base.PopulateExpression(trapFile);
+            trapFile.stackalloc_array_creation(this);
+        }
+
         public static Expression Create(ExpressionNodeInfo info) => new StackAllocArrayCreation(info).TryPopulate();
     }
 
+    class ImplicitStackAllocArrayCreation : ArrayCreation<ImplicitStackAllocArrayCreationExpressionSyntax>
+    {
+        ImplicitStackAllocArrayCreation(ExpressionNodeInfo info) : base(info.SetKind(ExprKind.ARRAY_CREATION)) { }
+
+        public static Expression Create(ExpressionNodeInfo info) => new ImplicitStackAllocArrayCreation(info).TryPopulate();
+
+        protected override void PopulateExpression(TextWriter trapFile)
+        {
+            ArrayInitializer.Create(new ExpressionNodeInfo(cx, Syntax.Initializer, this, -1));
+            trapFile.implicitly_typed_array_creation(this);
+            trapFile.stackalloc_array_creation(this);
+        }
+    }
+
     class ImplicitArrayCreation : ArrayCreation<ImplicitArrayCreationExpressionSyntax>
     {
         ImplicitArrayCreation(ExpressionNodeInfo info) : base(info.SetKind(ExprKind.ARRAY_CREATION)) { }

csharp/extractor/Semmle.Extraction.CSharp/Entities/Expressions/Factory.cs

@@ -207,6 +207,9 @@ internal static Expression Create(ExpressionNodeInfo info)
                     case SyntaxKind.StackAllocArrayCreationExpression:
                         return StackAllocArrayCreation.Create(info);
 
+                    case SyntaxKind.ImplicitStackAllocArrayCreationExpression:
+                        return ImplicitStackAllocArrayCreation.Create(info);
+
                     case SyntaxKind.ArgListExpression:
                         return ArgList.Create(info);