Merge branch 'main' into aeisenberg/pack/cpp

Author: aeisenberg

cpp/ql/src/experimental/Security/CWE/CWE-758/UndefinedOrImplementationDefinedBehavior.ql

@@ -163,4 +163,5 @@ where
     or
     eots.dangerousCrementChanges()
   )
-select eots, "This expression may have undefined behavior."
+select eots,
+  "This expression may have undefined behavior, because the order of evaluation is not specified."

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-758/semmle/tests/UndefinedOrImplementationDefinedBehavior.expected

@@ -1,3 +1,3 @@
-| test.c:13:10:13:21 | call to tmpFunction1 | This expression may have undefined behavior. |
-| test.c:13:30:13:41 | call to tmpFunction2 | This expression may have undefined behavior. |
-| test.c:16:15:16:20 | ... ++ | This expression may have undefined behavior. |
+| test.c:13:10:13:21 | call to tmpFunction1 | This expression may have undefined behavior, because the order of evaluation is not specified. |
+| test.c:13:30:13:41 | call to tmpFunction2 | This expression may have undefined behavior, because the order of evaluation is not specified. |
+| test.c:16:15:16:20 | ... ++ | This expression may have undefined behavior, because the order of evaluation is not specified. |

cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/test_buffer_overrun.cpp

@@ -0,0 +1,39 @@
+typedef unsigned char uint8_t;
+#define SIZE (32)
+
+void test_buffer_overrun_in_for_loop()
+{
+    uint8_t data[SIZE] = {0};
+    for (int x = 0; x < SIZE * 2; x++) {
+        data[x] = 0x41; // BAD [NOT DETECTED]
+    }
+}
+
+void test_buffer_overrun_in_while_loop_using_pointer_arithmetic()
+{
+    uint8_t data[SIZE] = {0};
+    int offset = 0;
+    while (offset < SIZE * 2) {
+        *(data + offset) = 0x41; // BAD [NOT DETECTED]
+        offset++;
+    }
+}
+
+void test_buffer_overrun_in_while_loop_using_array_indexing()
+{
+    uint8_t data[SIZE] = {0};
+    int offset = 0;
+    while (offset < SIZE * 2) {
+        data[offset] = 0x41; // BAD [NOT DETECTED]
+        offset++;
+    }
+}
+
+int main(int argc, char *argv[])
+{
+    test_buffer_overrun_in_for_loop();
+    test_buffer_overrun_in_while_loop_using_pointer_arithmetic();
+    test_buffer_overrun_in_while_loop_using_array_indexing();
+
+    return 0;
+}

cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/tests.cpp

@@ -114,7 +114,7 @@ void test6(bool cond)
 
 	c = 100;
 	buffer[c] = 'x'; // BAD: over-write [NOT DETECTED]
-	ch = buffer[c]; // BAD: under-read [NOT DETECTED]
+	ch = buffer[c]; // BAD: over-read [NOT DETECTED]
 
 	d = 0;
 	d = 1000;

csharp/extractor/Semmle.Extraction.CSharp/SymbolExtensions.cs

@@ -482,22 +482,6 @@ private static void BuildNamedTypeDisplayName(this INamedTypeSymbol namedType, C
             {
                 trapFile.Write(TrapExtensions.EncodeString(namedType.Name));
             }
-
-            if (namedType.IsGenericType && namedType.TypeKind != TypeKind.Error && namedType.TypeArguments.Any())
-            {
-                trapFile.Write('<');
-                trapFile.BuildList(
-                    ",",
-                    namedType.TypeArguments,
-                    p =>
-                    {
-                        if (IsReallyBound(namedType))
-                        {
-                            p.BuildDisplayName(cx, trapFile);
-                        }
-                    });
-                trapFile.Write('>');
-            }
         }
 
         public static bool IsReallyUnbound(this INamedTypeSymbol type) =>

csharp/ql/src/Security Features/CWE-502/UnsafeDeserializationUntrustedInput.qhelp

@@ -27,6 +27,17 @@ it may be necessary to use a different deserialization framework.</p>
 
 <sample src="UnsafeDeserializationUntrustedInputGood.cs" />
 
+<p>In the following example potentially untrusted stream and type is deserialized using a
+<code>DataContractJsonSerializer</code> which is known to be vulnerable with user supplied types.</p>
+
+<sample src="UnsafeDeserializationUntrustedInputTypeBad.cs" />
+
+<p>To fix this specific vulnerability, we are using hardcoded
+Plain Old CLR Object (<a href="https://en.wikipedia.org/wiki/Plain_old_CLR_object">POCO</a>) type. In other cases,
+it may be necessary to use a different deserialization framework.</p>
+
+<sample src="UnsafeDeserializationUntrustedInputTypeGood.cs" />
+
 </example>
 <references>
 

