Merge pull request #824 from geoffw0/fread

CPP: Add 'fread' to BufferAccess.qll

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]

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 |

cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/tests.cpp

@@ -522,6 +522,44 @@ void test19(bool b)
 	}
 }
 
+typedef struct {} FILE;
+FILE *fileSource;
+
+size_t fread(void *ptr, size_t size, size_t nmemb, FILE *stream);
+
+void test20()
+{
+	char charBuffer[100];
+	int intBuffer[100];
+	int num;
+
+	if (fread(charBuffer, sizeof(char), 100, fileSource) > 0) // GOOD
+	{
+		// ...
+	}
+
+	if (fread(charBuffer, sizeof(char), 101, fileSource) > 0) // BAD
+	{
+		// ...
+	}
+
+	if (fread(charBuffer, sizeof(int), 100, fileSource) > 0) // BAD
+	{
+		// ...
+	}
+
+	if (fread(intBuffer, sizeof(int), 100, fileSource) > 0) // GOOD
+	{
+		// ...
+	}
+
+	num = 101;
+	if (fread(intBuffer, sizeof(int), num, fileSource) > 0) // BAD [NOT DETECTED]
+	{
+		// ...
+	}
+}
+
 int main(int argc, char *argv[])
 {
 	long long arr17[19];
@@ -543,6 +581,7 @@ int main(int argc, char *argv[])
 	test17(arr17);
 	test18();
 	test19(argc == 0);
+	test20();
 
 	return 0;
 }