Merge pull request #2151 from raulgarciamsft/users/raul/oss

Users/raul/oss

Author: hvitved

cpp/ql/src/Likely Bugs/Memory Management/Buffer Overflow/BufferAccess.qll

@@ -0,0 +1,174 @@
+import cpp
+import semmle.code.cpp.dataflow.TaintTracking
+private import semmle.code.cpp.dataflow.RecursionPrevention
+
+/**
+ * A buffer which includes an allocation size.
+ */
+abstract class BufferWithSize extends DataFlow::Node {
+  abstract Expr getSizeExpr();
+
+  BufferAccess getAnAccess() {
+    any(BufferWithSizeConfig bsc).hasFlow(this, DataFlow::exprNode(result.getPointer()))
+  }
+}
+
+/** An allocation function. */
+abstract class Alloc extends Function { }
+
+/**
+ * Allocation functions identified by the QL for C/C++ standard library.
+ */
+class DefaultAlloc extends Alloc {
+  DefaultAlloc() { allocationFunction(this) }
+}
+
+/** A buffer created through a call to an allocation function. */
+class AllocBuffer extends BufferWithSize {
+  FunctionCall call;
+
+  AllocBuffer() {
+    asExpr() = call and
+    call.getTarget() instanceof Alloc
+  }
+
+  override Expr getSizeExpr() { result = call.getArgument(0) }
+}
+
+/**
+ * Find accesses of buffers for which we have a size expression.
+ */
+private class BufferWithSizeConfig extends TaintTracking::Configuration {
+  BufferWithSizeConfig() { this = "BufferWithSize" }
+
+  override predicate isSource(DataFlow::Node n) { n = any(BufferWithSize b) }
+
+  override predicate isSink(DataFlow::Node n) { n.asExpr() = any(BufferAccess ae).getPointer() }
+
+  override predicate isSanitizer(DataFlow::Node s) {
+    s = any(BufferWithSize b) and
+    s.asExpr().getControlFlowScope() instanceof Alloc
+  }
+}
+
+/**
+ * An access (read or write) to a buffer, provided as a pair of
+ * a pointer to the buffer and the length of data to be read or written.
+ * Extend this class to support different kinds of buffer access.
+ */
+abstract class BufferAccess extends Locatable {
+  /** Gets the pointer to the buffer being accessed. */
+  abstract Expr getPointer();
+
+  /** Gets the length of the data being read or written by this buffer access. */
+  abstract Expr getAccessedLength();
+}
+
+/**
+ * A buffer access through an array expression.
+ */
+class ArrayBufferAccess extends BufferAccess, ArrayExpr {
+  override Expr getPointer() { result = this.getArrayBase() }
+
+  override Expr getAccessedLength() { result = this.getArrayOffset() }
+}
+
+/**
+ * A buffer access through an overloaded array expression.
+ */
+class OverloadedArrayBufferAccess extends BufferAccess, OverloadedArrayExpr {
+  override Expr getPointer() { result = this.getQualifier() }
+
+  override Expr getAccessedLength() { result = this.getAnArgument() }
+}
+
+/**
+ * A buffer access through pointer arithmetic.
+ */
+class PointerArithmeticAccess extends BufferAccess, Expr {
+  PointerArithmeticOperation p;
+
+  PointerArithmeticAccess() {
+    this = p and
+    p.getAnOperand().getType().getUnspecifiedType() instanceof IntegralType and
+    not p.getParent() instanceof ComparisonOperation
+  }
+
+  override Expr getPointer() {
+    result = p.getAnOperand() and
+    result.getType().getUnspecifiedType() instanceof PointerType
+  }
+
+  override Expr getAccessedLength() {
+    result = p.getAnOperand() and
+    result.getType().getUnspecifiedType() instanceof IntegralType
+  }
+}
+
+/**
+ * A pair of buffer accesses through a call to memcpy.
+ */
+class MemCpy extends BufferAccess, FunctionCall {
+  MemCpy() { getTarget().hasName("memcpy") }
+
+  override Expr getPointer() {
+    result = getArgument(0) or
+    result = getArgument(1)
+  }
+
+  override Expr getAccessedLength() { result = getArgument(2) }
+}
+
+class StrncpySizeExpr extends BufferAccess, FunctionCall {
+  StrncpySizeExpr() { getTarget().hasName("strncpy") }
+
+  override Expr getPointer() {
+    result = getArgument(0) or
+    result = getArgument(1)
+  }
+
+  override Expr getAccessedLength() { result = getArgument(2) }
+}
+
+class RecvSizeExpr extends BufferAccess, FunctionCall {
+  RecvSizeExpr() { getTarget().hasName("recv") }
+
+  override Expr getPointer() { result = getArgument(1) }
+
+  override Expr getAccessedLength() { result = getArgument(2) }
+}
+
+class SendSizeExpr extends BufferAccess, FunctionCall {
+  SendSizeExpr() { getTarget().hasName("send") }
+
+  override Expr getPointer() { result = getArgument(1) }
+
+  override Expr getAccessedLength() { result = getArgument(2) }
+}
+
+class SnprintfSizeExpr extends BufferAccess, FunctionCall {
+  SnprintfSizeExpr() { getTarget().hasName("snprintf") }
+
+  override Expr getPointer() { result = getArgument(0) }
+
+  override Expr getAccessedLength() { result = getArgument(1) }
+}
+
+class MemcmpSizeExpr extends BufferAccess, FunctionCall {
+  MemcmpSizeExpr() { getTarget().hasName("Memcmp") }
+
+  override Expr getPointer() {
+    result = getArgument(0) or
+    result = getArgument(1)
+  }
+
+  override Expr getAccessedLength() { result = getArgument(2) }
+}
+
+class MallocSizeExpr extends BufferAccess, FunctionCall {
+  MallocSizeExpr() { getTarget().hasName("malloc") }
+
+  override Expr getPointer() { none() }
+
+  override Expr getAccessedLength() { result = getArgument(1) }
+}

