Adding ArgumentIsSizeofOrOperation.qhelp, ql, and example files.

Author: bdrodes

cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/ArgumentIsSizeofOrOperation.qhelp

@@ -0,0 +1,22 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+  <overview>
+    <p>If the argument for a <code>sizeof</code> call is a binary operation or a <code>sizeof</code> call, it is typically a sign that there is a confusion on the usage of the sizeof usage.</p>
+  </overview>
+
+<recommendation>
+  <p>Any findings from this rule may indicate that the <code>sizeof</code> is being used incorrectly.</p>
+  <p>Review the logic of the call.</p>
+</recommendation>
+
+<example>
+  <p>The following example shows a case where <code>sizeof</code> a binary operation by mistake.</p>
+  <sample src="ArgumentIsSizeofOrOperationBad.c" />
+
+  <p>In this example, the fix is to multiply the result of <code>sizeof</code> by the number of elements.</p>
+  <sample src="ArgumentIsSizeofOrOperationGood.c" />
+</example>
+
+</qhelp>

cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/ArgumentIsSizeofOrOperation.ql

@@ -0,0 +1,59 @@
+/**
+ * @id cpp/sizeof/sizeof-or-operation-as-argument
+ * @name Usage of an expression that is a binary operation, or sizeof call passed as an argument to a sizeof call
+ * @description When the `expr` passed to `sizeof` is a binary operation, or a sizeof call, this is typically a sign that there is a confusion on the usage of sizeof.
+ * @tags security
+ */
+
+import cpp
+import SizeOfTypeUtils
+
+/**
+ * Windows SDK corecrt_math.h defines a macro _CLASS_ARG that
+ * intentionally misuses sizeof to determine the size of a floating point type.
+ * Explicitly ignoring any hit in this macro.
+ */
+predicate isPartOfCrtFloatingPointMacroExpansion(Expr e) {
+  exists(MacroInvocation mi |
+    mi.getMacroName() = "_CLASS_ARG" and
+    mi.getMacro().getFile().getBaseName() = "corecrt_math.h" and
+    mi.getAnExpandedElement() = e
+  )
+}
+
+/**
+ * Determines if the sizeOfExpr is ignorable.
+ */
+predicate ignorableSizeof(SizeofExprOperator sizeofExpr) {
+  // a common pattern found is to sizeof a binary operation to check a type
+  // to then perfomr an onperaiton for a 32 or 64 bit type.
+  // these cases often look like sizeof(x) >=4
+  // more generally we see binary operations frequently used in different type
+  // checks, where the sizeof is part of some comparison operation of a switch statement guard.
+  // sizeof as an argument is also similarly used, but seemingly less frequently.
+  exists(ComparisonOperation comp | comp.getAnOperand() = sizeofExpr)
+  or
+  exists(ConditionalStmt s | s.getControllingExpr() = sizeofExpr)
+  or
+  // another common practice is to use bit-wise operations in sizeof to allow the compiler to
+  // 'pack' the size appropriate but get the size of the result out of a sizeof operation.
+  sizeofExpr.getExprOperand() instanceof BinaryBitwiseOperation
+}
+
+from SizeofExprOperator sizeofExpr, string message, Expr op
+where
+  exists(string tmpMsg |
+    (
+      op instanceof BinaryOperation and tmpMsg = "binary operator"
+      or
+      op instanceof SizeofOperator and tmpMsg = "sizeof"
+    ) and
+    if sizeofExpr.isInMacroExpansion()
+    then message = tmpMsg + "(in a macro expansion)"
+    else message = tmpMsg
+  ) and
+  op = sizeofExpr.getExprOperand() and
+  not isPartOfCrtFloatingPointMacroExpansion(op) and
+  not ignorableSizeof(sizeofExpr)
+select sizeofExpr, "$@: $@ of $@ inside sizeof.", sizeofExpr, message,
+  sizeofExpr.getEnclosingFunction(), "Usage", op, message

cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/ArgumentIsSizeofOrOperationBad.c

@@ -0,0 +1,5 @@
+#define SIZEOF_CHAR sizeof(char)
+
+char* buffer;
+// bug - the code is really going to allocate sizeof(size_t) instead o fthe intended sizeof(char) * 10
+buffer = (char*) malloc(sizeof(SIZEOF_CHAR * 10));

cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/ArgumentIsSizeofOrOperationGood.c

@@ -0,0 +1,4 @@
+#define SIZEOF_CHAR sizeof(char)
+
+char* buffer;
+buffer = (char*) malloc(SIZEOF_CHAR * 10);