Merge pull request #10 from kevinbackhouse/libssh2_eating_error_codes

Libssh2 eating error codes

Author: jf205

ql_demos/cpp/README.md

@@ -0,0 +1 @@
+[snapshot](https://downloads.lgtm.com/snapshots/cpp/libssh2/libssh2_libssh2_C_C++_38bf7ce.zip)

ql_demos/cpp/libssh2_eating_error_codes/00_error_codes.ql

@@ -0,0 +1,15 @@
+/**
+ * @name 00_error_codes
+ */
+
+import cpp
+
+// Look for return statements that return a negative integer constant.
+// For example:
+//
+//   return -1;
+//
+// The negative return value might be an error code.
+from ReturnStmt ret
+where ret.getExpr().getValue().toInt() < 0
+select ret

ql_demos/cpp/libssh2_eating_error_codes/01_error_codes_call.ql

@@ -0,0 +1,14 @@
+/**
+ * @name 01_error_codes_call
+ */
+
+import cpp
+
+// Extend the previous query to also find calls to functions that sometimes
+// return a negative integer constant.
+from Function f, FunctionCall call, ReturnStmt ret
+where
+  call.getTarget() = f and
+  ret.getEnclosingFunction() = f and
+  ret.getExpr().getValue().toInt() < 0
+select ret, call

ql_demos/cpp/libssh2_eating_error_codes/02_eating_error_codes.ql

@@ -0,0 +1,15 @@
+/**
+ * @name 02_eating_error_codes
+ */
+
+import cpp
+
+// Look for calls that are cast to unsigned, which means that the error
+// code might be accidentally ignored.
+from Function f, FunctionCall call, ReturnStmt ret
+where
+  call.getTarget() = f and
+  ret.getEnclosingFunction() = f and
+  ret.getExpr().getValue().toInt() < 0 and
+  call.getFullyConverted().getType().getUnderlyingType().(IntegralType).isUnsigned()
+select call, ret

ql_demos/cpp/libssh2_eating_error_codes/03_eating_error_codes_localflow.ql

@@ -0,0 +1,24 @@
+/**
+ * @name 03_eating_error_codes_localflow
+ */
+
+import cpp
+import semmle.code.cpp.dataflow.DataFlow
+
+// The previous query only handled cases where the result of the function
+// call is immediately cast to unsigned. So it will fail to detect examples
+// like this, where the cast doesn't happen immediately:
+//
+//   int r = f();
+//   unsigned int x = r;
+//
+// In this query, we add local dataflow so that we can also handle such
+// cases.
+from Function f, FunctionCall call, ReturnStmt ret, DataFlow::Node source, DataFlow::Node sink
+where call.getTarget() = f
+and ret.getEnclosingFunction() = f
+and ret.getExpr().getValue().toInt() < 0
+and source.asExpr() = call
+and DataFlow::localFlow(source, sink)
+and sink.asExpr().getFullyConverted().getType().getUnderlyingType().(IntegralType).isUnsigned()
+select source, sink

ql_demos/cpp/libssh2_eating_error_codes/04_eating_error_codes_localflow_rangeanalysis.ql

@@ -0,0 +1,30 @@
+/**
+ * @name 04_eating_error_codes_localflow_rangeanalysis
+ */
+
+import cpp
+import semmle.code.cpp.dataflow.DataFlow
+import semmle.code.cpp.rangeanalysis.SimpleRangeAnalysis
+
+// The previous query produced some weird results. The problem is that it
+// treats any expression with an unsigned type as a potential sink. What we
+// really want is to find where the cast from signed to unsigned happens,
+// because that's where the integer overflow occurs. So we want the sink to
+// be a potentially negative expression that gets cast to unsigned.
+//
+// Note that by using range analysis, we can avoid producing false positive
+// results for examples like this:
+//
+//   int r = f();
+//   if (r < 0) return -1;
+//   unsigned int x = r;
+from Function f, FunctionCall call, ReturnStmt ret, DataFlow::Node source, DataFlow::Node sink
+where
+  call.getTarget() = f and
+  ret.getEnclosingFunction() = f and
+  ret.getExpr().getValue().toInt() < 0 and
+  source.asExpr() = call and
+  DataFlow::localFlow(source, sink) and
+  sink.asExpr().getFullyConverted().getType().getUnderlyingType().(IntegralType).isUnsigned() and
+  lowerBound(sink.asExpr()) < 0
+select source, sink

ql_demos/cpp/libssh2_eating_error_codes/README.md

@@ -0,0 +1,9 @@
+# Eating error codes in libssh2
+
+Download this [snapshot](https://downloads.lgtm.com/snapshots/cpp/libssh2/libssh2_libssh2_C_C++_38bf7ce.zip) for the demo.
+
+This demo shows how to develop, step-by-step, the query from the [blog post](https://blog.semmle.com/libssh2-integer-overflow/) about libssh2 CVE-2019-13115. This query did not find the bug that caused the CVE. It is instead about doing variant analysis on a bug that we noticed on the development branch of libssh2. We sent the query results to the libssh2 development team and they were able to fix all the variants before the next version of libssh2 was released.
+
+[This](https://lgtm.com/projects/g/libssh2/libssh2/snapshot/6e2f5563c80521b3cde72a6fcdb675c2e085f9cf/files/src/hostkey.c?sort=name&dir=ASC&mode=heatmap&__hstc=70225743.5fa8704c8874c6eafaef219923a26734.1534954774206.1564532078978.1564925733575.72&__hssc=70225743.2.1565139962633&__hsfp=997709570#L677) is an example of the bug. The problem is that `_libssh2_get_c_string` returns a negative integer as an error code, but the type of `r_len` is `unsigned int`, so the error code is accidentally ignored.
+
+For a shorter demo, stop at step 02. Steps 03 and 04 make the query more sophisticated by adding local data flow and range analysis.