diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-754/ImproperCheckReturnValueScanf.cpp b/cpp/ql/src/experimental/Security/CWE/CWE-754/ImproperCheckReturnValueScanf.cpp new file mode 100644 index 000000000000..4928db37a178 --- /dev/null +++ b/cpp/ql/src/experimental/Security/CWE/CWE-754/ImproperCheckReturnValueScanf.cpp @@ -0,0 +1,10 @@ +... + r = scanf("%i", &i); + if (r == 1) // GOOD + return i; + else + return -1; +... + scanf("%i", &i); // BAD + return i; +... diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-754/ImproperCheckReturnValueScanf.qhelp b/cpp/ql/src/experimental/Security/CWE/CWE-754/ImproperCheckReturnValueScanf.qhelp new file mode 100644 index 000000000000..ab40910f5d3f --- /dev/null +++ b/cpp/ql/src/experimental/Security/CWE/CWE-754/ImproperCheckReturnValueScanf.qhelp @@ -0,0 +1,27 @@ + + + +

The `scanf` family functions does not require the memory pointed to by its additional pointer arguments to be initialized before calling. The user is required to check the return value of `scanf` and similar functions to establish how many of the additional arguments were assigned values. Not checking the return value and reading one of the arguments not assigned a value is undefined behavior and may have unexpected consequences.

+
+ + +

+The user should check the return value of `scanf` and related functions and check that any additional argument was assigned a value before reading the additional argument. +

+
+ +

The first first example below is correct, as value of `i` is only read once it is checked that `scanf` has read one item. The second example is incorrect, as the return value of `scanf` is not checked, and as `scanf` might have failed to read any item before returning.

