diff --git a/change-notes/1.26/analysis-cpp.md b/change-notes/1.26/analysis-cpp.md index 6b31cf76050..b86068d905e 100644 --- a/change-notes/1.26/analysis-cpp.md +++ b/change-notes/1.26/analysis-cpp.md @@ -26,5 +26,6 @@ The following changes in version 1.26 affect C/C++ analysis in all applications. * The models library now models many taint flows through `std::istream` and `std::ostream`. * The models library now models some taint flows through `std::shared_ptr`, `std::unique_ptr`, `std::make_shared` and `std::make_unique`. * The models library now models many taint flows through `std::pair`, `std::map`, `std::unordered_map`, `std::set` and `std::unordered_set`. +* The models library now models `bcopy`. * The `SimpleRangeAnalysis` library now supports multiplications of the form `e1 * e2` and `x *= e2` when `e1` and `e2` are unsigned or constant. diff --git a/cpp/ql/src/semmle/code/cpp/controlflow/Dereferenced.qll b/cpp/ql/src/semmle/code/cpp/controlflow/Dereferenced.qll index a667c39943f..a8f14b89159 100644 --- a/cpp/ql/src/semmle/code/cpp/controlflow/Dereferenced.qll +++ b/cpp/ql/src/semmle/code/cpp/controlflow/Dereferenced.qll @@ -4,47 +4,25 @@ import cpp import Nullness +import semmle.code.cpp.models.interfaces.ArrayFunction /** * Holds if the call `fc` will dereference argument `i`. */ predicate callDereferences(FunctionCall fc, int i) { - exists(string name | - fc.getTarget().hasGlobalOrStdName(name) and + exists(ArrayFunction af | + fc.getTarget() = af and ( - name = "bcopy" and i in [0 .. 1] - or - name = "memcpy" and i in [0 .. 1] - or - name = "memmove" and i in [0 .. 1] - or - name = "strcpy" and i in [0 .. 1] - or - name = "strncpy" and i in [0 .. 1] - or - name = "strdup" and i = 0 - or - name = "strndup" and i = 0 - or - name = "strlen" and i = 0 - or - name = "printf" and fc.getArgument(i).getType() instanceof PointerType - or - name = "fprintf" and fc.getArgument(i).getType() instanceof PointerType - or - name = "sprintf" and fc.getArgument(i).getType() instanceof PointerType - or - name = "snprintf" and fc.getArgument(i).getType() instanceof PointerType - or - name = "vprintf" and fc.getArgument(i).getType() instanceof PointerType - or - name = "vfprintf" and fc.getArgument(i).getType() instanceof PointerType - or - name = "vsprintf" and fc.getArgument(i).getType() instanceof PointerType - or - name = "vsnprintf" and fc.getArgument(i).getType() instanceof PointerType + af.hasArrayInput(i) or + af.hasArrayOutput(i) ) ) + or + exists(FormattingFunction ff | + fc.getTarget() = ff and + i >= ff.getFirstFormatArgumentIndex() and + fc.getArgument(i).getType() instanceof PointerType + ) } /** diff --git a/cpp/ql/src/semmle/code/cpp/models/implementations/Memcpy.qll b/cpp/ql/src/semmle/code/cpp/models/implementations/Memcpy.qll index ef4aa8b7290..b6dac9e2d0b 100644 --- a/cpp/ql/src/semmle/code/cpp/models/implementations/Memcpy.qll +++ b/cpp/ql/src/semmle/code/cpp/models/implementations/Memcpy.qll @@ -10,45 +10,57 @@ import semmle.code.cpp.models.interfaces.SideEffect import semmle.code.cpp.models.interfaces.Taint /** - * The standard functions `memcpy` and `memmove`, and the gcc variant - * `__builtin___memcpy_chk` + * The standard functions `memcpy`, `memmove` and `bcopy`; and the gcc variant + * `__builtin___memcpy_chk`. */ -class MemcpyFunction extends ArrayFunction, DataFlowFunction, SideEffectFunction, TaintFunction { +class MemcpyFunction extends ArrayFunction, DataFlowFunction, SideEffectFunction { MemcpyFunction() { - this.hasName("memcpy") or - this.hasName("memmove") or - this.hasName("__builtin___memcpy_chk") + // memcpy(dest, src, num) + // memmove(dest, src, num) + // memmove(dest, src, num, remaining) + this.hasName(["memcpy", "memmove", "__builtin___memcpy_chk"]) + or + // bcopy(src, dest, num) + this.hasGlobalOrStdName("bcopy") } - override predicate hasArrayInput(int bufParam) { bufParam = 1 } + /** + * Gets the index of the parameter that is the source buffer for the copy. + */ + int getParamSrc() { if this.hasGlobalOrStdName("bcopy") then result = 0 else result = 1 } - override predicate hasArrayOutput(int bufParam) { bufParam = 0 } + /** + * Gets the index of the parameter that is the destination buffer for the + * copy. + */ + int getParamDest() { if this.hasGlobalOrStdName("bcopy") then result = 1 else result = 0 } + + /** + * Gets the index of the parameter that is the size of the copy (in bytes). + */ + int getParamSize() { result = 2 } + + override predicate hasArrayInput(int bufParam) { bufParam = getParamSrc() } + + override predicate hasArrayOutput(int bufParam) { bufParam = getParamDest() } override predicate hasDataFlow(FunctionInput input, FunctionOutput output) { - input.isParameterDeref(1) and - output.isParameterDeref(0) + input.isParameterDeref(getParamSrc()) and + output.isParameterDeref(getParamDest()) or - input.isParameterDeref(1) and + input.isParameterDeref(getParamSrc()) and output.isReturnValueDeref() or - input.isParameter(0) and + input.isParameter(getParamDest()) and output.isReturnValue() } - override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) { - input.isParameter(2) and - output.isParameterDeref(0) - or - input.isParameter(2) and - output.isReturnValueDeref() - } - override predicate hasArrayWithVariableSize(int bufParam, int countParam) { ( - bufParam = 0 or - bufParam = 1 + bufParam = getParamDest() or + bufParam = getParamSrc() ) and - countParam = 2 + countParam = getParamSize() } override predicate hasOnlySpecificReadSideEffects() { any() } @@ -56,18 +68,18 @@ class MemcpyFunction extends ArrayFunction, DataFlowFunction, SideEffectFunction override predicate hasOnlySpecificWriteSideEffects() { any() } override predicate hasSpecificWriteSideEffect(ParameterIndex i, boolean buffer, boolean mustWrite) { - i = 0 and buffer = true and mustWrite = true + i = getParamDest() and buffer = true and mustWrite = true } override predicate hasSpecificReadSideEffect(ParameterIndex i, boolean buffer) { - i = 1 and buffer = true + i = getParamSrc() and buffer = true } override ParameterIndex getParameterSizeIndex(ParameterIndex i) { - result = 2 and + result = getParamSize() and ( - i = 0 or - i = 1 + i = getParamDest() or + i = getParamSrc() ) } } diff --git a/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected b/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected index 518f202fbe7..9f3e4103005 100644 --- a/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected +++ b/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected @@ -5507,8 +5507,6 @@ | taint.cpp:194:10:194:10 | x | taint.cpp:194:9:194:10 | & ... | | | taint.cpp:194:13:194:18 | source | taint.cpp:194:2:194:7 | call to memcpy | TAINT | | taint.cpp:194:13:194:18 | source | taint.cpp:194:9:194:10 | ref arg & ... | TAINT | -| taint.cpp:194:21:194:31 | sizeof(int) | taint.cpp:194:2:194:7 | call to memcpy | TAINT | -| taint.cpp:194:21:194:31 | sizeof(int) | taint.cpp:194:9:194:10 | ref arg & ... | TAINT | | taint.cpp:207:6:207:11 | call to source | taint.cpp:207:2:207:13 | ... = ... | | | taint.cpp:207:6:207:11 | call to source | taint.cpp:210:7:210:7 | x | | | taint.cpp:207:6:207:11 | call to source | taint.cpp:213:12:213:12 | x | | diff --git a/cpp/ql/test/query-tests/Critical/MissingNullTest/MissingNullTest.expected b/cpp/ql/test/query-tests/Critical/MissingNullTest/MissingNullTest.expected new file mode 100644 index 00000000000..ba6f90fb548 --- /dev/null +++ b/cpp/ql/test/query-tests/Critical/MissingNullTest/MissingNullTest.expected @@ -0,0 +1,6 @@ +| test.cpp:23:8:23:8 | p | Value may be null; it should be checked before dereferencing. | +| test.cpp:35:10:35:10 | q | Value may be null; it should be checked before dereferencing. | +| test.cpp:43:13:43:13 | q | Value may be null; it should be checked before dereferencing. | +| test.cpp:51:17:51:17 | q | Value may be null; it should be checked before dereferencing. | +| test.cpp:58:8:58:8 | p | Value may be null; it should be checked before dereferencing. | +| test.cpp:67:8:67:8 | p | Value may be null; it should be checked before dereferencing. | diff --git a/cpp/ql/test/query-tests/Critical/MissingNullTest/MissingNullTest.qlref b/cpp/ql/test/query-tests/Critical/MissingNullTest/MissingNullTest.qlref new file mode 100644 index 00000000000..f4e1c9888cb --- /dev/null +++ b/cpp/ql/test/query-tests/Critical/MissingNullTest/MissingNullTest.qlref @@ -0,0 +1 @@ +Critical/MissingNullTest.ql \ No newline at end of file diff --git a/cpp/ql/test/query-tests/Critical/MissingNullTest/test.cpp b/cpp/ql/test/query-tests/Critical/MissingNullTest/test.cpp new file mode 100644 index 00000000000..73ebe8b56fe --- /dev/null +++ b/cpp/ql/test/query-tests/Critical/MissingNullTest/test.cpp @@ -0,0 +1,71 @@ + +#define NULL (0) + +typedef unsigned long size_t; + +void *memcpy(void *s1, const void *s2, size_t n); +void bcopy(const void *source, void *dest, size_t amount); + +void mycopyint(const int *source, int *dest) +{ + *dest = *source; +} + +void test1(bool cond) +{ + int x, y; + + { + int *p, *q; + + y = *p; // BAD (p is uninitialized and could be 0) [NOT DETECTED] + p = NULL; + y = *p; // BAD (p is 0) + p = &x; + y = *p; // GOOD (p points to x) + p = q; + y = *p; // BAD (p is uninitialized and could be 0) [NOT DETECTED] + } + + { + int *p = &x; + int *q = 0; + + memcpy(p, &y, sizeof(int)); // GOOD (p points to x) + memcpy(q, &y, sizeof(int)); // BAD (p is 0) + } + + { + int *p = &x; + int *q = 0; + + bcopy(&y, p, sizeof(int)); // GOOD (p points to x) + bcopy(&y, q, sizeof(int)); // BAD (p is 0) + } + + { + int *p = &x; + int *q = 0; + + mycopyint(&y, p); // GOOD (p points to x) + mycopyint(&y, q); // BAD (p is 0) + } + + { + int *p = 0; + int *q = &x; + + y = *p; // BAD (p is 0) + memcpy(&p, &q, sizeof(p)); + y = *p; // GOOD (p points to x) + } + + { + int *p = 0; + int *q = &x; + + y = *p; // BAD (p is 0) + bcopy(&q, &p, sizeof(p)); + y = *p; // GOOD (p points to x) + } +}