diff --git a/cpp/ql/src/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean.cpp b/cpp/ql/src/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean.cpp new file mode 100644 index 000000000000..3e7e4b797d76 --- /dev/null +++ b/cpp/ql/src/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean.cpp @@ -0,0 +1,4 @@ +if(strcpy(szbuf1, "Manager") == 0) // most likely strcmp was intended instead of strcpy +{ + // ... +} diff --git a/cpp/ql/src/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean.qhelp b/cpp/ql/src/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean.qhelp new file mode 100644 index 000000000000..ccf0fa55d088 --- /dev/null +++ b/cpp/ql/src/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean.qhelp @@ -0,0 +1,43 @@ + + + + +

This rule finds uses of the string copy function calls that return the destination parameter, +and that do not have a return value reserved to indicate an error.

+ +

The rule flags occurrences using such string copy functions as the conditional of an if statement, either directly, as part of an equality operator or a logical operator.

+ +

The string copy functions that the rule takes into consideration are:

+ + +

NOTE: It is highly recommended to consider using a more secure version of string manipulation functions suchas as strcpy_s.

+ + + +

Check to ensure that the flagged expressions are not typos.

+

If a string comparison is intended, change the function to the appropriate string comparison function.

+

If a string copy is really intended, very likely a secure version of the string copy function such as strcpy_s was intended instead of the insecure version of the string copy function.