+ + +
+ + +
  • + CERT C Coding Standard: + EXP12-C. Do not ignore values returned by functions. +
  • + +
    +
    diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-754/ImproperCheckReturnValueScanf.ql b/cpp/ql/src/experimental/Security/CWE/CWE-754/ImproperCheckReturnValueScanf.ql new file mode 100644 index 000000000000..5f296752c1c4 --- /dev/null +++ b/cpp/ql/src/experimental/Security/CWE/CWE-754/ImproperCheckReturnValueScanf.ql @@ -0,0 +1,103 @@ +/** + * @name Improper check of return value of scanf + * @description Not checking the return value of scanf and related functions may lead to undefined behavior. + * @kind problem + * @id cpp/improper-check-return-value-scanf + * @problem.severity warning + * @precision medium + * @tags correctness + * security + * external/cwe/cwe-754 + * external/cwe/cwe-908 + */ + +import cpp +import semmle.code.cpp.commons.Exclusions + +/** Returns the position of the first argument being filled. */ +int posArgumentInFunctionCall(FunctionCall fc) { + ( + fc.getTarget().hasGlobalOrStdName(["scanf", "scanf_s"]) and + result = 1 + or + fc.getTarget().hasGlobalOrStdName(["fscanf", "sscanf", "fscanf_s", "sscanf_s"]) and + result = 2 + ) +} + +/** Holds if a function argument was not initialized but used after the call. */ +predicate argumentIsNotInitializedAndIsUsed(Variable vt, FunctionCall fc) { + // Fillable argument was not initialized. + vt instanceof LocalScopeVariable and + not vt.getAnAssignment().getASuccessor+() = fc and + ( + not vt.hasInitializer() + or + exists(Expr e, Variable v | + e = vt.getInitializer().getExpr() and + v = e.(AddressOfExpr).getOperand().(VariableAccess).getTarget() and + ( + not v.hasInitializer() and + not v.getAnAssignment().getASuccessor+() = fc + ) + ) + ) and + not exists(AssignExpr ae | + ae.getLValue() = vt.getAnAccess().getParent() and + ae.getASuccessor+() = fc + ) and + not exists(FunctionCall f0 | + f0.getAnArgument().getAChild() = vt.getAnAccess() and + f0.getASuccessor+() = fc + ) and + exists(Expr e0 | + // After the call, the completed arguments are assigned or returned as the result of the operation of the upper function. + fc.getASuccessor+() = e0 and + ( + ( + e0.(Assignment).getRValue().(VariableAccess).getTarget() = vt or + e0.(Assignment).getRValue().(ArrayExpr).getArrayBase().(VariableAccess).getTarget() = vt + ) + or + e0.getEnclosingStmt() instanceof ReturnStmt and + e0.(VariableAccess).getTarget() = vt + or + not exists(Expr e1 | + fc.getASuccessor+() = e1 and + e1.(VariableAccess).getTarget() = vt + ) + ) + ) +} + +from FunctionCall fc, int i +where + // Function return value is not evaluated. + fc instanceof ExprInVoidContext and + not isFromMacroDefinition(fc) and + i in [posArgumentInFunctionCall(fc) .. fc.getNumberOfArguments() - 1] and + ( + argumentIsNotInitializedAndIsUsed(fc.getArgument(i).(VariableAccess).getTarget(), fc) or + argumentIsNotInitializedAndIsUsed(fc.getArgument(i) + .(AddressOfExpr) + .getOperand() + .(VariableAccess) + .getTarget(), fc) or + argumentIsNotInitializedAndIsUsed(fc.getArgument(i) + .(ArrayExpr) + .getArrayBase() + .(VariableAccess) + .getTarget(), fc) + ) and + // After the call, filled arguments are not evaluated. + not exists(Expr e0, int i1 | + i1 in [posArgumentInFunctionCall(fc) .. fc.getNumberOfArguments() - 1] and + fc.getASuccessor+() = e0 and + e0.getEnclosingElement() instanceof ComparisonOperation and + ( + e0.(VariableAccess).getTarget() = fc.getArgument(i1).(VariableAccess).getTarget() or + e0.(VariableAccess).getTarget() = + fc.getArgument(i1).(AddressOfExpr).getOperand().(VariableAccess).getTarget() + ) + ) +select fc, "Unchecked return value for call to '" + fc.getTarget().getName() + "'." diff --git a/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-754/semmle/tests/ImproperCheckReturnValueScanf.expected b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-754/semmle/tests/ImproperCheckReturnValueScanf.expected new file mode 100644 index 000000000000..85a929ed39c9 --- /dev/null +++ b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-754/semmle/tests/ImproperCheckReturnValueScanf.expected @@ -0,0 +1,8 @@ +| test.cpp:52:3:52:7 | call to scanf | Unchecked return value for call to 'scanf'. | +| test.cpp:53:3:53:7 | call to scanf | Unchecked return value for call to 'scanf'. | +| test.cpp:54:3:54:7 | call to scanf | Unchecked return value for call to 'scanf'. | +| test.cpp:105:3:105:7 | call to scanf | Unchecked return value for call to 'scanf'. | +| test.cpp:106:3:106:7 | call to scanf | Unchecked return value for call to 'scanf'. | +| test.cpp:107:3:107:7 | call to scanf | Unchecked return value for call to 'scanf'. | +| test.cpp:115:3:115:7 | call to scanf | Unchecked return value for call to 'scanf'. | +| test.cpp:120:3:120:7 | call to scanf | Unchecked return value for call to 'scanf'. | diff --git a/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-754/semmle/tests/ImproperCheckReturnValueScanf.qlref b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-754/semmle/tests/ImproperCheckReturnValueScanf.qlref new file mode 100644 index 000000000000..f0cb9dd57c1e --- /dev/null +++ b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-754/semmle/tests/ImproperCheckReturnValueScanf.qlref @@ -0,0 +1 @@ +experimental/Security/CWE/CWE-754/ImproperCheckReturnValueScanf.ql diff --git a/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-754/semmle/tests/test.cpp b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-754/semmle/tests/test.cpp new file mode 100644 index 000000000000..b9608b757b9c --- /dev/null +++ b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-754/semmle/tests/test.cpp @@ -0,0 +1,129 @@ +int scanf(const char *format, ...); +int sscanf(const char *str, const char *format, ...); +int globalVal; +char * globalVala; +int * globalValp; +char globalVala2; +int functionWork1(int retIndex) { + int i; + char a[10]; + int b; + int *p = &b; + if (scanf("%i", &i) != 1) // GOOD + return -1; + if (scanf("%s", a) != 1) // GOOD + return -1; + if (scanf("%i", p) != 1) // GOOD + return -1; + if(retIndex == 0) + return (int)*a; + if(retIndex == 1) + return *p; + return i; +} + +int functionWork1_(int retIndex) { + int i; + char a[10]; + int b; + int *p = &b; + int r; + r = scanf("%i", &i); + if (r != 1) // GOOD + return -1; + r = scanf("%s", a); + if (r == 1) // GOOD + return -1; + r = scanf("%i", p); + if (r != 1) // GOOD + return -1; + if(retIndex == 0) + return (int)*a; + if(retIndex == 1) + return *p; + return i; +} + +int functionWork1b(int retIndex) { + int i; + char a[10]; + int b; + int *p = &b; + scanf("%i", &i); // BAD + scanf("%s", a); // BAD + scanf("%i", p); // BAD + if(retIndex == 0) + return (int)*a; + if(retIndex == 1) + return *p; + return i; +} +int functionWork1_() { + int i; + scanf("%i",&i); // BAD [NOT DETECTED] + if(i<10) + return -1; + return i; +} +int functionWork2(int retIndex) { + int i = 0; + char a[10] = ""; + int b = 1; + int *p = &b; + scanf("%i", &i); // GOOD:Argument initialized even when scanf fails. + scanf("%s", a); // GOOD:Argument initialized even when scanf fails. + scanf("%i", p); // GOOD:Argument initialized even when scanf fails. + if(retIndex == 0) + return (int)*a; + if(retIndex == 1) + return *p; + return i; +} + +int functionWork2_(int retIndex) { + int i; + i = 0; + char a[10]; + a[0] = '\0'; + int b; + b=1; + int *p = &b; + scanf("%i", &i); // GOOD:Argument initialized even when scanf fails. + scanf("%s", a); // GOOD:Argument initialized even when scanf fails. + scanf("%i", p); // GOOD:Argument initialized even when scanf fails. + if(retIndex == 0) + return (int)*a; + if(retIndex == 1) + return *p; + return i; +} +int functionWork2b() { + int i; + char a[10]; + int b; + int *p = &b; + scanf("%i", &i); // BAD + scanf("%s", a); // BAD + scanf("%i", p); // BAD + globalVal = i; + globalVala = a; + globalValp = p; + return 0; +} +int functionWork2b_() { + char a[10]; + scanf("%s", a); // BAD + globalVala2 = a[0]; + return 0; +} +int functionWork3b(int * i) { + scanf("%i", i); // BAD + return 0; +} +int functionWork3() { + char number[] = "42"; + int d; + sscanf(number, "%d", &d); // GOOD: sscanf always succeeds + if (d < 16) + return -1; +}