csharp/ql/src/Security Features/CWE-502/UnsafeDeserializationUntrustedInput.ql

@@ -15,7 +15,46 @@ import csharp
 import semmle.code.csharp.security.dataflow.UnsafeDeserializationQuery
 import DataFlow::PathGraph
 
-from TaintTrackingConfig config, DataFlow::PathNode source, DataFlow::PathNode sink
-where config.hasFlowPath(source, sink)
-select sink.getNode(), source, sink, "$@ flows to unsafe deserializer.", source.getNode(),
-  "User-provided data"
+from DataFlow::PathNode userInput, DataFlow::PathNode deserializeCallArg
+where
+  exists(TaintToObjectMethodTrackingConfig taintTracking |
+    // all flows from user input to deserialization with weak and strong type serializers
+    taintTracking.hasFlowPath(userInput, deserializeCallArg)
+  ) and
+  // intersect with strong types, but user controlled or weak types deserialization usages
+  (
+    exists(
+      DataFlow::Node weakTypeCreation, DataFlow::Node weakTypeUsage,
+      WeakTypeCreationToUsageTrackingConfig weakTypeDeserializerTracking, MethodCall mc
+    |
+      weakTypeDeserializerTracking.hasFlow(weakTypeCreation, weakTypeUsage) and
+      mc.getQualifier() = weakTypeUsage.asExpr() and
+      mc.getAnArgument() = deserializeCallArg.getNode().asExpr()
+    )
+    or
+    exists(
+      TaintToObjectTypeTrackingConfig userControlledTypeTracking, DataFlow::Node taintedTypeUsage,
+      DataFlow::Node userInput2, MethodCall mc
+    |
+      userControlledTypeTracking.hasFlow(userInput2, taintedTypeUsage) and
+      mc.getQualifier() = taintedTypeUsage.asExpr() and
+      mc.getAnArgument() = deserializeCallArg.getNode().asExpr()
+    )
+  )
+  or
+  // no type check needed - straightforward taint -> sink
+  exists(TaintToConstructorOrStaticMethodTrackingConfig taintTracking2 |
+    taintTracking2.hasFlowPath(userInput, deserializeCallArg)
+  )
+  or
+  // JsonConvert static method call, but with additional unsafe typename tracking
+  exists(
+    JsonConvertTrackingConfig taintTrackingJsonConvert, TypeNameTrackingConfig typenameTracking,
+    DataFlow::Node settingsCallArg
+  |
+    taintTrackingJsonConvert.hasFlowPath(userInput, deserializeCallArg) and
+    typenameTracking.hasFlow(_, settingsCallArg) and
+    deserializeCallArg.getNode().asExpr().getParent() = settingsCallArg.asExpr().getParent()
+  )
+select deserializeCallArg, userInput, deserializeCallArg, "$@ flows to unsafe deserializer.",
+  userInput, "User-provided data"

csharp/ql/src/Security Features/CWE-502/UnsafeDeserializationUntrustedInputGood.cs

@@ -6,7 +6,7 @@ class Good
     public static object Deserialize(TextBox textBox)
     {
         JavaScriptSerializer sr = new JavaScriptSerializer();
-        // GOOD
+        // GOOD: no unsafe type resolver
         return sr.DeserializeObject(textBox.Text);
     }
 }

csharp/ql/src/Security Features/CWE-502/UnsafeDeserializationUntrustedInputTypeBad.cs

@@ -0,0 +1,13 @@
+using System.Runtime.Serialization.Json;
+using System.IO;
+using System;
+
+class BadDataContractJsonSerializer
+{
+    public static object Deserialize(string type, Stream s)
+    {
+        // BAD: stream and type are potentially untrusted
+        var ds = new DataContractJsonSerializer(Type.GetType(type));
+        return ds.ReadObject(s);
+    }
+}

csharp/ql/src/Security Features/CWE-502/UnsafeDeserializationUntrustedInputTypeGood.cs

@@ -0,0 +1,20 @@
+using System.Runtime.Serialization.Json;
+using System.IO;
+using System;
+
+class Poco
+{
+    public int Count;
+
+    public string Comment;
+}
+
+class GoodDataContractJsonSerializer
+{
+    public static Poco Deserialize(Stream s)
+    {
+        // GOOD: while stream is potentially untrusted, the instantiated type is hardcoded
+        var ds = new DataContractJsonSerializer(typeof(Poco));
+        return (Poco)ds.ReadObject(s);
+    }
+}