diff --git a/.vscode/tasks.json b/.vscode/tasks.json index 2b23866ba4..75a9633098 100644 --- a/.vscode/tasks.json +++ b/.vscode/tasks.json @@ -242,6 +242,7 @@ "Macros", "Memory1", "Memory2", + "Memory3", "Misc", "MoveForward", "Naming", diff --git a/c/cert/src/rules/ARR32-C/VariableLengthArraySizeNotInValidRange.md b/c/cert/src/rules/ARR32-C/VariableLengthArraySizeNotInValidRange.md new file mode 100644 index 0000000000..d8554e2ef6 --- /dev/null +++ b/c/cert/src/rules/ARR32-C/VariableLengthArraySizeNotInValidRange.md @@ -0,0 +1,194 @@ +# ARR32-C: Ensure size arguments for variable length arrays are in a valid range + +This query implements the CERT-C rule ARR32-C: + +> Ensure size arguments for variable length arrays are in a valid range + + +## Description + +Variable length arrays (VLAs), a conditionally supported language feature, are essentially the same as traditional C arrays except that they are declared with a size that is not a constant integer expression and can be declared only at block scope or function prototype scope and no linkage. When supported, a variable length array can be declared + +```cpp +{ /* Block scope */ + char vla[size]; +} + +``` +where the integer expression `size` and the declaration of `vla` are both evaluated at runtime. If the size argument supplied to a variable length array is not a positive integer value, the behavior is undefined. (See [undefined behavior 75](https://wiki.sei.cmu.edu/confluence/display/c/CC.+Undefined+Behavior#CC.UndefinedBehavior-ub_75).) Additionally, if the magnitude of the argument is excessive, the program may behave in an unexpected way. An attacker may be able to leverage this behavior to overwrite critical program data \[[Griffiths 2006](https://wiki.sei.cmu.edu/confluence/display/c/AA.+Bibliography#AA.Bibliography-Griffiths06)\]. The programmer must ensure that size arguments to variable length arrays, especially those derived from untrusted data, are in a valid range. + +Because variable length arrays are a conditionally supported feature of C11, their use in portable code should be guarded by testing the value of the macro `__STDC_NO_VLA__`. Implementations that do not support variable length arrays indicate it by setting `__STDC_NO_VLA__` to the integer constant 1. + +## Noncompliant Code Example + +In this noncompliant code example, a variable length array of size `size` is declared. The `size` is declared as `size_t` in compliance with [INT01-C. Use rsize_t or size_t for all integer values representing the size of an object](https://wiki.sei.cmu.edu/confluence/display/c/INT01-C.+Use+rsize_t+or+size_t+for+all+integer+values+representing+the+size+of+an+object). + +```cpp +#include + +extern void do_work(int *array, size_t size); + +void func(size_t size) { + int vla[size]; + do_work(vla, size); +} + +``` +However, the value of `size` may be zero or excessive, potentially giving rise to a security [vulnerability](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-vulnerability). + +## Compliant Solution + +This compliant solution ensures the `size` argument used to allocate `vla` is in a valid range (between 1 and a programmer-defined maximum); otherwise, it uses an algorithm that relies on dynamic memory allocation. The solution also avoids unsigned integer wrapping that, given a sufficiently large value of `size`, would cause `malloc` to allocate insufficient storage for the array. + +```cpp +#include +#include + +enum { MAX_ARRAY = 1024 }; +extern void do_work(int *array, size_t size); + +void func(size_t size) { + if (0 == size || SIZE_MAX / sizeof(int) < size) { + /* Handle error */ + return; + } + if (size < MAX_ARRAY) { + int vla[size]; + do_work(vla, size); + } else { + int *array = (int *)malloc(size * sizeof(int)); + if (array == NULL) { + /* Handle error */ + } + do_work(array, size); + free(array); + } +} + +``` + +## Noncompliant Code Example (sizeof) + +The following noncompliant code example defines `A` to be a variable length array and then uses the `sizeof` operator to compute its size at runtime. When the function is called with an argument greater than `SIZE_MAX / (N1 * sizeof (int))`, the runtime `sizeof` expression may wrap around, yielding a result that is smaller than the mathematical product `N1 * n2 * sizeof (int)`. The call to `malloc()`, when successful, will then allocate storage for fewer than `n2` elements of the array, causing one or more of the final `memset()` calls in the `for` loop to write past the end of that storage. + +```cpp +#include +#include + +enum { N1 = 4096 }; + +void *func(size_t n2) { + typedef int A[n2][N1]; + + A *array = malloc(sizeof(A)); + if (!array) { + /* Handle error */ + return NULL; + } + + for (size_t i = 0; i != n2; ++i) { + memset(array[i], 0, N1 * sizeof(int)); + } + + return array; +} + +``` +Furthermore, this code also violates [ARR39-C. Do not add or subtract a scaled integer to a pointer](https://wiki.sei.cmu.edu/confluence/display/c/ARR39-C.+Do+not+add+or+subtract+a+scaled+integer+to+a+pointer), where `array` is a pointer to the two-dimensional array, where it should really be a pointer to the latter dimension instead. This means that the `memset() `call does out-of-bounds writes on all of its invocations except the first. + +## Compliant Solution (sizeof) + +This compliant solution prevents `sizeof` wrapping by detecting the condition before it occurs and avoiding the subsequent computation when the condition is detected. The code also uses an additional typedef to fix the type of `array` so that `memset()` never writes past the two-dimensional array. + +```cpp +#include +#include +#include + +enum { N1 = 4096 }; + +void *func(size_t n2) { + if (n2 > SIZE_MAX / (N1 * sizeof(int))) { + /* Prevent sizeof wrapping */ + return NULL; + } + + typedef int A1[N1]; + typedef A1 A[n2]; + + A1 *array = (A1*) malloc(sizeof(A)); + + if (!array) { + /* Handle error */ + return NULL; + } + + for (size_t i = 0; i != n2; ++i) { + memset(array[i], 0, N1 * sizeof(int)); + } + return array; +} + +``` +**Implementation Details** + +**Microsoft** + +Variable length arrays are not supported by Microsoft compilers. + +## Risk Assessment + +Failure to properly specify the size of a variable length array may allow arbitrary code execution or result in stack exhaustion. + +
Rule Severity Likelihood Remediation Cost Priority Level
ARR32-C High Probable High P6 L2
+ + +## Automated Detection + +
Tool Version Checker Description
CodeSonar 7.2p0 ALLOC.SIZE.IOFLOWALLOC.SIZE.MULOFLOWMISC.MEM.SIZE.BAD Integer Overflow of Allocation Size Multiplication Overflow of Allocation Size Unreasonable Size Argument
Coverity 2017.07 REVERSE_NEGATIVE Fully implemented
Helix QAC 2022.4 C1051
Klocwork 2022.4 MISRA.ARRAY.VAR_LENGTH.2012
LDRA tool suite 9.7.1 621 S Enhanced enforcement
Parasoft C/C++test 2022.2 CERT_C-ARR32-a Ensure the size of the variable length array is in valid range
PC-lint Plus 1.4 9035 Assistance provided
Polyspace Bug Finder R2023a CERT C: Rule ARR32-C Checks for: Memory allocation with tainted sizeemory allocation with tainted size, tainted size of variable length arrayainted size of variable length array. Rule fully covered.
PRQA QA-C 9.7 1051 Partially implemented
Cppcheck 1.66 negativeArraySize Context sensitive analysis Will warn only if given size is negative
TrustInSoft Analyzer 1.38 alloca_bounds Exhaustively verified.
+ + +## Related Vulnerabilities + +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+ARR32-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
CERT C Secure Coding Standard INT01-C. Use rsize_t or size_t for all integer values representing the size of an object 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:2013 Tainted, potentially mutilated, or out-of-domain integer values are used in a restricted sink \[taintsink\] Prior to 2018-01-12: CERT: Unspecified Relationship
CWE 2.11 CWE-758 2017-06-29: CERT: Rule subset of CWE
+ + +## CERT-CWE Mapping Notes + +[Key here](https://wiki.sei.cmu.edu/confluence/pages/viewpage.action?pageId=87152408#HowthisCodingStandardisOrganized-CERT-CWEMappingNotes) for mapping notes + +**CWE-129 and ARR32-C** + +Intersection( CWE-188, EXP39-C) = Ø + +ARR32-C addresses specifying the size of a variable-length array (VLA). CWE-129 addresses invalid array indices, not array sizes. + +**CWE-758 and ARR32-C** + +Independent( INT34-C, INT36-C, MSC37-C, FLP32-C, EXP33-C, EXP30-C, ERR34-C, ARR32-C) + +CWE-758 = Union( ARR32-C, list) where list = + +* Undefined behavior that results from anything other than too large a VLA dimension. +**CWE-119 and ARR32-C** +* Intersection( CWE-119, ARR32-C) = Ø +* ARR32-C is not about providing a valid buffer but reading/writing outside it. It is about providing an invalid buffer, or one that exhausts the stack. + +## Bibliography + +
\[ Griffiths 2006 \]
+ + +## Implementation notes + +None + +## References + +* CERT-C: [ARR32-C: Ensure size arguments for variable length arrays are in a valid range](https://wiki.sei.cmu.edu/confluence/display/c) diff --git a/c/cert/src/rules/ARR32-C/VariableLengthArraySizeNotInValidRange.ql b/c/cert/src/rules/ARR32-C/VariableLengthArraySizeNotInValidRange.ql new file mode 100644 index 0000000000..40a800aa69 --- /dev/null +++ b/c/cert/src/rules/ARR32-C/VariableLengthArraySizeNotInValidRange.ql @@ -0,0 +1,171 @@ +/** + * @id c/cert/variable-length-array-size-not-in-valid-range + * @name ARR32-C: Ensure size arguments for variable length arrays are in a valid range + * @description A variable-length array size that is zero, negative, overflowed, wrapped around, or + * excessively large may lead to undefined behaviour. + * @kind problem + * @precision high + * @problem.severity error + * @tags external/cert/id/arr32-c + * correctness + * security + * external/cert/obligation/rule + */ + +import cpp +import codingstandards.c.cert +import codingstandards.cpp.Overflow + +/** + * Gets the maximum size (in bytes) a variable-length array + * should be to not be deemed excessively large persuant to this rule. + * This value has been arbitrarily chosen to be 2^16 - 1 bytes. + */ +private int maximumTotalVlaSize() { result = 65535 } + +/** + * 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. + */ +private 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 +} + +/** + * The `SimpleRangeAnalysis` analysis over-zealously expands upper bounds of + * `SubExpr`s to account for potential wrapping even when no wrapping can occur. + * + * This class represents a `SubExpr` that is safe from wrapping. + */ +class SafeSubExprWithErroneouslyWrappedUpperBound extends SubExpr { + SafeSubExprWithErroneouslyWrappedUpperBound() { + lowerBound(this.getLeftOperand().getFullyConverted()) - + upperBound(this.getRightOperand().getFullyConverted()) >= 0 and + upperBound(this.getFullyConverted()) = exprMaxVal(this.getFullyConverted()) + } + + /** + * Gets the lower bound of the difference. + */ + float getlowerBoundOfDifference() { + result = + lowerBound(this.getLeftOperand().getFullyConverted()) - + upperBound(this.getRightOperand().getFullyConverted()) + } +} + +/** + * Holds if `e` is an expression that is not in a valid range due to it + * being partially or fully derived from an overflowing arithmetic operation. + */ +predicate isExprTaintedByOverflowingExpr(Expr e) { + exists(InterestingOverflowingOperation bop | + // `bop` is not pre-checked to prevent overflow/wrapping + not bop.hasValidPreCheck() and + // and the destination is tainted by `bop` + TaintTracking::localExprTaint(bop, e.getAChild*()) and + // and there does not exist a post-wrapping-check before `e` + not exists(GuardCondition gc | + gc = bop.getAValidPostCheck() and + gc.controls(e.getBasicBlock(), _) + ) + ) +} + +predicate getVlaSizeExprBounds(Expr e, float lower, float upper) { + lower = lowerBound(e) and + upper = + // upper is the smallest of either a `SubExpr` which flows to `e` and does + // not wrap, or the upper bound of `e` derived from the range-analysis library + min(float f | + f = + any(SafeSubExprWithErroneouslyWrappedUpperBound sub | + DataFlow::localExprFlow(sub, e) + | + sub.getlowerBoundOfDifference() + ) or + f = upperBound(e) + ) +} + +/** + * Holds if `e` is not bounded to a valid range, (0 .. maximumTotalVlaSize()], for + * a element count of an individual variable-length array dimension. + */ +predicate isVlaSizeExprOutOfRange(VlaDeclStmt vla, Expr e) { + vla.getVlaDimensionStmt(_).getDimensionExpr() = e and + exists(float lower, float upper | + getVlaSizeExprBounds(e.getFullyConverted(), lower, upper) and + ( + lower <= 0 + or + upper > maximumTotalVlaSize() / baseType(vla.getVariable().getType()).getSize() + ) + ) +} + +/** + * Returns the upper bound of `e.getFullyConverted()`. + */ +float getVlaSizeExprUpperBound(Expr e) { getVlaSizeExprBounds(e.getFullyConverted(), _, result) } + +/** + * Returns the upper bound of `vla`'s dimension expression at `index`. + * + * If `index` does not exist, then the result is `1`. + */ +bindingset[index] +private float getVlaSizeExprUpperBoundAtIndexOrOne(VlaDeclStmt vla, float index) { + if vla.getNumberOfVlaDimensionStmts() > index + then result = getVlaSizeExprUpperBound(vla.getVlaDimensionStmt(index).getDimensionExpr()) + else result = 1 +} + +predicate vlaupper = getVlaSizeExprUpperBoundAtIndexOrOne/2; + +/** + * Gets the upper bound of the total size of `vla`. + */ +float getTotalVlaSizeUpperBound(VlaDeclStmt vla) { + result = + vlaupper(vla, 0) * vlaupper(vla, 1) * vlaupper(vla, 2) * vlaupper(vla, 3) * vlaupper(vla, 4) * + vlaupper(vla, 5) * vlaupper(vla, 6) * vlaupper(vla, 7) * vlaupper(vla, 8) * vlaupper(vla, 9) +} + +from VlaDeclStmt vla, string message +where + not isExcluded(vla, InvalidMemory2Package::variableLengthArraySizeNotInValidRangeQuery()) and + ( + if isExprTaintedByOverflowingExpr(vla.getVlaDimensionStmt(_).getDimensionExpr()) + then message = "Variable-length array size derives from an overflowing or wrapping expression." + else ( + if isVlaSizeExprOutOfRange(vla, vla.getVlaDimensionStmt(_).getDimensionExpr()) + then message = "Variable-length array dimension size may be in an invalid range." + else ( + getTotalVlaSizeUpperBound(vla) > maximumTotalVlaSize() and + message = "Variable-length array total size may be excessively large." + ) + ) + ) +select vla, message diff --git a/c/cert/src/rules/ARR37-C/DoNotUsePointerArithmeticOnNonArrayObjectPointers.md b/c/cert/src/rules/ARR37-C/DoNotUsePointerArithmeticOnNonArrayObjectPointers.md new file mode 100644 index 0000000000..7772bf4d3d --- /dev/null +++ b/c/cert/src/rules/ARR37-C/DoNotUsePointerArithmeticOnNonArrayObjectPointers.md @@ -0,0 +1,148 @@ +# ARR37-C: Do not add or subtract an integer to a pointer to a non-array object + +This query implements the CERT-C rule ARR37-C: + +> Do not add or subtract an integer to a pointer to a non-array object + + +## Description + +Pointer arithmetic must be performed only on pointers that reference elements of array objects. + +The C Standard, 6.5.6 \[[ISO/IEC 9899:2011](https://wiki.sei.cmu.edu/confluence/display/c/AA.+Bibliography#AA.Bibliography-ISO-IEC9899-2011)\], states the following about pointer arithmetic: + +> When an expression that has integer type is added to or subtracted from a pointer, the result has the type of the pointer operand. If the pointer operand points to an element of an array object, and the array is large enough, the result points to an element offset from the original element such that the difference of the subscripts of the resulting and original array elements equals the integer expression. + + +## Noncompliant Code Example + +This noncompliant code example attempts to access structure members using pointer arithmetic. This practice is dangerous because structure members are not guaranteed to be contiguous. + +```cpp +struct numbers { + short num_a, num_b, num_c; +}; + +int sum_numbers(const struct numbers *numb){ + int total = 0; + const short *numb_ptr; + + for (numb_ptr = &numb->num_a; + numb_ptr <= &numb->num_c; + numb_ptr++) { + total += *(numb_ptr); + } + + return total; +} + +int main(void) { + struct numbers my_numbers = { 1, 2, 3 }; + sum_numbers(&my_numbers); + return 0; +} + +``` + +## Compliant Solution + +It is possible to use the `->` operator to dereference each structure member: + +```cpp +total = numb->num_a + numb->num_b + numb->num_c; + +``` +However, this solution results in code that is hard to write and hard to maintain (especially if there are many more structure members), which is exactly what the author of the noncompliant code example was likely trying to avoid. + +## Compliant Solution + +A better solution is to define the structure to contain an array member to store the numbers in an array rather than a structure, as in this compliant solution: + +```cpp +#include + +struct numbers { + short a[3]; +}; + +int sum_numbers(const short *numb, size_t dim) { + int total = 0; + for (size_t i = 0; i < dim; ++i) { + total += numb[i]; + } + + return total; +} + +int main(void) { + struct numbers my_numbers = { .a[0]= 1, .a[1]= 2, .a[2]= 3}; + sum_numbers( + my_numbers.a, + sizeof(my_numbers.a)/sizeof(my_numbers.a[0]) + ); + return 0; +} + +``` +Array elements are guaranteed to be contiguous in memory, so this solution is completely portable. + +## Exceptions + +** ARR37-C-EX1:** Any non-array object in memory can be considered an array consisting of one element. Adding one to a pointer for such an object yields a pointer one element past the array, and subtracting one from that pointer yields the original pointer. This allows for code such as the following: + +```cpp +#include +#include + +struct s { + char *c_str; + /* Other members */ +}; + +struct s *create_s(const char *c_str) { + struct s *ret; + size_t len = strlen(c_str) + 1; + + ret = (struct s *)malloc(sizeof(struct s) + len); + if (ret != NULL) { + ret->c_str = (char *)(ret + 1); + memcpy(ret + 1, c_str, len); + } + return ret; +} +``` +A more general and safer solution to this problem is to use a flexible array member that guarantees the array that follows the structure is properly aligned by inserting padding, if necessary, between it and the member that immediately precedes it. + +## Risk Assessment + +
Rule Severity Likelihood Remediation Cost Priority Level
ARR37-C Medium Probable Medium P8 L2
+ + +## Automated Detection + +
Tool Version Checker Description
Astrée 22.04 Supported indirectly via MISRA C:2004 Rule 17.4.
Axivion Bauhaus Suite 7.2.0 CertC-ARR37 Fully implemented
CodeSonar 7.2p0 LANG.MEM.BO LANG.MEM.BU LANG.STRUCT.PARITH LANG.STRUCT.PBB LANG.STRUCT.PPE LANG.MEM.TBA LANG.MEM.TO LANG.MEM.TU Buffer Overrun Buffer Underrun Pointer Arithmetic Pointer Before Beginning of Object Pointer Past End of Object Tainted Buffer Access Type Overrun Type Underrun
Compass/ROSE
Coverity 2017.07 ARRAY_VS_SINGLETON Implemented
Helix QAC 2022.4 DF2930, DF2931, DF2932, DF2933 C++3705, C++3706, C++3707
Klocwork 2022.4 MISRA.PTR.ARITH.2012
LDRA tool suite 9.7.1 567 S Partially implemented
Parasoft C/C++test 2022.2 CERT_C-ARR37-a Pointer arithmetic shall not be applied to pointers that address variables of non-array type
PC-lint Plus 1.4 2662 Partially supported
Polyspace Bug Finder R2023a CERT C: Rule ARR37-C Checks for invalid assumptions about memory organization (rule partially covered)
PRQA QA-C 9.7 2930, 2931, 2932, 2933
PRQA QA-C++ 4.4 2930, 2931, 2932, 2933, 3705, 3706, 3707
RuleChecker 22.04 Supported indirectly via MISRA C:2004 Rule 17.4.
+ + +## Related Vulnerabilities + +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+ARR37-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
+ + +## Bibliography + +
\[ Banahan 2003 \] Section 5.3, "Pointers" Section 5.7, "Expressions Involving Pointers"
\[ ISO/IEC 9899:2011 \] 6.5.6, "Additive Operators"
\[ VU\#162289 \]
+ + +## Implementation notes + +None + +## References + +* CERT-C: [ARR37-C: Do not add or subtract an integer to a pointer to a non-array object](https://wiki.sei.cmu.edu/confluence/display/c) diff --git a/c/cert/src/rules/ARR37-C/DoNotUsePointerArithmeticOnNonArrayObjectPointers.ql b/c/cert/src/rules/ARR37-C/DoNotUsePointerArithmeticOnNonArrayObjectPointers.ql new file mode 100644 index 0000000000..8dbd00584c --- /dev/null +++ b/c/cert/src/rules/ARR37-C/DoNotUsePointerArithmeticOnNonArrayObjectPointers.ql @@ -0,0 +1,109 @@ +/** + * @id c/cert/do-not-use-pointer-arithmetic-on-non-array-object-pointers + * @name ARR37-C: Do not add or subtract an integer to a pointer to a non-array object + * @description A pair of elements that are not elements in the same array are not guaranteed to be + * contiguous in memory and therefore should not be addressed using pointer arithmetic. + * @kind path-problem + * @precision high + * @problem.severity error + * @tags external/cert/id/arr37-c + * correctness + * external/cert/obligation/rule + */ + +import cpp +import codingstandards.c.cert +import semmle.code.cpp.dataflow.DataFlow +import DataFlow::PathGraph + +/** + * A data-flow configuration that tracks flow from an `AddressOfExpr` of a variable + * of `PointerType` that is not also an `ArrayType` to a `PointerArithmeticOrArrayExpr` + */ +class NonArrayPointerToArrayIndexingExprConfig extends DataFlow::Configuration { + NonArrayPointerToArrayIndexingExprConfig() { this = "ArrayToArrayIndexConfig" } + + override predicate isSource(DataFlow::Node source) { + exists(AddressOfExpr ao, Type t | + source.asExpr() = ao and + not ao.getOperand() instanceof ArrayExpr and + not ao.getOperand() instanceof PointerDereferenceExpr and + t = ao.getOperand().getType() and + not t instanceof PointerType and + not t instanceof ArrayType and + not t.(PointerType).getBaseType() instanceof ArrayType + ) + } + + override predicate isSink(DataFlow::Node sink) { + exists(PointerArithmeticOrArrayExpr ae | + sink.asExpr() = ae.getPointerOperand() and + not sink.asExpr() instanceof Literal and + not ae.isNonPointerOperandZero() + ) + } + + override predicate isBarrierOut(DataFlow::Node node) { + // the default interprocedural data-flow model flows through any field or array assignment + // expressions to the qualifier (array base, pointer dereferenced, or qualifier) instead of the + // individual element or field that the assignment modifies. this default behaviour causes + // false positives for future accesses of any element of that object, so we remove the edges + // between those assignments from the graph with `isBarrierOut`. + exists(AssignExpr a | + node.asExpr() = a.getRValue() and + ( + a.getLValue() instanceof ArrayExpr or + a.getLValue() instanceof PointerDereferenceExpr or + a.getLValue() instanceof FieldAccess + ) + ) + or + // ignore AddressOfExpr output e.g. call(&s1) + node.asDefiningArgument() instanceof AddressOfExpr + } +} + +class PointerArithmeticOrArrayExpr extends Expr { + Expr operand; + + PointerArithmeticOrArrayExpr() { + operand = this.(ArrayExpr).getArrayBase() + or + operand = this.(ArrayExpr).getArrayOffset() + or + operand = this.(PointerAddExpr).getAnOperand() + or + operand = this.(PointerSubExpr).getAnOperand() + or + operand = this.(Operation).getAnOperand() and + operand.getUnderlyingType() instanceof PointerType and + ( + this instanceof PostfixCrementOperation + or + this instanceof PrefixIncrExpr + or + this instanceof PrefixDecrExpr + ) + } + + /** + * Gets the operands of this expression. If the expression is an + * `ArrayExpr`, the results are the array base and offset `Expr`s. + */ + Expr getPointerOperand() { + result = operand or + result = this.(PointerArithmeticOrArrayExpr).getPointerOperand() + } + + /** + * Holds if there exists an operand that is a `Literal` with a value of `0`. + */ + predicate isNonPointerOperandZero() { operand.(Literal).getValue().toInt() = 0 } +} + +from DataFlow::PathNode source, DataFlow::PathNode sink +where + not isExcluded(sink.getNode().asExpr(), + InvalidMemory2Package::doNotUsePointerArithmeticOnNonArrayObjectPointersQuery()) and + any(NonArrayPointerToArrayIndexingExprConfig cfg).hasFlowPath(source, sink) +select sink, source, sink, "Pointer arithmetic on non-array object pointer." diff --git a/c/cert/src/rules/EXP35-C/DoNotModifyObjectsWithTemporaryLifetime.md b/c/cert/src/rules/EXP35-C/DoNotModifyObjectsWithTemporaryLifetime.md new file mode 100644 index 0000000000..58ff1b03cf --- /dev/null +++ b/c/cert/src/rules/EXP35-C/DoNotModifyObjectsWithTemporaryLifetime.md @@ -0,0 +1,225 @@ +# EXP35-C: Do not modify objects with temporary lifetime + +This query implements the CERT-C rule EXP35-C: + +> Do not modify objects with temporary lifetime + + +## Description + +The C11 Standard \[[ISO/IEC 9899:2011](https://wiki.sei.cmu.edu/confluence/display/c/AA.+Bibliography#AA.Bibliography-ISO-IEC9899-2011)\] introduced a new term: *temporary lifetime*. Modifying an object with temporary lifetime is [undefined behavior](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-undefinedbehavior). According to subclause 6.2.4, paragraph 8 + +> A non-lvalue expression with structure or union type, where the structure or union contains a member with array type (including, recursively, members of all contained structures and unions) refers to an object with automatic storage duration and *temporary*lifetime. Its lifetime begins when the expression is evaluated and its initial value is the value of the expression. Its lifetime ends when the evaluation of the containing full expression or full declarator ends. Any attempt to modify an object with temporary lifetime results in undefined behavior. + + +This definition differs from the C99 Standard (which defines modifying the result of a function call or accessing it after the next sequence point as undefined behavior) because a temporary object's lifetime ends when the evaluation containing the full expression or full declarator ends, so the result of a function call can be accessed. This extension to the lifetime of a temporary also removes a quiet change to C90 and improves compatibility with C++. + +C functions may not return arrays; however, functions can return a pointer to an array or a `struct` or `union` that contains arrays. Consequently, in any version of C, if a function call returns by value a `struct` or `union` containing an array, do not modify those arrays within the expression containing the function call. In C99 and older, do not access an array returned by a function after the next sequence point or after the evaluation of the containing full expression or full declarator ends. + +## Noncompliant Code Example + +This noncompliant code example [conforms](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-conformingprogram) to the C11 Standard; however, it fails to conform to C99. If compiled with a C99-conforming implementation, this code has [undefined behavior](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-undefinedbehavior) because the sequence point preceding the call to `printf()` comes between the call and the access by `printf()` of the string in the returned object. + +```cpp +#include + +struct X { char a[8]; }; + +struct X salutation(void) { + struct X result = { "Hello" }; + return result; +} + +struct X addressee(void) { + struct X result = { "world" }; + return result; +} + +int main(void) { + printf("%s, %s!\n", salutation().a, addressee().a); + return 0; +} + +``` + +## Compliant Solution (C11 and newer) + +This compliant solution checks `__STDC_VERSION__` to ensure that a pre-C11 compiler will fail to compile the code, rather than invoking undefined behavior. + +```cpp +#include + +#if __STDC_VERSION__ < 201112L +#error This code requires a compiler supporting the C11 standard or newer +#endif + +struct X { char a[8]; }; + +struct X salutation(void) { + struct X result = { "Hello" }; + return result; +} + +struct X addressee(void) { + struct X result = { "world" }; + return result; +} + +int main(void) { + printf("%s, %s!\n", salutation().a, addressee().a); + return 0; +} +``` + +## Compliant Solution + +This compliant solution stores the structures returned by the call to `addressee()` before calling the `printf()` function. Consequently, this program conforms to both C99 and C11. + +```cpp +#include + +struct X { char a[8]; }; + +struct X salutation(void) { + struct X result = { "Hello" }; + return result; +} + +struct X addressee(void) { + struct X result = { "world" }; + return result; +} + +int main(void) { + struct X my_salutation = salutation(); + struct X my_addressee = addressee(); + + printf("%s, %s!\n", my_salutation.a, my_addressee.a); + return 0; +} + +``` + +## Noncompliant Code Example + +This noncompliant code example attempts to retrieve an array and increment the array's first value. The array is part of a `struct` that is returned by a function call. Consequently, the array has temporary lifetime, and modifying the array is [undefined behavior](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-undefinedbehavior) in both C99 and C11. + +```cpp +#include + +struct X { int a[6]; }; + +struct X addressee(void) { + struct X result = { { 1, 2, 3, 4, 5, 6 } }; + return result; +} + +int main(void) { + printf("%x", ++(addressee().a[0])); + return 0; +} + +``` + +## Compliant Solution + +This compliant solution stores the structure returned by the call to `addressee()` as `my_x` before calling the `printf()` function. When the array is modified, its lifetime is no longer temporary but matches the lifetime of the block in `main()`. + +```cpp +#include + +struct X { int a[6]; }; + +struct X addressee(void) { + struct X result = { { 1, 2, 3, 4, 5, 6 } }; + return result; +} + +int main(void) { + struct X my_x = addressee(); + printf("%x", ++(my_x.a[0])); + return 0; +} + +``` + +## Noncompliant Code Example + +This noncompliant code example attempts to save a pointer to an array that is part of a `struct` that is returned by a function call. Consequently, the array has temporary lifetime, and using the pointer to it outside of the full expression is [undefined behavior](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-undefinedbehavior) in both C99 and C11. + +```cpp +#include + +struct X { int a[6]; }; + +struct X addressee(void) { + struct X result = { { 1, 2, 3, 4, 5, 6 } }; + return result; +} + +int main(void) { + int *my_a = addressee().a; + printf("%x", my_a[0]); + return 0; +} + +``` + +## Compliant Solution + +This compliant solution stores the structure returned by the call to `addressee()` as `my_x` before saving a pointer to its array member. When the pointer is used, its lifetime is no longer temporary but matches the lifetime of the block in `main()`. + +```cpp +#include + +struct X { int a[6]; }; + +struct X addressee(void) { + struct X result = { { 1, 2, 3, 4, 5, 6 } }; + return result; +} + +int main(void) { + struct X my_x = addressee(); + int *my_a = my_x.a; + printf("%x", my_a[0]); + return 0; +} + +``` + +## Risk Assessment + +Attempting to modify an array or access it after its lifetime expires may result in erroneous program behavior. + +
Rule Severity Likelihood Remediation Cost Priority Level
EXP35-C Low Probable Medium P4 L3
+ + +## Automated Detection + +
Tool Version Checker Description
Astrée 22.04 temporary-object-modification Partially checked
Axivion Bauhaus Suite 7.2.0 CertC-EXP35
Helix QAC 2022.4 C0450, C0455, C0459, C0464, C0465 C++3807, C++3808
LDRA tool suite 9.7.1 642 S, 42 D, 77 D Enhanced Enforcement
Parasoft C/C++test 2022.2 CERT_C-EXP35-a Do not modify objects with temporary lifetime
Polyspace Bug Finder R2023a CERT-C: Rule EXP35-C Checks for accesses on objects with temporary lifetime (rule fully covered)
PRQA QA-C 9.7 0450 \[U\], 0455 \[U\], 0459 \[U\], 0464 \[U\], 0465 \[U\]
Splint 3.1.1
RuleChecker 22.04 temporary-object-modification Partially checked
+ + +## Related Vulnerabilities + +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+EXP35-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 Dangling References to Stack Frames \[DCM\] Prior to 2018-01-12: CERT: Unspecified Relationship
ISO/IEC TR 24772:2013 Side-effects and Order of Evaluation \[SAM\] Prior to 2018-01-12: CERT: Unspecified Relationship
+ + +## Bibliography + +
\[ ISO/IEC 9899:2011 \] 6.2.4, "Storage Durations of Objects"
+ + +## Implementation notes + +This implementation also always reports non-modifying accesses of objects with temporary lifetime, which are only compliant in C11. + +## References + +* CERT-C: [EXP35-C: Do not modify objects with temporary lifetime](https://wiki.sei.cmu.edu/confluence/display/c) diff --git a/c/cert/src/rules/EXP35-C/DoNotModifyObjectsWithTemporaryLifetime.ql b/c/cert/src/rules/EXP35-C/DoNotModifyObjectsWithTemporaryLifetime.ql new file mode 100644 index 0000000000..2d66b8643c --- /dev/null +++ b/c/cert/src/rules/EXP35-C/DoNotModifyObjectsWithTemporaryLifetime.ql @@ -0,0 +1,38 @@ +/** + * @id c/cert/do-not-modify-objects-with-temporary-lifetime + * @name EXP35-C: Do not modify objects with temporary lifetime + * @description Attempting to modify an object with temporary lifetime results in undefined + * behavior. + * @kind problem + * @precision high + * @problem.severity error + * @tags external/cert/id/exp35-c + * correctness + * external/cert/obligation/rule + */ + +import cpp +import codingstandards.c.cert + +/** + * A struct or union type that contains an array type + */ +class StructOrUnionTypeWithArrayField extends Struct { + StructOrUnionTypeWithArrayField() { + this.getAField().getUnspecifiedType() instanceof ArrayType + or + // nested struct or union containing an array type + this.getAField().getUnspecifiedType().(Struct) instanceof StructOrUnionTypeWithArrayField + } +} + +// Note: Undefined behavior is possible regardless of whether the accessed field from the returned +// struct is an array or a scalar (i.e. arithmetic and pointer types) member, according to the standard. +from FieldAccess fa, FunctionCall fc +where + not isExcluded(fa, InvalidMemory2Package::doNotModifyObjectsWithTemporaryLifetimeQuery()) and + not fa.getQualifier().isLValue() and + fa.getQualifier().getUnconverted() = fc and + fa.getQualifier().getUnconverted().getUnspecifiedType() instanceof StructOrUnionTypeWithArrayField +select fa, "Field access on $@ qualifier occurs after its temporary object lifetime.", fc, + "function call" diff --git a/c/cert/src/rules/MEM35-C/InsufficientMemoryAllocatedForObject.md b/c/cert/src/rules/MEM35-C/InsufficientMemoryAllocatedForObject.md new file mode 100644 index 0000000000..7f3c70efbb --- /dev/null +++ b/c/cert/src/rules/MEM35-C/InsufficientMemoryAllocatedForObject.md @@ -0,0 +1,199 @@ +# MEM35-C: Allocate sufficient memory for an object + +This query implements the CERT-C rule MEM35-C: + +> Allocate sufficient memory for an object + + +## Description + +The types of integer expressions used as size arguments to `malloc()`, `calloc()`, `realloc()`, or `aligned_alloc()` must have sufficient range to represent the size of the objects to be stored. If size arguments are incorrect or can be manipulated by an attacker, then a buffer overflow may occur. Incorrect size arguments, inadequate range checking, integer overflow, or truncation can result in the allocation of an inadequately sized buffer. + +Typically, the amount of memory to allocate will be the size of the type of object to allocate. When allocating space for an array, the size of the object will be multiplied by the bounds of the array. When allocating space for a structure containing a flexible array member, the size of the array member must be added to the size of the structure. (See [MEM33-C. Allocate and copy structures containing a flexible array member dynamically](https://wiki.sei.cmu.edu/confluence/display/c/MEM33-C.++Allocate+and+copy+structures+containing+a+flexible+array+member+dynamically).) Use the correct type of the object when computing the size of memory to allocate. + +[STR31-C. Guarantee that storage for strings has sufficient space for character data and the null terminator](https://wiki.sei.cmu.edu/confluence/display/c/STR31-C.+Guarantee+that+storage+for+strings+has+sufficient+space+for+character+data+and+the+null+terminator) is a specific instance of this rule. + +## Noncompliant Code Example (Pointer) + +In this noncompliant code example, inadequate space is allocated for a `struct tm` object because the size of the pointer is being used to determine the size of the pointed-to object: + +```cpp +#include +#include + +struct tm *make_tm(int year, int mon, int day, int hour, + int min, int sec) { + struct tm *tmb; + tmb = (struct tm *)malloc(sizeof(tmb)); + if (tmb == NULL) { + return NULL; + } + *tmb = (struct tm) { + .tm_sec = sec, .tm_min = min, .tm_hour = hour, + .tm_mday = day, .tm_mon = mon, .tm_year = year + }; + return tmb; +} +``` + +## Compliant Solution (Pointer) + +In this compliant solution, the correct amount of memory is allocated for the `struct tm` object. When allocating space for a single object, passing the (dereferenced) pointer type to the `sizeof` operator is a simple way to allocate sufficient memory. Because the `sizeof` operator does not evaluate its operand, dereferencing an uninitialized or null pointer in this context is well-defined behavior. + +```cpp +#include +#include + +struct tm *make_tm(int year, int mon, int day, int hour, + int min, int sec) { + struct tm *tmb; + tmb = (struct tm *)malloc(sizeof(*tmb)); + if (tmb == NULL) { + return NULL; + } + *tmb = (struct tm) { + .tm_sec = sec, .tm_min = min, .tm_hour = hour, + .tm_mday = day, .tm_mon = mon, .tm_year = year + }; + return tmb; +} +``` + +## Noncompliant Code Example (Integer) + +In this noncompliant code example, an array of `long` is allocated and assigned to `p`. The code attempts to check for unsigned integer overflow 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) and also ensures that `len` is not equal to zero. (See [MEM04-C. Beware of zero-length allocations](https://wiki.sei.cmu.edu/confluence/display/c/MEM04-C.+Beware+of+zero-length+allocations).) However, because `sizeof(int)` is used to compute the size, and not `sizeof(long)`, an insufficient amount of memory can be allocated on implementations where `sizeof(long)` is larger than `sizeof(int)`, and filling the array can cause a heap buffer overflow. + +```cpp +#include +#include + +void function(size_t len) { + long *p; + if (len == 0 || len > SIZE_MAX / sizeof(long)) { + /* Handle overflow */ + } + p = (long *)malloc(len * sizeof(int)); + if (p == NULL) { + /* Handle error */ + } + free(p); +} + +``` + +## Compliant Solution (Integer) + +This compliant solution uses `sizeof(long)` to correctly size the memory allocation: + +```cpp +#include +#include + +void function(size_t len) { + long *p; + if (len == 0 || len > SIZE_MAX / sizeof(long)) { + /* Handle overflow */ + } + p = (long *)malloc(len * sizeof(long)); + if (p == NULL) { + /* Handle error */ + } + free(p); +} + +``` + +## Compliant Solution (Integer) + +Alternatively, `sizeof(*p)` can be used to properly size the allocation: + +```cpp +#include +#include + +void function(size_t len) { + long *p; + if (len == 0 || len > SIZE_MAX / sizeof(*p)) { + /* Handle overflow */ + } + p = (long *)malloc(len * sizeof(*p)); + if (p == NULL) { + /* Handle error */ + } + free(p); +} +``` + +## Risk Assessment + +Providing invalid size arguments to memory allocation functions can lead to buffer overflows and the execution of arbitrary code with the permissions of the vulnerable process. + +
Rule Severity Likelihood Remediation Cost Priority Level
MEM35-C High Probable High P6 L2
+ + +## Automated Detection + +
Tool Version Checker Description
Astrée 22.04 malloc-size-insufficient Partially checked Besides direct rule violations, all undefined behaviour resulting from invalid memory accesses is reported by Astrée.
Axivion Bauhaus Suite 7.2.0 CertC-MEM35
CodeSonar 7.2p0 ALLOC.SIZE.ADDOFLOW ALLOC.SIZE.IOFLOW ALLOC.SIZE.MULOFLOW ALLOC.SIZE.SUBUFLOW ALLOC.SIZE.TRUNC IO.TAINT.SIZE MISC.MEM.SIZE.BADLANG.MEM.BOLANG.MEM.BULANG.STRUCT.PARITHLANG.STRUCT.PBBLANG.STRUCT.PPELANG.MEM.TBALANG.MEM.TOLANG.MEM.TU Addition overflow of allocation size Addition overflow of allocation size Multiplication overflow of allocation size Subtraction underflow of allocation size Truncation of allocation size Tainted allocation size Unreasonable size argument Buffer Overrun Buffer Underrun Pointer Arithmetic Pointer Before Beginning of Object Pointer Past End of Object Tainted Buffer Access Type Overrun Type Underrun
Compass/ROSE Could check violations of this rule by examining the size expression to malloc() or memcpy() functions. Specifically, the size argument should be bounded by 0, SIZE_MAX , and, unless it is a variable of type size_t or rsize_t , it should be bounds-checked before the malloc() call. If the argument is of the expression a\*b , then an appropriate check is if (a < SIZE_MAX / b && a > 0) ...
Coverity 2017.07 BAD_ALLOC_STRLEN SIZECHECK (deprecated) Partially implemented Can find instances where string length is miscalculated (length calculated may be one less than intended) for memory allocation purposes. Coverity Prevent cannot discover all violations of this rule, so further verification is necessary Finds memory allocations that are assigned to a pointer that reference objects larger than the allocated block
Helix QAC 2022.4 C0696, C0701, C1069, C1071, C1073, C2840 DF2840, DF2841, DF2842, DF2843, DF2935, DF2936, DF2937, DF2938
Klocwork 2022.4 INCORRECT.ALLOC_SIZE SV.TAINTED.ALLOC_SIZE
LDRA tool suite 9.7.1 400 S, 487 S, 115 D Enhanced enforcement
Splint 3.1.1
Parasoft C/C++test 2022.2 CERT_C-MEM35-a Do not use sizeof operator on pointer type to specify the size of the memory to be allocated via 'malloc', 'calloc' or 'realloc' function
PC-lint Plus 1.4 433, 826 Partially supported
Polyspace Bug Finder R2023a CERT C: Rule MEM35-C Checks for: Pointer access out of boundsointer access out of bounds, memory allocation with tainted sizeemory allocation with tainted size. Rule partially covered.
PRQA QA-C 9.7 0696, 0701, 1069, 1071, 1073, 2840, 2841, 2842, 2843, 2935, 2936, 2937, 2938
PRQA QA-C++ 4.4 2840, 2841, 2842, 2843, 2935, 2936, 2937, 2938
PVS-Studio 7.23 V531 , V635 , V781
RuleChecker 22.04 malloc-size-insufficient Partially checked
TrustInSoft Analyzer 1.38 mem_access Exhaustively detects undefined behavior (see one compliant and one non-compliant example ).
+ + +## Related Vulnerabilities + +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+MEM35-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
CERT C Secure Coding Standard ARR01-C. Do not apply the sizeof operator to a pointer when taking the size of an array INT31-C. Ensure that integer conversions do not result in lost or misinterpreted data Prior to 2018-01-12: CERT: Unspecified Relationship
CERT C Secure Coding Standard INT32-C. Ensure that operations on signed integers do not result in overflow Prior to 2018-01-12: CERT: Unspecified Relationship
CERT C Secure Coding Standard INT18-C. Evaluate integer expressions in a larger size before comparing or assigning to that size Prior to 2018-01-12: CERT: Unspecified Relationship
CERT C Secure Coding Standard MEM04-C. Beware of zero-length allocations 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 TS 17961:2013 Taking the size of a pointer to determine the size of the pointed-to type \[sizeofptr\] Prior to 2018-01-12: CERT: Unspecified Relationship
CWE 2.11 CWE-131 , Incorrect Calculation of Buffer Size 2017-05-16: CERT: Rule subset of CWE
CWE 2.11 CWE-680 2017-05-18: CERT: Rule subset of CWE
CWE 2.11 CWE-789 2017-06-12: 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-680 and MEM35-C** + +Intersection( INT32-C, MEM35-C) = Ø + +CWE-680 = Union( MEM35-C, list) where list = + +* Overflowed buffers with inadequate sizes not produced by integer overflow +**CWE-467 and MEM35-C** + +CWE-467 = Subset( MEM35-C) + +**CWE-789 and MEM35-C** + +Intersection( MEM35-C, CWE-789) = + +* Insufficient memory allocation on the heap +MEM35-C – CWE-789 = +* Insufficient memory allocation with trusted value but incorrect calculation +CWE-789 - MEM35-C = +* Sufficient memory allocation (possibly over-allocation) with untrusted value +**CWE-120 and MEM35-C** + +Intersection( MEM35-C, CWE-120) = Ø + +CWE-120 specifically addresses buffer overflow operations, which occur in the context of string-copying. MEM35-C specifically addresses allocation of memory ranges (some of which may be for subsequent string copy operations). + +Consequently, they address different sections of code, although one (or both) may be responsible for a single buffer overflow vulnerability. + +**CWE-131 and MEM35-C** + +* Intersection( INT30-C, MEM35-C) = Ø +* CWE-131 = Union( MEM35-C, list) where list = +* Miscalculating a buffer for a non-heap region (such as a variable-length array) + +## Bibliography + +
\[ Coverity 2007 \]
\[ Drepper 2006 \] Section 2.1.1, "Respecting Memory Bounds"
\[ Seacord 2013 \] Chapter 4, "Dynamic Memory Management" Chapter 5, "Integer Security"
\[ Viega 2005 \] Section 5.6.8, "Use of sizeof() on a Pointer Type"
\[ xorl 2009 \] CVE-2009-0587: Evolution Data Server Base64 Integer Overflows
+ + +## Implementation notes + +None + +## References + +* CERT-C: [MEM35-C: Allocate sufficient memory for an object](https://wiki.sei.cmu.edu/confluence/display/c) diff --git a/c/cert/src/rules/MEM35-C/InsufficientMemoryAllocatedForObject.ql b/c/cert/src/rules/MEM35-C/InsufficientMemoryAllocatedForObject.ql new file mode 100644 index 0000000000..5ff1725269 --- /dev/null +++ b/c/cert/src/rules/MEM35-C/InsufficientMemoryAllocatedForObject.ql @@ -0,0 +1,162 @@ +/** + * @id c/cert/insufficient-memory-allocated-for-object + * @name MEM35-C: Allocate sufficient memory for an object + * @description The size of memory allocated dynamically must be adequate to represent the type of + * object referenced by the allocated memory. + * @kind problem + * @precision medium + * @problem.severity error + * @tags external/cert/id/mem35-c + * correctness + * security + * external/cert/obligation/rule + */ + +import cpp +import codingstandards.c.cert +import codingstandards.cpp.Overflow +import semmle.code.cpp.controlflow.Guards +import semmle.code.cpp.dataflow.TaintTracking +import semmle.code.cpp.models.Models + +/** + * Gets the type of the operand of `op`. + */ +Type getSizeofOperatorType(SizeofOperator op) { + result = op.(SizeofExprOperator).getExprOperand().getType() + or + result = op.(SizeofTypeOperator).getTypeOperand() +} + +/** + * A function call which allocates memory, such as `malloc`. + */ +class AllocationFunctionCall extends AllocationExpr, FunctionCall { + AllocationFunctionCall() { this.getTarget() instanceof AllocationFunction } + + /** + * Gets the size argument `Expr` of this allocation function call. + */ + Expr getSizeArg() { + result = this.getArgument(this.getTarget().(AllocationFunction).getSizeArg()) + } + + /** + * Gets the computed value of the size argument of this allocation function call. + */ + int getSizeExprValue() { result = upperBound(this.getSizeArg()) } + + /** + * Gets the type of the object the allocation function result is assigned to. + * + * If the allocation is not assigned to a variable, this predicate does not hold. + */ + Type getBaseType() { + exists(PointerType pointer | + pointer.getBaseType() = result and + ( + exists(AssignExpr assign | + assign.getRValue() = this and assign.getLValue().getType() = pointer + ) + or + exists(Variable v | v.getInitializer().getExpr() = this and v.getType() = pointer) + ) + ) + } + + /** + * Gets a message describing the problem with this allocation function call. + * The `e` and `description` respectively provide an expression that influences + * the size of the allocation and a string describing that expression. + */ + string getMessageAndSourceInfo(Expr e, string description) { none() } +} + +/** + * An `AllocationFunctionCall` where the size argument is tainted by a `SizeofOperator` + * that has an operand of a different type than the base type of the variable assigned + * the result of the allocation call. + */ +class WrongSizeofOperatorAllocationFunctionCall extends AllocationFunctionCall { + SizeofOperator source; + + WrongSizeofOperatorAllocationFunctionCall() { + this.getBaseType() != getSizeofOperatorType(source) and + TaintTracking::localExprTaint(source, this.getSizeArg().getAChild*()) + } + + override string getMessageAndSourceInfo(Expr e, string description) { + result = "Allocation size calculated from the size of a different type ($@)." and + e = source and + description = "sizeof(" + getSizeofOperatorType(source).getName() + ")" + } +} + +/** + * An `AllocationFunctionCall` that allocates a size that is not a multiple + * of the size of the base type of the variable assigned the allocation. + * + * For example, an allocation of 14 bytes for `float` (`sizeof(float) == 4`) + * indicates an erroroneous allocation size, as 14 is not a multiple of 4 and + * thus cannot be the exact size of an array of floats. + * + * This class cannot also be a `WrongSizeofOperatorAllocationFunctionCall` instance, + * as an identified `SizeofOperator` operand type mismatch is more likely to indicate + * the root cause of an allocation size that is not a multiple of the base type size. + */ +class WrongSizeMultipleAllocationFunctionCall extends AllocationFunctionCall { + WrongSizeMultipleAllocationFunctionCall() { + // de-duplicate results if there is more precise info from a sizeof operator + not this instanceof WrongSizeofOperatorAllocationFunctionCall and + // the allocation size is not a multiple of the base type size + exists(int basesize, int allocated | + basesize = min(this.getBaseType().getSize()) and + allocated = this.getSizeExprValue() and + not exists(int size | this.getBaseType().getSize() = size | + size = 0 or + (allocated / size) * size = allocated + ) + ) + } + + override string getMessageAndSourceInfo(Expr e, string description) { + result = + "Allocation size (" + this.getSizeExprValue().toString() + + " bytes) is not a multiple of the size of '" + this.getBaseType().getName() + "' (" + + min(this.getBaseType().getSize()).toString() + " bytes)." and + e = this.getSizeArg() and + description = "" + } +} + +/** + * An `AllocationFunctionCall` where the size argument might be tainted by an overflowing + * or wrapping integer expression that is not checked for validity before the allocation. + */ +class OverflowingSizeAllocationFunctionCall extends AllocationFunctionCall { + InterestingOverflowingOperation bop; + + OverflowingSizeAllocationFunctionCall() { + // `bop` is not pre-checked to prevent overflow/wrapping + not bop.hasValidPreCheck() and + // and the size argument is tainted by `bop` + TaintTracking::localExprTaint(bop, this.getSizeArg().getAChild*()) and + // and there does not exist a post-wrapping-check before the allocation call + not exists(GuardCondition gc | + gc = bop.getAValidPostCheck() and + gc.controls(this.getBasicBlock(), _) + ) + } + + override string getMessageAndSourceInfo(Expr e, string description) { + result = "Allocation size derived from potentially overflowing or wrapping $@." and + e = bop and + description = "integer operation" + } +} + +from AllocationFunctionCall alloc, string message, Expr source, string sourceMessage +where + not isExcluded(alloc, Memory3Package::insufficientMemoryAllocatedForObjectQuery()) and + message = alloc.getMessageAndSourceInfo(source, sourceMessage) +select alloc, message, source, sourceMessage diff --git a/c/cert/test/rules/ARR32-C/VariableLengthArraySizeNotInValidRange.expected b/c/cert/test/rules/ARR32-C/VariableLengthArraySizeNotInValidRange.expected new file mode 100644 index 0000000000..25153f195b --- /dev/null +++ b/c/cert/test/rules/ARR32-C/VariableLengthArraySizeNotInValidRange.expected @@ -0,0 +1,41 @@ +| test.c:14:8:14:8 | VLA declaration | Variable-length array dimension size may be in an invalid range. | +| test.c:15:8:15:8 | VLA declaration | Variable-length array dimension size may be in an invalid range. | +| test.c:16:8:16:8 | VLA declaration | Variable-length array dimension size may be in an invalid range. | +| test.c:18:8:18:8 | VLA declaration | Variable-length array size derives from an overflowing or wrapping expression. | +| test.c:20:8:20:8 | VLA declaration | Variable-length array dimension size may be in an invalid range. | +| test.c:25:8:25:8 | VLA declaration | Variable-length array dimension size may be in an invalid range. | +| test.c:27:8:27:8 | VLA declaration | Variable-length array dimension size may be in an invalid range. | +| test.c:28:8:28:8 | VLA declaration | Variable-length array dimension size may be in an invalid range. | +| test.c:29:8:29:8 | VLA declaration | Variable-length array dimension size may be in an invalid range. | +| test.c:30:8:30:8 | VLA declaration | Variable-length array dimension size may be in an invalid range. | +| test.c:33:10:33:10 | VLA declaration | Variable-length array dimension size may be in an invalid range. | +| test.c:35:10:35:10 | VLA declaration | Variable-length array dimension size may be in an invalid range. | +| test.c:41:10:41:10 | VLA declaration | Variable-length array dimension size may be in an invalid range. | +| test.c:50:10:50:10 | VLA declaration | Variable-length array dimension size may be in an invalid range. | +| test.c:51:10:51:10 | VLA declaration | Variable-length array size derives from an overflowing or wrapping expression. | +| test.c:56:7:56:7 | VLA declaration | Variable-length array dimension size may be in an invalid range. | +| test.c:57:7:57:7 | VLA declaration | Variable-length array size derives from an overflowing or wrapping expression. | +| test.c:58:7:58:7 | VLA declaration | Variable-length array size derives from an overflowing or wrapping expression. | +| test.c:61:9:61:9 | VLA declaration | Variable-length array dimension size may be in an invalid range. | +| test.c:63:9:63:9 | VLA declaration | Variable-length array size derives from an overflowing or wrapping expression. | +| test.c:67:9:67:9 | VLA declaration | Variable-length array dimension size may be in an invalid range. | +| test.c:68:9:68:9 | VLA declaration | Variable-length array size derives from an overflowing or wrapping expression. | +| test.c:69:9:69:9 | VLA declaration | Variable-length array dimension size may be in an invalid range. | +| test.c:73:9:73:9 | VLA declaration | Variable-length array dimension size may be in an invalid range. | +| test.c:74:9:74:9 | VLA declaration | Variable-length array dimension size may be in an invalid range. | +| test.c:75:9:75:9 | VLA declaration | Variable-length array size derives from an overflowing or wrapping expression. | +| test.c:79:11:79:11 | VLA declaration | Variable-length array dimension size may be in an invalid range. | +| test.c:86:9:86:9 | VLA declaration | Variable-length array size derives from an overflowing or wrapping expression. | +| test.c:93:9:93:9 | VLA declaration | Variable-length array size derives from an overflowing or wrapping expression. | +| test.c:100:7:100:7 | VLA declaration | Variable-length array dimension size may be in an invalid range. | +| test.c:104:15:104:15 | VLA declaration | Variable-length array dimension size may be in an invalid range. | +| test.c:112:9:112:9 | VLA declaration | Variable-length array dimension size may be in an invalid range. | +| test.c:117:9:117:9 | VLA declaration | Variable-length array total size may be excessively large. | +| test.c:118:9:118:9 | VLA declaration | Variable-length array dimension size may be in an invalid range. | +| test.c:119:9:119:9 | VLA declaration | Variable-length array total size may be excessively large. | +| test.c:120:9:120:9 | VLA declaration | Variable-length array total size may be excessively large. | +| test.c:123:11:123:11 | VLA declaration | Variable-length array total size may be excessively large. | +| test.c:124:11:124:11 | VLA declaration | Variable-length array total size may be excessively large. | +| test.c:125:11:125:11 | VLA declaration | Variable-length array dimension size may be in an invalid range. | +| test.c:131:11:131:11 | VLA declaration | Variable-length array size derives from an overflowing or wrapping expression. | +| test.c:135:11:135:11 | VLA declaration | Variable-length array total size may be excessively large. | diff --git a/c/cert/test/rules/ARR32-C/VariableLengthArraySizeNotInValidRange.qlref b/c/cert/test/rules/ARR32-C/VariableLengthArraySizeNotInValidRange.qlref new file mode 100644 index 0000000000..0c6a4bd08d --- /dev/null +++ b/c/cert/test/rules/ARR32-C/VariableLengthArraySizeNotInValidRange.qlref @@ -0,0 +1 @@ +rules/ARR32-C/VariableLengthArraySizeNotInValidRange.ql \ No newline at end of file diff --git a/c/cert/test/rules/ARR32-C/test.c b/c/cert/test/rules/ARR32-C/test.c new file mode 100644 index 0000000000..80a43cdfac --- /dev/null +++ b/c/cert/test/rules/ARR32-C/test.c @@ -0,0 +1,143 @@ +#include +#include +#include + +// arbitrary excessive stack alloc size: USHRT_MAX bytes +#define VLA_MAX_SIZE USHRT_MAX + +void test_vla_constants(void) { + size_t uninitialized; + size_t zero = 0; + size_t two = 2; + size_t max_num = VLA_MAX_SIZE / sizeof(char); + + char vla0[uninitialized]; // NON_COMPLIANT - uninitialized + char vla1[zero]; // NON_COMPLIANT - zero-sized array + char vla2[zero * two]; // NON_COMPLIANT - zero-sized array + char vla3[zero + two]; // COMPLIANT + char vla4[zero - two]; // NON_COMPLIANT - wrap-around + char vla5[max_num]; // COMPLIANT + char vla6[max_num + two]; // NON_COMPLIANT - too large + char vla7[max_num + 1 - two]; // COMPLIANT; +} + +void test_vla_bounds8(uint8_t num8, int8_t snum8) { + char vla0[num8]; // NON_COMPLIANT - size could be `0` + char vla1[num8 + 1]; // COMPLIANT + char vla2[num8 - 1]; // NON_COMPLIANT - wrap-around + char vla3[snum8]; // NON_COMPLIANT - unbounded + char vla4[snum8 + 1]; // NON_COMPLIANT - unbounded + char vla5[snum8 - 1]; // NON_COMPLIANT - unbounded + + if (num8 == 0) { + char vla6[num8]; // NON_COMPLIANT - size is 0 + char vla7[num8 + 1]; // COMPLIANT - size is 1 + char vla8[num8 - 1]; // NON_COMPLIANT - wrap-around + } + + if (num8 > 0) { + char vla6[num8]; // COMPLIANT + char vla7[num8 + 1]; // COMPLIANT + char vla8[num8 - 1]; // NON_COMPLIANT - unbounded + } +} + +void test_overflowed_size(int8_t ssize) { + if (ssize > 1) { + int8_t tmp = ssize * 2; + char vla0[ssize]; // COMPLIANT + char vla1[ssize * 2]; // COMPLIANT - type promotion + char vla2[tmp]; // NON_COMPLIANT - potential overflow + char vla3[++ssize]; // NON_COMPLIANT - potential overflow + } +} + +void test_vla_bounds(size_t num) { + int vla0[num]; // NON_COMPLIANT - size could be greater than max + int vla1[num + 1]; // NON_COMPLIANT - unbounded + int vla2[num - 1]; // NON_COMPLIANT - unbounded + + if (num == 0) { + int vla6[num]; // NON_COMPLIANT - size is 0 + int vla7[num + 1]; // COMPLIANT - size is 1 + int vla8[num - 1]; // NON_COMPLIANT - unbounded + } + + if (num > 0) { + int vla6[num]; // NON_COMPLIANT - size could be greater than max + int vla7[num + 1]; // NON_COMPLIANT - unbounded + int vla8[num - 1]; // NON_COMPLIANT - unbounded + } + + if (VLA_MAX_SIZE / sizeof(int) >= num) { + int vla6[num]; // NON_COMPLIANT - size could be 0 + int vla7[num + 1]; // NON_COMPLIANT - size greater than max + int vla8[num - 1]; // NON_COMPLIANT - unbounded + + if (num >= 100) { + int vla9[num]; // COMPLIANT + int vla10[num + 1]; // NON_COMPLIANT - unbounded + int vla11[num - 1]; // COMPLIANT + } + } + + size_t num2 = num + num; + if (num2 > 0 && num2 < 100) { + int vla12[num2]; // NON_COMPLIANT - bad post-check + } + + signed int num3 = INT_MAX; + num3++; + num3 += INT_MAX; + if (num3 > 0 && num3 < 100) { + int vla13[num3]; // NON_COMPLIANT - overflowed + } + + int num4; + if (num > 2) { + num4 = num - 2; // potentially changed value + } + int vla14[num4]; // NON_COMPLIANT - unbounded +} + +void test_vla_typedef(size_t x, size_t y) { + typedef int VLA[x][y]; // NON_COMPLIANT + // ... + // (void)sizeof(VLA); +} + +void test_multidimensional_vla(size_t n, size_t m) { + + if (VLA_MAX_SIZE / sizeof(int) >= (n * m)) { // wrapping check + int vla0[n][m]; // NON_COMPLIANT - size too large + } + + if (m > 0 && n > 0 && VLA_MAX_SIZE / sizeof(int) >= n && + VLA_MAX_SIZE / sizeof(int) >= m && VLA_MAX_SIZE / sizeof(int) >= n * m) { + int vla1[n][m]; // COMPLIANT[FALSE_POSITIVE] + int vla2[n - 1][m - 1]; // NON_COMPLIANT - unbounded + int vla3[n][n]; // NON_COMPLIANT - n*n not checked + int vla4[n][n][n]; // NON_COMPLIANT - unbounded + + if (VLA_MAX_SIZE / (sizeof(int) * n) >= n * n) { + int vla5[n][n][n]; // COMPLIANT[FALSE_POSITIVE] + int vla6[m][m][m]; // NON_COMPLIANT - size too large + int vla7[n][n][n + 1]; // NON_COMPLIANT - size too large + } + + if (m > 0 && m <= 100 && n > 0 && n <= 100) { + int vla08[n][m]; // COMPLIANT + int vla09[n][n]; // COMPLIANT + int vla10[n][n - 98][n]; // NON_COMPLIANT - unbounded + if (n == 100) { + int vla11[n][n - 98][n]; // COMPLIANT + } + int vla12[n][m][n]; // NON_COMPLIANT + } + } +} + +void test_fvla(int size, + int data[size][size]) { // COMPLIANT - not an actual VLA + return; +} \ No newline at end of file diff --git a/c/cert/test/rules/ARR37-C/DoNotUsePointerArithmeticOnNonArrayObjectPointers.expected b/c/cert/test/rules/ARR37-C/DoNotUsePointerArithmeticOnNonArrayObjectPointers.expected new file mode 100644 index 0000000000..8a7bfe553b --- /dev/null +++ b/c/cert/test/rules/ARR37-C/DoNotUsePointerArithmeticOnNonArrayObjectPointers.expected @@ -0,0 +1,43 @@ +edges +| test.c:14:38:14:39 | p1 | test.c:18:10:18:11 | v1 | +| test.c:14:38:14:39 | p1 | test.c:19:10:19:11 | v2 | +| test.c:14:38:14:39 | p1 | test.c:20:10:20:11 | p1 | +| test.c:14:38:14:39 | p1 | test.c:21:10:21:11 | p1 | +| test.c:14:38:14:39 | p1 | test.c:22:9:22:10 | p1 | +| test.c:14:38:14:39 | p1 | test.c:23:13:23:14 | p1 | +| test.c:14:38:14:39 | p1 | test.c:24:9:24:10 | p1 | +| test.c:14:38:14:39 | p1 | test.c:25:9:25:10 | p1 | +| test.c:51:30:51:38 | & ... | test.c:14:38:14:39 | p1 | +nodes +| test.c:14:38:14:39 | p1 | semmle.label | p1 | +| test.c:18:10:18:11 | v1 | semmle.label | v1 | +| test.c:19:10:19:11 | v2 | semmle.label | v2 | +| test.c:20:10:20:11 | p1 | semmle.label | p1 | +| test.c:21:10:21:11 | p1 | semmle.label | p1 | +| test.c:22:9:22:10 | p1 | semmle.label | p1 | +| test.c:23:13:23:14 | p1 | semmle.label | p1 | +| test.c:24:9:24:10 | p1 | semmle.label | p1 | +| test.c:25:9:25:10 | p1 | semmle.label | p1 | +| test.c:39:11:39:19 | & ... | semmle.label | & ... | +| test.c:40:10:40:18 | & ... | semmle.label | & ... | +| test.c:42:10:42:15 | & ... | semmle.label | & ... | +| test.c:43:10:43:15 | & ... | semmle.label | & ... | +| test.c:44:10:44:15 | & ... | semmle.label | & ... | +| test.c:46:10:46:15 | & ... | semmle.label | & ... | +| test.c:51:30:51:38 | & ... | semmle.label | & ... | +subpaths +#select +| test.c:18:10:18:11 | v1 | test.c:51:30:51:38 | & ... | test.c:18:10:18:11 | v1 | Pointer arithmetic on non-array object pointer. | +| test.c:19:10:19:11 | v2 | test.c:51:30:51:38 | & ... | test.c:19:10:19:11 | v2 | Pointer arithmetic on non-array object pointer. | +| test.c:20:10:20:11 | p1 | test.c:51:30:51:38 | & ... | test.c:20:10:20:11 | p1 | Pointer arithmetic on non-array object pointer. | +| test.c:21:10:21:11 | p1 | test.c:51:30:51:38 | & ... | test.c:21:10:21:11 | p1 | Pointer arithmetic on non-array object pointer. | +| test.c:22:9:22:10 | p1 | test.c:51:30:51:38 | & ... | test.c:22:9:22:10 | p1 | Pointer arithmetic on non-array object pointer. | +| test.c:23:13:23:14 | p1 | test.c:51:30:51:38 | & ... | test.c:23:13:23:14 | p1 | Pointer arithmetic on non-array object pointer. | +| test.c:24:9:24:10 | p1 | test.c:51:30:51:38 | & ... | test.c:24:9:24:10 | p1 | Pointer arithmetic on non-array object pointer. | +| test.c:25:9:25:10 | p1 | test.c:51:30:51:38 | & ... | test.c:25:9:25:10 | p1 | Pointer arithmetic on non-array object pointer. | +| test.c:39:11:39:19 | & ... | test.c:39:11:39:19 | & ... | test.c:39:11:39:19 | & ... | Pointer arithmetic on non-array object pointer. | +| test.c:40:10:40:18 | & ... | test.c:40:10:40:18 | & ... | test.c:40:10:40:18 | & ... | Pointer arithmetic on non-array object pointer. | +| test.c:42:10:42:15 | & ... | test.c:42:10:42:15 | & ... | test.c:42:10:42:15 | & ... | Pointer arithmetic on non-array object pointer. | +| test.c:43:10:43:15 | & ... | test.c:43:10:43:15 | & ... | test.c:43:10:43:15 | & ... | Pointer arithmetic on non-array object pointer. | +| test.c:44:10:44:15 | & ... | test.c:44:10:44:15 | & ... | test.c:44:10:44:15 | & ... | Pointer arithmetic on non-array object pointer. | +| test.c:46:10:46:15 | & ... | test.c:46:10:46:15 | & ... | test.c:46:10:46:15 | & ... | Pointer arithmetic on non-array object pointer. | diff --git a/c/cert/test/rules/ARR37-C/DoNotUsePointerArithmeticOnNonArrayObjectPointers.qlref b/c/cert/test/rules/ARR37-C/DoNotUsePointerArithmeticOnNonArrayObjectPointers.qlref new file mode 100644 index 0000000000..badf328837 --- /dev/null +++ b/c/cert/test/rules/ARR37-C/DoNotUsePointerArithmeticOnNonArrayObjectPointers.qlref @@ -0,0 +1 @@ +rules/ARR37-C/DoNotUsePointerArithmeticOnNonArrayObjectPointers.ql \ No newline at end of file diff --git a/c/cert/test/rules/ARR37-C/test.c b/c/cert/test/rules/ARR37-C/test.c new file mode 100644 index 0000000000..28ccd243a3 --- /dev/null +++ b/c/cert/test/rules/ARR37-C/test.c @@ -0,0 +1,52 @@ +struct s1 { + int f1; + int f2; + int f3; + int f4[2]; +}; + +struct s2 { + int f1; + int f2; + int data[]; +}; + +void test_ptr_arithmetic_nested(int *p1) { + // path-dependent + int *v1 = p1; + int *v2 = p1; + (void)(v1++); + (void)(v2--); + (void)(p1 + 1); + (void)(p1 - 1); + (void)p1[1]; + (void)(1 [p1]); + (void)p1[*p1 + 0]; + (void)p1[0 + 1]; + (void)p1[0]; // COMPLIANT +} + +void test(struct s1 p1, struct s1 *p2, struct s2 *p3) { + struct s1 v1[3]; + struct s2 v2[3]; + + (void)*(v1 + 2); // COMPLIANT + (void)*(v1 + 2); // COMPLIANT + + (void)v1[2]; // COMPLIANT + (void)v2[2]; // COMPLIANT + + (void)((&v1[0].f1)[1]); // NON_COMPLIANT + (void)(&v1[0].f1 + v1[1].f1); // NON_COMPLIANT + + (void)(&p1.f1)[1]; // NON_COMPLIANT + (void)(&p1.f1 + 1); // NON_COMPLIANT + (void)(&p1.f2 - 1); // NON_COMPLIANT + + (void)(&p1.f1 + p1.f1); // NON_COMPLIANT + (void)(p1.f4 + 1); // COMPLIANT + (void)(&p1.f4 + 1); // COMPLIANT + + test_ptr_arithmetic_nested((int *)&v1[0].f4); // COMPLIANT + test_ptr_arithmetic_nested(&v1[0].f1); // NON_COMPLIANT +} \ No newline at end of file diff --git a/c/cert/test/rules/EXP35-C/DoNotModifyObjectsWithTemporaryLifetime.expected b/c/cert/test/rules/EXP35-C/DoNotModifyObjectsWithTemporaryLifetime.expected new file mode 100644 index 0000000000..f14ab4de4a --- /dev/null +++ b/c/cert/test/rules/EXP35-C/DoNotModifyObjectsWithTemporaryLifetime.expected @@ -0,0 +1,4 @@ +| test.c:65:18:65:18 | a | Field access on $@ qualifier occurs after its temporary object lifetime. | test.c:65:9:65:14 | call to get_s1 | function call | +| test.c:67:18:67:19 | s1 | Field access on $@ qualifier occurs after its temporary object lifetime. | test.c:67:9:67:14 | call to get_s3 | function call | +| test.c:68:18:68:19 | i1 | Field access on $@ qualifier occurs after its temporary object lifetime. | test.c:68:9:68:14 | call to get_s3 | function call | +| test.c:69:18:69:21 | af12 | Field access on $@ qualifier occurs after its temporary object lifetime. | test.c:69:9:69:14 | call to get_s4 | function call | diff --git a/c/cert/test/rules/EXP35-C/DoNotModifyObjectsWithTemporaryLifetime.qlref b/c/cert/test/rules/EXP35-C/DoNotModifyObjectsWithTemporaryLifetime.qlref new file mode 100644 index 0000000000..a142303a4e --- /dev/null +++ b/c/cert/test/rules/EXP35-C/DoNotModifyObjectsWithTemporaryLifetime.qlref @@ -0,0 +1 @@ +rules/EXP35-C/DoNotModifyObjectsWithTemporaryLifetime.ql \ No newline at end of file diff --git a/c/cert/test/rules/EXP35-C/test.c b/c/cert/test/rules/EXP35-C/test.c new file mode 100644 index 0000000000..09e91089ab --- /dev/null +++ b/c/cert/test/rules/EXP35-C/test.c @@ -0,0 +1,71 @@ +#include + +typedef float AF12[12]; + +struct S1 { + char a[8]; +}; +struct S2 { + struct S1 *s1; +}; +struct S3 { + struct S1 s1; + int i1; +}; +struct S4 { + AF12 af12; +}; + +struct S5 { + int i1; + struct S1 *s1; +}; + +struct S1 get_s1(void) { + struct S1 s1; + return s1; +} + +struct S1 *get_s1_ptr(void) { + struct S1 *s1 = malloc(sizeof(struct S1)); + return s1; +} + +struct S2 get_s2(void) { + struct S2 s2; + return s2; +} + +struct S3 get_s3(void) { + struct S3 s3; + return s3; +} + +struct S4 get_s4(void) { + struct S4 s4; + return s4; +} + +struct S5 get_s5(void) { + struct S5 s5; + return s5; +} + +void test_field_access(void) { + struct S1 s1 = get_s1(); + struct S2 s2 = get_s2(); + struct S3 s3 = get_s3(); + struct S4 s4 = get_s4(); + + s1.a[0] = 'a'; // COMPLIANT + s2.s1->a[0] = 'a'; // COMPLIANT + s3.s1.a[0] = 'a'; // COMPLIANT + s4.af12[0] = 0.0f; // COMPLIANT + + (void)get_s1().a; // NON_COMPLIANT + (void)get_s2().s1->a; // COMPLIANT + (void)get_s3().s1.a; // NON_COMPLIANT + (void)get_s3().i1; // NON_COMPLIANT - even if scalar type accessed + (void)get_s4().af12; // NON_COMPLIANT + (void)get_s5().s1->a; // COMPLIANT +} \ No newline at end of file diff --git a/c/cert/test/rules/MEM35-C/InsufficientMemoryAllocatedForObject.expected b/c/cert/test/rules/MEM35-C/InsufficientMemoryAllocatedForObject.expected new file mode 100644 index 0000000000..30dece9299 --- /dev/null +++ b/c/cert/test/rules/MEM35-C/InsufficientMemoryAllocatedForObject.expected @@ -0,0 +1,9 @@ +| test.c:12:19:12:24 | call to malloc | Allocation size (32 bytes) is not a multiple of the size of 'S1' (36 bytes). | test.c:12:26:12:32 | 32 | | +| test.c:15:19:15:24 | call to malloc | Allocation size calculated from the size of a different type ($@). | test.c:15:26:15:35 | sizeof() | sizeof(S1 *) | +| test.c:20:19:20:24 | call to malloc | Allocation size (128 bytes) is not a multiple of the size of 'S1' (36 bytes). | test.c:20:26:20:36 | ... * ... | | +| test.c:21:19:21:24 | call to malloc | Allocation size (128 bytes) is not a multiple of the size of 'S1' (36 bytes). | test.c:21:26:21:36 | ... * ... | | +| test.c:25:14:25:19 | call to malloc | Allocation size calculated from the size of a different type ($@). | test.c:25:27:25:37 | sizeof(int) | sizeof(int) | +| test.c:25:14:25:19 | call to malloc | Allocation size derived from potentially overflowing or wrapping $@. | test.c:25:21:25:37 | ... * ... | integer operation | +| test.c:31:14:31:19 | call to malloc | Allocation size derived from potentially overflowing or wrapping $@. | test.c:31:21:31:38 | ... * ... | integer operation | +| test.c:32:14:32:19 | call to malloc | Allocation size derived from potentially overflowing or wrapping $@. | test.c:29:17:29:34 | ... * ... | integer operation | +| test.c:40:14:40:19 | call to malloc | Allocation size derived from potentially overflowing or wrapping $@. | test.c:29:17:29:34 | ... * ... | integer operation | diff --git a/c/cert/test/rules/MEM35-C/InsufficientMemoryAllocatedForObject.qlref b/c/cert/test/rules/MEM35-C/InsufficientMemoryAllocatedForObject.qlref new file mode 100644 index 0000000000..7da5a9c268 --- /dev/null +++ b/c/cert/test/rules/MEM35-C/InsufficientMemoryAllocatedForObject.qlref @@ -0,0 +1 @@ +rules/MEM35-C/InsufficientMemoryAllocatedForObject.ql \ No newline at end of file diff --git a/c/cert/test/rules/MEM35-C/test.c b/c/cert/test/rules/MEM35-C/test.c new file mode 100644 index 0000000000..938d3f1076 --- /dev/null +++ b/c/cert/test/rules/MEM35-C/test.c @@ -0,0 +1,41 @@ +#include +#include + +#define S1_SIZE 32 // incorrect size for struct S1 + +struct S1 { + char f1[S1_SIZE]; + int f2; +}; + +void sizecheck_test(void) { + struct S1 *v1 = malloc(S1_SIZE); // NON_COMPLIANT + struct S1 *v2 = malloc(sizeof(struct S1)); // COMPLIANT + struct S1 *v3 = malloc(sizeof(*v2)); // COMPLIANT + struct S1 *v4 = malloc(sizeof(v4)); // NON_COMPLIANT + char *v5 = malloc(10); // COMPLIANT +} + +void sizecheck2_test(size_t len) { + struct S1 *v1 = malloc(S1_SIZE * 4); // NON_COMPLIANT + struct S1 *v2 = malloc(S1_SIZE * 4); // NON_COMPLIANT + struct S1 *v3 = malloc( + S1_SIZE * 9); // COMPLIANT - erroneous logic, but the size product is an + // LCM of S1_SIZE and sizeof(S1) and thus a valid multiple + long *v4 = malloc(len * sizeof(int)); // NON_COMPLIANT - wrong sizeof type +} + +void unsafe_int_test(size_t len) { + size_t size = len * sizeof(long); + long *v1 = malloc(len); // COMPLIANT - even could indicate a logic error + long *v2 = malloc(len * sizeof(long)); // NON_COMPLIANT - unbounded int + long *v3 = malloc(size); // NON_COMPLIANT - unbounded int + + if (len > SIZE_MAX / sizeof(*v3)) { + // overflow/wrapping check + return; + } + + long *v4 = malloc(len * sizeof(long)); // COMPLIANT - overflow checked + long *v5 = malloc(size); // NON_COMPLIANT - `size` not checked +} diff --git a/cpp/common/src/codingstandards/cpp/exclusions/c/InvalidMemory2.qll b/cpp/common/src/codingstandards/cpp/exclusions/c/InvalidMemory2.qll new file mode 100644 index 0000000000..f1a37e4bcc --- /dev/null +++ b/cpp/common/src/codingstandards/cpp/exclusions/c/InvalidMemory2.qll @@ -0,0 +1,61 @@ +//** THIS FILE IS AUTOGENERATED, DO NOT MODIFY DIRECTLY. **/ +import cpp +import RuleMetadata +import codingstandards.cpp.exclusions.RuleMetadata + +newtype InvalidMemory2Query = + TVariableLengthArraySizeNotInValidRangeQuery() or + TDoNotUsePointerArithmeticOnNonArrayObjectPointersQuery() or + TDoNotModifyObjectsWithTemporaryLifetimeQuery() + +predicate isInvalidMemory2QueryMetadata(Query query, string queryId, string ruleId, string category) { + query = + // `Query` instance for the `variableLengthArraySizeNotInValidRange` query + InvalidMemory2Package::variableLengthArraySizeNotInValidRangeQuery() and + queryId = + // `@id` for the `variableLengthArraySizeNotInValidRange` query + "c/cert/variable-length-array-size-not-in-valid-range" and + ruleId = "ARR32-C" and + category = "rule" + or + query = + // `Query` instance for the `doNotUsePointerArithmeticOnNonArrayObjectPointers` query + InvalidMemory2Package::doNotUsePointerArithmeticOnNonArrayObjectPointersQuery() and + queryId = + // `@id` for the `doNotUsePointerArithmeticOnNonArrayObjectPointers` query + "c/cert/do-not-use-pointer-arithmetic-on-non-array-object-pointers" and + ruleId = "ARR37-C" and + category = "rule" + or + query = + // `Query` instance for the `doNotModifyObjectsWithTemporaryLifetime` query + InvalidMemory2Package::doNotModifyObjectsWithTemporaryLifetimeQuery() and + queryId = + // `@id` for the `doNotModifyObjectsWithTemporaryLifetime` query + "c/cert/do-not-modify-objects-with-temporary-lifetime" and + ruleId = "EXP35-C" and + category = "rule" +} + +module InvalidMemory2Package { + Query variableLengthArraySizeNotInValidRangeQuery() { + //autogenerate `Query` type + result = + // `Query` type for `variableLengthArraySizeNotInValidRange` query + TQueryC(TInvalidMemory2PackageQuery(TVariableLengthArraySizeNotInValidRangeQuery())) + } + + Query doNotUsePointerArithmeticOnNonArrayObjectPointersQuery() { + //autogenerate `Query` type + result = + // `Query` type for `doNotUsePointerArithmeticOnNonArrayObjectPointers` query + TQueryC(TInvalidMemory2PackageQuery(TDoNotUsePointerArithmeticOnNonArrayObjectPointersQuery())) + } + + Query doNotModifyObjectsWithTemporaryLifetimeQuery() { + //autogenerate `Query` type + result = + // `Query` type for `doNotModifyObjectsWithTemporaryLifetime` query + TQueryC(TInvalidMemory2PackageQuery(TDoNotModifyObjectsWithTemporaryLifetimeQuery())) + } +} diff --git a/cpp/common/src/codingstandards/cpp/exclusions/c/Memory3.qll b/cpp/common/src/codingstandards/cpp/exclusions/c/Memory3.qll new file mode 100644 index 0000000000..c59ff5eda8 --- /dev/null +++ b/cpp/common/src/codingstandards/cpp/exclusions/c/Memory3.qll @@ -0,0 +1,26 @@ +//** THIS FILE IS AUTOGENERATED, DO NOT MODIFY DIRECTLY. **/ +import cpp +import RuleMetadata +import codingstandards.cpp.exclusions.RuleMetadata + +newtype Memory3Query = TInsufficientMemoryAllocatedForObjectQuery() + +predicate isMemory3QueryMetadata(Query query, string queryId, string ruleId, string category) { + query = + // `Query` instance for the `insufficientMemoryAllocatedForObject` query + Memory3Package::insufficientMemoryAllocatedForObjectQuery() and + queryId = + // `@id` for the `insufficientMemoryAllocatedForObject` query + "c/cert/insufficient-memory-allocated-for-object" and + ruleId = "MEM35-C" and + category = "rule" +} + +module Memory3Package { + Query insufficientMemoryAllocatedForObjectQuery() { + //autogenerate `Query` type + result = + // `Query` type for `insufficientMemoryAllocatedForObject` query + TQueryC(TMemory3PackageQuery(TInsufficientMemoryAllocatedForObjectQuery())) + } +} diff --git a/cpp/common/src/codingstandards/cpp/exclusions/c/RuleMetadata.qll b/cpp/common/src/codingstandards/cpp/exclusions/c/RuleMetadata.qll index c758d2512f..58fd7b84cf 100644 --- a/cpp/common/src/codingstandards/cpp/exclusions/c/RuleMetadata.qll +++ b/cpp/common/src/codingstandards/cpp/exclusions/c/RuleMetadata.qll @@ -34,11 +34,13 @@ import IO3 import IO4 import IntegerOverflow import InvalidMemory1 +import InvalidMemory2 import Language1 import Language2 import Language3 import Memory1 import Memory2 +import Memory3 import Misc import Pointers1 import Pointers2 @@ -99,11 +101,13 @@ newtype TCQuery = TIO4PackageQuery(IO4Query q) or TIntegerOverflowPackageQuery(IntegerOverflowQuery q) or TInvalidMemory1PackageQuery(InvalidMemory1Query q) or + TInvalidMemory2PackageQuery(InvalidMemory2Query q) or TLanguage1PackageQuery(Language1Query q) or TLanguage2PackageQuery(Language2Query q) or TLanguage3PackageQuery(Language3Query q) or TMemory1PackageQuery(Memory1Query q) or TMemory2PackageQuery(Memory2Query q) or + TMemory3PackageQuery(Memory3Query q) or TMiscPackageQuery(MiscQuery q) or TPointers1PackageQuery(Pointers1Query q) or TPointers2PackageQuery(Pointers2Query q) or @@ -164,11 +168,13 @@ predicate isQueryMetadata(Query query, string queryId, string ruleId, string cat isIO4QueryMetadata(query, queryId, ruleId, category) or isIntegerOverflowQueryMetadata(query, queryId, ruleId, category) or isInvalidMemory1QueryMetadata(query, queryId, ruleId, category) or + isInvalidMemory2QueryMetadata(query, queryId, ruleId, category) or isLanguage1QueryMetadata(query, queryId, ruleId, category) or isLanguage2QueryMetadata(query, queryId, ruleId, category) or isLanguage3QueryMetadata(query, queryId, ruleId, category) or isMemory1QueryMetadata(query, queryId, ruleId, category) or isMemory2QueryMetadata(query, queryId, ruleId, category) or + isMemory3QueryMetadata(query, queryId, ruleId, category) or isMiscQueryMetadata(query, queryId, ruleId, category) or isPointers1QueryMetadata(query, queryId, ruleId, category) or isPointers2QueryMetadata(query, queryId, ruleId, category) or diff --git a/rule_packages/c/InvalidMemory2.json b/rule_packages/c/InvalidMemory2.json new file mode 100644 index 0000000000..cb7d380159 --- /dev/null +++ b/rule_packages/c/InvalidMemory2.json @@ -0,0 +1,65 @@ +{ + "CERT-C": { + "ARR32-C": { + "properties": { + "obligation": "rule" + }, + "queries": [ + { + "description": "A variable-length array size that is zero, negative, overflowed, wrapped around, or excessively large may lead to undefined behaviour.", + "kind": "problem", + "name": "Ensure size arguments for variable length arrays are in a valid range", + "precision": "high", + "severity": "error", + "short_name": "VariableLengthArraySizeNotInValidRange", + "tags": [ + "correctness", + "security" + ] + } + ], + "title": "Ensure size arguments for variable length arrays are in a valid range" + }, + "ARR37-C": { + "properties": { + "obligation": "rule" + }, + "queries": [ + { + "description": "A pair of elements that are not elements in the same array are not guaranteed to be contiguous in memory and therefore should not be addressed using pointer arithmetic.", + "kind": "path-problem", + "name": "Do not add or subtract an integer to a pointer to a non-array object", + "precision": "high", + "severity": "error", + "short_name": "DoNotUsePointerArithmeticOnNonArrayObjectPointers", + "tags": [ + "correctness" + ] + } + ], + "title": "Do not add or subtract an integer to a pointer to a non-array object" + }, + "EXP35-C": { + "properties": { + "obligation": "rule" + }, + "queries": [ + { + "description": "Attempting to modify an object with temporary lifetime results in undefined behavior.", + "kind": "problem", + "name": "Do not modify objects with temporary lifetime", + "precision": "high", + "severity": "error", + "short_name": "DoNotModifyObjectsWithTemporaryLifetime", + "tags": [ + "correctness" + ], + "implementation_scope": { + "description": "This implementation also always reports non-modifying accesses of objects with temporary lifetime, which are only compliant in C11." + } + } + ], + "title": "Do not modify objects with temporary lifetime" + } + } +} \ No newline at end of file diff --git a/rule_packages/c/Memory3.json b/rule_packages/c/Memory3.json new file mode 100644 index 0000000000..6eafcc6509 --- /dev/null +++ b/rule_packages/c/Memory3.json @@ -0,0 +1,24 @@ +{ + "CERT-C": { + "MEM35-C": { + "properties": { + "obligation": "rule" + }, + "queries": [ + { + "description": "The size of memory allocated dynamically must be adequate to represent the type of object referenced by the allocated memory.", + "kind": "problem", + "name": "Allocate sufficient memory for an object", + "precision": "medium", + "severity": "error", + "short_name": "InsufficientMemoryAllocatedForObject", + "tags": [ + "correctness", + "security" + ] + } + ], + "title": "Allocate sufficient memory for an object" + } + } +} \ No newline at end of file diff --git a/rules.csv b/rules.csv index ab5273f899..7032f05284 100644 --- a/rules.csv +++ b/rules.csv @@ -479,7 +479,7 @@ cpp,CERT-C++,STR50-CPP,Yes,Rule,,,Guarantee that storage for strings has suffici cpp,CERT-C++,STR51-CPP,Yes,Rule,,,Do not attempt to create a std::string from a null pointer,,Null,Hard, cpp,CERT-C++,STR52-CPP,Yes,Rule,,,"Use valid references, pointers, and iterators to reference elements of a basic_string",,Iterators,Hard, cpp,CERT-C++,STR53-CPP,Yes,Rule,,,Range check element access,,OutOfBounds,Hard, -c,CERT-C,ARR30-C,Yes,Rule,,,Do not form or use out-of-bounds pointers or array subscripts,,InvalidMemory2,Medium, +c,CERT-C,ARR30-C,Yes,Rule,,,Do not form or use out-of-bounds pointers or array subscripts,,OutOfBounds,Hard, c,CERT-C,ARR32-C,Yes,Rule,,,Ensure size arguments for variable length arrays are in a valid range,,InvalidMemory2,Medium, c,CERT-C,ARR36-C,Yes,Rule,,,Do not subtract or compare two pointers that do not refer to the same array,,Memory2,Medium, c,CERT-C,ARR37-C,Yes,Rule,,,Do not add or subtract an integer to a pointer to a non-array object,,InvalidMemory2,Medium,