Update comments.

Author: kevinbackhouse

ql_demos/cpp/libssh2_eating_error_codes/00_error_codes.ql

@@ -4,6 +4,12 @@
 
 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

@@ -4,8 +4,11 @@
 
 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
+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

@@ -4,9 +4,12 @@
 
 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()
+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

@@ -5,9 +5,15 @@
 import cpp
 import semmle.code.cpp.dataflow.DataFlow
 
-// int r = f();
-// unsigned int x = r;
-
+// 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

ql_demos/cpp/libssh2_eating_error_codes/04_eating_error_codes_localflow_rangeanalysis.ql

@@ -6,16 +6,25 @@ import cpp
 import semmle.code.cpp.dataflow.DataFlow
 import semmle.code.cpp.rangeanalysis.SimpleRangeAnalysis
 
-// int r = f();
-// if (r < 0) return -1;
-// unsigned int x = r;
-
+// 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
+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

@@ -1 +1,9 @@
-[Snapshot](https://downloads.lgtm.com/snapshots/cpp/libssh2/libssh2_libssh2_C_C++_38bf7ce.zip)
+# Eating error codes in libssh2
+
+Use 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 which 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 an 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 dataflow and range analysis.