From 6a0804068ad608dd9014093ff3e160b6452b5694 Mon Sep 17 00:00:00 2001 From: Kristen Newbury Date: Mon, 6 May 2024 14:12:43 -0400 Subject: [PATCH 1/2] STR32-C: reduce false negative related to realloc model --- ...lTerminatedToFunctionThatExpectsAString.ql | 103 ++++++++++++++---- ...natedToFunctionThatExpectsAString.expected | 38 ++++--- c/cert/test/rules/STR32-C/test.c | 38 ++++++- change_notes/2024-05-06-fix-fn-31-STR32C.md | 2 + .../cpp/PossiblyUnsafeStringOperation.qll | 21 ++-- 5 files changed, 156 insertions(+), 46 deletions(-) create mode 100644 change_notes/2024-05-06-fix-fn-31-STR32C.md diff --git a/c/cert/src/rules/STR32-C/NonNullTerminatedToFunctionThatExpectsAString.ql b/c/cert/src/rules/STR32-C/NonNullTerminatedToFunctionThatExpectsAString.ql index d661edade5..b5f246ca65 100644 --- a/c/cert/src/rules/STR32-C/NonNullTerminatedToFunctionThatExpectsAString.ql +++ b/c/cert/src/rules/STR32-C/NonNullTerminatedToFunctionThatExpectsAString.ql @@ -17,6 +17,7 @@ import codingstandards.c.cert import codingstandards.cpp.Naming import codingstandards.cpp.dataflow.TaintTracking import codingstandards.cpp.PossiblyUnsafeStringOperation +import semmle.code.cpp.valuenumbering.GlobalValueNumbering /** * Models a function that is part of the standard library that expects a @@ -43,32 +44,90 @@ class ExpectsNullTerminatedStringAsArgumentFunctionCall extends FunctionCall { Expr getAnExpectingExpr() { result = e } } -from ExpectsNullTerminatedStringAsArgumentFunctionCall fc, Expr e, Expr target -where - target = fc.getAnExpectingExpr() and - not isExcluded(fc, Strings1Package::nonNullTerminatedToFunctionThatExpectsAStringQuery()) and - ( - exists(PossiblyUnsafeStringOperation op | - // don't report violations of the same function call. - not op = fc and - e = op and - TaintTracking::localTaint(DataFlow::exprNode(op.getAnArgument()), DataFlow::exprNode(target)) +class PossiblyUnsafeStringOperationSource extends Source { + PossiblyUnsafeStringOperation op; + + PossiblyUnsafeStringOperationSource() { this.asExpr() = op.getAnArgument() } + + PossiblyUnsafeStringOperation getOp() { result = op } +} + +class CharArraySource extends Source { + CharArrayInitializedWithStringLiteral op; + + CharArraySource() { + op.getContainerLength() <= op.getStringLiteralLength() and + this.asExpr() = op + } +} + +abstract class Source extends DataFlow::Node { } + +class Sink extends DataFlow::Node { + Sink() { + exists(ExpectsNullTerminatedStringAsArgumentFunctionCall fc | + fc.getAnExpectingExpr() = this.asExpr() ) - or - exists(CharArrayInitializedWithStringLiteral op | - e = op and - op.getContainerLength() <= op.getStringLiteralLength() and - TaintTracking::localTaint(DataFlow::exprNode(op), DataFlow::exprNode(target)) + } +} + +module MyFlowConfiguration implements DataFlow::ConfigSig { + predicate isSink(DataFlow::Node sink) { + sink instanceof Sink and + //don't report violations of the same function call + not sink instanceof Source + } + + predicate isSource(DataFlow::Node source) { source instanceof Source } + + predicate isAdditionalFlowStep(DataFlow::Node innode, DataFlow::Node outnode) { + exists(FunctionCall realloc, ReallocFunction fn | + fn.getACallToThisFunction() = realloc and + realloc.getArgument(0) = innode.asExpr() and + realloc = outnode.asExpr() ) - ) and - // don't report cases flowing to this node where there is a flow from a - // literal assignment of a null terminator - not exists(AssignExpr aexp | + } +} + +class ReallocFunction extends AllocationFunction { + ReallocFunction() { exists(this.getReallocPtrArg()) } +} + +/** + * Determines if the string is acceptably null terminated + * The only condition we accept as a guarantee to null terminate is: + * `str[size_expr] = '\0';` + * where we do not check the value of the `size_expr` used + */ +predicate isGuarded(Expr guarded, Expr source) { + exists(AssignExpr aexp | aexp.getLValue() instanceof ArrayExpr and aexp.getRValue() instanceof Zero and - TaintTracking::localTaint(DataFlow::exprNode(aexp.getRValue()), DataFlow::exprNode(target)) and - // this must be AFTER the operation causing the non-null termination to be valid. - aexp.getAPredecessor*() = e + // this must be AFTER the operation causing the non-null termination + aexp.getAPredecessor+() = source and + //this guards anything after it + aexp.getASuccessor+() = guarded and + // no reallocs exist after this because they will be conservatively assumed to make the buffer smaller and remove the likliehood of this properly terminating + not exists(ReallocFunction realloc, FunctionCall fn | + fn = realloc.getACallToThisFunction() and + globalValueNumber(aexp.getLValue().(ArrayExpr).getArrayBase()) = + globalValueNumber(fn.getArgument(0)) and + aexp.getASuccessor+() = fn + ) ) +} + +module MyFlow = TaintTracking::Global; + +from + DataFlow::Node source, DataFlow::Node sink, ExpectsNullTerminatedStringAsArgumentFunctionCall fc, + Expr e +where + MyFlow::flow(source, sink) and + sink.asExpr() = fc.getAnExpectingExpr() and + not isGuarded(sink.asExpr(), source.asExpr()) and + if source instanceof PossiblyUnsafeStringOperationSource + then e = source.(PossiblyUnsafeStringOperationSource).getOp() + else e = source.asExpr() select fc, "String modified by $@ is passed to function expecting a null-terminated string.", e, "this expression" diff --git a/c/cert/test/rules/STR32-C/NonNullTerminatedToFunctionThatExpectsAString.expected b/c/cert/test/rules/STR32-C/NonNullTerminatedToFunctionThatExpectsAString.expected index 4099e3fb1a..8409d95628 100644 --- a/c/cert/test/rules/STR32-C/NonNullTerminatedToFunctionThatExpectsAString.expected +++ b/c/cert/test/rules/STR32-C/NonNullTerminatedToFunctionThatExpectsAString.expected @@ -1,16 +1,22 @@ -| test.c:19:3:19:8 | call to printf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:7:20:7:24 | Cod | this expression | -| test.c:20:3:20:8 | call to printf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:7:20:7:24 | Cod | this expression | -| test.c:22:3:22:8 | call to printf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:13:3:13:9 | call to strncpy | this expression | -| test.c:23:3:23:8 | call to printf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:13:3:13:9 | call to strncpy | this expression | -| test.c:24:3:24:8 | call to strlen | String modified by $@ is passed to function expecting a null-terminated string. | test.c:13:3:13:9 | call to strncpy | this expression | -| test.c:33:3:33:9 | call to wprintf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:30:24:30:29 | Cod | this expression | -| test.c:46:3:46:8 | call to printf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:41:3:41:10 | call to snprintf | this expression | -| test.c:47:3:47:8 | call to printf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:41:3:41:10 | call to snprintf | this expression | -| test.c:55:3:55:8 | call to printf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:53:3:53:9 | call to strncat | this expression | -| test.c:56:3:56:8 | call to printf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:53:3:53:9 | call to strncat | this expression | -| test.c:62:3:62:8 | call to printf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:60:20:60:24 | Cod | this expression | -| test.c:63:3:63:8 | call to printf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:60:20:60:24 | Cod | this expression | -| test.c:75:3:75:8 | call to printf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:72:20:72:24 | Cod | this expression | -| test.c:76:3:76:8 | call to printf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:72:20:72:24 | Cod | this expression | -| test.c:85:3:85:8 | call to printf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:83:3:83:9 | call to strncpy | this expression | -| test.c:86:3:86:8 | call to printf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:83:3:83:9 | call to strncpy | this expression | +| test.c:20:3:20:8 | call to printf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:8:20:8:24 | Cod | this expression | +| test.c:21:3:21:8 | call to printf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:8:20:8:24 | Cod | this expression | +| test.c:23:3:23:8 | call to printf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:14:3:14:9 | call to strncpy | this expression | +| test.c:24:3:24:8 | call to printf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:14:3:14:9 | call to strncpy | this expression | +| test.c:25:3:25:8 | call to strlen | String modified by $@ is passed to function expecting a null-terminated string. | test.c:14:3:14:9 | call to strncpy | this expression | +| test.c:34:3:34:9 | call to wprintf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:31:24:31:29 | Cod | this expression | +| test.c:47:3:47:8 | call to printf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:42:3:42:10 | call to snprintf | this expression | +| test.c:48:3:48:8 | call to printf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:42:3:42:10 | call to snprintf | this expression | +| test.c:56:3:56:8 | call to printf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:54:3:54:9 | call to strncat | this expression | +| test.c:57:3:57:8 | call to printf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:54:3:54:9 | call to strncat | this expression | +| test.c:63:3:63:8 | call to printf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:61:20:61:24 | Cod | this expression | +| test.c:64:3:64:8 | call to printf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:61:20:61:24 | Cod | this expression | +| test.c:76:3:76:8 | call to printf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:73:20:73:24 | Cod | this expression | +| test.c:77:3:77:8 | call to printf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:73:20:73:24 | Cod | this expression | +| test.c:86:3:86:8 | call to printf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:84:3:84:9 | call to strncpy | this expression | +| test.c:87:3:87:8 | call to printf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:84:3:84:9 | call to strncpy | this expression | +| test.c:95:3:95:8 | call to printf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:93:17:93:21 | Cod | this expression | +| test.c:95:3:95:8 | call to printf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:94:3:94:9 | call to strncpy | this expression | +| test.c:98:3:98:8 | call to printf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:93:17:93:21 | Cod | this expression | +| test.c:98:3:98:8 | call to printf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:94:3:94:9 | call to strncpy | this expression | +| test.c:122:3:122:8 | call to printf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:117:17:117:21 | Cod | this expression | +| test.c:122:3:122:8 | call to printf | String modified by $@ is passed to function expecting a null-terminated string. | test.c:118:3:118:9 | call to strncpy | this expression | diff --git a/c/cert/test/rules/STR32-C/test.c b/c/cert/test/rules/STR32-C/test.c index 288ef7e5e0..ea72abe201 100644 --- a/c/cert/test/rules/STR32-C/test.c +++ b/c/cert/test/rules/STR32-C/test.c @@ -1,4 +1,5 @@ #include +#include #include #include @@ -84,4 +85,39 @@ f5() { printf("%s", a1_nnt); // NON_COMPLIANT printf(a1_nnt); // NON_COMPLIANT -} \ No newline at end of file +} + +void test_fn_reported_in_31_simple() { + char *str; + str = (char *)malloc(3); + char a31[3] = "Cod"; // is NOT null terminated + strncpy(str, a31, 3); + printf(str); // NON_COMPLIANT + size_t cur_msg_size = 1024; + str = realloc(str, (cur_msg_size / 2 + 1) * sizeof(char)); + printf(str); // NON_COMPLIANT +} + +void test_fn_reported_in_31_simple_safe() { + char *str; + str = (char *)malloc(3); + char a31[3] = "Cod"; // is NOT null terminated + strncpy(str, a31, 3); + size_t cur_msg_size = 1024; + size_t temp_size = cur_msg_size / 2 + 1; + str = realloc(str, temp_size * sizeof(char)); + str[temp_size - 1] = L'\0'; // Properly null-terminate str + printf(str); // COMPLIANT +} + +void test_fn_reported_in_31_simple_relloc() { + char *str; + size_t cur_msg_size = 1024; + str = (char *)malloc(cur_msg_size); + char a31[3] = "Cod"; // is NOT null terminated + strncpy(str, a31, 3); + str[cur_msg_size - 1] = L'\0'; // Properly null-terminate str + size_t temp_size = cur_msg_size / 2 + 1; + str = realloc(str, temp_size * sizeof(char)); + printf(str); // NON_COMPLIANT +} diff --git a/change_notes/2024-05-06-fix-fn-31-STR32C.md b/change_notes/2024-05-06-fix-fn-31-STR32C.md new file mode 100644 index 0000000000..93dcca62c8 --- /dev/null +++ b/change_notes/2024-05-06-fix-fn-31-STR32C.md @@ -0,0 +1,2 @@ +- `STR32-C` - `NonNullTerminatedToFunctionThatExpectsAString.ql`: + - Fixes #31. Realloc and null termination were not modelled previously. \ No newline at end of file diff --git a/cpp/common/src/codingstandards/cpp/PossiblyUnsafeStringOperation.qll b/cpp/common/src/codingstandards/cpp/PossiblyUnsafeStringOperation.qll index ab454957e1..b790a4e02d 100644 --- a/cpp/common/src/codingstandards/cpp/PossiblyUnsafeStringOperation.qll +++ b/cpp/common/src/codingstandards/cpp/PossiblyUnsafeStringOperation.qll @@ -37,18 +37,25 @@ class PossiblyUnsafeStringOperation extends FunctionCall { bwc.getTarget() instanceof StrcatFunction or // Case 2: Consider the `strncpy(dest, src, n)` function. We do not - // consider `strcpy` since it is a banned function. The behavior of - // strncpy(dest, src, n) is that it will copy null terminators only if n - // > sizeof(src). If `src` is null-terminated then it will be null - // terminated if n >= sizeof(src). We take the conservative approach and - // use strictly greater. Thus this can be violated under the condition - // that n < strlen(src). Note that a buffer overflow is possible if + // consider `strcpy` since it is a banned function. + // We cannot know if the string is already null terminated or not and thus + // the conservative assumption is that it is not + // The behavior of strncpy(dest, src, n) is that if sizeof(src) < n + // then it will fill remainder of dst with ‘\0’ characters + // ie it is only in this case that it is guaranteed to null terminate + // Otherwise, dst is not terminated + // If `src` is already null-terminated then it will be null + // terminated if n >= sizeof(src). but we do not assume on this. + // Note that a buffer overflow is possible if // `n` is greater than sizeof(dest). The point of this query is not to // check for buffer overflows but we would certainly want to indicate // this would be a case where a string will not be null terminated. bwc.getTarget() instanceof StrcpyFunction and ( - (bwc.getExplicitLimit() / bwc.getCharSize()) < getBufferSize(src, _) or + // n <= sizeof(src) might not null terminate + (bwc.getExplicitLimit() / bwc.getCharSize()) <= getBufferSize(src, _) + or + // sizeof(dest) < n might not null terminate getBufferSize(dest, _) < (bwc.getExplicitLimit() / bwc.getCharSize()) ) or From 31d2f57cd81947c4311c217a031cacd859297659 Mon Sep 17 00:00:00 2001 From: Kristen Newbury Date: Mon, 6 May 2024 14:14:51 -0400 Subject: [PATCH 2/2] STR32-C: ammend change note --- change_notes/2024-05-06-fix-fn-31-STR32C.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/change_notes/2024-05-06-fix-fn-31-STR32C.md b/change_notes/2024-05-06-fix-fn-31-STR32C.md index 93dcca62c8..5abb1f2137 100644 --- a/change_notes/2024-05-06-fix-fn-31-STR32C.md +++ b/change_notes/2024-05-06-fix-fn-31-STR32C.md @@ -1,2 +1,2 @@ - `STR32-C` - `NonNullTerminatedToFunctionThatExpectsAString.ql`: - - Fixes #31. Realloc and null termination were not modelled previously. \ No newline at end of file + - Fixes #31. Realloc was not modelled previously. \ No newline at end of file