From 1a044a0a22bc36b56a0500bc298bd14e7f690723 Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Fri, 25 Jan 2019 12:55:31 +0000
Subject: [PATCH] CPP: Add 'fread' to BufferAccess.qll.

---
 .../semmle/code/cpp/security/BufferAccess.qll | 26 +++++++++++++++++++
 .../semmle/tests/OverflowBuffer.expected      |  2 ++
 .../CWE/CWE-119/semmle/tests/tests.cpp        |  4 +--
 3 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/cpp/ql/src/semmle/code/cpp/security/BufferAccess.qll b/cpp/ql/src/semmle/code/cpp/security/BufferAccess.qll
index 997c53fd8d0..a06c236b63e 100644
--- a/cpp/ql/src/semmle/code/cpp/security/BufferAccess.qll
+++ b/cpp/ql/src/semmle/code/cpp/security/BufferAccess.qll
@@ -292,6 +292,32 @@ class MemchrBA extends BufferAccess {
   }
 }
 
+/**
+ * Calls to fread.
+ *  fread(buffer, size, number, file)
+ */
+class FreadBA extends BufferAccess {
+  FreadBA() {
+    this.(FunctionCall).getTarget().getName() = "fread"
+  }
+  
+  override string getName() {
+    result = this.(FunctionCall).getTarget().getName()
+  }
+
+  override Expr getBuffer(string bufferDesc, int accessType) {
+    result = this.(FunctionCall).getArgument(0) and
+    bufferDesc = "destination buffer" and
+    accessType = 2
+  }
+
+  override int getSize() {
+    result =
+      this.(FunctionCall).getArgument(1).getValue().toInt() *
+      this.(FunctionCall).getArgument(2).getValue().toInt()
+  }
+}
+
 /**
  * A array access on a buffer:
  *  buffer[ix]
diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/OverflowBuffer.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/OverflowBuffer.expected
index d184b85db16..b88baf5c987 100644
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/OverflowBuffer.expected
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/OverflowBuffer.expected
@@ -54,6 +54,8 @@
 | tests.cpp:491:2:491:7 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:474:21:474:26 | call to malloc | array |
 | tests.cpp:519:3:519:8 | call to memset | This 'memset' operation accesses 20 bytes but the $@ is only 10 bytes. | tests.cpp:502:15:502:20 | call to malloc | destination buffer |
 | tests.cpp:519:3:519:8 | call to memset | This 'memset' operation accesses 20 bytes but the $@ is only 10 bytes. | tests.cpp:510:16:510:21 | call to malloc | destination buffer |
+| tests.cpp:541:6:541:10 | call to fread | This 'fread' operation may access 101 bytes but the $@ is only 100 bytes. | tests.cpp:532:7:532:16 | charBuffer | destination buffer |
+| tests.cpp:546:6:546:10 | call to fread | This 'fread' operation may access 400 bytes but the $@ is only 100 bytes. | tests.cpp:532:7:532:16 | charBuffer | destination buffer |
 | tests_restrict.c:12:2:12:7 | call to memcpy | This 'memcpy' operation accesses 2 bytes but the $@ is only 1 byte. | tests_restrict.c:7:6:7:13 | smallbuf | source buffer |
 | unions.cpp:26:2:26:7 | call to memset | This 'memset' operation accesses 200 bytes but the $@ is only 100 bytes. | unions.cpp:21:10:21:11 | mu | destination buffer |
 | unions.cpp:30:2:30:7 | call to memset | This 'memset' operation accesses 200 bytes but the $@ is only 100 bytes. | unions.cpp:15:7:15:11 | small | destination buffer |
diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/tests.cpp b/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/tests.cpp
index 04913cd97bb..0616da414cb 100644
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/tests.cpp
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/tests.cpp
@@ -538,12 +538,12 @@ void test20()
 		// ...
 	}
 
-	if (fread(charBuffer, sizeof(char), 101, fileSource) > 0) // BAD [NOT DETECTED]
+	if (fread(charBuffer, sizeof(char), 101, fileSource) > 0) // BAD
 	{
 		// ...
 	}
 
-	if (fread(charBuffer, sizeof(int), 100, fileSource) > 0) // BAD [NOT DETECTED]
+	if (fread(charBuffer, sizeof(int), 100, fileSource) > 0) // BAD
 	{
 		// ...
 	}