cpp/ql/src/Likely Bugs/Memory Management/Buffer Overflow/NtohlArrayBad.cpp

@@ -0,0 +1,6 @@
+int get_number_from_network();
+
+int process_network(int[] buff, int buffSize) {
+  int i = ntohl(get_number_from_network());
+  return buff[i];
+}

cpp/ql/src/Likely Bugs/Memory Management/Buffer Overflow/NtohlArrayGood.cpp

@@ -0,0 +1,10 @@
+uint32_t get_number_from_network();
+
+int process_network(int[] buff, uint32_t buffSize) {
+  uint32_t i = ntohl(get_number_from_network());
+  if (i < buffSize) {
+    return buff[i];
+  } else {
+    return -1;
+  }
+}

cpp/ql/src/Likely Bugs/Memory Management/Buffer Overflow/NtohlArrayNoBound.qll

@@ -0,0 +1,33 @@
+import cpp
+import semmle.code.cpp.dataflow.DataFlow
+import semmle.code.cpp.controlflow.Guards
+import BufferAccess
+import semmle.code.cpp.valuenumbering.GlobalValueNumbering
+
+class NetworkFunctionCall extends FunctionCall {
+  NetworkFunctionCall() {
+    getTarget().hasName("ntohd") or
+    getTarget().hasName("ntohf") or
+    getTarget().hasName("ntohl") or
+    getTarget().hasName("ntohll") or
+    getTarget().hasName("ntohs")
+  }
+}
+
+class NetworkToBufferSizeConfiguration extends DataFlow::Configuration {
+  NetworkToBufferSizeConfiguration() { this = "NetworkToBufferSizeConfiguration" }
+
+  override predicate isSource(DataFlow::Node node) { node.asExpr() instanceof NetworkFunctionCall }
+
+  override predicate isSink(DataFlow::Node node) {
+    node.asExpr() = any(BufferAccess ba).getAccessedLength()
+  }
+
+  override predicate isBarrier(DataFlow::Node node) {
+    exists(GuardCondition gc, GVN gvn |
+      gc.getAChild*() = gvn.getAnExpr() and
+      globalValueNumber(node.asExpr()) = gvn and
+      gc.controls(node.asExpr().getBasicBlock(), _)
+    )
+  }
+}

cpp/ql/src/Likely Bugs/Memory Management/Buffer Overflow/NtohlArrayNoBoundOpenSource.qhelp

@@ -0,0 +1,54 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>
+Data received over a network connection may be received in a different byte order than
+the byte order used by the local host, making the data difficult to process. To address this,
+data received over the wire is usually converted to host byte order by a call to a network-to-host
+byte order function, such as <code>ntohl</code>.
+</p>
+<p>
+The use of a network-to-host byte order function is therefore a good indicator that the returned
+value is unvalidated data retrieved from the network, and should not be used without further
+validation. In particular, the returned value should not be used as an array index or array length
+value without validation, which may result in a buffer overflow vulnerability.
+</p>
+</overview>
+
+<recommendation>
+<p>
+Validate data returned by network-to-host byte order functions before use and especially before
+using the value as an array index or bound.
+</p>
+</recommendation>
+
+<example>
+<p>In the example below, network data is retrieved and passed to <code>ntohl</code> to convert
+it to host byte order. The data is then used as an index in an array access expression. However,
+there is no validation that the data returned by <code>ntohl</code> is within the bounds of the array,
+which could lead to reading outside the bounds of the buffer.
+</p>
+<sample src="NtohlArrayBad.cpp" />
+<p>In the corrected example, the returned data is validated against the known size of the buffer,
+before being used as an array index.</p>
+<sample src="NtohlArrayGood.cpp" />
+</example>
+
+<references>
+<li>
+  <a href="https://docs.microsoft.com/en-us/windows/desktop/api/winsock/nf-winsock-ntohl">
+    ntohl - winsock reference
+  </a> 
+</li>
+<li>
+  <a href="https://linux.die.net/man/3/ntohl">
+    ntohl - Linux man page
+  </a> 
+</li>
+
+</references>
+
+</qhelp>

cpp/ql/src/Likely Bugs/Memory Management/Buffer Overflow/NtohlArrayNoBoundOpenSource.ql

@@ -0,0 +1,17 @@
+/**
+ * @id cpp/network-to-host-function-as-array-bound
+ * @name Untrusted network-to-host usage
+ * @description Using the result of a network-to-host byte order function, such as ntohl, as an
+ *              array bound or length value without checking it may result in buffer overflows or
+ *              other vulnerabilties.
+ * @kind problem
+ * @problem.severity error
+ */
+
+import cpp
+import NtohlArrayNoBound
+import semmle.code.cpp.dataflow.DataFlow
+
+from NetworkToBufferSizeConfiguration bufConfig, DataFlow::Node source, DataFlow::Node sink
+where bufConfig.hasFlow(source, sink)
+select sink, "Unchecked use of data from network function $@", source, source.toString()