Implement OutOfBounds.qll, ARR38-C, and ARR30-C

Nikita Kraiouchkine · Nikita Kraiouchkine · commit 5f8a6f74b836 · 2023-04-05T21:31:01.000+02:00
diff --git a/c/cert/src/rules/ARR30-C/DoNotFormOutOfBoundsPointersOrArraySubscripts.md b/c/cert/src/rules/ARR30-C/DoNotFormOutOfBoundsPointersOrArraySubscripts.md
@@ -0,0 +1,16 @@
+# ARR30-C: Do not form or use out-of-bounds pointers or array subscripts
+
+This query implements the CERT-C rule ARR30-C:
+
+> Do not form or use out-of-bounds pointers or array subscripts
+## CERT
+
+** REPLACE THIS BY RUNNING THE SCRIPT `scripts/help/cert-help-extraction.py` **
+
+## Implementation notes
+
+None
+
+## References
+
+* CERT-C: [ARR30-C: Do not form or use out-of-bounds pointers or array subscripts](https://wiki.sei.cmu.edu/confluence/display/c)
diff --git a/c/cert/src/rules/ARR30-C/DoNotFormOutOfBoundsPointersOrArraySubscripts.ql b/c/cert/src/rules/ARR30-C/DoNotFormOutOfBoundsPointersOrArraySubscripts.ql
@@ -0,0 +1,43 @@
+/**
+ * @id c/cert/do-not-form-out-of-bounds-pointers-or-array-subscripts
+ * @name ARR30-C: Do not form or use out-of-bounds pointers or array subscripts
+ * @description Forming or using an out-of-bounds pointer is undefined behavior and can result in
+ *              invalid memory accesses.
+ * @kind problem
+ * @precision high
+ * @problem.severity error
+ * @tags external/cert/id/arr30-c
+ *       correctness
+ *       security
+ *       external/cert/obligation/rule
+ */
+
+ import cpp
+ import codingstandards.c.cert
+ import codingstandards.c.OutOfBounds
+ 
+ from
+   OOB::BufferAccess ba, Expr bufferArg, Expr sizeArg, OOB::PointerToObjectSource bufferSource,
+   string message
+ where
+   not isExcluded(ba, OutOfBoundsPackage::doNotFormOutOfBoundsPointersOrArraySubscriptsQuery()) and
+   (
+     exists(int sizeArgValue, int bufferArgSize |
+      OOB::isSizeArgGreaterThanBufferSize(bufferArg, sizeArg, bufferSource, bufferArgSize, sizeArgValue, ba) and
+       message =
+         "Buffer accesses offset " + sizeArgValue + 
+         " which is greater than the fixed size " + bufferArgSize + " of the $@."
+     )
+     or
+     exists(int sizeArgUpperBound, int sizeMult, int bufferArgSize |
+       OOB::isSizeArgNotCheckedLessThanFixedBufferSize(bufferArg, sizeArg, bufferSource,
+         bufferArgSize, ba, sizeArgUpperBound, sizeMult) and
+       message =
+         "Buffer accesses may access up to offset " + sizeArgUpperBound + "*" + sizeMult +
+           " which is greater than the fixed size " + bufferArgSize + " of the $@."
+     )
+     or
+     OOB::isSizeArgNotCheckedGreaterThanZero(bufferArg, sizeArg, bufferSource, ba) and
+     message = "Buffer access may be to a negative index in the buffer."
+   )
+ select ba, message, bufferSource, "buffer"
diff --git a/c/cert/src/rules/ARR38-C/LibraryFunctionArgumentOutOfBounds.ql b/c/cert/src/rules/ARR38-C/LibraryFunctionArgumentOutOfBounds.ql
@@ -1,7 +1,7 @@
 /**
  * @id c/cert/library-function-argument-out-of-bounds
  * @name ARR38-C: Guarantee that library functions do not form invalid pointers
- * @description
+ * @description 
  * @kind problem
  * @precision high
  * @problem.severity error
@@ -21,4 +21,4 @@ from
 where
   not isExcluded(fc, OutOfBoundsPackage::libraryFunctionArgumentOutOfBoundsQuery()) and
   OOB::problems(fc, message, bufferArg, bufferArgStr, sizeOrOtherBufferArg, otherStr)
-select fc, message, bufferArg, bufferArgStr, sizeOrOtherBufferArg, otherStr
+select fc, message, bufferArg, bufferArgStr, sizeOrOtherBufferArg, otherStr
diff --git a/c/cert/test/rules/ARR30-C/DoNotFormOutOfBoundsPointersOrArraySubscripts.expected b/c/cert/test/rules/ARR30-C/DoNotFormOutOfBoundsPointersOrArraySubscripts.expected
@@ -0,0 +1,3 @@
+| test.c:8:3:8:11 | ... + ... | Buffer accesses offset 404 which is greater than the fixed size 400 of the $@. | test.c:8:3:8:5 | arr | buffer |
+| test.c:16:3:16:13 | ... + ... | Buffer access may be to a negative index in the buffer. | test.c:16:3:16:5 | arr | buffer |
+| test.c:21:5:21:15 | ... + ... | Buffer access may be to a negative index in the buffer. | test.c:21:5:21:7 | arr | buffer |
diff --git a/c/cert/test/rules/ARR30-C/DoNotFormOutOfBoundsPointersOrArraySubscripts.qlref b/c/cert/test/rules/ARR30-C/DoNotFormOutOfBoundsPointersOrArraySubscripts.qlref
@@ -0,0 +1 @@
+rules/ARR30-C/DoNotFormOutOfBoundsPointersOrArraySubscripts.ql
diff --git a/c/cert/test/rules/ARR30-C/test.c b/c/cert/test/rules/ARR30-C/test.c
@@ -0,0 +1,35 @@
+
+
+enum { ARRAY_SIZE = 100 };
+
+static int arr[ARRAY_SIZE];
+
+void test_fixed_wrong() {
+  arr + 101; // NON_COMPLIANT
+}
+
+void test_fixed_right() {
+  arr + 2; // COMPLIANT
+}
+
+void test_no_check(int index) {
+  arr + index; // NON_COMPLIANT
+}
+
+void test_invalid_check(int index) {
+  if (index < ARRAY_SIZE) {
+    arr + index; // NON_COMPLIANT - `index` could be negative
+  }
+}
+
+void test_valid_check(int index) {
+  if (index > 0 && index < ARRAY_SIZE) {
+    arr + index; // COMPLIANT - `index` cannot be negative
+  }
+}
+
+void test_valid_check_by_type(unsigned int index) {
+  if (index < ARRAY_SIZE) {
+    arr + index; // COMPLIANT - `index` cannot be be negative
+  }
+}
diff --git a/c/cert/test/rules/ARR38-C/LibraryFunctionArgumentOutOfBounds.expected b/c/cert/test/rules/ARR38-C/LibraryFunctionArgumentOutOfBounds.expected
diff --git a/c/cert/test/rules/ARR38-C/test.c b/c/cert/test/rules/ARR38-C/test.c
@@ -1,16 +1,28 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
+#include <time.h>
 #include <wchar.h>
 
-char *get_ca_5(void) { return malloc(5 * sizeof(char)); }
-
-char *get_ca_5_zeroed(void) {
-  char *p = malloc(5 * sizeof(char));
-  memset(p, 0, 5 * sizeof(char));
+char *get_ca_5(void) {
+  void *ptr = malloc(5 * sizeof(char));
+  memset(ptr, 0, 5 * sizeof(char));
+  return (char *)ptr;
 }
 
-int compare(void *, void *) {}
+int compare(void *a, void *b) {}
+
+void test_strings_loop(void) {
+  char ca5[5] = "test"; // ok
+  char buf5[5] = {0};
+
+  for (int i = 0; i < 5; i++) {
+    strcpy(buf5, ca5);         // COMPLIANT
+    strcpy(buf5 + i, ca5);     // NON_COMPLIANT[FALSE_NEGATIVE]
+    strncpy(buf5, ca5, i);     // COMPLIANT
+    strncpy(buf5, ca5, i + 1); // NON_COMPLIANT[FALSE_NEGATIVE]
+  }
+}
 
 void test_strings(int flow, int unk_size) {
   char ca5_good[5] = "test";  // ok
@@ -95,7 +107,7 @@ void test_strings(int flow, int unk_size) {
   } // COMPLIANT
   if (flow) {
     strncpy(ca6_bad, ca5_good, 6);
-  } // NON_COMPLIANT[FALSE_POSITIVE]
+  } // COMPLIANT
   if (flow) {
     strncpy(ca5_good + 1, ca5_good + 2, 3);
   } // COMPLIANT
@@ -132,7 +144,7 @@ void test_strings(int flow, int unk_size) {
     char buf3[10] = {'\0'};
     char buf4[10] = "12345";
 
-    strcat(buf0, " "); // COMPLIANT[FALSE_NEGATIVE] - not null terminated at
+    strcat(buf0, " "); // NON_COMPLIANT[FALSE_NEGATIVE] - not null terminated at
                        // initialization
 
     memset(buf0, 0, sizeof(buf0)); // COMPLIANT
@@ -156,25 +168,22 @@ void test_strings(int flow, int unk_size) {
     wchar_t buf3[10] = {L'\0'};
     wchar_t buf4[10] = L"12345";
 
-    wcsncat(
-        buf0, L" ",
-        1); // COMPLIANT[FALSE_NEGATIVE] - not null terminated at initialization
+    wcsncat(buf0, L" ",
+            1); // NON_COMPLIANT[FALSE_NEGATIVE] - not null terminated at
+                // initialization
 
     memset(buf0, 0, sizeof(buf0)); // COMPLIANT
     memset(buf2, 0, sizeof(buf2)); // COMPLIANT
 
     wcsncat(buf1, L" ", 1);     // NON_COMPLIANT - not null terminated
     wcsncat(buf2, L" ", 1);     // COMPLIANT
     wcsncat(buf3, L" ", 1);     // COMPLIANT
-    wcsncat(buf4, L"12345", 5); // NON_COMPLIANT
-
-    wcsncat(get_ca_5_zeroed(), L"12345", 5);    // NON_COMPLIANT
-    wcsncat(get_ca_5_zeroed(), L"1234", 4);     // NON_COMPLIANT
-    wcsncat(get_ca_5_zeroed() + 1, L"1234", 4); // NON_COMPLIANT
-    wcsncat(get_ca_5_zeroed(), L"12",
-            2); // NON_COMPLIANT - 4 (bytes) + 2 (null term) copied
-    wcsncat(get_ca_5_zeroed() + 1, L"1",
-            1); // COMPLIANT - 2 (bytes) + 2 (null term) copied
+    wcsncat(buf4, L"12345", 5); // NON_COMPLIANT[FALSE_NEGATIVE]
+
+    wcsncat(get_ca_5(), L"12345", 5);    // NON_COMPLIANT
+    wcsncat(get_ca_5(), L"1234", 4);     // NON_COMPLIANT
+    wcsncat(get_ca_5() + 1, L"1234", 4); // NON_COMPLIANT
+    wcsncat(get_ca_5(), L"12", 2);       // NON_COMPLIANT
   }
 
   // strcmp
@@ -189,7 +198,8 @@ void test_strings(int flow, int unk_size) {
   // strncmp
   if (flow) {
     strncmp(ca5_good, ca5_bad, 4); // COMPLIANT
-    strncmp(ca5_good, ca5_bad, 5); // NON_COMPLIANT
+    strncmp(ca5_good, ca5_bad, 5); // COMPLIANT
+    strncmp(ca5_good, ca5_bad, 6); // NON_COMPLIANT
   }
 }
 
@@ -201,7 +211,7 @@ void test_wrong_buf_size(void) {
     fgets(buf, sizeof(buf), stdin);         // COMPLIANT
     fgets(buf, sizeof(buf) - 1, stdin);     // COMPLIANT
     fgets(buf, sizeof(buf) + 1, stdin);     // NON_COMPLIANT
-    fgets(buf, 0, stdin);                   // NON_COMPLIANT
+    fgets(buf, 0, stdin);                   // COMPLIANT
     fgets(buf + 1, sizeof(buf) - 1, stdin); // COMPLIANT
     fgets(buf + 1, sizeof(buf), stdin);     // NON_COMPLIANT
   }
@@ -213,8 +223,7 @@ void test_wrong_buf_size(void) {
     fgetws(wbuf, sizeof(wbuf) / sizeof(*wbuf), stdin);         // COMPLIANT
     fgetws(wbuf, sizeof(wbuf) / sizeof(*wbuf) - 1, stdin);     // COMPLIANT
     fgetws(wbuf, sizeof(wbuf) / sizeof(*wbuf) + 1, stdin);     // NON_COMPLIANT
-    fgetws(wbuf, 0, stdin);                                    // NON_COMPLIANT
-    fgetws(wbuf + 1, sizeof(wbuf) / sizeof(*wbuf) - 1, stdin); // NON_COMPLIANT
+    fgetws(wbuf, 0, stdin);                                    // COMPLIANT
     fgetws(wbuf + 1, sizeof(wbuf) / sizeof(*wbuf) - 2, stdin); // COMPLIANT
     fgetws(wbuf + 1, sizeof(wbuf) / sizeof(*wbuf), stdin);     // NON_COMPLIANT
   }
@@ -246,7 +255,7 @@ void test_wrong_buf_size(void) {
   // mbtowc
   {
     wchar_t c;
-    char buf[2];
+    char buf[2] = {0};
     mbtowc(&c, buf, sizeof(buf));     // COMPLIANT
     mbtowc(&c, buf, sizeof(buf) - 1); // COMPLIANT
     mbtowc(&c, buf, sizeof(buf) + 1); // NON_COMPLIANT
@@ -255,7 +264,7 @@ void test_wrong_buf_size(void) {
 
   // mblen
   {
-    char buf[3];
+    char buf[3] = {0};
     mblen(buf, sizeof(buf));                   // COMPLIANT
     mblen(buf, sizeof(buf) + 1);               // NON_COMPLIANT
     mblen((char *)malloc(5), sizeof(buf) * 2); // NON_COMPLIANT
@@ -302,10 +311,11 @@ void test_wrong_buf_size(void) {
   {
     char buf[64];
     char buf2[128];
-    strxfrm(buf, "abc", sizeof(buf));        // COMPLIANT
-    strxfrm(buf, "abc", sizeof(buf) + 1);    // NON_COMPLIANT
-    strxfrm(buf, "abc", sizeof(buf) - 1);    // COMPLIANT
-    strxfrm(buf + 1, buf2, sizeof(buf) - 1); // COMPLIANT
+    strxfrm(buf, "abc", sizeof(buf));     // COMPLIANT
+    strxfrm(buf, "abc", sizeof(buf) + 1); // NON_COMPLIANT
+    strxfrm(buf, "abc", sizeof(buf) - 1); // COMPLIANT
+    strxfrm(buf + 1, buf2,
+            sizeof(buf) - 1); // NON_COMPLIANT - not null terminated
   }
 
   // wcsxfrm
diff --git a/c/common/src/codingstandards/c/OutOfBounds.qll b/c/common/src/codingstandards/c/OutOfBounds.qll

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,3 @@`
	`1`	`+\| test.c:8:3:8:11 \| ... + ... \| Buffer accesses offset 404 which is greater than the fixed size 400 of the $@. \| test.c:8:3:8:5 \| arr \| buffer \|`
	`2`	`+\| test.c:16:3:16:13 \| ... + ... \| Buffer access may be to a negative index in the buffer. \| test.c:16:3:16:5 \| arr \| buffer \|`
	`3`	`+\| test.c:21:5:21:15 \| ... + ... \| Buffer access may be to a negative index in the buffer. \| test.c:21:5:21:7 \| arr \| buffer \|`
Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`1`	`+rules/ARR30-C/DoNotFormOutOfBoundsPointersOrArraySubscripts.ql`