diff --git a/.vscode/tasks.json b/.vscode/tasks.json index a2a6dc0750..575dda1e99 100644 --- a/.vscode/tasks.json +++ b/.vscode/tasks.json @@ -249,6 +249,7 @@ "Null", "OperatorInvariants", "Operators", + "OutOfBounds", "Pointers", "Pointers1", "Pointers2", diff --git a/c/cert/src/rules/ARR30-C/DoNotFormOutOfBoundsPointersOrArraySubscripts.md b/c/cert/src/rules/ARR30-C/DoNotFormOutOfBoundsPointersOrArraySubscripts.md new file mode 100644 index 0000000000..221b008786 --- /dev/null +++ b/c/cert/src/rules/ARR30-C/DoNotFormOutOfBoundsPointersOrArraySubscripts.md @@ -0,0 +1,485 @@ +# 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 + + +## Description + +The C Standard identifies the following distinct situations in which undefined behavior (UB) can arise as a result of invalid pointer operations: + +
UB Description Example Code
46 Addition or subtraction of a pointer into, or just beyond, an array object and an integer type produces a result that does not point into, or just beyond, the same array object. Forming Out-of-Bounds Pointer , Null Pointer Arithmetic
47 Addition or subtraction of a pointer into, or just beyond, an array object and an integer type produces a result that points just beyond the array object and is used as the operand of a unary \* operator that is evaluated. Dereferencing Past the End Pointer , Using Past the End Index
49 An array subscript is out of range, even if an object is apparently accessible with the given subscript, for example, in the lvalue expression a\[1\]\[7\] given the declaration int a\[4\]\[5\] ). Apparently Accessible Out-of-Range Index
62 An attempt is made to access, or generate a pointer to just past, a flexible array member of a structure when the referenced object provides no elements for that array. Pointer Past Flexible Array Member
+ + +## Noncompliant Code Example (Forming Out-of-Bounds Pointer) + +In this noncompliant code example, the function `f()` attempts to validate the `index` before using it as an offset to the statically allocated `table` of integers. However, the function fails to reject negative `index` values. When `index` is less than zero, the behavior of the addition expression in the return statement of the function is [undefined behavior 46](https://wiki.sei.cmu.edu/confluence/display/c/CC.+Undefined+Behavior#CC.UndefinedBehavior-ub_46). On some implementations, the addition alone can trigger a hardware trap. On other implementations, the addition may produce a result that when dereferenced triggers a hardware trap. Other implementations still may produce a dereferenceable pointer that points to an object distinct from `table`. Using such a pointer to access the object may lead to information exposure or cause the wrong object to be modified. + +```cpp +enum { TABLESIZE = 100 }; + +static int table[TABLESIZE]; + +int *f(int index) { + if (index < TABLESIZE) { + return table + index; + } + return NULL; +} + +``` + +## Compliant Solution + +One compliant solution is to detect and reject invalid values of `index` if using them in pointer arithmetic would result in an invalid pointer: + +```cpp +enum { TABLESIZE = 100 }; + +static int table[TABLESIZE]; + +int *f(int index) { + if (index >= 0 && index < TABLESIZE) { + return table + index; + } + return NULL; +} + +``` + +## Compliant Solution + +Another slightly simpler and potentially more efficient compliant solution is to use an unsigned type to avoid having to check for negative values while still rejecting out-of-bounds positive values of `index`: + +```cpp +#include + +enum { TABLESIZE = 100 }; + +static int table[TABLESIZE]; + +int *f(size_t index) { + if (index < TABLESIZE) { + return table + index; + } + return NULL; +} + +``` + +## Noncompliant Code Example (Dereferencing Past-the-End Pointer) + +This noncompliant code example shows the flawed logic in the Windows Distributed Component Object Model (DCOM) Remote Procedure Call (RPC) interface that was exploited by the W32.Blaster.Worm. The error is that the `while` loop in the `GetMachineName()` function (used to extract the host name from a longer string) is not sufficiently bounded. When the character array pointed to by `pwszTemp` does not contain the backslash character among the first `MAX_COMPUTERNAME_LENGTH_FQDN + 1` elements, the final valid iteration of the loop will dereference past the end pointer, resulting in exploitable [undefined behavior 47](https://wiki.sei.cmu.edu/confluence/display/c/CC.+Undefined+Behavior#CC.UndefinedBehavior-ub_47). In this case, the actual exploit allowed the attacker to inject executable code into a running program. Economic damage from the Blaster worm has been estimated to be at least $525 million \[[Pethia 2003](https://wiki.sei.cmu.edu/confluence/display/c/AA.+Bibliography#AA.Bibliography-Pethia03)\]. + +For a discussion of this programming error in the Common Weakness Enumeration database, see [CWE-119](http://cwe.mitre.org/data/definitions/119.html), "Improper Restriction of Operations within the Bounds of a Memory Buffer," and [CWE-121](http://cwe.mitre.org/data/definitions/121.html), "Stack-based Buffer Overflow" \[[MITRE 2013](https://wiki.sei.cmu.edu/confluence/display/c/AA.+Bibliography#AA.Bibliography-MITRE)\]. + +```cpp +error_status_t _RemoteActivation( + /* ... */, WCHAR *pwszObjectName, ... ) { + *phr = GetServerPath( + pwszObjectName, &pwszObjectName); + /* ... */ +} + +HRESULT GetServerPath( + WCHAR *pwszPath, WCHAR **pwszServerPath ){ + WCHAR *pwszFinalPath = pwszPath; + WCHAR wszMachineName[MAX_COMPUTERNAME_LENGTH_FQDN+1]; + hr = GetMachineName(pwszPath, wszMachineName); + *pwszServerPath = pwszFinalPath; +} + +HRESULT GetMachineName( + WCHAR *pwszPath, + WCHAR wszMachineName[MAX_COMPUTERNAME_LENGTH_FQDN+1]) +{ + pwszServerName = wszMachineName; + LPWSTR pwszTemp = pwszPath + 2; + while (*pwszTemp != L'\\') + *pwszServerName++ = *pwszTemp++; + /* ... */ +} + +``` + +## Compliant Solution + +In this compliant solution, the `while` loop in the `GetMachineName()` function is bounded so that the loop terminates when a backslash character is found, the null-termination character (`L'\0'`) is discovered, or the end of the buffer is reached. Or, as coded, the while loop continues as long as each character is neither a backslash nor a null character and is not at the end of the buffer. This code does not result in a buffer overflow even if no backslash character is found in `wszMachineName`. + +```cpp +HRESULT GetMachineName( + wchar_t *pwszPath, + wchar_t wszMachineName[MAX_COMPUTERNAME_LENGTH_FQDN+1]) +{ + wchar_t *pwszServerName = wszMachineName; + wchar_t *pwszTemp = pwszPath + 2; + wchar_t *end_addr + = pwszServerName + MAX_COMPUTERNAME_LENGTH_FQDN; + while ((*pwszTemp != L'\\') && + (*pwszTemp != L'\0') && + (pwszServerName < end_addr)) + { + *pwszServerName++ = *pwszTemp++; + } + + /* ... */ +} + +``` +This compliant solution is for illustrative purposes and is not necessarily the solution implemented by Microsoft. This particular solution may not be correct because there is no guarantee that a backslash is found. + +## Noncompliant Code Example (Using Past-the-End Index) + +Similar to the [dereferencing-past-the-end-pointer](https://wiki.sei.cmu.edu/confluence/display/c/ARR30-C.+Do+not+form+or+use+out-of-bounds+pointers+or+array+subscripts#ARR30C.Donotformoruseoutofboundspointersorarraysubscripts-DereferencingPasttheEndPointer) error, the function `insert_in_table()` in this noncompliant code example uses an otherwise valid index to attempt to store a value in an element just past the end of an array. + +First, the function incorrectly validates the index `pos` against the size of the buffer. When `pos` is initially equal to `size`, the function attempts to store `value` in a memory location just past the end of the buffer. + +Second, when the index is greater than `size`, the function modifies `size` before growing the size of the buffer. If the call to `realloc()` fails to increase the size of the buffer, the next call to the function with a value of `pos` equal to or greater than the original value of `size` will again attempt to store `value` in a memory location just past the end of the buffer or beyond. + +Third, the function violates [INT30-C. Ensure that unsigned integer operations do not wrap](https://wiki.sei.cmu.edu/confluence/display/c/INT30-C.+Ensure+that+unsigned+integer+operations+do+not+wrap), which could lead to wrapping when 1 is added to `pos` or when `size` is multiplied by the size of `int`. + +For a discussion of this programming error in the Common Weakness Enumeration database, see [CWE-122](http://cwe.mitre.org/data/definitions/122.html), "Heap-based Buffer Overflow," and [CWE-129](http://cwe.mitre.org/data/definitions/129.html), "Improper Validation of Array Index" \[[MITRE 2013](https://wiki.sei.cmu.edu/confluence/display/c/AA.+Bibliography#AA.Bibliography-MITRE)\]. + +```cpp +#include + +static int *table = NULL; +static size_t size = 0; + +int insert_in_table(size_t pos, int value) { + if (size < pos) { + int *tmp; + size = pos + 1; + tmp = (int *)realloc(table, sizeof(*table) * size); + if (tmp == NULL) { + return -1; /* Failure */ + } + table = tmp; + } + + table[pos] = value; + return 0; +} + +``` + +## Compliant Solution + +This compliant solution correctly validates the index `pos` by using the `<=` relational operator, ensures the multiplication will not overflow, and avoids modifying `size` until it has verified that the call to `realloc()` was successful: + +```cpp +#include +#include + +static int *table = NULL; +static size_t size = 0; + +int insert_in_table(size_t pos, int value) { + if (size <= pos) { + if ((SIZE_MAX - 1 < pos) || + ((pos + 1) > SIZE_MAX / sizeof(*table))) { + return -1; + } + + int *tmp = (int *)realloc(table, sizeof(*table) * (pos + 1)); + if (tmp == NULL) { + return -1; + } + /* Modify size only after realloc() succeeds */ + size = pos + 1; + table = tmp; + } + + table[pos] = value; + return 0; +} + +``` + +## Noncompliant Code Example (Apparently Accessible Out-of-Range Index) + +This noncompliant code example declares `matrix` to consist of 7 rows and 5 columns in row-major order. The function `init_matrix` iterates over all 35 elements in an attempt to initialize each to the value given by the function argument `x`. However, because multidimensional arrays are declared in C in row-major order, the function iterates over the elements in column-major order, and when the value of `j` reaches the value `COLS` during the first iteration of the outer loop, the function attempts to access element `matrix[0][5]`. Because the type of `matrix` is `int[7][5]`, the `j` subscript is out of range, and the access has [undefined behavior 49](https://wiki.sei.cmu.edu/confluence/display/c/CC.+Undefined+Behavior#CC.UndefinedBehavior-ub_49). + +```cpp +#include +#define COLS 5 +#define ROWS 7 +static int matrix[ROWS][COLS]; + +void init_matrix(int x) { + for (size_t i = 0; i < COLS; i++) { + for (size_t j = 0; j < ROWS; j++) { + matrix[i][j] = x; + } + } +} + +``` + +## Compliant Solution + +This compliant solution avoids using out-of-range indices by initializing `matrix` elements in the same row-major order as multidimensional objects are declared in C: + +```cpp +#include +#define COLS 5 +#define ROWS 7 +static int matrix[ROWS][COLS]; + +void init_matrix(int x) { + for (size_t i = 0; i < ROWS; i++) { + for (size_t j = 0; j < COLS; j++) { + matrix[i][j] = x; + } + } +} + +``` + +## Noncompliant Code Example (Pointer Past Flexible Array Member) + +In this noncompliant code example, the function `find()` attempts to iterate over the elements of the flexible array member `buf`, starting with the second element. However, because function `g()` does not allocate any storage for the member, the expression `first++` in `find()` attempts to form a pointer just past the end of `buf` when there are no elements. This attempt is [undefined behavior 62](https://wiki.sei.cmu.edu/confluence/display/c/CC.+Undefined+Behavior#CC.UndefinedBehavior-ub_62). (See [MSC21-C. Use robust loop termination conditions](https://wiki.sei.cmu.edu/confluence/display/c/MSC21-C.+Use+robust+loop+termination+conditions) for more information.) + +```cpp +#include + +struct S { + size_t len; + char buf[]; /* Flexible array member */ +}; + +const char *find(const struct S *s, int c) { + const char *first = s->buf; + const char *last = s->buf + s->len; + + while (first++ != last) { /* Undefined behavior */ + if (*first == (unsigned char)c) { + return first; + } + } + return NULL; +} + +void g(void) { + struct S *s = (struct S *)malloc(sizeof(struct S)); + if (s == NULL) { + /* Handle error */ + } + s->len = 0; + find(s, 'a'); +} +``` + +## Compliant Solution + +This compliant solution avoids incrementing the pointer unless a value past the pointer's current value is known to exist: + +```cpp +#include + +struct S { + size_t len; + char buf[]; /* Flexible array member */ +}; + +const char *find(const struct S *s, int c) { + const char *first = s->buf; + const char *last = s->buf + s->len; + + while (first != last) { /* Avoid incrementing here */ + if (*++first == (unsigned char)c) { + return first; + } + } + return NULL; +} + +void g(void) { + struct S *s = (struct S *)malloc(sizeof(struct S)); + if (s == NULL) { + /* Handle error */ + } + s->len = 0; + find(s, 'a'); +} +``` + +## Noncompliant Code Example (Null Pointer Arithmetic) + +This noncompliant code example is similar to an [Adobe Flash Player vulnerability](http://www.iss.net/threats/289.html) that was first exploited in 2008. This code allocates a block of memory and initializes it with some data. The data does not belong at the beginning of the block, which is left uninitialized. Instead, it is placed `offset` bytes within the block. The function ensures that the data fits within the allocated block. + +```cpp +#include +#include + +char *init_block(size_t block_size, size_t offset, + char *data, size_t data_size) { + char *buffer = malloc(block_size); + if (data_size > block_size || block_size - data_size < offset) { + /* Data won't fit in buffer, handle error */ + } + memcpy(buffer + offset, data, data_size); + return buffer; +} +``` +This function fails to check if the allocation succeeds, which is a violation of [ERR33-C. Detect and handle standard library errors](https://wiki.sei.cmu.edu/confluence/display/c/ERR33-C.+Detect+and+handle+standard+library+errors). If the allocation fails, then `malloc()` returns a null pointer. The null pointer is added to `offset` and passed as the destination argument to `memcpy()`. Because a null pointer does not point to a valid object, the result of the pointer arithmetic is [undefined behavior 46](https://wiki.sei.cmu.edu/confluence/display/c/CC.+Undefined+Behavior#CC.UndefinedBehavior-ub_46). + +An attacker who can supply the arguments to this function can exploit it to execute arbitrary code. This can be accomplished by providing an overly large value for `block_size`, which causes `malloc()` to fail and return a null pointer. The `offset` argument will then serve as the destination address to the call to `memcpy()`. The attacker can specify the `data` and `data_size` arguments to provide the address and length of the address, respectively, that the attacker wishes to write into the memory referenced by `offset`. The overall result is that the call to `memcpy()` can be exploited by an attacker to overwrite an arbitrary memory location with an attacker-supplied address, typically resulting in arbitrary code execution. + +## Compliant Solution (Null Pointer Arithmetic) + +This compliant solution ensures that the call to `malloc()` succeeds: + +```cpp +#include +#include + +char *init_block(size_t block_size, size_t offset, + char *data, size_t data_size) { + char *buffer = malloc(block_size); + if (NULL == buffer) { + /* Handle error */ + } + if (data_size > block_size || block_size - data_size < offset) { + /* Data won't fit in buffer, handle error */ + } + memcpy(buffer + offset, data, data_size); + return buffer; +} + +``` + +## Risk Assessment + +Writing to out-of-range pointers or array subscripts can result in a buffer overflow and the execution of arbitrary code with the permissions of the vulnerable process. Reading from out-of-range pointers or array subscripts can result in unintended information disclosure. + +
Rule Severity Likelihood Remediation Cost Priority Level
ARR30-C High Likely High P9 L2
+ + +## Automated Detection + +
Tool Version Checker Description
Astrée 22.04 array-index-rangearray-index-range-constantnull-dereferencingpointered-deallocation return-reference-local Partially checked Can detect all accesses to invalid pointers as well as array index out-of-bounds accesses and prove their absence. This rule is only partially checked as invalid but unused pointers may not be reported.
Axivion Bauhaus Suite 7.2.0 CertC-ARR30 Can detect out-of-bound access to array / buffer
CodeSonar 7.3p0 LANG.MEM.BO LANG.MEM.BU LANG.MEM.TBA LANG.MEM.TO LANG.MEM.TULANG.STRUCT.PARITH LANG.STRUCT.PBB LANG.STRUCT.PPE BADFUNC.BO.\* Buffer overrun Buffer underrun Tainted buffer access Type overrun Type underrun Pointer Arithmetic Pointer before beginning of object Pointer past end of object A collection of warning classes that report uses of library functions prone to internal buffer overflows.
Compass/ROSE Could be configured to catch violations of this rule. The way to catch the noncompliant code example is to first hunt for example code that follows this pattern: for (LPWSTR pwszTemp = pwszPath + 2; \*pwszTemp != L'\\\\'; \*pwszTemp++;) In particular, the iteration variable is a pointer, it gets incremented, and the loop condition does not set an upper bound on the pointer. Once this case is handled, ROSE can handle cases like the real noncompliant code example, which is effectively the same semantics, just different syntax
Coverity 2017.07 OVERRUN NEGATIVE_RETURNS ARRAY_VS_SINGLETON BUFFER_SIZE Can detect the access of memory past the end of a memory buffer/array Can detect when the loop bound may become negative Can detect the out-of-bound read/write to array allocated statically or dynamically Can detect buffer overflows
Cppcheck 1.66 arrayIndexOutOfBounds, outOfBounds, negativeIndex, arrayIndexThenCheck, arrayIndexOutOfBoundsCond, possibleBufferAccessOutOfBounds Context sensitive analysis of array index, pointers, etc. Array index out of bounds Buffer overflow when calling various functions memset,strcpy,.. Warns about condition (a\[i\] == 0 && i < unknown_value) and recommends that (i < unknown_value && a\[i\] == 0) is used instead Detects unsafe code when array is accessed before/after it is tested if the array index is out of bounds
Helix QAC 2023.1 C2840 DF2820, DF2821, DF2822, DF2823, DF2840, DF2841, DF2842, DF2843, DF2930, DF2931, DF2932, DF2933, DF2935, DF2936, DF2937, DF2938, DF2950, DF2951, DF2952, DF2953
Klocwork 2023.1 ABV.GENERAL ABV.GENERAL.MULTIDIMENSION NPD.FUNC.CALL.MIGHT ABV.ANY_SIZE_ARRAY ABV.STACK ABV.TAINTED ABV.UNICODE.BOUND_MAP ABV.UNICODE.FAILED_MAP ABV.UNICODE.NNTS_MAP ABV.UNICODE.SELF_MAP ABV.UNKNOWN_SIZE NNTS.MIGHT NNTS.MUST NNTS.TAINTED SV.TAINTED.INDEX_ACCESS SV.TAINTED.LOOP_BOUND
LDRA tool suite 9.7.1 45 D, 47 S, 476 S, 489 S, 64 X, 66 X, 68 X, 69 X, 70 X, 71 X , 79 X Partially implemented
Parasoft C/C++test 2022.2 CERT_C-ARR30-a Avoid accessing arrays out of bounds
Parasoft Insure++ Runtime analysis
PC-lint Plus 1.4 413, 415, 416, 613, 661, 662, 676 Fully supported
Polyspace Bug Finder R2023a CERT C: Rule ARR30-C Checks for: Array access out of boundsrray access out of bounds, pointer access out of boundsointer access out of bounds, array access with tainted indexrray access with tainted index, use of tainted pointerse of tainted pointer, pointer dereference with tainted offsetointer dereference with tainted offset. Rule partially covered.
PRQA QA-C 9.7 2820, 2821, 2822, 2823, 2840, 2841, 2842, 2843, 2930, 2931, 2932, 2933, 2935, 2936, 2937, 2938, 2950, 2951, 2952, 2953 Partially implemented
PRQA QA-C++ 4.4 2820, 2821, 2822, 2823, 2840, 2841, 2842, 2843, 2930, 2931, 2932, 2933, 2935, 2936, 2937, 2938, 2950, 2951, 2952, 2953 Partially implemented
PVS-Studio 7.24 V512 , V557 , V582 , V594 , V643 , V645 , V694, V1086
RuleChecker 22.04 array-index-range-constantreturn-reference-local Partially checked
TrustInSoft Analyzer 1.38 index_in_address Exhaustively verified (see one compliant and one non-compliant example ).
+ + +## Related Vulnerabilities + +[CVE-2008-1517](http://web.nvd.nist.gov/view/vuln/detail?vulnId=CVE-2008-1517) results from a violation of this rule. Before Mac OSX version 10.5.7, the XNU kernel accessed an array at an unverified user-input index, allowing an attacker to execute arbitrary code by passing an index greater than the length of the array and therefore accessing outside memory \[[xorl 2009](http://xorl.wordpress.com/2009/06/09/cve-2008-1517-apple-mac-os-x-xnu-missing-array-index-validation/)\]. + +Search for [vulnerabilities](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-vulnerability) resulting from the violation of this rule on the [CERT website](https://www.kb.cert.org/vulnotes/bymetric?searchview&query=FIELD+KEYWORDS+contains+ARR30-C). + +## Related Guidelines + +[Key here](https://wiki.sei.cmu.edu/confluence/display/c/How+this+Coding+Standard+is+Organized#HowthisCodingStandardisOrganized-RelatedGuidelines) (explains table format and definitions) + +
Taxonomy Taxonomy item Relationship
ISO/IEC TR 24772:2013 Arithmetic Wrap-Around Error \[FIF\] Prior to 2018-01-12: CERT: Unspecified Relationship
ISO/IEC TR 24772:2013 Unchecked Array Indexing \[XYZ\] Prior to 2018-01-12: CERT: Unspecified Relationship
ISO/IEC TS 17961 Forming or using out-of-bounds pointers or array subscripts \[invptr\] Prior to 2018-01-12: CERT: Unspecified Relationship
CWE 2.11 CWE-119 , Improper Restriction of Operations within the Bounds of a Memory Buffer 2017-05-18: CERT: Rule subset of CWE
CWE 2.11 CWE-123 , Write-what-where Condition 2017-05-18: CERT: Partial overlap
CWE 2.11 CWE-125 , Out-of-bounds Read 2017-05-18: CERT: Partial overlap
MISRA C:2012 Rule 18.1 (required) Prior to 2018-01-12: CERT: Unspecified Relationship
+ + +## CERT-CWE Mapping Notes + +[Key here](https://wiki.sei.cmu.edu/confluence/pages/viewpage.action?pageId=87152408#HowthisCodingStandardisOrganized-CERT-CWEMappingNotes) for mapping notes + +**CWE-119 and ARR30-C** + +Independent( ARR30-C, ARR38-C, ARR32-C, INT30-C, INT31-C, EXP39-C, EXP33-C, FIO37-C) + +STR31-C = Subset( Union( ARR30-C, ARR38-C)) + +STR32-C = Subset( ARR38-C) + +CWE-119 = Union( ARR30-C, ARR38-C) + +Intersection( ARR30-C, ARR38-C) = Ø + +**CWE-394 and ARR30-C** + +Intersection( ARR30-C, CWE-394) = Ø + +CWE-394 deals with potentially-invalid function return values. Which may be used as an (invalid) array index, but validating the return value is a separate operation. + +**CWE-125 and ARR30-C** + +Independent( ARR30-C, ARR38-C, EXP39-C, INT30-C) + +STR31-C = Subset( Union( ARR30-C, ARR38-C)) + +STR32-C = Subset( ARR38-C) + +CWE-125 = Subset( CWE-119) = Union( ARR30-C, ARR38-C) + +Intersection( ARR30-C, CWE-125) = + +* Reading from an out-of-bounds array index, or off the end of an array +ARR30-C – CWE-125 = +* Writing to an out-of-bounds array index, or off the end of an array +CWE-125 – ARR30-C = +* Reading beyond a non-array buffer +* Using a library function to achieve an out-of-bounds read. +**CWE-123 and ARR30-C** + +Independent(ARR30-C, ARR38-C) + +STR31-C = Subset( Union( ARR30-C, ARR38-C)) + +STR32-C = Subset( ARR38-C) + +Intersection( CWE-123, ARR30-C) = + +* Write of arbitrary value to arbitrary (probably invalid) array index +ARR30-C – CWE-123 = +* Read of value from arbitrary (probably invalid) array index +* Construction of invalid index (pointer arithmetic) +CWE-123 – ARR30-C = +* Arbitrary writes that do not involve directly constructing an invalid array index +**CWE-129 and ARR30-C** + +Independent( ARR30-C, ARR32-C, INT31-C, INT32-C) + +ARR30-C = Union( CWE-129, list), where list = + +* Dereferencing an out-of-bounds array index, where index is a trusted value +* Forming an out-of-bounds array index, without dereferencing it, whether or not index is a trusted value. (This excludes the array’s TOOFAR index, which is one past the final element; this behavior is well-defined in C11.) +**CWE-120 and ARR30-C** + +See CWE-120 and MEM35-C + +**CWE-122 and ARR30-C** + +Intersection( ARR30-C, CWE-122) = Ø + +CWE-122 specifically addresses buffer overflows on the heap operations, which occur in the context of string-copying. ARR30 specifically addresses improper creation or references of array indices. Which might happen as part of a heap buffer overflow, but is on a lower programming level. + +**CWE-20 and ARR30-C** + +See CWE-20 and ERR34-C + +**CWE-687 and ARR30-C** + +Intersection( CWE-687, ARR30-C) = Ø + +ARR30-C is about invalid array indices which are created through pointer arithmetic, and dereferenced through an operator (\* or \[\]). Neither involve function calls, thus CWE-687 does not apply. + +**CWE-786 and ARR30-C** + +ARR30-C = Union( CWE-786, list) where list = + +* Access of memory location after end of buffer +* Construction of invalid arry reference (pointer). This does not include an out-of-bounds array index (an integer). +**CWE-789 and ARR30-C** + +Intersection( CWE-789, ARR30-C) = Ø + +CWE-789 is about allocating memory, not array subscripting + +## Bibliography + +
\[ Finlay 2003 \]
\[ Microsoft 2003 \]
\[ Pethia 2003 \]
\[ Seacord 2013b \] Chapter 1, "Running with Scissors"
\[ Viega 2005 \] Section 5.2.13, "Unchecked Array Indexing"
\[ xorl 2009 \] "CVE-2008-1517: Apple Mac OS X (XNU) Missing Array Index Validation"
+ + +## 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 new file mode 100644 index 0000000000..332928c240 --- /dev/null +++ b/c/cert/src/rules/ARR30-C/DoNotFormOutOfBoundsPointersOrArraySubscripts.ql @@ -0,0 +1,52 @@ +/** + * @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 medium + * @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 + // exclude loops + not exists(Loop loop | loop.getStmt().getChildStmt*() = ba.getEnclosingStmt()) and + // exclude size arguments that are of type ssize_t + not sizeArg.getAChild*().(VariableAccess).getTarget().getType() instanceof Ssize_t and + // exclude size arguments that are assigned the result of a function call e.g. ftell + not sizeArg.getAChild*().(VariableAccess).getTarget().getAnAssignedValue() instanceof FunctionCall and + // exclude field or array accesses for the size arguments + not sizeArg.getAChild*() instanceof FieldAccess and + not sizeArg.getAChild*() instanceof ArrayExpr 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 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" \ No newline at end of file diff --git a/c/cert/src/rules/ARR38-C/LibraryFunctionArgumentOutOfBounds.md b/c/cert/src/rules/ARR38-C/LibraryFunctionArgumentOutOfBounds.md new file mode 100644 index 0000000000..c3306036d2 --- /dev/null +++ b/c/cert/src/rules/ARR38-C/LibraryFunctionArgumentOutOfBounds.md @@ -0,0 +1,486 @@ +# ARR38-C: Guarantee that library functions do not form invalid pointers + +This query implements the CERT-C rule ARR38-C: + +> Guarantee that library functions do not form invalid pointers + + +## Description + +C library functions that make changes to arrays or objects take at least two arguments: a pointer to the array or object and an integer indicating the number of elements or bytes to be manipulated. For the purposes of this rule, the element count of a pointer is the size of the object to which it points, expressed by the number of elements that are valid to access. Supplying arguments to such a function might cause the function to form a pointer that does not point into or just past the end of the object, resulting in [undefined behavior](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-undefinedbehavior). + +Annex J of the C Standard \[[ISO/IEC 9899:2011](https://wiki.sei.cmu.edu/confluence/display/c/AA.+Bibliography#AA.Bibliography-ISO%2FIEC9899-2011)\] states that it is undefined behavior if the "pointer passed to a library function array parameter does not have a value such that all address computations and object accesses are valid." (See [undefined behavior 109](https://wiki.sei.cmu.edu/confluence/display/c/CC.+Undefined+Behavior#CC.UndefinedBehavior-ub_109).) + +In the following code, + +```cpp +int arr[5]; +int *p = arr; + +unsigned char *p2 = (unsigned char *)arr; +unsigned char *p3 = arr + 2; +void *p4 = arr; +``` +the element count of the pointer `p` is `sizeof(arr) / sizeof(arr[0])`, that is, `5`. The element count of the pointer `p2` is `sizeof(arr)`, that is, `20`, on implementations where `sizeof(int) == 4`. The element count of the pointer `p3` is `12` on implementations where `sizeof(int) == 4`, because `p3` points two elements past the start of the array `arr`. The element count of `p4` is treated as though it were `unsigned char *` instead of `void *`, so it is the same as `p2`. + +## Pointer + Integer + +The following standard library functions take a pointer argument and a size argument, with the constraint that the pointer must point to a valid memory object of at least the number of elements indicated by the size argument. + +
fgets() fgetws() mbstowcs() 1 wcstombs() 1
mbrtoc16() 2 mbrtoc32() 2 mbsrtowcs() 1 wcsrtombs() 1
mbtowc() 2 mbrtowc() 2 mblen() mbrlen()
memchr() wmemchr() memset() wmemset()
strftime() wcsftime() strxfrm()1 wcsxfrm()1
strncat()2 wcsncat()2 snprintf() vsnprintf()
swprintf() vswprintf() setvbuf() tmpnam_s()
snprintf_s() sprintf_s() vsnprintf_s() vsprintf_s()
gets_s() getenv_s() wctomb_s() mbstowcs_s()3
wcstombs_s()3 memcpy_s()3 memmove_s()3 strncpy_s()3
strncat_s()3 strtok_s()2 strerror_s() strnlen_s()
asctime_s() ctime_s() snwprintf_s() swprintf_s()
vsnwprintf_s() vswprintf_s() wcsncpy_s()3 wmemcpy_s()3
wmemmove_s()3 wcsncat_s()3 wcstok_s()2 wcsnlen_s()
wcrtomb_s() mbsrtowcs_s()3 wcsrtombs_s()3 memset_s()4
+1 Takes two pointers and an integer, but the integer specifies the element count only of the output buffer, not of the input buffer.2 Takes two pointers and an integer, but the integer specifies the element count only of the input buffer, not of the output buffer.3 Takes two pointers and two integers; each integer corresponds to the element count of one of the pointers.4 Takes a pointer and two size-related integers; the first size-related integer parameter specifies the number of bytes available in the buffer; the second size-related integer parameter specifies the number of bytes to write within the buffer. + + +For calls that take a pointer and an integer size, the given size should not be greater than the element count of the pointer. + +** Noncompliant Code Example (Element Count)** + +In this noncompliant code example, the incorrect element count is used in a call to `wmemcpy()`. The `sizeof` operator returns the size expressed in bytes, but `wmemcpy()` uses an element count based on `wchar_t *`. + +```cpp +#include +#include + +static const char str[] = "Hello world"; +static const wchar_t w_str[] = L"Hello world"; +void func(void) { + char buffer[32]; + wchar_t w_buffer[32]; + memcpy(buffer, str, sizeof(str)); /* Compliant */ + wmemcpy(w_buffer, w_str, sizeof(w_str)); /* Noncompliant */ +} +``` +**Compliant Solution (Element Count)** + +When using functions that operate on pointed-to regions, programmers must always express the integer size in terms of the element count expected by the function. For example, `memcpy()` expects the element count expressed in terms of `void *`, but `wmemcpy()` expects the element count expressed in terms of `wchar_t *`. Instead of the `sizeof` operator, functions that return the number of elements in the string are called, which matches the expected element count for the copy functions. In the case of this compliant solution, where the argument is an array `A` of type `T`, the expression `sizeof(A) / sizeof(T)`, or equivalently `sizeof(A) / sizeof(*A)`, can be used to compute the number of elements in the array. + +```cpp +#include +#include + +static const char str[] = "Hello world"; +static const wchar_t w_str[] = L"Hello world"; +void func(void) { + char buffer[32]; + wchar_t w_buffer[32]; + memcpy(buffer, str, strlen(str) + 1); + wmemcpy(w_buffer, w_str, wcslen(w_str) + 1); +} +``` +**Noncompliant Code Example (Pointer + Integer)** + +This noncompliant code example assigns a value greater than the number of bytes of available memory to `n`, which is then passed to `memset()`: + +```cpp +#include +#include + +void f1(size_t nchars) { + char *p = (char *)malloc(nchars); + /* ... */ + const size_t n = nchars + 1; + /* ... */ + memset(p, 0, n); +} + +``` +**Compliant Solution (Pointer + Integer)** + +This compliant solution ensures that the value of `n` is not greater than the number of bytes of the dynamic memory pointed to by the pointer `p`: + +```cpp +#include +#include + +void f1(size_t nchars) { + char *p = (char *)malloc(nchars); + /* ... */ + const size_t n = nchars; + /* ... */ + memset(p, 0, n); +} + +``` +**Noncompliant Code Example (Pointer + Integer)** + +In this noncompliant code example, the element count of the array `a` is `ARR_SIZE` elements. Because `memset()` expects a byte count, the size of the array is scaled incorrectly by `sizeof(int)` instead of `sizeof(long)`, which can form an invalid pointer on architectures where `sizeof(int) != sizeof(long)`. + +```cpp +#include + +void f2(void) { + const size_t ARR_SIZE = 4; + long a[ARR_SIZE]; + const size_t n = sizeof(int) * ARR_SIZE; + void *p = a; + + memset(p, 0, n); +} + +``` +**Compliant Solution (Pointer + Integer)** + +In this compliant solution, the element count required by `memset()` is properly calculated without resorting to scaling: + +```cpp +#include + +void f2(void) { + const size_t ARR_SIZE = 4; + long a[ARR_SIZE]; + const size_t n = sizeof(a); + void *p = a; + + memset(p, 0, n); +} + +``` + +## Two Pointers + One Integer + +The following standard library functions take two pointer arguments and a size argument, with the constraint that both pointers must point to valid memory objects of at least the number of elements indicated by the size argument. + +
memcpy() wmemcpy() memmove() wmemmove()
strncpy() wcsncpy() memcmp() wmemcmp()
strncmp() wcsncmp() strcpy_s() wcscpy_s()
strcat_s() wcscat_s()
+For calls that take two pointers and an integer size, the given size should not be greater than the element count of either pointer. + + +**Noncompliant Code Example (Two Pointers + One Integer)** + +In this noncompliant code example, the value of `n` is incorrectly computed, allowing a read past the end of the object referenced by `q`: + +```cpp +#include + +void f4() { + char p[40]; + const char *q = "Too short"; + size_t n = sizeof(p); + memcpy(p, q, n); +} +``` +**Compliant Solution (Two Pointers + One Integer)** + +This compliant solution ensures that `n` is equal to the size of the character array: + +```cpp +#include + +void f4() { + char p[40]; + const char *q = "Too short"; + size_t n = sizeof(p) < strlen(q) + 1 ? sizeof(p) : strlen(q) + 1; + memcpy(p, q, n); +} +``` + +## One Pointer + Two Integers + +The following standard library functions take a pointer argument and two size arguments, with the constraint that the pointer must point to a valid memory object containing at least as many bytes as the product of the two size arguments. + +
bsearch() bsearch_s() qsort() qsort_s()
fread() fwrite()
+For calls that take a pointer and two integers, one integer represents the number of bytes required for an individual object, and a second integer represents the number of elements in the array. The resulting product of the two integers should not be greater than the element count of the pointer were it expressed as an `unsigned char *`. + + +**Noncompliant Code Example (One Pointer + Two Integers)** + +This noncompliant code example allocates a variable number of objects of type `struct obj`. The function checks that `num_objs` is small enough to prevent wrapping, in compliance with [INT30-C. Ensure that unsigned integer operations do not wrap](https://wiki.sei.cmu.edu/confluence/display/c/INT30-C.+Ensure+that+unsigned+integer+operations+do+not+wrap). The size of `struct obj` is assumed to be 16 bytes to account for padding to achieve the assumed alignment of `long long`. However, the padding typically depends on the target architecture, so this object size may be incorrect, resulting in an incorrect element count. + +```cpp +#include +#include + +struct obj { + char c; + long long i; +}; + +void func(FILE *f, struct obj *objs, size_t num_objs) { + const size_t obj_size = 16; + if (num_objs > (SIZE_MAX / obj_size) || + num_objs != fwrite(objs, obj_size, num_objs, f)) { + /* Handle error */ + } +} +``` +**Compliant Solution (One Pointer + Two Integers)** + +This compliant solution uses the `sizeof` operator to correctly provide the object size and `num_objs` to provide the element count: + +```cpp +#include +#include + +struct obj { + char c; + long long i; +}; + +void func(FILE *f, struct obj *objs, size_t num_objs) { + const size_t obj_size = sizeof *objs; + if (num_objs > (SIZE_MAX / obj_size) || + num_objs != fwrite(objs, obj_size, num_objs, f)) { + /* Handle error */ + } +} +``` +**Noncompliant Code Example (One Pointer + Two Integers)** + +In this noncompliant code example, the function `f()` calls `fread()` to read `nitems` of type `wchar_t`, each `size` bytes in size, into an array of `BUFFER_SIZE` elements, `wbuf`. However, the expression used to compute the value of `nitems` fails to account for the fact that, unlike the size of `char`, the size of `wchar_t` may be greater than 1. Consequently, `fread()` could attempt to form pointers past the end of `wbuf` and use them to assign values to nonexistent elements of the array. Such an attempt is [undefined behavior](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-undefinedbehavior). (See [undefined behavior 109](https://wiki.sei.cmu.edu/confluence/display/c/CC.+Undefined+Behavior#CC.UndefinedBehavior-ub_109).) A likely consequence of this undefined behavior is a buffer overflow. For a discussion of this programming error in the Common Weakness Enumeration database, see [CWE-121](http://cwe.mitre.org/data/definitions/121.html), "Stack-based Buffer Overflow," and [CWE-805](http://cwe.mitre.org/data/definitions/805.html), "Buffer Access with Incorrect Length Value." + +```cpp +#include +#include + +void f(FILE *file) { + enum { BUFFER_SIZE = 1024 }; + wchar_t wbuf[BUFFER_SIZE]; + + const size_t size = sizeof(*wbuf); + const size_t nitems = sizeof(wbuf); + + size_t nread = fread(wbuf, size, nitems, file); + /* ... */ +} + +``` +**Compliant Solution (One Pointer + Two Integers)** + +This compliant solution correctly computes the maximum number of items for `fread()` to read from the file: + +```cpp +#include +#include + +void f(FILE *file) { + enum { BUFFER_SIZE = 1024 }; + wchar_t wbuf[BUFFER_SIZE]; + + const size_t size = sizeof(*wbuf); + const size_t nitems = sizeof(wbuf) / size; + + size_t nread = fread(wbuf, size, nitems, file); + /* ... */ +} +``` +**Noncompliant Code Example (Heartbleed)** + +CERT vulnerability [720951](http://www.kb.cert.org/vuls/id/720951) describes a vulnerability in OpenSSL versions 1.0.1 through 1.0.1f, popularly known as "Heartbleed." This vulnerability allows an attacker to steal information that under normal conditions would be protected by Secure Socket Layer/Transport Layer Security (SSL/TLS) encryption. + +Despite the seriousness of the vulnerability, Heartbleed is the result of a common programming error and an apparent lack of awareness of secure coding principles. Following is the vulnerable code: + +```cpp +int dtls1_process_heartbeat(SSL *s) { + unsigned char *p = &s->s3->rrec.data[0], *pl; + unsigned short hbtype; + unsigned int payload; + unsigned int padding = 16; /* Use minimum padding */ + + /* Read type and payload length first */ + hbtype = *p++; + n2s(p, payload); + pl = p; + + /* ... More code ... */ + + if (hbtype == TLS1_HB_REQUEST) { + unsigned char *buffer, *bp; + int r; + + /* + * Allocate memory for the response; size is 1 byte + * message type, plus 2 bytes payload length, plus + * payload, plus padding. + */ + buffer = OPENSSL_malloc(1 + 2 + payload + padding); + bp = buffer; + + /* Enter response type, length, and copy payload */ + *bp++ = TLS1_HB_RESPONSE; + s2n(payload, bp); + memcpy(bp, pl, payload); + + /* ... More code ... */ + } + /* ... More code ... */ +} +``` +This code processes a "heartbeat" packet from a client. As specified in [RFC 6520](https://tools.ietf.org/html/rfc6520), when the program receives a heartbeat packet, it must echo the packet's data back to the client. In addition to the data, the packet contains a length field that conventionally indicates the number of bytes in the packet data, but there is nothing to prevent a malicious packet from lying about its data length. + +The `p` pointer, along with `payload` and `p1`, contains data from a packet. The code allocates a `buffer` sufficient to contain `payload` bytes, with some overhead, then copies `payload` bytes starting at `p1` into this buffer and sends it to the client. Notably absent from this code are any checks that the payload integer variable extracted from the heartbeat packet corresponds to the size of the packet data. Because the client can specify an arbitrary value of `payload`, an attacker can cause the server to read and return the contents of memory beyond the end of the packet data, which violates [INT04-C. Enforce limits on integer values originating from tainted sources](https://wiki.sei.cmu.edu/confluence/display/c/INT04-C.+Enforce+limits+on+integer+values+originating+from+tainted+sources). The resulting call to `memcpy()` can then copy the contents of memory past the end of the packet data and the packet itself, potentially exposing sensitive data to the attacker. This call to `memcpy()` violates [ARR38-C. Guarantee that library functions do not form invalid pointers](https://wiki.sei.cmu.edu/confluence/display/c/ARR38-C.+Guarantee+that+library+functions+do+not+form+invalid+pointers). A version of ARR38-C also appears in [ISO/IEC TS 17961:2013](https://wiki.sei.cmu.edu/confluence/display/c/AA.+Bibliography#AA.Bibliography-ISO-IECTS17961), "Forming invalid pointers by library functions \[libptr\]." This rule would require a conforming analyzer to diagnose the Heartbleed vulnerability. + +**Compliant Solution (Heartbleed)** + +OpenSSL version 1.0.1g contains the following patch, which guarantees that `payload` is within a valid range. The range is limited by the size of the input record. + +```cpp +int dtls1_process_heartbeat(SSL *s) { + unsigned char *p = &s->s3->rrec.data[0], *pl; + unsigned short hbtype; + unsigned int payload; + unsigned int padding = 16; /* Use minimum padding */ + + /* ... More code ... */ + + /* Read type and payload length first */ + if (1 + 2 + 16 > s->s3->rrec.length) + return 0; /* Silently discard */ + hbtype = *p++; + n2s(p, payload); + if (1 + 2 + payload + 16 > s->s3->rrec.length) + return 0; /* Silently discard per RFC 6520 */ + pl = p; + + /* ... More code ... */ + + if (hbtype == TLS1_HB_REQUEST) { + unsigned char *buffer, *bp; + int r; + + /* + * Allocate memory for the response; size is 1 byte + * message type, plus 2 bytes payload length, plus + * payload, plus padding. + */ + buffer = OPENSSL_malloc(1 + 2 + payload + padding); + bp = buffer; + /* Enter response type, length, and copy payload */ + *bp++ = TLS1_HB_RESPONSE; + s2n(payload, bp); + memcpy(bp, pl, payload); + /* ... More code ... */ + } + /* ... More code ... */ +} +``` + +## Risk Assessment + +Depending on the library function called, an attacker may be able to use a heap or stack overflow [vulnerability](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-vulnerability) to run arbitrary code. + +
Rule Severity Likelihood Remediation Cost Priority Level
ARR38-C High Likely Medium P18 L1
+ + +## Automated Detection + +
Tool Version Checker Description
Astrée 22.04 array_out_of_bounds Supported Astrée reports all out-of-bound accesses within library analysis stubs. The user may provide additional stubs for arbitrary (library) functions.
CodeSonar 7.2p0 LANG.MEM.BO LANG.MEM.BU BADFUNC.BO.\* Buffer overrun Buffer underrun A collection of warning classes that report uses of library functions prone to internal buffer overflows
Compass/ROSE
Coverity 2017.07 BUFFER_SIZE BAD_SIZEOF BAD_ALLOC_STRLEN BAD_ALLOC_ARITHMETIC Implemented
Fortify SCA 5.0 Can detect violations of this rule with CERT C Rule Pack
Helix QAC 2022.4 C2840 DF2840, DF2841, DF2842, DF2843, DF2845, DF2846, DF2847, DF2848, DF2935, DF2936, DF2937, DF2938, DF4880, DF4881, DF4882, DF4883
Klocwork 2022.4 ABV.GENERALABV.GENERAL.MULTIDIMENSION
LDRA tool suite 9.7.1 64 X, 66 X, 68 X, 69 X, 70 X, 71 X, 79 X Partially Implmented
Parasoft C/C++test 2022.2 CERT_C-ARR38-a CERT_C-ARR38-b CERT_C-ARR38-c CERT_C-ARR38-d Avoid overflow when reading from a buffer Avoid overflow when writing to a buffer Avoid buffer overflow due to defining incorrect format limits Avoid overflow due to reading a not zero terminated string
Parasoft Insure++ Runtime analysis
PC-lint Plus 1.4 419, 420 Partially supported
Polyspace Bug Finder R2023a CERT C: Rule ARR38-C Checks for: Mismatch between data length and sizeismatch between data length and size, invalid use of standard library memory routinenvalid use of standard library memory routine, possible misuse of sizeofossible misuse of sizeof, buffer overflow from incorrect string format specifieruffer overflow from incorrect string format specifier, invalid use of standard library string routinenvalid use of standard library string routine, destination buffer overflow in string manipulationestination buffer overflow in string manipulation, destination buffer underflow in string manipulationestination buffer underflow in string manipulation. Rule partially covered.
PRQA QA-C 9.7 2840, 2841, 2842, 2843, 2845, 2846, 2847, 2848, 2935, 2936, 2937, 2938 Fully implemented
PRQA QA-C++ 4.4 2840, 2841, 2842, 2843, 2845, 2846, 2847, 2848, 2935, 2936, 2937, 2938 Fully implemented
Splint 3.1.1
TrustInSoft Analyzer 1.38 out of bounds read Partially verified.
+ + +## Related Vulnerabilities + +[CVE-2016-2208](https://bugs.chromium.org/p/project-zero/issues/detail?id=820) results from a violation of this rule. The attacker can supply a value used to determine how much data is copied into a buffer via `memcpy()`, resulting in a buffer overlow of attacker-controlled data. + +Search for [vulnerabilities](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-vulnerability) resulting from the violation of this rule on the [CERT website](https://www.kb.cert.org/vulnotes/bymetric?searchview&query=FIELD+KEYWORDS+contains+ARR38-C). + +## Related Guidelines + +[Key here](https://wiki.sei.cmu.edu/confluence/display/c/How+this+Coding+Standard+is+Organized#HowthisCodingStandardisOrganized-RelatedGuidelines) (explains table format and definitions) + +
Taxonomy Taxonomy item Relationship
C Secure Coding Standard API00-C. Functions should validate their parameters Prior to 2018-01-12: CERT: Unspecified Relationship
C Secure Coding Standard ARR01-C. Do not apply the sizeof operator to a pointer when taking the size of an array Prior to 2018-01-12: CERT: Unspecified Relationship
C Secure Coding Standard INT30-C. Ensure that unsigned integer operations do not wrap Prior to 2018-01-12: CERT: Unspecified Relationship
ISO/IEC TS 17961:2013 Forming invalid pointers by library functions \[libptr\] Prior to 2018-01-12: CERT: Unspecified Relationship
ISO/IEC TR 24772:2013 Buffer Boundary Violation (Buffer Overflow) \[HCB\] Prior to 2018-01-12: CERT: Unspecified Relationship
ISO/IEC TR 24772:2013 Unchecked Array Copying \[XYW\] Prior to 2018-01-12: CERT: Unspecified Relationship
CWE 2.11 CWE-119 , Improper Restriction of Operations within the Bounds of a Memory Buffer 2017-05-18: CERT: Rule subset of CWE
CWE 2.11 CWE-121 , Stack-based Buffer Overflow 2017-05-18: CERT: Partial overlap
CWE 2.11 CWE-123 , Write-what-where Condition 2017-05-18: CERT: Partial overlap
CWE 2.11 CWE-125 , Out-of-bounds Read 2017-05-18: CERT: Partial overlap
CWE 2.11 CWE-805 , Buffer Access with Incorrect Length Value 2017-05-18: CERT: Partial overlap
CWE 3.1 CWE-129 , Improper Validation of Array Index 2017-10-30:MITRE:Unspecified Relationship 2018-10-18:CERT: Partial Overlap
+ + +## CERT-CWE Mapping Notes + +[Key here](https://wiki.sei.cmu.edu/confluence/pages/viewpage.action?pageId=87152408#HowthisCodingStandardisOrganized-CERT-CWEMappingNotes) for mapping notes + +**CWE-121 and ARR38-C** + +Intersection( CWE-121, ARR38-C) = + +* Stack buffer overflow from passing invalid arguments to library function +CWE-121 – ARR38-C = +* Stack buffer overflows from direct out-of-bounds write +ARR38-C – CWE-121 = +* Out-of-bounds read from passing invalid arguments to library function +* Buffer overflow on heap or data segment from passing invalid arguments to library function +**CWE-119 and ARR38-C** + +See CWE-119 and ARR30-C + +**CWE-125 and ARR38-C** + +Independent( ARR30-C, ARR38-C, EXP39-C, INT30-C) + +STR31-C = Subset( Union( ARR30-C, ARR38-C)) + +STR32-C = Subset( ARR38-C) + +Intersection( ARR38-C, CWE-125) = + +* Reading from an out-of-bounds array index or off the end of an array via standard library function +ARR38-C – CWE-125 = +* Writing to an out-of-bounds array index or off the end of an array via standard library function +CWE-125 – ARR38-C = +* Reading beyond a non-array buffer +* Reading beyond an array directly (using pointer arithmetic, or \[\] notation) +**CWE-805 and ARR38-C** + +Intersection( CWE-805, ARR38-C) = + +* Buffer access with incorrect length via passing invalid arguments to library function +CWE-805 – ARR38-C = +* Buffer access with incorrect length directly (such as a loop construct) +ARR38-C – CWE-805 = +* Out-of-bounds read or write that does not involve incorrect length (could use incorrect offset instead), that uses library function +**CWE-123 and ARR38-C** + +Independent(ARR30-C, ARR38-C) + +STR31-C = Subset( Union( ARR30-C, ARR38-C)) + +STR32-C = Subset( ARR38-C) + +CWE-123 includes any operation that allows an attacker to write an arbitrary value to an arbitrary memory location. This could be accomplished via overwriting a pointer with data that refers to the address to write, then when the program writes to a pointed-to value, supplying a malicious value. Vulnerable pointer values can be corrupted by: + +* Stack return address +* Buffer overflow on the heap (which typically overwrites back/next pointer values) +* Write to untrusted array index (if it is also invalid) +* Format string exploit +* Overwriting a C++ object with virtual functions (because it has a virtual pointer) +* Others? +Intersection( CWE-123, ARR38-C) = +* Buffer overflow via passing invalid arguments to library function +ARR38-C – CWE-123 = +* Buffer overflow to “harmless” memory from passing invalid arguments to library function +* Out-of-bounds read from passing invalid arguments to library function +CWE-123 – ARR38-C = +* Arbitrary writes that do not involve standard C library functions +**CWE-129 and ARR38-C** + +ARR38-C - CWE-129 = making library functions create invalid pointers without using untrusted data. + +E.g. : `char[3] array;` + +`strcpy(array, "123456");` + +CWE-129 - ARR38-C = not validating an integer used as an array index or in pointer arithmetic + +E.g.: `void foo(int i) {` + +` char array[3];` + +` array[i];` + +`}` + +Intersection(ARR38-C, CWE-129) = making library functions create invalid pointers using untrusted data. + +`eg: void foo(int i) {` + +` char src[3], dest[3];` + +` memcpy(dest, src, i);` + +`}` + +## Bibliography + +
\[ Cassidy 2014 \] Existential Type Crisis : Diagnosis of the OpenSSL Heartbleed Bug
\[ IETF: RFC 6520 \]
\[ ISO/IEC TS 17961:2013 \]
\[ VU\#720951 \]
+ + +## Implementation notes + +None + +## References + +* CERT-C: [ARR38-C: Guarantee that library functions do not form invalid pointers](https://wiki.sei.cmu.edu/confluence/display/c) diff --git a/c/cert/src/rules/ARR38-C/LibraryFunctionArgumentOutOfBounds.ql b/c/cert/src/rules/ARR38-C/LibraryFunctionArgumentOutOfBounds.ql new file mode 100644 index 0000000000..6b499d0282 --- /dev/null +++ b/c/cert/src/rules/ARR38-C/LibraryFunctionArgumentOutOfBounds.ql @@ -0,0 +1,25 @@ +/** + * @id c/cert/library-function-argument-out-of-bounds + * @name ARR38-C: Guarantee that library functions do not form invalid pointers + * @description Passing out-of-bounds pointers or erroneous size arguments to standard library + * functions can result in out-of-bounds accesses and other undefined behavior. + * @kind problem + * @precision high + * @problem.severity error + * @tags external/cert/id/arr38-c + * correctness + * security + * external/cert/obligation/rule + */ + +import cpp +import codingstandards.c.cert +import codingstandards.c.OutOfBounds + +from + OOB::BufferAccessLibraryFunctionCall fc, string message, Expr bufferArg, string bufferArgStr, + Expr sizeOrOtherBufferArg, string otherStr +where + not isExcluded(fc, OutOfBoundsPackage::libraryFunctionArgumentOutOfBoundsQuery()) and + OOB::problems(fc, message, bufferArg, bufferArgStr, sizeOrOtherBufferArg, otherStr) +select fc, message, bufferArg, bufferArgStr, sizeOrOtherBufferArg, otherStr \ No newline at end of file diff --git a/c/cert/test/rules/ARR30-C/DoNotFormOutOfBoundsPointersOrArraySubscripts.expected b/c/cert/test/rules/ARR30-C/DoNotFormOutOfBoundsPointersOrArraySubscripts.expected new file mode 100644 index 0000000000..fe7ac757a6 --- /dev/null +++ b/c/cert/test/rules/ARR30-C/DoNotFormOutOfBoundsPointersOrArraySubscripts.expected @@ -0,0 +1,11 @@ +| 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 | +| test.c:41:17:41:30 | ... + ... | Buffer access may be to a negative index in the buffer. | test.c:41:17:41:22 | buffer | buffer | +| test.c:45:17:45:30 | ... + ... | Buffer may access up to offset 101*1 which is greater than the fixed size 100 of the $@. | test.c:45:17:45:22 | buffer | buffer | +| test.c:55:5:55:13 | ... - ... | Buffer access may be to a negative index in the buffer. | test.c:55:5:55:9 | ptr16 | buffer | +| test.c:57:5:57:14 | ... + ... | Buffer accesses offset 22 which is greater than the fixed size 20 of the $@. | test.c:57:5:57:9 | ptr16 | buffer | +| test.c:58:5:58:14 | ... - ... | Buffer access may be to a negative index in the buffer. | test.c:58:5:58:9 | ptr16 | buffer | +| test.c:63:3:63:9 | access to array | Buffer access may be to a negative index in the buffer. | test.c:63:3:63:5 | arr | buffer | +| test.c:65:3:65:9 | access to array | Buffer accesses offset 44 which is greater than the fixed size 40 of the $@. | test.c:65:3:65:5 | arr | buffer | +| test.c:66:3:66:10 | access to array | Buffer access may be to a negative index in the buffer. | test.c:66:3:66:5 | arr | buffer | diff --git a/c/cert/test/rules/ARR30-C/DoNotFormOutOfBoundsPointersOrArraySubscripts.qlref b/c/cert/test/rules/ARR30-C/DoNotFormOutOfBoundsPointersOrArraySubscripts.qlref new file mode 100644 index 0000000000..a6e032ec87 --- /dev/null +++ b/c/cert/test/rules/ARR30-C/DoNotFormOutOfBoundsPointersOrArraySubscripts.qlref @@ -0,0 +1 @@ +rules/ARR30-C/DoNotFormOutOfBoundsPointersOrArraySubscripts.ql \ No newline at end of file diff --git a/c/cert/test/rules/ARR30-C/test.c b/c/cert/test/rules/ARR30-C/test.c new file mode 100644 index 0000000000..4d3f077c33 --- /dev/null +++ b/c/cert/test/rules/ARR30-C/test.c @@ -0,0 +1,67 @@ + + +enum { ARRAY_SIZE = 100 }; + +static int arr[ARRAY_SIZE]; + +void test_fixed_wrong(void) { + arr + 101; // NON_COMPLIANT +} + +void test_fixed_right(void) { + 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 + } +} + +void test_local_buffer_invalid_check(int index) { + char buffer[ARRAY_SIZE]; + + if (index < ARRAY_SIZE) { + char *ptr = buffer + index; // NON_COMPLIANT - `index` could be negative + } + + if (index >= 0 && index < ARRAY_SIZE + 2) { + char *ptr = buffer + index; // NON_COMPLIANT - `index` could be too large + } + + if (index >= 0 && index < ARRAY_SIZE) { + char *ptr = buffer + index; // COMPLIANT + } +} + +void test_dereference_pointer_arithmetic_const(void) { + short ptr16[10]; + *(ptr16 - 1); // NON_COMPLIANT - offset is negative + *(ptr16 + 5); // COMPLIANT + *(ptr16 + 11); // NON_COMPLIANT - offset is too large + *(ptr16 - 11); // NON_COMPLIANT - offset is negative +} + +void test_array_expr_const(void) { + int arr[10]; + arr[-1]; // NON_COMPLIANT - offset is negative + arr[5]; // COMPLIANT + arr[11]; // NON_COMPLIANT - offset is too large + arr[-11]; // NON_COMPLIANT - offset is negative +} \ No newline at end of file diff --git a/c/cert/test/rules/ARR38-C/LibraryFunctionArgumentOutOfBounds.expected b/c/cert/test/rules/ARR38-C/LibraryFunctionArgumentOutOfBounds.expected new file mode 100644 index 0000000000..8cb1ff79d0 --- /dev/null +++ b/c/cert/test/rules/ARR38-C/LibraryFunctionArgumentOutOfBounds.expected @@ -0,0 +1,75 @@ +| test.c:40:3:40:8 | call to strchr | The $@ passed to strchr might not be null-terminated. | test.c:40:10:40:16 | ca5_bad | argument | test.c:40:10:40:16 | ca5_bad | | +| test.c:42:3:42:8 | call to strchr | The $@ passed to strchr is 5 bytes, but an offset of 5 bytes is used to access it. | test.c:42:10:42:21 | ... + ... | read buffer | test.c:42:10:42:21 | ... + ... | | +| test.c:46:5:46:10 | call to strcpy | The size of the $@ passed to strcpy is 6 bytes, but the size of the $@ is only 5 bytes. | test.c:46:22:46:28 | test1 | read buffer | test.c:46:12:46:19 | ca5_good | write buffer | +| test.c:53:5:53:10 | call to strcpy | The $@ passed to strcpy might not be null-terminated. | test.c:53:24:53:30 | ca5_bad | argument | test.c:53:24:53:30 | ca5_bad | | +| test.c:54:5:54:10 | call to strcpy | The size of the $@ passed to strcpy is 6 bytes, but the size of the $@ is only 5 bytes. | test.c:54:24:54:31 | ca6_good | read buffer | test.c:54:12:54:19 | call to get_ca_5 | write buffer | +| test.c:59:5:59:10 | call to strcpy | The size of the $@ passed to strcpy is 6 bytes, but the size of the $@ is only 5 bytes. | test.c:59:22:59:29 | ca6_good | read buffer | test.c:59:12:59:19 | ca5_good | write buffer | +| test.c:62:5:62:10 | call to strcpy | The $@ passed to strcpy might not be null-terminated. | test.c:62:22:62:28 | ca6_bad | argument | test.c:62:22:62:28 | ca6_bad | | +| test.c:62:5:62:10 | call to strcpy | The size of the $@ passed to strcpy is 6 bytes, but the size of the $@ is only 5 bytes. | test.c:62:22:62:28 | ca6_bad | read buffer | test.c:62:12:62:19 | ca5_good | write buffer | +| test.c:65:5:65:10 | call to strcpy | The size of the $@ passed to strcpy is 6 bytes, but the size of the $@ is only 5 bytes. | test.c:65:21:65:28 | ca6_good | read buffer | test.c:65:12:65:18 | ca5_bad | write buffer | +| test.c:71:5:71:10 | call to strcpy | The $@ passed to strcpy might not be null-terminated. | test.c:71:21:71:27 | ca5_bad | argument | test.c:71:21:71:27 | ca5_bad | | +| test.c:77:5:77:10 | call to strcpy | The $@ passed to strcpy might not be null-terminated. | test.c:77:24:77:30 | ca5_bad | argument | test.c:77:24:77:30 | ca5_bad | | +| test.c:80:5:80:10 | call to strcpy | The size of the $@ passed to strcpy is 6 bytes, but the size of the $@ is only 5 bytes. | test.c:80:24:80:31 | ca6_good | read buffer | test.c:80:12:80:19 | call to get_ca_5 | write buffer | +| test.c:103:5:103:11 | call to strncpy | The size of the $@ passed to strncpy is 5 bytes, but the $@ is 6 bytes. | test.c:103:13:103:19 | ca5_bad | write buffer | test.c:103:32:103:32 | 6 | size argument | +| test.c:127:5:127:10 | call to memcpy | The $@ passed to memcpy is accessed at an excessive offset of 1 element(s) from the $@. | test.c:127:12:127:13 | p2 | write buffer | test.c:120:21:120:26 | call to strlen | allocation size base | +| test.c:153:5:153:10 | call to strcat | The $@ passed to strcat might not be null-terminated. | test.c:153:12:153:15 | buf1 | argument | test.c:153:12:153:15 | buf1 | | +| test.c:158:5:158:10 | call to strcat | The size of the $@ passed to strcat is 6 bytes, but the size of the $@ is only 5 bytes. | test.c:158:24:158:30 | 12345 | read buffer | test.c:158:12:158:19 | call to get_ca_5 | write buffer | +| test.c:160:5:160:10 | call to strcat | The size of the $@ passed to strcat is 5 bytes, but the size of the $@ is only 4 bytes. | test.c:160:28:160:33 | 1234 | read buffer | test.c:160:12:160:25 | ... + ... | write buffer | +| test.c:183:5:183:11 | call to wcsncat | The size of the $@ passed to wcsncat is 24 bytes, but the size of the $@ is only 5 bytes. | test.c:183:25:183:32 | 12345 | read buffer | test.c:183:13:183:20 | call to get_ca_5 | write buffer | +| test.c:184:5:184:11 | call to wcsncat | The size of the $@ passed to wcsncat is 20 bytes, but the size of the $@ is only 5 bytes. | test.c:184:25:184:31 | 1234 | read buffer | test.c:184:13:184:20 | call to get_ca_5 | write buffer | +| test.c:185:5:185:11 | call to wcsncat | The size of the $@ passed to wcsncat is 20 bytes, but the size of the $@ is only 1 bytes. | test.c:185:29:185:35 | 1234 | read buffer | test.c:185:13:185:26 | ... + ... | write buffer | +| test.c:186:5:186:11 | call to wcsncat | The size of the $@ passed to wcsncat is 12 bytes, but the size of the $@ is only 5 bytes. | test.c:186:25:186:29 | 12 | read buffer | test.c:186:13:186:20 | call to get_ca_5 | write buffer | +| test.c:191:5:191:10 | call to strcmp | The $@ passed to strcmp might not be null-terminated. | test.c:191:22:191:28 | ca5_bad | argument | test.c:191:22:191:28 | ca5_bad | | +| test.c:193:5:193:10 | call to strcmp | The $@ passed to strcmp might not be null-terminated. | test.c:193:12:193:18 | ca5_bad | argument | test.c:193:12:193:18 | ca5_bad | | +| test.c:202:5:202:11 | call to strncmp | The size of the $@ passed to strncmp is 5 bytes, but the $@ is 6 bytes. | test.c:202:13:202:20 | ca5_good | write buffer | test.c:202:32:202:32 | 6 | size argument | +| test.c:202:5:202:11 | call to strncmp | The size of the $@ passed to strncmp is 5 bytes, but the $@ is 6 bytes. | test.c:202:23:202:29 | ca5_bad | read buffer | test.c:202:32:202:32 | 6 | size argument | +| test.c:213:5:213:9 | call to fgets | The size of the $@ passed to fgets is 128 bytes, but the $@ is 129 bytes. | test.c:213:11:213:13 | buf | write buffer | test.c:213:16:213:30 | ... + ... | size argument | +| test.c:216:5:216:9 | call to fgets | The size of the $@ passed to fgets is 127 bytes, but the $@ is 128 bytes. | test.c:216:11:216:17 | ... + ... | write buffer | test.c:216:20:216:30 | sizeof() | size argument | +| test.c:222:5:222:10 | call to fgetws | The size of the $@ passed to fgetws is 512 bytes, but the $@ is 2048 bytes. | test.c:222:12:222:15 | wbuf | write buffer | test.c:222:18:222:29 | sizeof() | size argument | +| test.c:225:5:225:10 | call to fgetws | The size of the $@ passed to fgetws is 512 bytes, but the $@ is 516 bytes. | test.c:225:12:225:15 | wbuf | write buffer | test.c:225:18:225:49 | ... + ... | size argument | +| test.c:228:5:228:10 | call to fgetws | The size of the $@ passed to fgetws is 508 bytes, but the $@ is 512 bytes. | test.c:228:12:228:19 | ... + ... | write buffer | test.c:228:22:228:49 | ... / ... | size argument | +| test.c:237:5:237:12 | call to mbstowcs | The size of the $@ passed to mbstowcs is 512 bytes, but the $@ is 2048 bytes. | test.c:237:14:237:17 | wbuf | write buffer | test.c:237:26:237:37 | sizeof() | size argument | +| test.c:239:5:239:12 | call to mbstowcs | The $@ passed to mbstowcs might not be null-terminated. | test.c:239:20:239:23 | buf2 | argument | test.c:239:20:239:23 | buf2 | | +| test.c:249:5:249:12 | call to wcstombs | The size of the $@ passed to wcstombs is 128 bytes, but the $@ is 512 bytes. | test.c:249:14:249:16 | buf | write buffer | test.c:249:25:249:36 | sizeof() | size argument | +| test.c:249:5:249:12 | call to wcstombs | The size of the $@ passed to wcstombs is 512 bytes, but the size of the $@ is only 128 bytes. | test.c:249:19:249:22 | wbuf | read buffer | test.c:249:14:249:16 | buf | write buffer | +| test.c:252:5:252:12 | call to wcstombs | The size of the $@ passed to wcstombs is 127 bytes, but the $@ is 128 bytes. | test.c:252:14:252:20 | ... + ... | write buffer | test.c:252:33:252:43 | sizeof() | size argument | +| test.c:252:5:252:12 | call to wcstombs | The size of the $@ passed to wcstombs is 508 bytes, but the size of the $@ is only 127 bytes. | test.c:252:23:252:30 | ... + ... | read buffer | test.c:252:14:252:20 | ... + ... | write buffer | +| test.c:261:5:261:10 | call to mbtowc | The size of the $@ passed to mbtowc is 2 bytes, but the $@ is 3 bytes. | test.c:261:16:261:18 | buf | read buffer | test.c:261:21:261:35 | ... + ... | size argument | +| test.c:269:5:269:9 | call to mblen | The size of the $@ passed to mblen is 3 bytes, but the $@ is 4 bytes. | test.c:269:11:269:13 | buf | read buffer | test.c:269:16:269:30 | ... + ... | size argument | +| test.c:270:5:270:9 | call to mblen | The size of the $@ passed to mblen is 5 bytes, but the $@ is 6 bytes. | test.c:270:19:270:24 | call to malloc | read buffer | test.c:270:30:270:44 | ... * ... | size argument | +| test.c:278:5:278:10 | call to memchr | The size of the $@ passed to memchr is 128 bytes, but the $@ is 129 bytes. | test.c:278:12:278:14 | buf | read buffer | test.c:278:20:278:34 | ... + ... | size argument | +| test.c:279:5:279:10 | call to memset | The size of the $@ passed to memset is 128 bytes, but the $@ is 129 bytes. | test.c:279:12:279:14 | buf | write buffer | test.c:279:20:279:34 | ... + ... | size argument | +| test.c:281:5:281:10 | call to memchr | The $@ passed to memchr is null. | test.c:281:12:281:15 | 0 | argument | test.c:281:12:281:15 | 0 | | +| test.c:288:5:288:12 | call to strftime | The size of the $@ passed to strftime is 128 bytes, but the $@ is 129 bytes. | test.c:288:14:288:16 | buf | write buffer | test.c:288:19:288:33 | ... + ... | size argument | +| test.c:290:5:290:12 | call to strftime | The size of the $@ passed to strftime is 127 bytes, but the $@ is 128 bytes. | test.c:290:14:290:20 | ... + ... | write buffer | test.c:290:23:290:33 | sizeof() | size argument | +| test.c:299:5:299:12 | call to wcsftime | The size of the $@ passed to wcsftime is 512 bytes, but the $@ is 520 bytes. | test.c:299:14:299:17 | wbuf | write buffer | test.c:299:20:299:53 | ... + ... | size argument | +| test.c:305:5:305:12 | call to wcsftime | The size of the $@ passed to wcsftime is 508 bytes, but the $@ is 512 bytes. | test.c:305:14:305:21 | ... + ... | write buffer | test.c:305:24:305:53 | ... / ... | size argument | +| test.c:307:5:307:12 | call to wcsftime | The size of the $@ passed to wcsftime is 512 bytes, but the $@ is 2048 bytes. | test.c:307:14:307:17 | wbuf | write buffer | test.c:307:20:307:31 | sizeof() | size argument | +| test.c:315:5:315:11 | call to strxfrm | The size of the $@ passed to strxfrm is 64 bytes, but the $@ is 65 bytes. | test.c:315:13:315:15 | buf | write buffer | test.c:315:25:315:39 | ... + ... | size argument | +| test.c:317:5:317:11 | call to strxfrm | The $@ passed to strxfrm might not be null-terminated. | test.c:317:22:317:25 | buf2 | argument | test.c:317:22:317:25 | buf2 | | +| test.c:326:5:326:11 | call to wcsxfrm | The size of the $@ passed to wcsxfrm is 256 bytes, but the $@ is 260 bytes. | test.c:326:13:326:16 | wbuf | write buffer | test.c:326:27:326:60 | ... + ... | size argument | +| test.c:338:5:338:12 | call to snprintf | The size of the $@ passed to snprintf is 64 bytes, but the $@ is 65 bytes. | test.c:338:14:338:16 | buf | write buffer | test.c:338:19:338:33 | ... + ... | size argument | +| test.c:346:5:346:11 | call to setvbuf | The size of the $@ passed to setvbuf is 64 bytes, but the $@ is 65 bytes. | test.c:346:16:346:18 | buf | read buffer | test.c:346:29:346:43 | ... + ... | size argument | +| test.c:348:5:348:11 | call to setvbuf | The size of the $@ passed to setvbuf is 63 bytes, but the $@ is 64 bytes. | test.c:348:16:348:22 | ... + ... | read buffer | test.c:348:33:348:43 | sizeof() | size argument | +| test.c:362:5:362:10 | call to memcpy | The size of the $@ passed to memcpy is 64 bytes, but the $@ is 65 bytes. | test.c:362:12:362:14 | buf | write buffer | test.c:362:23:362:37 | ... + ... | size argument | +| test.c:362:5:362:10 | call to memcpy | The size of the $@ passed to memcpy is 64 bytes, but the $@ is 65 bytes. | test.c:362:17:362:20 | buf2 | read buffer | test.c:362:23:362:37 | ... + ... | size argument | +| test.c:364:5:364:10 | call to memcpy | The size of the $@ passed to memcpy is 63 bytes, but the $@ is 64 bytes. | test.c:364:12:364:18 | ... + ... | write buffer | test.c:364:27:364:37 | sizeof() | size argument | +| test.c:364:5:364:10 | call to memcpy | The size of the $@ passed to memcpy is 64 bytes, but the size of the $@ is only 63 bytes. | test.c:364:21:364:24 | buf2 | read buffer | test.c:364:12:364:18 | ... + ... | write buffer | +| test.c:365:5:365:10 | call to memcpy | The size of the $@ passed to memcpy is 63 bytes, but the $@ is 128 bytes. | test.c:365:17:365:24 | ... + ... | read buffer | test.c:365:27:365:41 | ... * ... | size argument | +| test.c:365:5:365:10 | call to memcpy | The size of the $@ passed to memcpy is 64 bytes, but the $@ is 128 bytes. | test.c:365:12:365:14 | buf | write buffer | test.c:365:27:365:41 | ... * ... | size argument | +| test.c:374:5:374:11 | call to wmemcpy | The size of the $@ passed to wmemcpy is 256 bytes, but the $@ is 512 bytes. | test.c:374:22:374:27 | wbuf64 | read buffer | test.c:375:13:375:45 | ... / ... | size argument | +| test.c:377:5:377:11 | call to wmemcpy | The size of the $@ passed to wmemcpy is 252 bytes, but the $@ is 256 bytes. | test.c:377:13:377:22 | ... + ... | write buffer | test.c:378:13:378:44 | ... / ... | size argument | +| test.c:377:5:377:11 | call to wmemcpy | The size of the $@ passed to wmemcpy is 256 bytes, but the size of the $@ is only 252 bytes. | test.c:377:25:377:30 | wbuf64 | read buffer | test.c:377:13:377:22 | ... + ... | write buffer | +| test.c:379:5:379:11 | call to wmemcpy | The size of the $@ passed to wmemcpy is 252 bytes, but the $@ is 256 bytes. | test.c:379:13:379:22 | ... + ... | write buffer | test.c:380:13:380:44 | ... / ... | size argument | +| test.c:379:5:379:11 | call to wmemcpy | The size of the $@ passed to wmemcpy is 252 bytes, but the $@ is 256 bytes. | test.c:379:25:379:34 | ... + ... | read buffer | test.c:380:13:380:44 | ... / ... | size argument | +| test.c:401:5:401:11 | call to bsearch | The $@ passed to bsearch is null. | test.c:401:19:401:22 | 0 | argument | test.c:401:19:401:22 | 0 | | +| test.c:411:5:411:9 | call to qsort | The size of the $@ passed to qsort is 40 bytes, but the $@ is 44 bytes. | test.c:411:11:411:13 | arr | write buffer | test.c:411:16:411:44 | ... + ... | size argument | +| test.c:425:3:425:7 | call to fread | The size of the $@ passed to fread is 64 bytes, but the $@ is 65 bytes. | test.c:425:9:425:11 | buf | write buffer | test.c:425:31:425:31 | 1 | size argument | +| test.c:427:3:427:7 | call to fread | The $@ passed to fread is 64 bytes, but an offset of 64 bytes is used to access it. | test.c:427:9:427:15 | ... + ... | write buffer | test.c:427:9:427:15 | ... + ... | | +| test.c:427:3:427:7 | call to fread | The size of the $@ passed to fread is 0 bytes, but the $@ is 64 bytes. | test.c:427:9:427:15 | ... + ... | write buffer | test.c:427:31:427:31 | 1 | size argument | +| test.c:428:3:428:7 | call to fread | The size of the $@ passed to fread is 64 bytes, but the $@ is 128 bytes. | test.c:428:9:428:11 | buf | write buffer | test.c:428:31:428:31 | 1 | size argument | +| test.c:430:3:430:8 | call to fwrite | The size of the $@ passed to fwrite is 64 bytes, but the $@ is 65 bytes. | test.c:430:10:430:12 | buf | read buffer | test.c:430:32:430:32 | 1 | size argument | +| test.c:432:3:432:8 | call to fwrite | The $@ passed to fwrite is 64 bytes, but an offset of 64 bytes is used to access it. | test.c:432:10:432:16 | ... + ... | read buffer | test.c:432:10:432:16 | ... + ... | | +| test.c:432:3:432:8 | call to fwrite | The size of the $@ passed to fwrite is 0 bytes, but the $@ is 64 bytes. | test.c:432:10:432:16 | ... + ... | read buffer | test.c:432:32:432:32 | 1 | size argument | +| test.c:433:3:433:8 | call to fwrite | The size of the $@ passed to fwrite is 64 bytes, but the $@ is 128 bytes. | test.c:433:10:433:12 | buf | read buffer | test.c:433:32:433:32 | 1 | size argument | +| test.c:464:3:464:8 | call to memcpy | The $@ passed to memcpy is accessed at an excessive offset of 1 element(s) from the $@. | test.c:464:10:464:10 | p | write buffer | test.c:462:21:462:41 | ... * ... | allocation size base | diff --git a/c/cert/test/rules/ARR38-C/LibraryFunctionArgumentOutOfBounds.qlref b/c/cert/test/rules/ARR38-C/LibraryFunctionArgumentOutOfBounds.qlref new file mode 100644 index 0000000000..ac4dcf9bf7 --- /dev/null +++ b/c/cert/test/rules/ARR38-C/LibraryFunctionArgumentOutOfBounds.qlref @@ -0,0 +1 @@ +rules/ARR38-C/LibraryFunctionArgumentOutOfBounds.ql \ No newline at end of file diff --git a/c/cert/test/rules/ARR38-C/test.c b/c/cert/test/rules/ARR38-C/test.c new file mode 100644 index 0000000000..872b3455e9 --- /dev/null +++ b/c/cert/test/rules/ARR38-C/test.c @@ -0,0 +1,467 @@ +#include +#include +#include +#include +#include + +char *get_ca_5(void) { + void *ptr = malloc(5 * sizeof(char)); + memset(ptr, 0, 5 * sizeof(char)); + return (char *)ptr; +} + +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 + char ca5_bad[5] = "test1"; // no null terminator + char ca6_good[6] = "test1"; // ok + char ca6_bad[6] = "test12"; // no null terminator + + wchar_t wa5_good[5] = L"test"; // ok + wchar_t wa5_bad[5] = L"test1"; // no null terminator + wchar_t wa6_good[6] = L"test"; // ok + wchar_t wa6_bad[6] = L"test12"; // no null terminator + + // strchr + strchr(ca5_good, 't'); // COMPLIANT + strchr(ca5_bad, 't'); // NON_COMPLIANT + strchr(ca5_good + 4, 't'); // COMPLIANT + strchr(ca5_good + 5, 't'); // NON_COMPLIANT + + if (flow) { + // strcpy from literal + strcpy(ca5_good, "test1"); // NON_COMPLIANT + strcpy(ca5_bad, "test"); // COMPLIANT + } + + if (flow) { + // strcpy to char buffer indirect + strcpy(get_ca_5(), ca5_good); // COMPLIANT + strcpy(get_ca_5(), ca5_bad); // NON_COMPLIANT + strcpy(get_ca_5(), ca6_good); // NON_COMPLIANT + } + + // strcpy between string buffers (must be null-terminated) + if (flow) { + strcpy(ca5_good, ca6_good); + } // NON_COMPLIANT + if (flow) { + strcpy(ca5_good, ca6_bad); + } // NON_COMPLIANT + if (flow) { + strcpy(ca5_bad, ca6_good); + } // NON_COMPLIANT + if (flow) { + strcpy(ca6_bad, ca5_good); + } // COMPLIANT + if (flow) { + strcpy(ca6_bad, ca5_bad); + } // NON_COMPLIANT + if (flow) { + strcpy(get_ca_5(), ca5_good); + } // COMPLIANT + if (flow) { + strcpy(get_ca_5(), ca5_bad); + } // NON_COMPLIANT + if (flow) { + strcpy(get_ca_5(), ca6_good); + } // NON_COMPLIANT + if (flow) { + strcpy(ca5_good, get_ca_5()); + } // NON_COMPLIANT[FALSE_NEGATIVE] + + // strncpy between char buffers (does not have to be null-terminated) + if (flow) { + strncpy(ca5_good, ca6_good, 4); + } // COMPLIANT + if (flow) { + strncpy(ca5_good, ca6_good, 5); + } // COMPLIANT + if (flow) { + strncpy(ca5_good, ca6_bad, 4); + } // COMPLIANT + if (flow) { + strncpy(ca5_good, ca5_good, 5); + } // COMPLIANT + if (flow) { + strncpy(ca5_bad, ca5_bad, 5); + } // COMPLIANT + if (flow) { + strncpy(ca5_bad, ca5_good, 6); + } // NON_COMPLIANT + if (flow) { + strncpy(ca6_bad, ca5_good, 5); + } // COMPLIANT + if (flow) { + strncpy(ca6_bad, ca5_good, 6); + } // COMPLIANT + if (flow) { + strncpy(ca5_good + 1, ca5_good + 2, 3); + } // COMPLIANT + if (flow) { + strncpy(ca5_good + 1, ca5_good + 2, 2); + } // COMPLIANT + + // wrong allocation size + char *p1 = malloc(strlen(ca5_good) + 1); + char *p2 = malloc(strlen(ca5_good)); + + // memcpy with strings and strlen + if (flow) { + memcpy(p1, ca5_good, strlen(ca5_good) + 1); + } // COMPLIANT + if (flow) { + memcpy(p2, ca5_good, strlen(ca5_good) + 1); + } // NON_COMPLIANT + if (flow) { + memcpy(p2 + 1, ca5_good, strlen(ca5_good) - 1); + } // COMPLIANT + if (flow) { + memcpy(p1, ca5_good, strlen(ca5_good)); + } // COMPLIANT - but not terminated + if (flow) { + memcpy(p2, ca5_good, strlen(ca5_good)); + } // COMPLIANT - but not terminated + + // strcat + if (flow) { + char buf0[10]; // memset after first use + char buf1[10]; // no memset + char buf2[10]; // memset before first use + char buf3[10] = {'\0'}; + char buf4[10] = "12345"; + + strcat(buf0, " "); // NON_COMPLIANT[FALSE_NEGATIVE] - not null terminated at + // initialization + + memset(buf0, 0, sizeof(buf0)); // COMPLIANT + memset(buf2, 0, sizeof(buf2)); // COMPLIANT + + strcat(buf1, " "); // NON_COMPLIANT - not null terminated + strcat(buf2, " "); // COMPLIANT + strcat(buf3, " "); // COMPLIANT + strcat(buf4, "12345"); // NON_COMPLIANT + + strcat(get_ca_5(), "12345"); // NON_COMPLIANT + strcat(get_ca_5(), "1234"); // COMPLIANT + strcat(get_ca_5() + 1, "1234"); // NON_COMPLIANT + } + + // wcsncat + if (flow) { + wchar_t buf0[10]; // memset after first use + wchar_t buf1[10]; // no memset + wchar_t buf2[10]; // memset before first use + wchar_t buf3[10] = {L'\0'}; + wchar_t buf4[10] = L"12345"; + + 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[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 + if (flow) { + strcmp(ca5_good, ca5_bad); // NON_COMPLIANT + strcmp(ca5_good, ca5_good); // COMPLIANT + strcmp(ca5_bad, ca5_good); // NON_COMPLIANT + strcmp(ca5_good, ca6_good); // COMPLIANT + strcmp(ca6_good, ca5_good); // COMPLIANT + } + + // strncmp + if (flow) { + strncmp(ca5_good, ca5_bad, 4); // COMPLIANT + strncmp(ca5_good, ca5_bad, 5); // COMPLIANT + strncmp(ca5_good, ca5_bad, 6); // NON_COMPLIANT + } +} + +void test_wrong_buf_size(void) { + + // fgets + { + char buf[128]; + 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); // COMPLIANT + fgets(buf + 1, sizeof(buf) - 1, stdin); // COMPLIANT + fgets(buf + 1, sizeof(buf), stdin); // NON_COMPLIANT + } + + // fgetws + { + wchar_t wbuf[128]; + fgetws(wbuf, sizeof(wbuf), stdin); // NON_COMPLIANT + 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); // COMPLIANT + fgetws(wbuf + 1, sizeof(wbuf) / sizeof(*wbuf) - 2, stdin); // COMPLIANT + fgetws(wbuf + 1, sizeof(wbuf) / sizeof(*wbuf), stdin); // NON_COMPLIANT + } + + // mbstowcs + { + char buf1[128] = {0}; + char buf2[128]; + wchar_t wbuf[128]; + + mbstowcs(wbuf, buf1, sizeof(wbuf)); // NON_COMPLIANT - count too large + mbstowcs(wbuf, buf1, sizeof(buf1)); // COMPLIANT - but wrong arithmetic + mbstowcs(wbuf, buf2, + sizeof(wbuf) / + sizeof(wchar_t)); // NON_COMPLIANT - not null-terminated + mbstowcs(wbuf, buf1, sizeof(wbuf) / sizeof(wchar_t)); // COMPLIANT + } + + // wcstombs + { + char buf[128]; + wchar_t wbuf[128] = {0}; + wcstombs(buf, wbuf, sizeof(wbuf)); // NON_COMPLIANT - count too large + wcstombs(buf, wbuf, sizeof(buf)); // COMPLIANT + wcstombs(buf + 1, wbuf + 1, sizeof(buf) - 1); // COMPLIANT + wcstombs(buf + 1, wbuf + 1, sizeof(buf)); // NON_COMPLIANT + } + + // mbtowc + { + wchar_t c; + 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 + mbtowc(NULL, NULL, 0); // COMPLIANT - exception + } + + // mblen + { + 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 + mblen(NULL, 0); // COMPLIANT - exception + } + + // memchr, memset + { + char buf[128]; + memchr(buf, 0, sizeof(buf)); // COMPLIANT + memchr(buf, 0, sizeof(buf) + 1); // NON_COMPLIANT + memset(buf, 0, sizeof(buf) + 1); // NON_COMPLIANT + memchr(buf, 0, sizeof(buf) - 1); // COMPLIANT + memchr(NULL, 0, sizeof(buf)); // NON_COMPLIANT + } + + // strftime + { + char buf[128]; + strftime(buf, sizeof(buf), "%Y-%m-%d", NULL); // COMPLIANT + strftime(buf, sizeof(buf) + 1, "%Y-%m-%d", NULL); // NON_COMPLIANT + strftime(buf, sizeof(buf) - 1, "%Y-%m-%d", NULL); // COMPLIANT + strftime(buf + 1, sizeof(buf), "%Y-%m-%d", NULL); // NON_COMPLIANT + } + + // wcsftime + { + wchar_t wbuf[128] = {0}; + wchar_t format_bad[8] = L"%Y-%m-%d"; + wcsftime(wbuf, sizeof(wbuf) / sizeof(wchar_t), L"%Y-%m-%d", + NULL); // COMPLIANT + wcsftime(wbuf, sizeof(wbuf) / sizeof(wchar_t) + 2, L"%Y-%m-%d", + NULL); // NON_COMPLIANT + wcsftime(wbuf, sizeof(wbuf) / sizeof(wchar_t) - 2, L"%Y-%m-%d", + NULL); // COMPLIANT + wcsftime(wbuf, sizeof(wbuf) / sizeof(wchar_t), format_bad, + NULL); // NON_COMPLIANT + wcsftime(wbuf + 1, sizeof(wbuf) / sizeof(wchar_t), L"%Y-%m-%d", + NULL); // NON_COMPLIANT + wcsftime(wbuf, sizeof(wbuf), L"%Y-%m-%d", NULL); // NON_COMPLIANT + } + + // strxfrm + { + 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); // NON_COMPLIANT - not null terminated + } + + // wcsxfrm + { + wchar_t wbuf[64]; + wchar_t wbuf2[128]; + wcsxfrm(wbuf, L"abc", sizeof(wbuf) / sizeof(wchar_t)); // COMPLIANT + wcsxfrm(wbuf, L"abc", sizeof(wbuf) / sizeof(wchar_t) + 1); // NON_COMPLIANT + wcsxfrm(wbuf, L"abc", sizeof(wbuf) / sizeof(wchar_t) - 1); // COMPLIANT + wcsxfrm(wbuf + 1, wbuf2, sizeof(wbuf) / sizeof(wchar_t) - 1); // COMPLIANT + } + + // snprintf (and vsnprintf, swprintf, vswprintf) + { + char str_bad[2] = "12"; + char buf[64]; + snprintf(buf, sizeof(buf), "%s", ""); // COMPLIANT + snprintf(buf, sizeof(buf), "%s", + str_bad); // NON_COMPLIANT[FALSE_NEGATIVE] - not checked + snprintf(buf, sizeof(buf) + 1, "test"); // NON_COMPLIANT + } + + // setvbuf + { + FILE *f; + char buf[64]; + setvbuf(f, buf, _IOFBF, sizeof(buf)); // COMPLIANT + setvbuf(f, buf, _IOFBF, sizeof(buf) + 1); // NON_COMPLIANT + setvbuf(f, buf, _IOFBF, sizeof(buf) - 1); // COMPLIANT + setvbuf(f, buf + 1, _IOFBF, sizeof(buf)); // NON_COMPLIANT + setvbuf(f, NULL, _IOFBF, 0); // COMPLIANT - exception + } + + // "memcpy", "wmemcpy", "memmove", "wmemmove", "memcmp", "wmemcmp" + + // memcpy + { + char buf[64]; + char buf2[64]; + wchar_t wbuf[64]; + wchar_t wbuf2[64]; + + memcpy(buf, buf2, sizeof(buf)); // COMPLIANT + memcpy(buf, buf2, sizeof(buf) + 1); // NON_COMPLIANT + memcpy(buf, buf2, sizeof(buf) - 1); // COMPLIANT + memcpy(buf + 1, buf2, sizeof(buf)); // NON_COMPLIANT + memcpy(buf, buf2 + 1, sizeof(buf) * 2); // NON_COMPLIANT + } + + // wmemcpy + { + wchar_t wbuf128[128]; + wchar_t wbuf64[64]; + + wmemcpy(wbuf128, wbuf64, sizeof(wbuf64) / sizeof(wchar_t)); // COMPLIANT + wmemcpy(wbuf128, wbuf64, + sizeof(wbuf128) / sizeof(wchar_t)); // NON_COMPLIANT + wmemcpy(wbuf128, wbuf64, sizeof(wbuf64) / sizeof(wchar_t) - 1); // COMPLIANT + wmemcpy(wbuf64 + 1, wbuf64, + sizeof(wbuf64) / sizeof(wchar_t)); // NON_COMPLIANT + wmemcpy(wbuf64 + 1, wbuf64 + 1, + sizeof(wbuf64) / sizeof(wchar_t)); // NON_COMPLIANT + wmemcpy(wbuf64 + 1, wbuf64 + 1, + sizeof(wbuf64) / sizeof(wchar_t) - 1); // NON_COMPLIANT + wmemcpy(wbuf64 + 1, wbuf64 + 1, + sizeof(wbuf64) / sizeof(wchar_t) - 2); // COMPLIANT + } + + // bsearch + { + int arr[10]; + int key = 0; + bsearch(&key, arr, sizeof(arr) / sizeof(int), sizeof(int), + compare); // COMPLIANT + bsearch(&key, arr, sizeof(arr) / sizeof(int) + 1, sizeof(int), + compare); // NON_COMPLIANT + bsearch(&key, arr, sizeof(arr) / sizeof(int) - 1, sizeof(int), + compare); // COMPLIANT + bsearch(&key, arr + 1, sizeof(arr) / sizeof(int) - 1, sizeof(int), + compare); // NON_COMPLIANT + bsearch(NULL, arr, sizeof(arr) / sizeof(int), sizeof(int), + compare); // NON_COMPLIANT + bsearch(&key, NULL, sizeof(arr) / sizeof(int), sizeof(int), + compare); // NON_COMPLIANT + bsearch(&key, arr, sizeof(arr) / sizeof(int), sizeof(int), + NULL); // NON_COMPLIANT + } + + // qsort + { + int arr[10]; + qsort(arr, sizeof(arr) / sizeof(int), sizeof(int), compare); // COMPLIANT + qsort(arr, sizeof(arr) / sizeof(int) + 1, sizeof(int), + compare); // NON_COMPLIANT + qsort(arr, sizeof(arr) / sizeof(int) - 1, sizeof(int), + compare); // COMPLIANT + qsort(arr + 1, sizeof(arr) / sizeof(int) - 1, sizeof(int), + compare); // NON_COMPLIANT + qsort(arr, sizeof(arr) / sizeof(int), sizeof(int), NULL); // NON_COMPLIANT + } +} + +void test_fread_fwrite_static(char *file_name) { + FILE *f = fopen(file_name, "r"); + char buf[64]; + fread(buf, sizeof(buf), 1, f); // COMPLIANT + fread(buf, sizeof(buf) + 1, 1, f); // NON_COMPLIANT + fread(buf, sizeof(buf) - 1, 1, f); // COMPLIANT + fread(buf + 1, sizeof(buf), 1, f); // NON_COMPLIANT + fread(buf, sizeof(buf) * 2, 1, f); // NON_COMPLIANT + fwrite(buf, sizeof(buf), 1, f); // COMPLIANT + fwrite(buf, sizeof(buf) + 1, 1, f); // NON_COMPLIANT + fwrite(buf, sizeof(buf) - 1, 1, f); // COMPLIANT + fwrite(buf + 1, sizeof(buf), 1, f); // NON_COMPLIANT + fwrite(buf, sizeof(buf) * 2, 1, f); // NON_COMPLIANT + fclose(f); +} + +void test_read_file(const char *file_name) { + FILE *f = fopen(file_name, "rb"); + + fseek(f, 0, SEEK_END); + long len = ftell(f); + rewind(f); + + char *buf = malloc(len + 1); + + // not correct behaviour below but suffices to test overflow + rewind(f); + fread(buf + 1, len - 1, 1, f); // COMPLIANT + rewind(f); + fread(buf + 1, len, 1, f); // COMPLIANT + rewind(f); + fread(buf + 1, len + 1, 1, f); // COMPLIANT + rewind(f); + fread(buf + 1, len + 2, 1, f); // COMPLIANT + rewind(f); + fread(buf + 1, len + 3, 1, f); // NON_COMPLIANT + + fclose(f); +} + +void test_equivalent_expressions(void *in, int x, int y, int a, int b) { + short *p = malloc(x * y * sizeof(short)); + memcpy(p, in, x * y * sizeof(short)); // COMPLIANT + memcpy(p, in, x * y * sizeof(short) + 1); // NON_COMPLIANT + memcpy(p, in, x * y * sizeof(short) - 1); // COMPLIANT + memcpy(p, in, a * b * sizeof(short) + 1); // COMPLIANT - unknown +} \ No newline at end of file diff --git a/c/common/src/codingstandards/c/OutOfBounds.qll b/c/common/src/codingstandards/c/OutOfBounds.qll new file mode 100644 index 0000000000..33f1e9cd39 --- /dev/null +++ b/c/common/src/codingstandards/c/OutOfBounds.qll @@ -0,0 +1,1348 @@ +/** + * This module provides classes and predicates for analyzing the size of buffers + * or objects from their base or a byte-offset, and identifying the potential for + * expressions accessing those buffers to overflow. + */ + +import cpp +import codingstandards.c.Pointers +import codingstandards.c.Variable +import codingstandards.cpp.Allocations +import codingstandards.cpp.Overflow +import codingstandards.cpp.PossiblyUnsafeStringOperation +import codingstandards.cpp.SimpleRangeAnalysisCustomizations +import semmle.code.cpp.dataflow.DataFlow +import semmle.code.cpp.valuenumbering.GlobalValueNumbering + +module OOB { + /** + * Holds if `result` is either `name` or a string matching a pattern such as + * `__builtin_*name*_chk` or similar. This predicate exists to model internal functions + * such as `__builtin___memcpy_chk` under a common `memcpy` name in the table. + */ + bindingset[name, result] + string getNameOrInternalName(string name) { + result.regexpMatch("^(?:__.*_+)?" + name + "(?:_[^s].*)?$") + } + + /** + * MISRA-C Rule 21.17 function table of names and parameter indices + * which covers functions from that rely on null-terminated strings. + * + * This table is a subset of `libraryFunctionNameParamTable`. + * + * Note: These functions do not share a common semantic pattern of source and destination + * parameters with the other functions explicitly defined in `libraryFunctionNameParamTable`, + * although they do share a common issue of parsing non-null-terminated strings. + * The `SimpleStringLibraryFunction` base class provides an appropriate + * interface for analyzing the functions in the below table. + */ + private Function libraryFunctionNameParamTableSimpleString( + string name, int dst, int src, int src_sz, int dst_sz + ) { + result.getName() = getNameOrInternalName(name) and + src_sz = -1 and + dst_sz = -1 and + ( + name = "strcat" and + dst = 0 and + src = 1 + or + name = "strchr" and + dst = -1 and + src = 0 + or + name = ["strcmp", "strcoll"] and + dst = -1 and + src = [0, 1] + or + name = "strcpy" and + dst = 0 and + src = 1 + or + name = "strcspn" and + dst = -1 and + src = [0, 1] + or + name = "strlen" and + dst = -1 and + src = 0 + or + name = "strpbrk" and + dst = -1 and + src = [0, 1] + or + name = "strrchr" and + dst = -1 and + src = 0 + or + name = "strspn" and + dst = -1 and + src = [0, 1] + or + name = "strstr" and + dst = -1 and + src = [0, 1] + or + // do not specify a src and dst to avoid buffer size assumptions + name = ["strtok", "strtok_r"] and + dst = -1 and + src = [0, 1] + ) + } + + /** + * A relation of the indices of buffer and size parameters of standard library functions + * which are defined in rules CERT ARR38-C and MISRA-C rules 21.17 and 21.18. + */ + private Function libraryFunctionNameParamTable( + string name, int dst, int src, int src_sz, int dst_sz + ) { + result = libraryFunctionNameParamTableSimpleString(name, dst, src, src_sz, dst_sz) + or + result.getName() = getNameOrInternalName(name) and + ( + name = ["fgets", "fgetws"] and + dst = 0 and + src = -1 and + src_sz = -1 and + dst_sz = 1 + or + name = ["mbstowcs", "wcstombs"] and + dst = 0 and + src = 1 and + src_sz = -1 and + dst_sz = 2 + or + name = ["mbrtoc16", "mbrtoc32"] and + dst = 0 and + src = 1 and + src_sz = 2 and + dst_sz = -1 + or + name = ["mbsrtowcs", "wcsrtombs"] and + dst = 0 and + src = 1 and + src_sz = -1 and + dst_sz = 2 + or + name = ["mbtowc", "mbrtowc"] and + dst = 0 and + src = 1 and + src_sz = 2 and + dst_sz = -1 + or + name = ["mblen", "mbrlen"] and + dst = -1 and + src = 0 and + src_sz = 1 and + dst_sz = -1 + or + name = ["memchr", "wmemchr"] and + dst = -1 and + src = 0 and + src_sz = 2 and + dst_sz = -1 + or + name = ["memset", "wmemset"] and + dst = 0 and + src = -1 and + src_sz = -1 and + dst_sz = 2 + or + name = ["strftime", "wcsftime"] and + dst = 0 and + src = -1 and + src_sz = -1 and + dst_sz = 1 + or + name = ["strxfrm", "wcsxfrm"] and + dst = 0 and + src = 1 and + src_sz = -1 and + dst_sz = 2 + or + name = ["strncat", "wcsncat"] and + dst = 0 and + src = 1 and + src_sz = 2 and + dst_sz = -1 + or + name = ["snprintf", "vsnprintf", "swprintf", "vswprintf"] and + dst = 0 and + src = -1 and + src_sz = -1 and + dst_sz = 1 + or + name = "setvbuf" and + dst = -1 and + src = 1 and + src_sz = 3 and + dst_sz = -1 + or + name = ["memcpy", "wmemcpy", "memmove", "wmemmove", "memcmp", "wmemcmp", "strncmp", "wcsncmp"] and + dst = 0 and + src = 1 and + src_sz = 2 and + dst_sz = 2 + or + name = ["strncpy", "wcsncpy"] and + dst = 0 and + src = 1 and + src_sz = -1 and + dst_sz = 2 + or + name = "qsort" and + dst = 0 and + src = -1 and + src_sz = -1 and + dst_sz = 1 + or + name = "bsearch" and + dst = -1 and + src = 1 and + src_sz = -1 and + dst_sz = 2 + or + name = "fread" and + dst = 0 and + src = -1 and + src_sz = -1 and + dst_sz = 2 + or + name = "fwrite" and + dst = -1 and + src = 0 and + src_sz = 2 and + dst_sz = -1 + ) + } + + /** + * A library function that accesses one or more buffers supplied via arguments. + */ + class BufferAccessLibraryFunction extends Function { + BufferAccessLibraryFunction() { this = libraryFunctionNameParamTable(_, _, _, _, _) } + + /** + * Returns the indices of parameters that are a destination buffer. + */ + int getWriteParamIndex() { + this = libraryFunctionNameParamTable(_, result, _, _, _) and + result >= 0 + } + + /** + * Returns the indices of parameters that are a source buffer. + */ + int getReadParamIndex() { + this = libraryFunctionNameParamTable(_, _, result, _, _) and + result >= 0 + } + + /** + * Returns the index of the parameter that is the source buffer size. + */ + int getReadSizeParamIndex() { + this = libraryFunctionNameParamTable(_, _, _, result, _) and + result >= 0 + } + + /** + * Returns the index of the parameter that is the destination buffer size. + */ + int getWriteSizeParamIndex() { + this = libraryFunctionNameParamTable(_, _, _, _, result) and + result >= 0 + } + + /** + * Gets a parameter than is a source (read) buffer. + */ + Parameter getReadParam() { result = this.getParameter(this.getReadParamIndex()) } + + /** + * Gets a parameter than is a destination (write) buffer. + */ + Parameter getWriteParam() { result = this.getParameter(this.getWriteParamIndex()) } + + /** + * Gets a parameter than is a source (read) buffer size. + */ + Parameter getReadSizeParam() { result = this.getParameter(this.getReadSizeParamIndex()) } + + /** + * Gets a parameter than is a destination (write) buffer size. + */ + Parameter getWriteSizeParam() { result = this.getParameter(this.getWriteSizeParamIndex()) } + + /** + * Gets the size of an element in the destination buffer class + */ + int getWriteParamElementSize(Parameter p) { + p = this.getWriteParam() and + p.getType().getUnspecifiedType().(DerivedType).getBaseType().getSize().maximum(1) = result + } + + /** + * Gets the size of an element in the source buffer class + */ + int getReadParamElementSize(Parameter p) { + p = this.getReadParam() and + p.getType().getUnspecifiedType().(DerivedType).getBaseType().getSize().maximum(1) = result + } + + /** + * Holds if `i` is the index of a parameter of this function that requires arguments to be null-terminated. + * This predicate should be overriden by extending classes to specify null-terminated parameters, if necessary. + */ + predicate getANullTerminatedParameterIndex(int i) { + // by default, require null-terminated parameters for src but + // only if the type of src is a plain char pointer or wchar_t. + this.getReadParamIndex() = i and + exists(Type baseType | + baseType = this.getReadParam().getUnspecifiedType().(PointerType).getBaseType() and + ( + baseType.getUnspecifiedType() instanceof PlainCharType or + baseType.getUnspecifiedType() instanceof Wchar_t + ) + ) + } + + /** + * Holds if `i` is the index of a parameter of this function that is a size multiplier. + * This predicate should be overriden by extending classes to specify size multiplier parameters, if necessary. + */ + predicate getASizeMultParameterIndex(int i) { + // by default, there is no size multiplier parameter + // exceptions: fread, fwrite, bsearch, qsort + none() + } + + /** + * Holds if `i` is the index of a parameter of this function that expects an element count rather than buffer size argument. + * This predicate should be overriden by extending classes to specify length parameters, if necessary. + */ + predicate getALengthParameterIndex(int i) { + // by default, size parameters do not exclude the size of a null terminator + none() + } + + /** + * Holds if the read or write parameter at index `i` is allowed to be null. + * This predicate should be overriden by extending classes to specify permissibly null parameters, if necessary. + */ + predicate getAPermissiblyNullParameterIndex(int i) { + // by default, pointer parameters are not allowed to be null + none() + } + } + + /** + * A library function that accesses one or more string buffers and has no + * additional parameters for specifying the size of the buffers. + */ + class SimpleStringLibraryFunction extends BufferAccessLibraryFunction { + SimpleStringLibraryFunction() { + this = libraryFunctionNameParamTableSimpleString(_, _, _, -1, -1) + } + + override predicate getANullTerminatedParameterIndex(int i) { + // by default, require null-terminated parameters for src but + // only if the type of src is a plain char pointer. + this.getReadParamIndex() = i and + this.getReadParam().getUnspecifiedType().(PointerType).getBaseType().getUnspecifiedType() + instanceof PlainCharType + } + } + + /** + * A `BufferAccessLibraryFunction` that performs string concatenation. + */ + abstract class StringConcatenationFunctionLibraryFunction extends BufferAccessLibraryFunction { + override predicate getANullTerminatedParameterIndex(int i) { + // `strcat` and variants require null-terminated params for both src and dst + i = [0, 1] + } + } + + /** + * A `BufferAccessLibraryFunction` modelling `strcat` + */ + class StrcatLibraryFunction extends StringConcatenationFunctionLibraryFunction { + StrcatLibraryFunction() { this.getName() = getNameOrInternalName("strcat") } + } + + /** + * A `BufferAccessLibraryFunction` modelling `strncat` or `wcsncat` + */ + class StrncatLibraryFunction extends StringConcatenationFunctionLibraryFunction { + StrncatLibraryFunction() { this.getName() = getNameOrInternalName(["strncat", "wcsncat"]) } + + override predicate getALengthParameterIndex(int i) { + // `strncat` and `wcsncat` exclude the size of a null terminator + i = 2 + } + } + + /** + * A `BufferAccessLibraryFunction` modelling `strncpy` + */ + class StrncpyLibraryFunction extends StringConcatenationFunctionLibraryFunction { + StrncpyLibraryFunction() { this.getName() = getNameOrInternalName("strncpy") } + + override predicate getANullTerminatedParameterIndex(int i) { + // `strncpy` does not require null-terminated parameters + none() + } + } + + /** + * A `BufferAccessLibraryFunction` modelling `strncmp` + */ + class StrncmpLibraryFunction extends BufferAccessLibraryFunction { + StrncmpLibraryFunction() { this.getName() = getNameOrInternalName("strncmp") } + + override predicate getANullTerminatedParameterIndex(int i) { + // `strncmp` does not require null-terminated parameters + none() + } + } + + /** + * A `BufferAccessLibraryFunction` modelling `mbtowc` and `mbrtowc` + */ + class MbtowcLibraryFunction extends BufferAccessLibraryFunction { + MbtowcLibraryFunction() { this.getName() = getNameOrInternalName(["mbtowc", "mbrtowc"]) } + + override predicate getAPermissiblyNullParameterIndex(int i) { + // `mbtowc` requires null-terminated parameters for both src and dst + i = [0, 1] + } + } + + /** + * A `BufferAccessLibraryFunction` modelling `mblen` and `mbrlen` + */ + class MblenLibraryFunction extends BufferAccessLibraryFunction { + MblenLibraryFunction() { this.getName() = getNameOrInternalName(["mblen", "mbrlen"]) } + + override predicate getAPermissiblyNullParameterIndex(int i) { i = 0 } + } + + /** + * A `BufferAccessLibraryFunction` modelling `setvbuf` + */ + class SetvbufLibraryFunction extends BufferAccessLibraryFunction { + SetvbufLibraryFunction() { this.getName() = getNameOrInternalName("setvbuf") } + + override predicate getAPermissiblyNullParameterIndex(int i) { i = 1 } + + override predicate getANullTerminatedParameterIndex(int i) { + // `setvbuf` does not require a null-terminated buffer + none() + } + } + + /** + * A `BufferAccessLibraryFunction` modelling `snprintf`, `vsnprintf`, `swprintf`, and `vswprintf`. + * This class overrides the `getANullTerminatedParameterIndex` predicate to include the `format` parameter. + */ + class PrintfLibraryFunction extends BufferAccessLibraryFunction { + PrintfLibraryFunction() { + this.getName() = getNameOrInternalName(["snprintf", "vsnprintf", "swprintf", "vswprintf"]) + } + + override predicate getANullTerminatedParameterIndex(int i) { + // `snprintf` and variants require a null-terminated format string + i = 2 + } + } + + /** + * A `BufferAccessLibraryFunction` modelling `fread` and `fwrite`. + */ + class FreadFwriteLibraryFunction extends BufferAccessLibraryFunction { + FreadFwriteLibraryFunction() { this.getName() = getNameOrInternalName(["fread", "fwrite"]) } + + override predicate getASizeMultParameterIndex(int i) { + // `fread` and `fwrite` have a size multiplier parameter + i = 1 + } + } + + /** + * A `BufferAccessLibraryFunction` modelling `bsearch` + */ + class BsearchLibraryFunction extends BufferAccessLibraryFunction { + BsearchLibraryFunction() { this.getName() = getNameOrInternalName("bsearch") } + + override predicate getASizeMultParameterIndex(int i) { + // `bsearch` has a size multiplier parameter + i = 3 + } + } + + /** + * A `BufferAccessLibraryFunction` modelling `qsort` + */ + class QsortLibraryFunction extends BufferAccessLibraryFunction { + QsortLibraryFunction() { this.getName() = getNameOrInternalName("qsort") } + + override predicate getASizeMultParameterIndex(int i) { + // `qsort` has a size multiplier parameter + i = 2 + } + } + + /** + * A `BufferAccessLibraryFunction` modelling `strtok` + */ + class StrtokLibraryFunction extends BufferAccessLibraryFunction { + StrtokLibraryFunction() { this.getName() = getNameOrInternalName(["strtok", "strtok_r"]) } + + override predicate getAPermissiblyNullParameterIndex(int i) { + // `strtok` does not require a non-null `str` parameter + i = 0 + } + } + + /** + * An construction of a pointer to a buffer. + */ + abstract class BufferAccess extends Expr { + abstract predicate hasABuffer(Expr buffer, Expr size, int sizeMult); + + Expr getARelevantExpr() { + hasABuffer(result, _, _) + or + hasABuffer(_, result, _) + } + } + + class PointerArithmeticBufferAccess extends BufferAccess instanceof PointerArithmeticExpr { + override predicate hasABuffer(Expr buffer, Expr size, int sizeMult) { + buffer = this.(PointerArithmeticExpr).getPointer() and + size = this.(PointerArithmeticExpr).getOperand() and + sizeMult = + buffer.getType().getUnspecifiedType().(DerivedType).getBaseType().getSize().maximum(1) + } + } + + class ArrayBufferAccess extends BufferAccess, ArrayExpr { + override predicate hasABuffer(Expr buffer, Expr size, int sizeMult) { + buffer = this.getArrayBase() and + size = this.getArrayOffset() and + sizeMult = + buffer.getType().getUnspecifiedType().(DerivedType).getBaseType().getSize().maximum(1) + } + } + + /** + * A `FunctionCall` to a `BufferAccessLibraryFunction` that provides predicates for + * reasoning about buffer overflow and other buffer access violations. + */ + class BufferAccessLibraryFunctionCall extends FunctionCall, BufferAccess { + BufferAccessLibraryFunctionCall() { this.getTarget() instanceof BufferAccessLibraryFunction } + + override predicate hasABuffer(Expr buffer, Expr size, int sizeMult) { + buffer = this.getWriteArg() and + size = this.getWriteSizeArg(sizeMult) + or + buffer = this.getReadArg() and + size = this.getReadSizeArg(sizeMult) + } + + Expr getReadArg() { + result = this.getArgument(this.getTarget().(BufferAccessLibraryFunction).getReadParamIndex()) + } + + Expr getWriteArg() { + result = this.getArgument(this.getTarget().(BufferAccessLibraryFunction).getWriteParamIndex()) + } + + Expr getReadSizeArg(int mult) { + result = + this.getArgument(this.getTarget().(BufferAccessLibraryFunction).getReadSizeParamIndex()) and + getReadSizeArgMult() = mult + } + + Expr getWriteSizeArg(int mult) { + result = + this.getArgument(this.getTarget().(BufferAccessLibraryFunction).getWriteSizeParamIndex()) and + getWriteSizeArgMult() = mult + } + + int getReadSizeArgMult() { + result = + this.getTarget().(BufferAccessLibraryFunction).getReadParamElementSize(_) * + getSizeMultArgValue() + } + + int getWriteSizeArgMult() { + result = + this.getTarget().(BufferAccessLibraryFunction).getWriteParamElementSize(_) * + getSizeMultArgValue() + } + + int getSizeMultArgValue() { + // Note: This predicate currently expects the size multiplier argument to be a constant. + // This implementation could be improved with range-analysis or data-flow to determine the argument value. + exists(int i | + this.getTarget().(BufferAccessLibraryFunction).getASizeMultParameterIndex(i) and + result = this.getArgument(i).getValue().toInt() + ) + or + not this.getTarget().(BufferAccessLibraryFunction).getASizeMultParameterIndex(_) and + result = 1 + } + } + + /** + * A `FunctionCall` to a `SimpleStringLibraryFunction` + */ + class SimpleStringLibraryFunctionCall extends BufferAccessLibraryFunctionCall { + SimpleStringLibraryFunctionCall() { this.getTarget() instanceof SimpleStringLibraryFunction } + } + + bindingset[dest] + private Expr getSourceConstantExpr(Expr dest) { + exists(result.getValue().toInt()) and + DataFlow::localExprFlow(result, dest) + } + + /** + * Gets the smallest of the upper bound of `e` or the largest source value (i.e. "stated value") that flows to `e`. + * Because range-analysis can over-widen bounds, take the minimum of range analysis and data-flow sources. + * + * If there is no source value that flows to `e`, this predicate does not hold. + * + * This predicate, if `e` is the size argument to malloc, would return `20` for the following example: + * ``` + * size_t sz = condition ? 10 : 20; + * malloc(sz); + * ``` + */ + private int getMaxStatedValue(Expr e) { + result = upperBound(e).minimum(max(getSourceConstantExpr(e).getValue().toInt())) + } + + /** + * Gets the smallest of the upper bound of `e` or the smallest source value (i.e. "stated value") that flows to `e`. + * Because range-analysis can over-widen bounds, take the minimum of range analysis and data-flow sources. + * + * If there is no source value that flows to `e`, this predicate does not hold. + * + * This predicate, if `e` is the size argument to malloc, would return `10` for the following example: + * ``` + * size_t sz = condition ? 10 : 20; + * malloc(sz); + * ``` + */ + bindingset[e] + private int getMinStatedValue(Expr e) { + result = upperBound(e).minimum(min(getSourceConstantExpr(e).getValue().toInt())) + } + + /** + * A class for reasoning about the offset of a variable from the original value flowing to it + * as a result of arithmetic or pointer arithmetic expressions. + */ + bindingset[expr] + private int getArithmeticOffsetValue(Expr expr, Expr base) { + result = getMinStatedValue(expr.(PointerArithmeticExpr).getOperand()) and + base = expr.(PointerArithmeticExpr).getPointer() + or + // &(array[index]) expressions + result = + getMinStatedValue(expr.(AddressOfExpr).getOperand().(PointerArithmeticExpr).getOperand()) and + base = expr.(AddressOfExpr).getOperand().(PointerArithmeticExpr).getPointer() + or + result = getMinStatedValue(expr.(AddExpr).getRightOperand()) and + base = expr.(AddExpr).getLeftOperand() + or + result = -getMinStatedValue(expr.(SubExpr).getRightOperand()) and + base = expr.(SubExpr).getLeftOperand() + or + expr instanceof IncrementOperation and + result = 1 and + base = expr.(IncrementOperation).getOperand() + or + expr instanceof DecrementOperation and + result = -1 and + base = expr.(DecrementOperation).getOperand() + or + // fall-back if `expr` is not an arithmetic or pointer arithmetic expression + not expr instanceof PointerArithmeticExpr and + not expr.(AddressOfExpr).getOperand() instanceof PointerArithmeticExpr and + not expr instanceof AddExpr and + not expr instanceof SubExpr and + not expr instanceof IncrementOperation and + not expr instanceof DecrementOperation and + base = expr and + result = 0 + } + + private int constOrZero(Expr e) { + result = e.getValue().toInt() + or + not exists(e.getValue().toInt()) and result = 0 + } + + abstract class PointerToObjectSource extends Expr { + /** + * Gets the expression that points to the object. + */ + abstract Expr getPointer(); + + /** + * Gets the expression, if any, that defines the size of the object. + */ + abstract Expr getSizeExpr(); + + /** + * Gets the size of the object, if it is statically known. + */ + abstract int getFixedSize(); + + /** + * Holds if the object is not null-terminated. + */ + abstract predicate isNotNullTerminated(); + } + + private class DynamicAllocationSource extends PointerToObjectSource instanceof AllocationExpr, + FunctionCall { + DynamicAllocationSource() { + // exclude OperatorNewAllocationFunction to only deal with raw malloc-style calls, + // which do not apply a multiple to the size of the allocation passed to them. + not this.(FunctionCall).getTarget() instanceof OperatorNewAllocationFunction + } + + override Expr getPointer() { result = this } + + override Expr getSizeExpr() { + // AllocationExpr may sometimes return a subexpression of the size expression + // in order to separate the size from a sizeof expression in a MulExpr. + exists(AllocationFunction f | + f = this.(FunctionCall).getTarget() and + result = this.(FunctionCall).getArgument(f.getSizeArg()) + ) + } + + /** + * Returns either `getSizeExpr()`, or, if a value assigned to a variable flows + * to `getSizeExpr()` or an `AddExpr` within it, the value assigned to that variable. + * + * If an `AddExpr` exists in the value assignment or `getSizeExpr()`, and that `AddExpr` + * has a constant right operand, then value of that operand is `offset`. Otherwise, `offset` is 0. + * + * If no `AddExpr` exists, `base = result`. Otherwise, `base` is the left operand of the `AddExpr`. + * If the left operand of the `AddExpr` comes from a variable assignment, `base` is assigned value. + * + * This predicate serves as a rough heuristic for cases such as the following: + * 1. `size_t sz = strlen(src) + 1; malloc(sz);` + * 2. `size_t sz = strlen(src); malloc(sz + 1);` + */ + Expr getSizeExprSource(Expr base, int offset) { + if this.getSizeExpr() instanceof AddExpr + then + exists(AddExpr ae | + exists(Variable v | + // case 1: variable access + const in the size expression + this.getSizeExpr() = ae and + result = v.getAnAssignedValue() and + base = ae.getLeftOperand() and + offset = constOrZero(ae.getRightOperand()) and + DataFlow::localExprFlow(result, base) + or + // case 2: expr + const in the variable assignment + v.getAnAssignedValue() = ae and + result = ae and + base = ae.getLeftOperand() and + offset = constOrZero(ae.getRightOperand()) and + DataFlow::localExprFlow(result, this.getSizeExpr()) + ) + or + // case 3: function call + const + result = ae and + this.getSizeExpr() = ae and + ae.getLeftOperand() = base and + ae.getLeftOperand() instanceof FunctionCall and + offset = constOrZero(ae.getRightOperand()) + ) + else ( + offset = 0 and + // case 3: a variable is read in the size expression + // if the VariableAccess does not have a computable constant value, + // the source node could still be useful for data-flow and GVN comparisons + if this.getSizeExpr() instanceof VariableAccess + then + exists(Variable v | + v = this.getSizeExpr().(VariableAccess).getTarget() and + not v instanceof Field and + DataFlow::localExprFlow(v.getAnAssignedValue(), base) and + result = base + ) + else ( + // Case 4: no variable access in the size expression + // This case is equivalent to getSizeExpr. + base = this.getSizeExpr() and + result = base + ) + ) + } + + override int getFixedSize() { result = getMaxStatedValue(getSizeExpr()) } + + override predicate isNotNullTerminated() { none() } + } + + /** + * A `PointerToObjectSource` which is an `AddressOfExpr` to a variable + * that is not a field or pointer type. + */ + private class AddressOfExprSource extends PointerToObjectSource instanceof AddressOfExpr { + AddressOfExprSource() { + exists(Variable v | + v = this.getOperand().(VariableAccess).getTarget() and + not v.getUnderlyingType() instanceof PointerType and + not v instanceof Field + ) + } + + override Expr getPointer() { result = this } + + override Expr getSizeExpr() { none() } + + override int getFixedSize() { + result = min(this.(AddressOfExpr).getOperand().getType().getSize()) + } + + override predicate isNotNullTerminated() { none() } + } + + /** + * A `PointerToObjectSource` which is a `VariableAccess` to a static buffer + */ + private class StaticBufferAccessSource extends PointerToObjectSource instanceof VariableAccess { + StaticBufferAccessSource() { + not this.getTarget() instanceof Field and + not this.getTarget().getUnspecifiedType() instanceof PointerType and + this.getTarget().getUnderlyingType().(ArrayType).getSize() > 0 + } + + override Expr getPointer() { result = this } + + override Expr getSizeExpr() { none() } + + override int getFixedSize() { + result = this.(VariableAccess).getTarget().getUnderlyingType().(ArrayType).getSize() + } + + override predicate isNotNullTerminated() { + // StringLiteral::getOriginalLength uses Expr::getValue, which implicitly truncates string literal + // values to the length fitting the buffer they are assigned to, thus breaking the 'obvious' check. + // Note: `CharArrayInitializedWithStringLiteral` falsely reports the string literal length in certain cases + // (e.g. when the string literal contains escape characters or on certain compilers), resulting in false-negatives + exists(CharArrayInitializedWithStringLiteral init | + init = this.(VariableAccess).getTarget().getInitializer().getExpr() and + init.getStringLiteralLength() + 1 > init.getContainerLength() + ) + or + // if the buffer is not initialized and does not have any memset call zeroing it, it is not null-terminated. + // note that this heuristic does not evaluate the order of the memset calls made and whether they dominate + // any use of the buffer by functions requiring it to be null-terminated. + ( + this.(VariableAccess).getTarget().getUnspecifiedType().(ArrayType).getBaseType() instanceof + PlainCharType + or + this.(VariableAccess).getTarget().getUnspecifiedType().(ArrayType).getBaseType() instanceof + Wchar_t + ) and + not this.(VariableAccess).getTarget() instanceof GlobalVariable and + not exists(this.(VariableAccess).getTarget().getInitializer()) and + // exclude any BufferAccessLibraryFunction that writes to the buffer and does not require + // a null-terminated buffer argument for its write argument + not exists( + BufferAccessLibraryFunctionCall fc, BufferAccessLibraryFunction f, int writeParamIndex + | + f = fc.getTarget() and + writeParamIndex = f.getWriteParamIndex() and + not f.getANullTerminatedParameterIndex(writeParamIndex) and + fc.getArgument(writeParamIndex) = this.(VariableAccess).getTarget().getAnAccess() + ) and + // exclude any buffers that have an assignment, deref, or array expr with a zero constant + // note: heuristically implemented using getAChild*() + not exists(AssignExpr assign | + assign.getRValue().getValue().toInt() = 0 and + assign.getLValue().getAChild*() = this.(VariableAccess).getTarget().getAnAccess() + ) + // note: the case of initializers that are not string literals and non-zero constants is not handled here. + // e.g. char buf[10] = {1, 2, 3, 4, 5, 6, 7, 8, 9, 10}; (not null-terminated) + // char buf[10] = { 1 }; (not null-terminated) + } + } + + /** + * A `PointerToObjectSource` which is a string literal that is not + * part of an variable initializer (to deduplicate `StaticBufferAccessSource`) + */ + private class StringLiteralSource extends PointerToObjectSource instanceof StringLiteral { + StringLiteralSource() { not this instanceof CharArrayInitializedWithStringLiteral } + + override Expr getPointer() { result = this } + + override Expr getSizeExpr() { none() } + + override int getFixedSize() { + // (length of the string literal + null terminator) * (size of the base type) + result = + this.(StringLiteral).getOriginalLength() * + this.(StringLiteral).getUnderlyingType().(DerivedType).getBaseType().getSize() + } + + override predicate isNotNullTerminated() { none() } + } + + private class PointerToObjectSourceOrSizeToBufferAccessFunctionConfig extends DataFlow::Configuration { + PointerToObjectSourceOrSizeToBufferAccessFunctionConfig() { + this = "PointerToObjectSourceOrSizeToBufferAccessFunctionConfig" + } + + override predicate isSource(DataFlow::Node source) { + source.asExpr() instanceof PointerToObjectSource + or + exists(PointerToObjectSource ptr | + source.asExpr() = ptr.getSizeExpr() or + source.asExpr() = ptr.(DynamicAllocationSource).getSizeExprSource(_, _) + ) + } + + override predicate isSink(DataFlow::Node sink) { + exists(BufferAccess ba, Expr arg | + ( + arg = ba.(BufferAccessLibraryFunctionCall).getAnArgument() or + arg = ba.getARelevantExpr() + ) and + ( + sink.asExpr() = arg or + exists(getArithmeticOffsetValue(arg, sink.asExpr())) + ) + ) + } + + override predicate isBarrierOut(DataFlow::Node node) { + // the default interprocedural data-flow model flows through any array assignment expressions + // to the qualifier (array base or pointer dereferenced) instead of the individual element + // that the assignment modifies. this default behaviour causes false positives for any future + // access of the array base, so remove the assignment edge at the expense of false-negatives. + exists(AssignExpr a | + node.asExpr() = a.getRValue().getAChild*() and + ( + a.getLValue() instanceof ArrayExpr or + a.getLValue() instanceof PointerDereferenceExpr + ) + ) + or + // remove flow from `src` to `dst` in a buffer access function call + // the standard library models such flow through functions such as memcpy, strcpy, etc. + exists(BufferAccessLibraryFunctionCall fc | node.asExpr() = fc.getReadArg().getAChild*()) + or + node.asDefiningArgument() instanceof AddressOfExpr + } + } + + private predicate hasFlowFromBufferOrSizeExprToUse(Expr source, Expr use) { + exists(PointerToObjectSourceOrSizeToBufferAccessFunctionConfig config, Expr useOrChild | + exists(getArithmeticOffsetValue(use, useOrChild)) and + config.hasFlow(DataFlow::exprNode(source), DataFlow::exprNode(useOrChild)) + ) + } + + private predicate bufferUseComputableBufferSize( + Expr bufferUse, PointerToObjectSource source, int size + ) { + // flow from a PointerToObjectSource for which we can compute the exact size + size = source.getFixedSize() and + hasFlowFromBufferOrSizeExprToUse(source, bufferUse) + } + + private predicate bufferUseNonComputableSize(Expr bufferUse, Expr source) { + not bufferUseComputableBufferSize(bufferUse, source, _) and + hasFlowFromBufferOrSizeExprToUse(source.(DynamicAllocationSource), bufferUse) + } + + /** + * Relates `sizeExpr`, a buffer access size expresion, to `source`, which is either `sizeExpr` + * if `sizeExpr` has a stated value, or a `DynamicAllocationSource::getSizeExprSource` for which + * we can compute the exact size and that has flow to `sizeExpr`. + */ + private predicate sizeExprComputableSize(Expr sizeExpr, Expr source, int size) { + // computable direct value, e.g. array_base[10], where "10" is sizeExpr and source. + size = getMinStatedValue(sizeExpr) and + source = sizeExpr + or + // computable source value that flows to the size expression, e.g. in cases such as the following: + // size_t sz = 10; + // malloc(sz); + // ... sz passed interprocedurally to another function ... + // use(p, sz + 1); + size = source.(DynamicAllocationSource).getFixedSize() + getArithmeticOffsetValue(sizeExpr, _) and + hasFlowFromBufferOrSizeExprToUse(source.(DynamicAllocationSource).getSizeExprSource(_, _), + sizeExpr) + } + + /** + * If the size is not computable locally, then it is either: + * + * 1. A dynamic allocation, from which we can get `getSizeExprSource()', from which + * we can either check specific logic (e.g. string length with offset) or compare GVNs. + * 2. An unrelateable size expression, which we might, however, be able to compute the bounds + * of and check against the buffer size, if that is known. + * + * In case 2, this predicate does not hold. + * + * NOTE: This predicate does not actually perform the above mentioned heuristics. + */ + predicate sizeExprNonComputableSize( + Expr bufferSizeArg, Expr alloc, Expr allocSize, Expr allocSizeBase, int offset + ) { + bufferSizeArg = any(BufferAccess access).getARelevantExpr() and + not sizeExprComputableSize(bufferSizeArg, alloc, _) and + allocSize = alloc.(DynamicAllocationSource).getSizeExprSource(allocSizeBase, offset) and + hasFlowFromBufferOrSizeExprToUse(allocSize, bufferSizeArg) + } + + /** + * Holds if `arg` refers to the number of characters excluding a null terminator + */ + bindingset[fc, arg] + private predicate isArgNumCharacters(BufferAccessLibraryFunctionCall fc, Expr arg) { + exists(int i | + arg = fc.getArgument(i) and + fc.getTarget().(BufferAccessLibraryFunction).getALengthParameterIndex(i) + ) + } + + /** + * Returns '1' if `arg` refers to the number of characters excluding a null terminator, + * otherwise '0' if `arg` refers to the number of characters including a null terminator. + */ + bindingset[fc, arg] + private int argNumCharactersOffset(BufferAccess fc, Expr arg) { + if isArgNumCharacters(fc, arg) then result = 1 else result = 0 + } + + /** + * Holds if the call `fc` may result in an invalid buffer access due a read buffer being bigger + * than the write buffer. This heuristic is useful for cases such as strcpy(dst, src). + */ + predicate isReadBufferSizeGreaterThanWriteBufferSize( + Expr readBuffer, Expr writeBuffer, int readBufferSize, int writeBufferSize, + BufferAccessLibraryFunctionCall fc + ) { + readBuffer = fc.getReadArg() and + writeBuffer = fc.getWriteArg() and + exists(int readSizeMult, int writeSizeMult, int readBufferSizeBase, int writeBufferSizeBase | + // the read and write buffer sizes must be derived from computable constants + bufferUseComputableBufferSize(readBuffer, _, readBufferSizeBase) and + bufferUseComputableBufferSize(writeBuffer, _, writeBufferSizeBase) and + // calculate the buffer byte sizes (size base is the number of elements) + readSizeMult = fc.getReadSizeArgMult() and + writeSizeMult = fc.getWriteSizeArgMult() and + readBufferSize = readBufferSizeBase - readSizeMult * getArithmeticOffsetValue(readBuffer, _) and + writeBufferSize = + writeBufferSizeBase - writeSizeMult * getArithmeticOffsetValue(writeBuffer, _) and + // the read buffer size is larger than the write buffer size + readBufferSize > writeBufferSize and + ( + // if a size arg exists and it is computable, then it must be <= to the write buffer size + exists(fc.getWriteSizeArg(writeSizeMult)) + implies + ( + sizeExprComputableSize(fc.getWriteSizeArg(writeSizeMult), _, _) and + not exists(Expr writeSizeArg, int writeSizeArgValue | + writeSizeArg = fc.getWriteSizeArg(writeSizeMult) and + sizeExprComputableSize(writeSizeArg, _, writeSizeArgValue) and + writeSizeMult.(float) * + (writeSizeArgValue + argNumCharactersOffset(fc, writeSizeArg)).(float) <= + writeBufferSize + ) + ) + ) + ) + } + + /** + * Holds if `sizeArg` is the right operand of a `PointerSubExpr` + */ + predicate isSizeArgPointerSubExprRightOperand(Expr sizeArg) { + exists(PointerSubExpr subExpr | sizeArg = subExpr.getRightOperand()) + } + + /** + * Holds if the BufferAccess `bufferAccess` results in a buffer overflow due to a size argument + * or buffer access offset being greater in size than the buffer size being accessed or written to. + */ + predicate isSizeArgGreaterThanBufferSize( + Expr bufferArg, Expr sizeArg, PointerToObjectSource bufferSource, int computedBufferSize, + int computedSizeAccessed, BufferAccess bufferAccess + ) { + exists(float sizeMult, int bufferArgSize, int sizeArgValue | + bufferAccess.hasABuffer(bufferArg, sizeArg, sizeMult) and + bufferUseComputableBufferSize(bufferArg, bufferSource, bufferArgSize) and + // If the bufferArg is an access of a static buffer, do not look for "long distance" sources + (bufferArg instanceof StaticBufferAccessSource implies bufferSource = bufferArg) and + sizeExprComputableSize(sizeArg, _, sizeArgValue) and + computedBufferSize = bufferArgSize - sizeMult.(float) * getArithmeticOffsetValue(bufferArg, _) and + // Handle cases such as *(ptr - 1) + ( + if isSizeArgPointerSubExprRightOperand(sizeArg) + then + computedSizeAccessed = + sizeMult.(float) * + (-sizeArgValue + argNumCharactersOffset(bufferAccess, sizeArg)).(float) + else + computedSizeAccessed = + sizeMult.(float) * + (sizeArgValue + argNumCharactersOffset(bufferAccess, sizeArg)).(float) + ) and + computedBufferSize < computedSizeAccessed + ) + } + + /** + * Holds if the call `fc` may result in an invalid buffer access due to a buffer argument + * being accessed at an offset that is greater than the size of the buffer. + */ + predicate isBufferOffsetGreaterThanBufferSize( + Expr bufferArg, int bufferArgOffset, int bufferSize, BufferAccessLibraryFunctionCall fc + ) { + exists(int bufferElementSize | + ( + bufferArg = fc.getReadArg() and + bufferElementSize = fc.getReadSizeArgMult() + or + bufferArg = fc.getWriteArg() and + bufferElementSize = fc.getWriteSizeArgMult() + ) and + bufferUseComputableBufferSize(bufferArg, _, bufferSize) and + bufferArgOffset = getArithmeticOffsetValue(bufferArg, _) * bufferElementSize and + bufferArgOffset >= bufferSize + ) + } + + /** + * Holds if `a` and `b` are function calls to the same target function and + * have identical arguments (determined by their global value number or `VariableAccess` targets). + */ + bindingset[a, b] + private predicate areFunctionCallsSyntacticallySame(FunctionCall a, FunctionCall b) { + a.getTarget() = b.getTarget() and + ( + exists(a.getAnArgument()) + implies + not exists(int i, Expr argA, Expr argB | + i = [0 .. a.getTarget().getNumberOfParameters() - 1] + | + argA = a.getArgument(i) and + argB = b.getArgument(i) and + not globalValueNumber(argA) = globalValueNumber(argB) and + not argA.(VariableAccess).getTarget() = argB.(VariableAccess).getTarget() + ) + ) + } + + /** + * Holds if `a` and `b` have the same global value number or are syntactically identical function calls + */ + bindingset[a, b] + private predicate isGVNOrFunctionCallSame(Expr a, Expr b) { + globalValueNumber(a) = globalValueNumber(b) + or + areFunctionCallsSyntacticallySame(a, b) + } + + /** + * Holds if the BufferAccess is accessed with a `base + accessOffset` on a buffer that was + * allocated a size of the form `base + allocationOffset`. + */ + predicate isGVNOffsetGreaterThanBufferSize( + Expr bufferArg, Expr bufferSizeArg, Expr sourceSizeExpr, int sourceSizeExprOffset, + int sizeArgOffset, BufferAccessLibraryFunctionCall fc + ) { + exists( + DynamicAllocationSource source, Expr sourceSizeExprBase, int bufferArgOffset, int sizeMult + | + ( + bufferArg = fc.getWriteArg() and + bufferSizeArg = fc.getWriteSizeArg(sizeMult) + or + bufferArg = fc.getReadArg() and + bufferSizeArg = fc.getReadSizeArg(sizeMult) + ) and + sourceSizeExpr = source.getSizeExprSource(sourceSizeExprBase, sourceSizeExprOffset) and + bufferUseNonComputableSize(bufferArg, source) and + not globalValueNumber(sourceSizeExpr) = globalValueNumber(bufferSizeArg) and + exists(Expr sizeArgBase | + sizeArgOffset = getArithmeticOffsetValue(bufferSizeArg.getAChild*(), sizeArgBase) and + isGVNOrFunctionCallSame(sizeArgBase, sourceSizeExprBase) and + bufferArgOffset = getArithmeticOffsetValue(bufferArg, _) and + sourceSizeExprOffset + bufferArgOffset < sizeArgOffset + ) + ) + } + + /** + * Holds if the call `fc` may result in an invalid buffer access due to a standard library + * function being called with a null pointer as a buffer argument while expecting only non-null input. + */ + predicate isMandatoryBufferArgNull(Expr bufferArg, BufferAccessLibraryFunctionCall fc) { + exists(int i | + i = + [ + fc.getTarget().(BufferAccessLibraryFunction).getReadParamIndex(), + fc.getTarget().(BufferAccessLibraryFunction).getWriteParamIndex() + ] and + not fc.getTarget().(BufferAccessLibraryFunction).getAPermissiblyNullParameterIndex(i) and + bufferArg = fc.getArgument(i) and + getMinStatedValue(bufferArg) = 0 + ) + } + + /** + * Holds if the call `fc` may result in an invalid buffer access due to a standard library function + * receiving a non-null terminated buffer as a buffer argument and accessing it. + */ + predicate isNullTerminatorMissingFromArg( + Expr arg, PointerToObjectSource source, BufferAccessLibraryFunctionCall fc + ) { + exists(int i, Expr argChild | + fc.getTarget().(BufferAccessLibraryFunction).getANullTerminatedParameterIndex(i) and + fc.getArgument(i) = arg and + source.isNotNullTerminated() and + argChild = arg.getAChild*() and + // ignore cases like strcpy(irrelevant_func(non_null_terminated_str, ...), src) + not exists(FunctionCall other | + not other = fc and + other.getAnArgument().getAChild*() = argChild + ) and + hasFlowFromBufferOrSizeExprToUse(source, argChild) + ) + } + + predicate isSizeArgNotCheckedLessThanFixedBufferSize( + Expr bufferArg, Expr sizeArg, PointerToObjectSource bufferSource, int bufferArgSize, + BufferAccess bufferAccess, int sizeArgUpperBound, int sizeMult + ) { + bufferAccess.hasABuffer(bufferArg, sizeArg, sizeMult) and + bufferUseComputableBufferSize(bufferArg, bufferSource, bufferArgSize) and + // If the bufferArg is an access of a static buffer, do not look for "long distant" sources + (bufferArg instanceof StaticBufferAccessSource implies bufferSource = bufferArg) and + // Not a size expression for which we can compute a specific size + not sizeExprComputableSize(sizeArg, _, _) and + // Range analysis considers the upper bound to be larger than the buffer size + sizeArgUpperBound = upperBound(sizeArg) and + // Ignore bitwise & operations + not sizeArg instanceof BitwiseAndExpr and + sizeArgUpperBound * sizeMult > bufferArgSize and + // There isn't a relational operation guarding this access that seems to check the + // upper bound against a plausible terminal value + not exists(RelationalOperation relOp, Expr checkedUpperBound | + globalValueNumber(relOp.getLesserOperand()) = globalValueNumber(sizeArg) and + checkedUpperBound = relOp.getGreaterOperand() and + // There's no closer inferred bounds - otherwise we let range analysis check it + upperBound(checkedUpperBound) = exprMaxVal(checkedUpperBound) + ) + } + + predicate isSizeArgNotCheckedGreaterThanZero( + Expr bufferArg, Expr sizeArg, PointerToObjectSource bufferSource, BufferAccess bufferAccess + ) { + exists(float sizeMult | + bufferAccess.hasABuffer(bufferArg, sizeArg, sizeMult) and + ( + bufferUseComputableBufferSize(bufferArg, bufferSource, _) or + bufferUseNonComputableSize(bufferArg, bufferSource) + ) and + ( + // Not a size expression for which we can compute a specific size + not sizeExprComputableSize(sizeArg, _, _) and + // and with a lower bound that is less than zero, taking into account offsets + lowerBound(sizeArg) + getArithmeticOffsetValue(bufferArg, _) < 0 + or + // A size expression for which we can compute a specific size and that size is less than zero + sizeExprComputableSize(sizeArg, _, _) and + ( + if isSizeArgPointerSubExprRightOperand(sizeArg) + then -sizeArg.getValue().toInt() + getArithmeticOffsetValue(bufferArg, _) < 0 + else sizeArg.getValue().toInt() + getArithmeticOffsetValue(bufferArg, _) < 0 + ) + ) + ) + } + + private string bufferArgType(BufferAccessLibraryFunctionCall fc, Expr bufferArg) { + fc.getReadArg() = bufferArg and + result = "read buffer" + or + fc.getWriteArg() = bufferArg and + result = "write buffer" + } + + predicate problems( + BufferAccessLibraryFunctionCall fc, string message, Expr bufferArg, string bufferArgStr, + Expr sizeOrOtherBufferArg, string otherStr + ) { + exists(int bufferArgSize, int sizeArgValue | + isSizeArgGreaterThanBufferSize(bufferArg, sizeOrOtherBufferArg, _, bufferArgSize, + sizeArgValue, fc) and + bufferArgStr = bufferArgType(fc, bufferArg) and + message = + "The size of the $@ passed to " + fc.getTarget().getName() + " is " + bufferArgSize + + " bytes, but the " + "$@ is " + sizeArgValue + " bytes." and + otherStr = "size argument" + or + isBufferOffsetGreaterThanBufferSize(bufferArg, sizeArgValue, bufferArgSize, fc) and + bufferArgStr = bufferArgType(fc, bufferArg) and + message = + "The $@ passed to " + fc.getTarget().getName() + " is " + bufferArgSize + + " bytes, but an offset of " + sizeArgValue + " bytes is used to access it." and + otherStr = "" and + sizeOrOtherBufferArg = bufferArg + ) + or + isMandatoryBufferArgNull(bufferArg, fc) and + message = "The $@ passed to " + fc.getTarget().getName() + " is null." and + bufferArgStr = "argument" and + otherStr = "" and + sizeOrOtherBufferArg = bufferArg + or + isNullTerminatorMissingFromArg(bufferArg, _, fc) and + message = "The $@ passed to " + fc.getTarget().getName() + " might not be null-terminated." and + bufferArgStr = "argument" and + otherStr = "" and + sizeOrOtherBufferArg = bufferArg + or + exists(int readBufferSize, int writeBufferSize | + isReadBufferSizeGreaterThanWriteBufferSize(bufferArg, sizeOrOtherBufferArg, readBufferSize, + writeBufferSize, fc) and + message = + "The size of the $@ passed to " + fc.getTarget().getName() + " is " + readBufferSize + + " bytes, but the size of the $@ is only " + writeBufferSize + " bytes." and + bufferArgStr = "read buffer" and + otherStr = "write buffer" + ) + or + exists(int accessOffset, Expr source | + isGVNOffsetGreaterThanBufferSize(bufferArg, _, source, _, accessOffset, fc) and + message = + "The $@ passed to " + fc.getTarget().getName() + " is accessed at an excessive offset of " + + accessOffset + " element(s) from the $@." and + bufferArgStr = bufferArgType(fc, bufferArg) and + sizeOrOtherBufferArg = source and + otherStr = "allocation size base" + ) + } +} diff --git a/c/common/src/codingstandards/c/Pointers.qll b/c/common/src/codingstandards/c/Pointers.qll index 6658ec9e81..86e2c02d30 100644 --- a/c/common/src/codingstandards/c/Pointers.qll +++ b/c/common/src/codingstandards/c/Pointers.qll @@ -87,3 +87,32 @@ class PointerToObjectType extends PointerType { ) } } + +/** + * Gets the base type of a pointer or array type. In the case of an array of + * arrays, the inner base type is returned. + * + * Copied from IncorrectPointerScalingCommon.qll. + */ +Type baseType(Type t) { + ( + exists(PointerType dt | + dt = t.getUnspecifiedType() and + result = dt.getBaseType().getUnspecifiedType() + ) + or + exists(ArrayType at | + at = t.getUnspecifiedType() and + not at.getBaseType().getUnspecifiedType() instanceof ArrayType and + result = at.getBaseType().getUnspecifiedType() + ) + or + exists(ArrayType at, ArrayType at2 | + at = t.getUnspecifiedType() and + at2 = at.getBaseType().getUnspecifiedType() and + result = baseType(at2) + ) + ) and + // Make sure that the type has a size and that it isn't ambiguous. + strictcount(result.getSize()) = 1 +} diff --git a/c/misra/src/rules/RULE-21-17/StringFunctionPointerArgumentOutOfBounds.ql b/c/misra/src/rules/RULE-21-17/StringFunctionPointerArgumentOutOfBounds.ql new file mode 100644 index 0000000000..cf1e8cda1b --- /dev/null +++ b/c/misra/src/rules/RULE-21-17/StringFunctionPointerArgumentOutOfBounds.ql @@ -0,0 +1,27 @@ +/** + * @id c/misra/string-function-pointer-argument-out-of-bounds + * @name RULE-21-17: Use of the string handling functions from shall not result in accesses beyond the bounds + * @description Use of string manipulation functions from with improper buffer sizes can + * result in out-of-bounds buffer accesses. + * @kind problem + * @precision high + * @problem.severity error + * @tags external/misra/id/rule-21-17 + * correctness + * security + * external/misra/obligation/mandatory + */ + +import cpp +import codingstandards.c.misra +import codingstandards.c.OutOfBounds + +class RULE_21_17_Subset_FC = OOB::SimpleStringLibraryFunctionCall; + +from + RULE_21_17_Subset_FC fc, string message, Expr bufferArg, string bufferArgStr, + Expr sizeOrOtherBufferArg, string otherStr +where + not isExcluded(fc, OutOfBoundsPackage::stringFunctionPointerArgumentOutOfBoundsQuery()) and + OOB::problems(fc, message, bufferArg, bufferArgStr, sizeOrOtherBufferArg, otherStr) +select fc, message, bufferArg, bufferArgStr, sizeOrOtherBufferArg, otherStr \ No newline at end of file diff --git a/c/misra/src/rules/RULE-21-18/StringLibrarySizeArgumentOutOfBounds.ql b/c/misra/src/rules/RULE-21-18/StringLibrarySizeArgumentOutOfBounds.ql new file mode 100644 index 0000000000..3554b2791e --- /dev/null +++ b/c/misra/src/rules/RULE-21-18/StringLibrarySizeArgumentOutOfBounds.ql @@ -0,0 +1,35 @@ +/** + * @id c/misra/string-library-size-argument-out-of-bounds + * @name RULE-21-18: The size_t argument passed to any function in shall have an appropriate value + * @description Passing a size_t argument that is non-positive or greater than the size of the + * smallest buffer argument to any function in may result in out-of-bounds + * buffer accesses. + * @kind problem + * @precision high + * @problem.severity error + * @tags external/misra/id/rule-21-18 + * correctness + * security + * external/misra/obligation/mandatory + */ + +import cpp +import codingstandards.c.misra +import codingstandards.c.OutOfBounds + +class RULE_21_18_Subset_FC extends OOB::BufferAccessLibraryFunctionCall { + RULE_21_18_Subset_FC() { + this.getTarget().getName() = + OOB::getNameOrInternalName([ + "mem" + ["chr", "cmp", "cpy", "move", "set"], "str" + ["ncat", "ncmp", "ncpy", "xfrm"] + ]) + } +} + +from + RULE_21_18_Subset_FC fc, string message, Expr bufferArg, string bufferArgStr, + Expr sizeOrOtherBufferArg, string otherStr +where + not isExcluded(fc, OutOfBoundsPackage::stringLibrarySizeArgumentOutOfBoundsQuery()) and + OOB::problems(fc, message, bufferArg, bufferArgStr, sizeOrOtherBufferArg, otherStr) +select fc, message, bufferArg, bufferArgStr, sizeOrOtherBufferArg, otherStr diff --git a/c/misra/test/rules/RULE-21-17/StringFunctionPointerArgumentOutOfBounds.expected b/c/misra/test/rules/RULE-21-17/StringFunctionPointerArgumentOutOfBounds.expected new file mode 100644 index 0000000000..a7e269e292 --- /dev/null +++ b/c/misra/test/rules/RULE-21-17/StringFunctionPointerArgumentOutOfBounds.expected @@ -0,0 +1,26 @@ +| test.c:31:5:31:10 | call to strcat | The $@ passed to strcat might not be null-terminated. | test.c:31:12:31:15 | buf1 | argument | test.c:31:12:31:15 | buf1 | | +| test.c:36:5:36:10 | call to strcat | The size of the $@ passed to strcat is 6 bytes, but the size of the $@ is only 5 bytes. | test.c:36:24:36:30 | 12345 | read buffer | test.c:36:12:36:19 | call to get_ca_5 | write buffer | +| test.c:38:5:38:10 | call to strcat | The size of the $@ passed to strcat is 5 bytes, but the size of the $@ is only 4 bytes. | test.c:38:28:38:33 | 1234 | read buffer | test.c:38:12:38:25 | ... + ... | write buffer | +| test.c:43:5:43:10 | call to strchr | The $@ passed to strchr might not be null-terminated. | test.c:43:12:43:18 | ca5_bad | argument | test.c:43:12:43:18 | ca5_bad | | +| test.c:45:5:45:10 | call to strchr | The $@ passed to strchr is 5 bytes, but an offset of 5 bytes is used to access it. | test.c:45:12:45:23 | ... + ... | read buffer | test.c:45:12:45:23 | ... + ... | | +| test.c:47:5:47:11 | call to strrchr | The $@ passed to strrchr might not be null-terminated. | test.c:47:13:47:19 | ca5_bad | argument | test.c:47:13:47:19 | ca5_bad | | +| test.c:49:5:49:11 | call to strrchr | The $@ passed to strrchr is 5 bytes, but an offset of 5 bytes is used to access it. | test.c:49:13:49:24 | ... + ... | read buffer | test.c:49:13:49:24 | ... + ... | | +| test.c:53:5:53:10 | call to strcmp | The $@ passed to strcmp might not be null-terminated. | test.c:53:22:53:28 | ca5_bad | argument | test.c:53:22:53:28 | ca5_bad | | +| test.c:55:5:55:10 | call to strcmp | The $@ passed to strcmp might not be null-terminated. | test.c:55:12:55:18 | ca5_bad | argument | test.c:55:12:55:18 | ca5_bad | | +| test.c:58:5:58:11 | call to strcoll | The $@ passed to strcoll might not be null-terminated. | test.c:58:23:58:29 | ca5_bad | argument | test.c:58:23:58:29 | ca5_bad | | +| test.c:60:5:60:11 | call to strcoll | The $@ passed to strcoll might not be null-terminated. | test.c:60:13:60:19 | ca5_bad | argument | test.c:60:13:60:19 | ca5_bad | | +| test.c:66:5:66:10 | call to strcpy | The size of the $@ passed to strcpy is 6 bytes, but the size of the $@ is only 5 bytes. | test.c:66:22:66:28 | test1 | read buffer | test.c:66:12:66:19 | ca5_good | write buffer | +| test.c:70:5:70:10 | call to strcpy | The $@ passed to strcpy might not be null-terminated. | test.c:70:24:70:30 | ca5_bad | argument | test.c:70:24:70:30 | ca5_bad | | +| test.c:71:5:71:10 | call to strcpy | The size of the $@ passed to strcpy is 6 bytes, but the size of the $@ is only 5 bytes. | test.c:71:24:71:31 | ca6_good | read buffer | test.c:71:12:71:19 | call to get_ca_5 | write buffer | +| test.c:76:5:76:11 | call to strcspn | The $@ passed to strcspn might not be null-terminated. | test.c:76:13:76:19 | ca5_bad | argument | test.c:76:13:76:19 | ca5_bad | | +| test.c:78:5:78:11 | call to strcspn | The $@ passed to strcspn is null. | test.c:78:13:78:16 | 0 | argument | test.c:78:13:78:16 | 0 | | +| test.c:80:5:80:10 | call to strspn | The $@ passed to strspn might not be null-terminated. | test.c:80:12:80:18 | ca5_bad | argument | test.c:80:12:80:18 | ca5_bad | | +| test.c:82:5:82:10 | call to strspn | The $@ passed to strspn is null. | test.c:82:12:82:15 | 0 | argument | test.c:82:12:82:15 | 0 | | +| test.c:86:5:86:10 | call to strlen | The $@ passed to strlen might not be null-terminated. | test.c:86:12:86:18 | ca5_bad | argument | test.c:86:12:86:18 | ca5_bad | | +| test.c:88:5:88:10 | call to strlen | The $@ passed to strlen is 5 bytes, but an offset of 5 bytes is used to access it. | test.c:88:12:88:23 | ... + ... | read buffer | test.c:88:12:88:23 | ... + ... | | +| test.c:93:5:93:11 | call to strpbrk | The $@ passed to strpbrk might not be null-terminated. | test.c:93:13:93:19 | ca5_bad | argument | test.c:93:13:93:19 | ca5_bad | | +| test.c:95:5:95:11 | call to strpbrk | The $@ passed to strpbrk is null. | test.c:95:13:95:16 | 0 | argument | test.c:95:13:95:16 | 0 | | +| test.c:102:5:102:10 | call to strstr | The $@ passed to strstr might not be null-terminated. | test.c:102:12:102:18 | ca5_bad | argument | test.c:102:12:102:18 | ca5_bad | | +| test.c:111:5:111:10 | call to strtok | The $@ passed to strtok is null. | test.c:111:18:111:21 | 0 | argument | test.c:111:18:111:21 | 0 | | +| test.c:113:5:113:10 | call to strtok | The $@ passed to strtok might not be null-terminated. | test.c:113:12:113:18 | ca5_bad | argument | test.c:113:12:113:18 | ca5_bad | | +| test.c:117:5:117:10 | call to strtok | The $@ passed to strtok might not be null-terminated. | test.c:117:22:117:28 | ca6_bad | argument | test.c:117:22:117:28 | ca6_bad | | diff --git a/c/misra/test/rules/RULE-21-17/StringFunctionPointerArgumentOutOfBounds.qlref b/c/misra/test/rules/RULE-21-17/StringFunctionPointerArgumentOutOfBounds.qlref new file mode 100644 index 0000000000..001582cdd8 --- /dev/null +++ b/c/misra/test/rules/RULE-21-17/StringFunctionPointerArgumentOutOfBounds.qlref @@ -0,0 +1 @@ +rules/RULE-21-17/StringFunctionPointerArgumentOutOfBounds.ql \ No newline at end of file diff --git a/c/misra/test/rules/RULE-21-17/test.c b/c/misra/test/rules/RULE-21-17/test.c new file mode 100644 index 0000000000..900cb05eda --- /dev/null +++ b/c/misra/test/rules/RULE-21-17/test.c @@ -0,0 +1,119 @@ +// test partially copied from CERT-C ARR38-C test +#include +#include + +char *get_ca_5(void) { + void *ptr = malloc(5 * sizeof(char)); + memset(ptr, 0, 5 * sizeof(char)); + return (char *)ptr; +} + +void test(void) { + char ca5_good[5] = "test"; // ok + char ca5_bad[5] = "test1"; // no null terminator + char ca6_good[6] = "test1"; // ok + char ca6_bad[6] = "test12"; // no null terminator + + // strcat + { + char buf0[10]; // memset after first use + char buf1[10]; // no memset + char buf2[10]; // memset before first use + char buf3[10] = {'\0'}; + char buf4[10] = "12345"; + + strcat(buf0, " "); // NON_COMPLIANT[FALSE_NEGATIVE] - not null terminated at + // initialization + + memset(buf0, 0, sizeof(buf0)); // COMPLIANT + memset(buf2, 0, sizeof(buf2)); // COMPLIANT + + strcat(buf1, " "); // NON_COMPLIANT - not null terminated + strcat(buf2, " "); // COMPLIANT + strcat(buf3, " "); // COMPLIANT + strcat(buf4, "12345"); // NON_COMPLIANT[FALSE_NEGATIVE] + + strcat(get_ca_5(), "12345"); // NON_COMPLIANT + strcat(get_ca_5(), "1234"); // COMPLIANT + strcat(get_ca_5() + 1, "1234"); // NON_COMPLIANT + } + // strchr and strrchr + { + strchr(ca5_good, 't'); // COMPLIANT + strchr(ca5_bad, 't'); // NON_COMPLIANT + strchr(ca5_good + 4, 't'); // COMPLIANT + strchr(ca5_good + 5, 't'); // NON_COMPLIANT + strrchr(ca5_good, 1); // COMPLIANT + strrchr(ca5_bad, 1); // NON_COMPLIANT + strrchr(ca5_good + 4, 1); // COMPLIANT + strrchr(ca5_good + 5, 1); // NON_COMPLIANT + } + // strcmp and strcoll + { + strcmp(ca5_good, ca5_bad); // NON_COMPLIANT + strcmp(ca5_good, ca5_good); // COMPLIANT + strcmp(ca5_bad, ca5_good); // NON_COMPLIANT + strcmp(ca5_good, ca6_good); // COMPLIANT + strcmp(ca6_good, ca5_good); // COMPLIANT + strcoll(ca5_good, ca5_bad); // NON_COMPLIANT + strcoll(ca5_good, ca5_good); // COMPLIANT + strcoll(ca5_bad, ca5_good); // NON_COMPLIANT + strcoll(ca5_good, ca6_good); // COMPLIANT + strcoll(ca6_good, ca5_good); // COMPLIANT + } + // strcpy + { + strcpy(ca5_good, "test1"); // NON_COMPLIANT + strcpy(ca5_bad, "test"); // COMPLIANT + // strcpy to char buffer indirect + strcpy(get_ca_5(), ca5_good); // COMPLIANT + strcpy(get_ca_5(), ca5_bad); // NON_COMPLIANT + strcpy(get_ca_5(), ca6_good); // NON_COMPLIANT + } + // strcspn and strspn + { + strcspn(ca5_good, "test"); // COMPLIANT + strcspn(ca5_bad, "test"); // NON_COMPLIANT - not null-terminated + strcspn(ca5_good, "1234567890"); // COMPLIANT + strcspn(NULL, "12345"); // NON_COMPLIANT + strspn(ca5_good, "test"); // COMPLIANT + strspn(ca5_bad, "test"); // NON_COMPLIANT - not null-terminated + strspn(ca5_good, "1234567890"); // COMPLIANT + strspn(NULL, "12345"); // NON_COMPLIANT + } + // strlen + { + strlen(ca5_bad); // NON_COMPLIANT + strlen(ca5_good + 4); // COMPLIANT + strlen(ca5_good + 5); // NON_COMPLIANT + } + // strpbrk + { + strpbrk(ca5_good, "test"); // COMPLIANT + strpbrk(ca5_bad, "test"); // NON_COMPLIANT - not null-terminated + strpbrk(ca5_good, "1234567890"); // COMPLIANT + strpbrk(NULL, "12345"); // NON_COMPLIANT + } + // strstr + { + strstr("12345", "123"); // COMPLIANT + strstr("123", "12345"); // COMPLIANT + strstr(ca5_good, "test"); // COMPLIANT + strstr(ca5_bad, "test"); // NON_COMPLIANT - not null-terminated + strstr(ca5_good, "1234567890"); // COMPLIANT + } + // strtok + { + char ca5_good[5] = "test"; // ok + char ca5_bad[5] = "test1"; // no null terminator + char ca6_good[6] = "test1"; // ok + char ca6_bad[6] = "test12"; // no null terminator + strtok(NULL, NULL); // NON_COMPLIANT - 2nd arg null + strtok(NULL, ""); // COMPLIANT + strtok(ca5_bad, ""); // NON_COMPLIANT - 1st arg not null-terminated + strtok(ca5_good, ""); // COMPLIANT + strtok(ca6_good, ca5_good); // COMPLIANT + strtok(ca6_good + 4, ca6_good); // COMPLIANT + strtok(ca6_good, ca6_bad); // NON_COMPLIANT - 2nd arg not null-terminated + } +} \ No newline at end of file diff --git a/c/misra/test/rules/RULE-21-18/StringLibrarySizeArgumentOutOfBounds.expected b/c/misra/test/rules/RULE-21-18/StringLibrarySizeArgumentOutOfBounds.expected new file mode 100644 index 0000000000..fe3cbba947 --- /dev/null +++ b/c/misra/test/rules/RULE-21-18/StringLibrarySizeArgumentOutOfBounds.expected @@ -0,0 +1,35 @@ +| test.c:16:5:16:10 | call to memcpy | The size of the $@ passed to memcpy is 64 bytes, but the $@ is 65 bytes. | test.c:16:12:16:15 | buf1 | write buffer | test.c:16:24:16:39 | ... + ... | size argument | +| test.c:16:5:16:10 | call to memcpy | The size of the $@ passed to memcpy is 64 bytes, but the $@ is 65 bytes. | test.c:16:18:16:21 | buf2 | read buffer | test.c:16:24:16:39 | ... + ... | size argument | +| test.c:18:5:18:10 | call to memcpy | The size of the $@ passed to memcpy is 63 bytes, but the $@ is 64 bytes. | test.c:18:12:18:19 | ... + ... | write buffer | test.c:18:28:18:39 | sizeof() | size argument | +| test.c:18:5:18:10 | call to memcpy | The size of the $@ passed to memcpy is 64 bytes, but the size of the $@ is only 63 bytes. | test.c:18:22:18:25 | buf2 | read buffer | test.c:18:12:18:19 | ... + ... | write buffer | +| test.c:19:5:19:10 | call to memcpy | The size of the $@ passed to memcpy is 63 bytes, but the $@ is 128 bytes. | test.c:19:18:19:25 | ... + ... | read buffer | test.c:19:28:19:43 | ... * ... | size argument | +| test.c:19:5:19:10 | call to memcpy | The size of the $@ passed to memcpy is 64 bytes, but the $@ is 128 bytes. | test.c:19:12:19:15 | buf1 | write buffer | test.c:19:28:19:43 | ... * ... | size argument | +| test.c:25:5:25:10 | call to memcmp | The size of the $@ passed to memcmp is 64 bytes, but the $@ is 65 bytes. | test.c:25:12:25:15 | buf1 | write buffer | test.c:25:24:25:39 | ... + ... | size argument | +| test.c:25:5:25:10 | call to memcmp | The size of the $@ passed to memcmp is 64 bytes, but the $@ is 65 bytes. | test.c:25:18:25:21 | buf2 | read buffer | test.c:25:24:25:39 | ... + ... | size argument | +| test.c:27:5:27:10 | call to memcmp | The size of the $@ passed to memcmp is 63 bytes, but the $@ is 64 bytes. | test.c:27:12:27:19 | ... + ... | write buffer | test.c:27:28:27:39 | sizeof() | size argument | +| test.c:27:5:27:10 | call to memcmp | The size of the $@ passed to memcmp is 64 bytes, but the size of the $@ is only 63 bytes. | test.c:27:22:27:25 | buf2 | read buffer | test.c:27:12:27:19 | ... + ... | write buffer | +| test.c:28:5:28:10 | call to memcmp | The size of the $@ passed to memcmp is 63 bytes, but the $@ is 128 bytes. | test.c:28:18:28:25 | ... + ... | read buffer | test.c:28:28:28:43 | ... * ... | size argument | +| test.c:28:5:28:10 | call to memcmp | The size of the $@ passed to memcmp is 64 bytes, but the $@ is 128 bytes. | test.c:28:12:28:15 | buf1 | write buffer | test.c:28:28:28:43 | ... * ... | size argument | +| test.c:33:5:33:10 | call to memchr | The size of the $@ passed to memchr is 128 bytes, but the $@ is 129 bytes. | test.c:33:12:33:14 | buf | read buffer | test.c:33:20:33:34 | ... + ... | size argument | +| test.c:34:5:34:10 | call to memchr | The size of the $@ passed to memchr is 128 bytes, but the $@ is 129 bytes. | test.c:34:12:34:14 | buf | read buffer | test.c:34:20:34:34 | ... + ... | size argument | +| test.c:36:5:36:10 | call to memchr | The $@ passed to memchr is null. | test.c:36:12:36:15 | 0 | argument | test.c:36:12:36:15 | 0 | | +| test.c:41:5:41:10 | call to memset | The size of the $@ passed to memset is 128 bytes, but the $@ is 129 bytes. | test.c:41:12:41:14 | buf | write buffer | test.c:41:20:41:34 | ... + ... | size argument | +| test.c:42:5:42:10 | call to memset | The size of the $@ passed to memset is 128 bytes, but the $@ is 129 bytes. | test.c:42:12:42:14 | buf | write buffer | test.c:42:20:42:34 | ... + ... | size argument | +| test.c:44:5:44:10 | call to memset | The $@ passed to memset is null. | test.c:44:12:44:15 | 0 | argument | test.c:44:12:44:15 | 0 | | +| test.c:50:5:50:11 | call to memmove | The size of the $@ passed to memmove is 128 bytes, but the $@ is 129 bytes. | test.c:50:13:50:16 | buf1 | write buffer | test.c:50:25:50:40 | ... + ... | size argument | +| test.c:50:5:50:11 | call to memmove | The size of the $@ passed to memmove is 256 bytes, but the size of the $@ is only 128 bytes. | test.c:50:19:50:22 | buf2 | read buffer | test.c:50:13:50:16 | buf1 | write buffer | +| test.c:52:5:52:11 | call to memmove | The size of the $@ passed to memmove is 127 bytes, but the $@ is 128 bytes. | test.c:52:13:52:20 | ... + ... | write buffer | test.c:52:29:52:40 | sizeof() | size argument | +| test.c:52:5:52:11 | call to memmove | The size of the $@ passed to memmove is 256 bytes, but the size of the $@ is only 127 bytes. | test.c:52:23:52:26 | buf2 | read buffer | test.c:52:13:52:20 | ... + ... | write buffer | +| test.c:54:5:54:11 | call to memmove | The size of the $@ passed to memmove is 128 bytes, but the $@ is 256 bytes. | test.c:54:19:54:22 | buf1 | read buffer | test.c:54:25:54:36 | sizeof() | size argument | +| test.c:62:5:62:11 | call to strncpy | The size of the $@ passed to strncpy is 128 bytes, but the $@ is 129 bytes. | test.c:62:13:62:16 | buf1 | write buffer | test.c:62:25:62:40 | ... + ... | size argument | +| test.c:62:5:62:11 | call to strncpy | The size of the $@ passed to strncpy is 256 bytes, but the size of the $@ is only 128 bytes. | test.c:62:19:62:22 | buf2 | read buffer | test.c:62:13:62:16 | buf1 | write buffer | +| test.c:64:5:64:11 | call to strncpy | The size of the $@ passed to strncpy is 127 bytes, but the $@ is 128 bytes. | test.c:64:13:64:20 | ... + ... | write buffer | test.c:64:29:64:40 | sizeof() | size argument | +| test.c:64:5:64:11 | call to strncpy | The size of the $@ passed to strncpy is 256 bytes, but the size of the $@ is only 127 bytes. | test.c:64:23:64:26 | buf2 | read buffer | test.c:64:13:64:20 | ... + ... | write buffer | +| test.c:77:5:77:11 | call to strncat | The $@ passed to strncat might not be null-terminated. | test.c:77:13:77:16 | buf1 | argument | test.c:77:13:77:16 | buf1 | | +| test.c:81:5:81:11 | call to strncat | The size of the $@ passed to strncat is 6 bytes, but the size of the $@ is only 5 bytes. | test.c:81:25:81:31 | 12345 | read buffer | test.c:81:13:81:20 | call to get_ca_5 | write buffer | +| test.c:83:5:83:11 | call to strncat | The size of the $@ passed to strncat is 5 bytes, but the size of the $@ is only 4 bytes. | test.c:83:29:83:34 | 1234 | read buffer | test.c:83:13:83:26 | ... + ... | write buffer | +| test.c:93:5:93:11 | call to strncmp | The size of the $@ passed to strncmp is 5 bytes, but the $@ is 6 bytes. | test.c:93:23:93:30 | ca5_good | read buffer | test.c:93:33:93:33 | 6 | size argument | +| test.c:94:5:94:11 | call to strncmp | The size of the $@ passed to strncmp is 5 bytes, but the $@ is 6 bytes. | test.c:94:13:94:20 | ca5_good | write buffer | test.c:94:32:94:32 | 6 | size argument | +| test.c:94:5:94:11 | call to strncmp | The size of the $@ passed to strncmp is 5 bytes, but the $@ is 6 bytes. | test.c:94:23:94:29 | ca5_bad | read buffer | test.c:94:32:94:32 | 6 | size argument | +| test.c:101:5:101:11 | call to strxfrm | The size of the $@ passed to strxfrm is 64 bytes, but the $@ is 65 bytes. | test.c:101:13:101:15 | buf | write buffer | test.c:101:25:101:39 | ... + ... | size argument | +| test.c:103:5:103:11 | call to strxfrm | The $@ passed to strxfrm might not be null-terminated. | test.c:103:22:103:25 | buf2 | argument | test.c:103:22:103:25 | buf2 | | diff --git a/c/misra/test/rules/RULE-21-18/StringLibrarySizeArgumentOutOfBounds.qlref b/c/misra/test/rules/RULE-21-18/StringLibrarySizeArgumentOutOfBounds.qlref new file mode 100644 index 0000000000..9d3cdd6f64 --- /dev/null +++ b/c/misra/test/rules/RULE-21-18/StringLibrarySizeArgumentOutOfBounds.qlref @@ -0,0 +1 @@ +rules/RULE-21-18/StringLibrarySizeArgumentOutOfBounds.ql \ No newline at end of file diff --git a/c/misra/test/rules/RULE-21-18/test.c b/c/misra/test/rules/RULE-21-18/test.c new file mode 100644 index 0000000000..d1668a774b --- /dev/null +++ b/c/misra/test/rules/RULE-21-18/test.c @@ -0,0 +1,106 @@ +// test partially copied from CERT-C ARR38-C test +#include +#include + +char *get_ca_5(void) { + void *ptr = malloc(5 * sizeof(char)); + memset(ptr, 0, 5 * sizeof(char)); + return (char *)ptr; +} + +void test(void) { + { + char buf1[64]; + char buf2[64]; + memcpy(buf1, buf2, sizeof(buf1)); // COMPLIANT + memcpy(buf1, buf2, sizeof(buf1) + 1); // NON_COMPLIANT + memcpy(buf1, buf2, sizeof(buf1) - 1); // COMPLIANT + memcpy(buf1 + 1, buf2, sizeof(buf1)); // NON_COMPLIANT + memcpy(buf1, buf2 + 1, sizeof(buf1) * 2); // NON_COMPLIANT + } + { + char buf1[64]; + char buf2[64]; + memcmp(buf1, buf2, sizeof(buf1)); // COMPLIANT + memcmp(buf1, buf2, sizeof(buf1) + 1); // NON_COMPLIANT + memcmp(buf1, buf2, sizeof(buf1) - 1); // COMPLIANT + memcmp(buf1 + 1, buf2, sizeof(buf1)); // NON_COMPLIANT + memcmp(buf1, buf2 + 1, sizeof(buf1) * 2); // NON_COMPLIANT + } + { + char buf[128]; + memchr(buf, 0, sizeof(buf)); // COMPLIANT + memchr(buf, 0, sizeof(buf) + 1); // NON_COMPLIANT + memchr(buf, 0, sizeof(buf) + 1); // NON_COMPLIANT + memchr(buf, 0, sizeof(buf) - 1); // COMPLIANT + memchr(NULL, 0, sizeof(buf)); // NON_COMPLIANT + } + { + char buf[128]; + memset(buf, 0, sizeof(buf)); // COMPLIANT + memset(buf, 0, sizeof(buf) + 1); // NON_COMPLIANT + memset(buf, 0, sizeof(buf) + 1); // NON_COMPLIANT + memset(buf, 0, sizeof(buf) - 1); // COMPLIANT + memset(NULL, 0, sizeof(buf)); // NON_COMPLIANT + } + { + char buf1[128]; + char buf2[256]; + memmove(buf1, buf2, sizeof(buf1)); // COMPLIANT + memmove(buf1, buf2, sizeof(buf1) + 1); // NON_COMPLIANT + memmove(buf1, buf2, sizeof(buf1) - 1); // COMPLIANT + memmove(buf1 + 1, buf2, sizeof(buf1)); // NON_COMPLIANT + memmove(buf1, buf2 + 1, sizeof(buf1)); // COMPLIANT + memmove(buf2, buf1, sizeof(buf2)); // NON_COMPLIANT + memmove(buf2, buf1, sizeof(buf1)); // COMPLIANT + } + { + char buf1[128]; + char buf2[256] = {0}; + strncpy(buf2, buf1, sizeof(buf1)); // COMPLIANT + strncpy(buf1, buf2, sizeof(buf1)); // COMPLIANT + strncpy(buf1, buf2, sizeof(buf1) + 1); // NON_COMPLIANT + strncpy(buf1, buf2, sizeof(buf1) - 1); // COMPLIANT + strncpy(buf1 + 1, buf2, sizeof(buf1)); // NON_COMPLIANT + } + { + char buf0[10]; // memset after first use + char buf1[10]; // no memset + char buf2[10]; // memset before first use + char buf3[10] = {'\0'}; + char buf4[10] = "12345"; + + strncat(buf0, " ", + 1); // NON_COMPLIANT[FALSE_NEGATIVE] - buf0 not null-terminated + memset(buf0, 0, sizeof(buf0)); // COMPLIANT + memset(buf2, 0, sizeof(buf2)); // COMPLIANT + strncat(buf1, " ", 1); // NON_COMPLIANT - not null-terminated + strncat(buf2, " ", 1); // COMPLIANT + strncat(buf3, " ", 1); // COMPLIANT + strncat(buf4, "12345", 5); // NON_COMPLIANT[FALSE_NEGATIVE] + strncat(get_ca_5(), "12345", 5); // NON_COMPLIANT - null-terminator past end + strncat(get_ca_5(), "1234", 4); // COMPLIANT + strncat(get_ca_5() + 1, "1234", 4); // NON_COMPLIANT + strncat(get_ca_5(), "12", 2); // COMPLIANT + } + { + char ca5_good[5] = "test"; // ok + char ca5_bad[5] = "test1"; // no null terminator + char ca6_good[6] = "test1"; // ok + strncmp(ca5_good, ca5_bad, 4); // COMPLIANT + strncmp(ca5_good, ca5_bad, 5); // COMPLIANT + strncmp(ca6_good, ca5_bad, 5); // COMPLIANT + strncmp(ca6_good, ca5_good, 6); // COMPLIANT[FALSE_POSITIVE] + strncmp(ca5_good, ca5_bad, 6); // NON_COMPLIANT + } + // strxfrm + { + 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); // NON_COMPLIANT - not null-terminated + } +} \ No newline at end of file diff --git a/cpp/common/src/codingstandards/cpp/exclusions/c/OutOfBounds.qll b/cpp/common/src/codingstandards/cpp/exclusions/c/OutOfBounds.qll new file mode 100644 index 0000000000..1f606288fb --- /dev/null +++ b/cpp/common/src/codingstandards/cpp/exclusions/c/OutOfBounds.qll @@ -0,0 +1,78 @@ +//** THIS FILE IS AUTOGENERATED, DO NOT MODIFY DIRECTLY. **/ +import cpp +import RuleMetadata +import codingstandards.cpp.exclusions.RuleMetadata + +newtype OutOfBoundsQuery = + TDoNotFormOutOfBoundsPointersOrArraySubscriptsQuery() or + TLibraryFunctionArgumentOutOfBoundsQuery() or + TStringFunctionPointerArgumentOutOfBoundsQuery() or + TStringLibrarySizeArgumentOutOfBoundsQuery() + +predicate isOutOfBoundsQueryMetadata(Query query, string queryId, string ruleId, string category) { + query = + // `Query` instance for the `doNotFormOutOfBoundsPointersOrArraySubscripts` query + OutOfBoundsPackage::doNotFormOutOfBoundsPointersOrArraySubscriptsQuery() and + queryId = + // `@id` for the `doNotFormOutOfBoundsPointersOrArraySubscripts` query + "c/cert/do-not-form-out-of-bounds-pointers-or-array-subscripts" and + ruleId = "ARR30-C" and + category = "rule" + or + query = + // `Query` instance for the `libraryFunctionArgumentOutOfBounds` query + OutOfBoundsPackage::libraryFunctionArgumentOutOfBoundsQuery() and + queryId = + // `@id` for the `libraryFunctionArgumentOutOfBounds` query + "c/cert/library-function-argument-out-of-bounds" and + ruleId = "ARR38-C" and + category = "rule" + or + query = + // `Query` instance for the `stringFunctionPointerArgumentOutOfBounds` query + OutOfBoundsPackage::stringFunctionPointerArgumentOutOfBoundsQuery() and + queryId = + // `@id` for the `stringFunctionPointerArgumentOutOfBounds` query + "c/misra/string-function-pointer-argument-out-of-bounds" and + ruleId = "RULE-21-17" and + category = "mandatory" + or + query = + // `Query` instance for the `stringLibrarySizeArgumentOutOfBounds` query + OutOfBoundsPackage::stringLibrarySizeArgumentOutOfBoundsQuery() and + queryId = + // `@id` for the `stringLibrarySizeArgumentOutOfBounds` query + "c/misra/string-library-size-argument-out-of-bounds" and + ruleId = "RULE-21-18" and + category = "mandatory" +} + +module OutOfBoundsPackage { + Query doNotFormOutOfBoundsPointersOrArraySubscriptsQuery() { + //autogenerate `Query` type + result = + // `Query` type for `doNotFormOutOfBoundsPointersOrArraySubscripts` query + TQueryC(TOutOfBoundsPackageQuery(TDoNotFormOutOfBoundsPointersOrArraySubscriptsQuery())) + } + + Query libraryFunctionArgumentOutOfBoundsQuery() { + //autogenerate `Query` type + result = + // `Query` type for `libraryFunctionArgumentOutOfBounds` query + TQueryC(TOutOfBoundsPackageQuery(TLibraryFunctionArgumentOutOfBoundsQuery())) + } + + Query stringFunctionPointerArgumentOutOfBoundsQuery() { + //autogenerate `Query` type + result = + // `Query` type for `stringFunctionPointerArgumentOutOfBounds` query + TQueryC(TOutOfBoundsPackageQuery(TStringFunctionPointerArgumentOutOfBoundsQuery())) + } + + Query stringLibrarySizeArgumentOutOfBoundsQuery() { + //autogenerate `Query` type + result = + // `Query` type for `stringLibrarySizeArgumentOutOfBounds` query + TQueryC(TOutOfBoundsPackageQuery(TStringLibrarySizeArgumentOutOfBoundsQuery())) + } +} diff --git a/cpp/common/src/codingstandards/cpp/exclusions/c/RuleMetadata.qll b/cpp/common/src/codingstandards/cpp/exclusions/c/RuleMetadata.qll index e7ce8942c5..c2771f4171 100644 --- a/cpp/common/src/codingstandards/cpp/exclusions/c/RuleMetadata.qll +++ b/cpp/common/src/codingstandards/cpp/exclusions/c/RuleMetadata.qll @@ -42,6 +42,7 @@ import Memory1 import Memory2 import Memory3 import Misc +import OutOfBounds import Pointers1 import Pointers2 import Pointers3 @@ -112,6 +113,7 @@ newtype TCQuery = TMemory2PackageQuery(Memory2Query q) or TMemory3PackageQuery(Memory3Query q) or TMiscPackageQuery(MiscQuery q) or + TOutOfBoundsPackageQuery(OutOfBoundsQuery q) or TPointers1PackageQuery(Pointers1Query q) or TPointers2PackageQuery(Pointers2Query q) or TPointers3PackageQuery(Pointers3Query q) or @@ -182,6 +184,7 @@ predicate isQueryMetadata(Query query, string queryId, string ruleId, string cat isMemory2QueryMetadata(query, queryId, ruleId, category) or isMemory3QueryMetadata(query, queryId, ruleId, category) or isMiscQueryMetadata(query, queryId, ruleId, category) or + isOutOfBoundsQueryMetadata(query, queryId, ruleId, category) or isPointers1QueryMetadata(query, queryId, ruleId, category) or isPointers2QueryMetadata(query, queryId, ruleId, category) or isPointers3QueryMetadata(query, queryId, ruleId, category) or diff --git a/rule_packages/c/OutOfBounds.json b/rule_packages/c/OutOfBounds.json new file mode 100644 index 0000000000..31d0349a63 --- /dev/null +++ b/rule_packages/c/OutOfBounds.json @@ -0,0 +1,86 @@ +{ + "CERT-C": { + "ARR30-C": { + "properties": { + "obligation": "rule" + }, + "queries": [ + { + "description": "Forming or using an out-of-bounds pointer is undefined behavior and can result in invalid memory accesses.", + "kind": "problem", + "name": "Do not form or use out-of-bounds pointers or array subscripts", + "precision": "medium", + "severity": "error", + "short_name": "DoNotFormOutOfBoundsPointersOrArraySubscripts", + "tags": [ + "correctness", + "security" + ] + } + ], + "title": "Do not form or use out-of-bounds pointers or array subscripts" + }, + "ARR38-C": { + "properties": { + "obligation": "rule" + }, + "queries": [ + { + "description": "Passing out-of-bounds pointers or erroneous size arguments to standard library functions can result in out-of-bounds accesses and other undefined behavior.", + "kind": "problem", + "name": "Guarantee that library functions do not form invalid pointers", + "precision": "high", + "severity": "error", + "short_name": "LibraryFunctionArgumentOutOfBounds", + "tags": [ + "correctness", + "security" + ] + } + ], + "title": "Guarantee that library functions do not form invalid pointers" + } + }, + "MISRA-C-2012": { + "RULE-21-17": { + "properties": { + "obligation": "mandatory" + }, + "queries": [ + { + "description": "Use of string manipulation functions from with improper buffer sizes can result in out-of-bounds buffer accesses.", + "kind": "problem", + "name": "Use of the string handling functions from shall not result in accesses beyond the bounds", + "precision": "high", + "severity": "error", + "short_name": "StringFunctionPointerArgumentOutOfBounds", + "tags": [ + "correctness", + "security" + ] + } + ], + "title": "Use of the string handling functions from shall not result in accesses beyond the bounds of the objects referenced by their pointer parameters" + }, + "RULE-21-18": { + "properties": { + "obligation": "mandatory" + }, + "queries": [ + { + "description": "Passing a size_t argument that is non-positive or greater than the size of the smallest buffer argument to any function in may result in out-of-bounds buffer accesses.", + "kind": "problem", + "name": "The size_t argument passed to any function in shall have an appropriate value", + "precision": "high", + "severity": "error", + "short_name": "StringLibrarySizeArgumentOutOfBounds", + "tags": [ + "correctness", + "security" + ] + } + ], + "title": "The size_t argument passed to any function in shall have an appropriate value" + } + } +} \ No newline at end of file