+ + + + + + +
  • Microsoft Books on Line: C6324
    • +
  • Microsoft Books on Line: strcpy, wcscpy, _mbscpy
    • +
  • US-CERT: strncpy_s() and strncat_s()
    • + +     +     diff --git a/cpp/ql/src/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean.ql b/cpp/ql/src/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean.ql new file mode 100644 index 000000000000..55412b9729cf --- /dev/null +++ b/cpp/ql/src/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean.ql @@ -0,0 +1,77 @@ +/** + * @name Using the return value of a strcpy or related string copy function as a boolean operator + * @description The return value for strcpy, strncpy, or related string copy functions have no reserved return value to indicate an error. + * Using the return values of these functions as boolean function . + * Either the intent was to use a more secure version of a string copy function (such as strcpy_s), or a string compare function (such as strcmp). + * @kind problem + * @problem.severity error + * @precision high + * @id cpp/string-copy-return-value-as-boolean + * @tags external/microsoft/C6324 + */ + +import cpp +import semmle.code.cpp.dataflow.DataFlow + +predicate isStringComparisonFunction(string functionName) { + functionName = "strcpy" + or functionName = "wcscpy" + or functionName = "_mbscpy" + or functionName = "strncpy" + or functionName = "_strncpy_l" + or functionName = "wcsncpy" + or functionName = "_wcsncpy_l" + or functionName = "_mbsncpy" + or functionName = "_mbsncpy_l" +} + +predicate isBoolean( Expr e1 ) +{ + exists ( Type t1 | + t1 = e1.getType() and + (t1.hasName("bool") or t1.hasName("BOOL") or t1.hasName("_Bool")) + ) +} + +predicate isStringCopyCastedAsBoolean( FunctionCall func, Expr expr1, string msg ) { + DataFlow::localFlow(DataFlow::exprNode(func), DataFlow::exprNode(expr1)) + and isBoolean( expr1.getConversion*()) + and isStringComparisonFunction( func.getTarget().getQualifiedName()) + and msg = "Return Value of " + func.getTarget().getQualifiedName() + " used as boolean." +} + +predicate isStringCopyUsedInLogicalOperationOrCondition( FunctionCall func, Expr expr1, string msg ) { + isStringComparisonFunction( func.getTarget().getQualifiedName() ) + and ((( + // it is being used in an equality or logical operation + exists( EqualityOperation eop | + eop = expr1 + and func = eop.getAChild() + ) + or exists( UnaryLogicalOperation ule | + expr1 = ule + and func = ule.getAChild() + ) + or exists( BinaryLogicalOperation ble | + expr1 = ble + and func = ble.getAChild() + ) + ) + and msg = "Return Value of " + func.getTarget().getQualifiedName() + " used in a logical operation." + ) + or + exists( ConditionalStmt condstmt | + condstmt.getAChild() = expr1 | + // or the string copy function is used directly as the conditional expression + func = condstmt.getChild(0) + and msg = "Return Value of " + func.getTarget().getQualifiedName() + " used directly in a conditional expression." + )) +} + +from FunctionCall func, Expr expr1, string msg +where + ( isStringCopyCastedAsBoolean(func, expr1, msg) and + not isStringCopyUsedInLogicalOperationOrCondition(func, expr1, _) + ) + or isStringCopyUsedInLogicalOperationOrCondition(func, expr1, msg) +select expr1, msg diff --git a/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean/UsingStrcpyAsBoolean.expected b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean/UsingStrcpyAsBoolean.expected new file mode 100644 index 000000000000..3a56eaee4add --- /dev/null +++ b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean/UsingStrcpyAsBoolean.expected @@ -0,0 +1,34 @@ +| test.c:34:9:34:14 | call to strcpy | Return Value of strcpy used directly in a conditional expression. | +| test.c:38:9:38:31 | ! ... | Return Value of strcpy used in a logical operation. | +| test.c:42:9:42:35 | ... == ... | Return Value of strcpy used in a logical operation. | +| test.c:46:9:46:48 | ... && ... | Return Value of strcpy used in a logical operation. | +| test.c:50:9:50:15 | call to strncpy | Return Value of strncpy used directly in a conditional expression. | +| test.c:54:6:54:34 | ! ... | Return Value of strncpy used in a logical operation. | +| test.c:58:11:58:39 | ! ... | Return Value of strncpy used in a logical operation. | +| test.c:60:11:60:37 | ... && ... | Return Value of strcpy used in a logical operation. | +| test.c:62:11:62:37 | ... == ... | Return Value of strcpy used in a logical operation. | +| test.c:64:11:64:37 | ... != ... | Return Value of strcpy used in a logical operation. | +| test.cpp:75:9:75:14 | call to strcpy | Return Value of strcpy used directly in a conditional expression. | +| test.cpp:79:9:79:31 | ! ... | Return Value of strcpy used in a logical operation. | +| test.cpp:79:10:79:15 | call to strcpy | Return Value of strcpy used as boolean. | +| test.cpp:83:9:83:35 | ... == ... | Return Value of strcpy used in a logical operation. | +| test.cpp:87:9:87:48 | ... && ... | Return Value of strcpy used in a logical operation. | +| test.cpp:87:27:87:32 | call to strcpy | Return Value of strcpy used as boolean. | +| test.cpp:91:9:91:37 | call to wcscpy | Return Value of wcscpy used directly in a conditional expression. | +| test.cpp:95:9:95:14 | call to wcscpy | Return Value of wcscpy used directly in a conditional expression. | +| test.cpp:99:9:99:15 | call to _mbscpy | Return Value of _mbscpy used directly in a conditional expression. | +| test.cpp:103:9:103:15 | call to strncpy | Return Value of strncpy used directly in a conditional expression. | +| test.cpp:107:9:107:15 | call to wcsncpy | Return Value of wcsncpy used directly in a conditional expression. | +| test.cpp:111:9:111:16 | call to _mbsncpy | Return Value of _mbsncpy used directly in a conditional expression. | +| test.cpp:115:9:115:18 | call to _strncpy_l | Return Value of _strncpy_l used directly in a conditional expression. | +| test.cpp:119:9:119:18 | call to _wcsncpy_l | Return Value of _wcsncpy_l used directly in a conditional expression. | +| test.cpp:123:9:123:18 | call to _mbsncpy_l | Return Value of _mbsncpy_l used directly in a conditional expression. | +| test.cpp:127:6:127:34 | ! ... | Return Value of strncpy used in a logical operation. | +| test.cpp:127:7:127:13 | call to strncpy | Return Value of strncpy used as boolean. | +| test.cpp:131:11:131:17 | call to strncpy | Return Value of strncpy used as boolean. | +| test.cpp:133:16:133:44 | ! ... | Return Value of strncpy used in a logical operation. | +| test.cpp:133:17:133:23 | call to strncpy | Return Value of strncpy used as boolean. | +| test.cpp:135:11:135:16 | call to strcpy | Return Value of strcpy used as boolean. | +| test.cpp:135:11:135:37 | ... && ... | Return Value of strcpy used in a logical operation. | +| test.cpp:137:11:137:37 | ... == ... | Return Value of strcpy used in a logical operation. | +| test.cpp:139:11:139:37 | ... != ... | Return Value of strcpy used in a logical operation. | diff --git a/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean/UsingStrcpyAsBoolean.qlref b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean/UsingStrcpyAsBoolean.qlref new file mode 100644 index 000000000000..6ae254cc9747 --- /dev/null +++ b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean/UsingStrcpyAsBoolean.qlref @@ -0,0 +1 @@ +Likely Bugs/Likely Typos/UsingStrcpyAsBoolean.ql diff --git a/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean/test.c b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean/test.c new file mode 100644 index 000000000000..7e3753613f8a --- /dev/null +++ b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean/test.c @@ -0,0 +1,83 @@ +typedef unsigned int size_t; +typedef int* locale_t; + +char* strcpy(char* destination, const char* source) +{ + return destination; +} +char* strncpy(char* destination, const char* source, size_t count) +{ + return destination; +} + +int SomeFunction() +{ + return 1; +} + +int SomeFunctionThatTakesString(char* destination) +{ + return 1; +} + +int strcmp(char* destination, const char* source) +{ + return 1; +} + +void PositiveCases() +{ + char szbuf1[100]; + char szbuf2[100]; + int result; + + if (strcpy(szbuf1, "test")) // Bug, direct usage + { + } + + if (!strcpy(szbuf1, "test")) // Bug, unary binary operator + { + } + + if (strcpy(szbuf1, "test") == 0) // Bug, equality operator + { + } + + if (SomeFunction() && strcpy(szbuf1, "test")) // Bug, binary logical operator + { + } + + if (strncpy(szbuf1, "test", 100)) // Bug + { + } + + if (!strncpy(szbuf1, "test", 100)) // Bug + { + } + + result = !strncpy(szbuf1, "test", 100); + + result = strcpy(szbuf1, "test") && 1; + + result = strcpy(szbuf1, "test") == 0; + + result = strcpy(szbuf1, "test") != 0; +} + +void NegativeCases() +{ + char szbuf1[100]; + + if (SomeFunction()) + { + } + + if (SomeFunctionThatTakesString(strcpy(szbuf1, "test"))) + { + } + + if (strcmp(szbuf1, "test")) + { + } + +} \ No newline at end of file diff --git a/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean/test.cpp b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean/test.cpp new file mode 100644 index 000000000000..58aa67680582 --- /dev/null +++ b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean/test.cpp @@ -0,0 +1,163 @@ +typedef unsigned long size_t; +typedef int* locale_t; + +char* strcpy(char* destination, const char* source) +{ + return destination; +} +wchar_t* wcscpy(wchar_t* destination, const wchar_t* source) +{ + return destination; +} +unsigned char* _mbscpy(unsigned char* destination, const unsigned char* source) +{ + return destination; +} +char* strncpy(char* destination, const char* source, size_t count) +{ + return destination; +} +wchar_t* wcsncpy(wchar_t* destination, const wchar_t* source, size_t count) +{ + return destination; +} +unsigned char* _mbsncpy(unsigned char* destination, const unsigned char* source, size_t count) +{ + return destination; +} +char* _strncpy_l(char* destination, const char* source, size_t count, locale_t locale) +{ + return destination; +} +wchar_t* _wcsncpy_l(wchar_t* destination, const wchar_t* source, size_t count, locale_t locale) +{ + return destination; +} +unsigned char* _mbsncpy_l(unsigned char* destination, const unsigned char* source, size_t count, locale_t locale) +{ + return destination; +} + +int SomeFunction() +{ + return 1; +} + +int SomeFunctionThatTakesString(char* destination) +{ + return 1; +} + +int strcmp(char* destination, const char* source) +{ + return 1; +} + +int strcpy_s(char* destination, size_t dest_size, const char* source) +{ + return 1; +} + +#define WCSCPY_6324(x,y) wcscpy(x,y) + +void PositiveCases() +{ + char szbuf1[100]; + char szbuf2[100]; + wchar_t wscbuf1[100]; + wchar_t wscbuf2[100]; + unsigned char mbcbuf1[100]; + unsigned char mbcbuf2[100]; + + locale_t x; + *x = 0; + + if (strcpy(szbuf1, "test")) // Bug, direct usage + { + } + + if (!strcpy(szbuf1, "test")) // Bug, unary binary operator + { + } + + if (strcpy(szbuf1, "test") == 0) // Bug, equality operator + { + } + + if (SomeFunction() && strcpy(szbuf1, "test")) // Bug, binary logical operator + { + } + + if (WCSCPY_6324(wscbuf1, wscbuf2)) // Bug, using a macro + { + } + + if (wcscpy(wscbuf1, wscbuf2)) // Bug + { + } + + if (_mbscpy(mbcbuf1, mbcbuf2)) // Bug + { + } + + if (strncpy(szbuf1, "test", 100)) // Bug + { + } + + if (wcsncpy(wscbuf1, wscbuf2, 100)) // Bug + { + } + + if (_mbsncpy(mbcbuf1, (const unsigned char*)"test", 100)) // Bug + { + } + + if (_strncpy_l(szbuf1, "test", 100, x)) // Bug + { + } + + if (_wcsncpy_l(wscbuf1, wscbuf2, 100, x)) // Bug + { + } + + if (_mbsncpy_l(mbcbuf1, (const unsigned char*)"test", 100, x)) //Bug + { + } + + if (!strncpy(szbuf1, "test", 100)) // Bug + { + } + + bool b = strncpy(szbuf1, "test", 100); + + bool result = !strncpy(szbuf1, "test", 100); + + result = strcpy(szbuf1, "test") && 1; + + result = strcpy(szbuf1, "test") == 0; + + result = strcpy(szbuf1, "test") != 0; + +} + +void NegativeCases() +{ + char szbuf1[100]; + + if (SomeFunction()) + { + } + + if (SomeFunctionThatTakesString(strcpy(szbuf1, "test"))) + { + } + + if (strcmp(szbuf1, "test")) + { + } + + if (strcpy_s(szbuf1, 100, "test")) + { + } + +} \ No newline at end of file