mirror of
https://github.com/github/codeql.git
synced 2026-05-03 12:45:27 +02:00
Fixing the code based on PR feedback.
This commit is contained in:
Raul Garcia
@@ -9,18 +9,19 @@ and that do not have a return value reserved to indicate an error.</p>
|
||||
|
||||
<p>The rule flags occurrences using such string copy functions as the conditional of an <code>if</code> statement, either directly, as part of an equality operator or a logical operator.</p>
|
||||
|
||||
<p>The string copy functions that the rule takes into consideration are:
|
||||
<li>strcpy</li>
|
||||
<li>wcscpy</li>
|
||||
<li>_mbscpy</li>
|
||||
<li>strncpy</li>
|
||||
<li>_strncpy_l</li>
|
||||
<li>wcsncpy</li>
|
||||
<li>_wcsncpy_l</li>
|
||||
<li>_mbsncpy</li>
|
||||
<li>_mbsncpy_l</li>
|
||||
</p>
|
||||
|
||||
<p>The string copy functions that the rule takes into consideration are: </p>
|
||||
<ul>
|
||||
<li>strcpy</li>
|
||||
<li>wcscpy</li>
|
||||
<li>_mbscpy</li>
|
||||
<li>strncpy</li>
|
||||
<li>_strncpy_l</li>
|
||||
<li>wcsncpy</li>
|
||||
<li>_wcsncpy_l</li>
|
||||
<li>_mbsncpy</li>
|
||||
<li>_mbsncpy_l</li>
|
||||
</ul>
|
||||
|
||||
<p>NOTE: It is highly recommended to consider using a more secure version of string manipulation functions suchas as <code>strcpy_s</code>.</p>
|
||||
|
||||
</overview>
|
||||
@@ -30,7 +31,7 @@ and that do not have a return value reserved to indicate an error.</p>
|
||||
<p>If a string copy is really intended, very likely a secure version of the string copy function such as <code>strcpy_s</code> was intended instead of the insecure version of the string copy function.</p>
|
||||
|
||||
</recommendation>
|
||||
<example><sample src="UsingStrcpyInConditional.cpp" />
|
||||
<example><sample src="UsingStrcpyAsBoolean.cpp" />
|
||||
</example>
|
||||
|
||||
<references>
|
||||
92
cpp/ql/src/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean.ql
Normal file
92
cpp/ql/src/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean.ql
Normal file
@@ -0,0 +1,92 @@
|
||||
/**
|
||||
* @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"))
|
||||
)
|
||||
}
|
||||
|
||||
class StringCopyToBooleanConfiguration extends DataFlow::Configuration {
|
||||
StringCopyToBooleanConfiguration() {
|
||||
this = "StringCopyToBooleanConfiguration"
|
||||
}
|
||||
|
||||
override predicate isSource(DataFlow::Node source) {
|
||||
exists( FunctionCall func |
|
||||
func = source.asExpr()
|
||||
and isStringComparisonFunction( func.getTarget().getQualifiedName())
|
||||
)
|
||||
}
|
||||
|
||||
override predicate isSink(DataFlow::Node sink) {
|
||||
exists( Expr expr1 |
|
||||
expr1 = sink.asExpr()
|
||||
and isBoolean( expr1.getConversion*())
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
predicate isStringCopyCastedAsBoolean( FunctionCall func, Expr expr1, string msg ) {
|
||||
exists( StringCopyToBooleanConfiguration modeConfig |
|
||||
modeConfig.hasFlow(DataFlow::exprNode(func), DataFlow::exprNode(expr1))
|
||||
and msg = "Return Value of " + func.getTarget().getQualifiedName() + " used as boolean."
|
||||
)
|
||||
}
|
||||
|
||||
predicate isStringCopyUsedInCondition( FunctionCall func, Expr expr1, string msg ) {
|
||||
exists( ConditionalStmt condstmt |
|
||||
condstmt.getAChild() = expr1 |
|
||||
isStringComparisonFunction( func.getTarget().getQualifiedName() )
|
||||
and (
|
||||
// The string copy function is used directly as the conditional expression
|
||||
func = condstmt.getChild(0)
|
||||
// ... or it is being used in an equality or logical operation
|
||||
or 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 conditional."
|
||||
)
|
||||
}
|
||||
|
||||
from FunctionCall func, Expr expr1, string msg
|
||||
where
|
||||
isStringCopyCastedAsBoolean(func, expr1, msg)
|
||||
or isStringCopyUsedInCondition(func, expr1, msg)
|
||||
select expr1, msg
|
||||
@@ -1,45 +0,0 @@
|
||||
/**
|
||||
* @name Using the return value of a strcpy or related string copy function in an if statement
|
||||
* @description The return value for strcpy or related string copy functions have no reserved return value to indicate an error.
|
||||
* Using these functions as part of an if statement condition indicates a logic error.
|
||||
* 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-function-in-if-condition
|
||||
* @tags external/microsoft/C6324
|
||||
*/
|
||||
|
||||
import cpp
|
||||
|
||||
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"
|
||||
}
|
||||
|
||||
from IfStmt ifs,
|
||||
FunctionCall func
|
||||
where isStringComparisonFunction( func.getTarget().getQualifiedName() )
|
||||
and ( func = ifs.getCondition()
|
||||
or exists( UnaryLogicalOperation ule |
|
||||
ule = ifs.getCondition()
|
||||
and func = ule.getAChild()
|
||||
)
|
||||
or exists( BinaryLogicalOperation ble |
|
||||
ble = ifs.getCondition()
|
||||
and func = ble.getAChild()
|
||||
)
|
||||
or exists( EqualityOperation eop |
|
||||
eop = ifs.getCondition()
|
||||
and func = eop.getAChild()
|
||||
)
|
||||
)
|
||||
select func, "Incorrect use of function " + func.getTarget().getQualifiedName() + ". Verify the logic and replace with a secure string copy function, or a string comparison function."
|
||||
@@ -0,0 +1,34 @@
|
||||
| test.c:33:9:33:14 | call to strcpy | Return Value of strcpy used in a conditional. |
|
||||
| test.c:37:9:37:31 | ! ... | Return Value of strcpy used in a conditional. |
|
||||
| test.c:41:9:41:35 | ... == ... | Return Value of strcpy used in a conditional. |
|
||||
| test.c:45:9:45:48 | ... && ... | Return Value of strcpy used in a conditional. |
|
||||
| test.c:49:9:49:15 | call to strncpy | Return Value of strncpy used in a conditional. |
|
||||
| test.c:53:6:53:34 | ! ... | Return Value of strncpy used in a conditional. |
|
||||
| test.cpp:75:9:75:14 | call to strcpy | Return Value of strcpy used as boolean. |
|
||||
| test.cpp:75:9:75:14 | call to strcpy | Return Value of strcpy used in a conditional. |
|
||||
| test.cpp:79:9:79:31 | ! ... | Return Value of strcpy used in a conditional. |
|
||||
| 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 conditional. |
|
||||
| test.cpp:87:9:87:48 | ... && ... | Return Value of strcpy used in a conditional. |
|
||||
| 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 as boolean. |
|
||||
| test.cpp:91:9:91:37 | call to wcscpy | Return Value of wcscpy used in a conditional. |
|
||||
| test.cpp:95:9:95:14 | call to wcscpy | Return Value of wcscpy used as boolean. |
|
||||
| test.cpp:95:9:95:14 | call to wcscpy | Return Value of wcscpy used in a conditional. |
|
||||
| test.cpp:99:9:99:15 | call to _mbscpy | Return Value of _mbscpy used as boolean. |
|
||||
| test.cpp:99:9:99:15 | call to _mbscpy | Return Value of _mbscpy used in a conditional. |
|
||||
| test.cpp:103:9:103:15 | call to strncpy | Return Value of strncpy used as boolean. |
|
||||
| test.cpp:103:9:103:15 | call to strncpy | Return Value of strncpy used in a conditional. |
|
||||
| test.cpp:107:9:107:15 | call to wcsncpy | Return Value of wcsncpy used as boolean. |
|
||||
| test.cpp:107:9:107:15 | call to wcsncpy | Return Value of wcsncpy used in a conditional. |
|
||||
| test.cpp:111:9:111:16 | call to _mbsncpy | Return Value of _mbsncpy used as boolean. |
|
||||
| test.cpp:111:9:111:16 | call to _mbsncpy | Return Value of _mbsncpy used in a conditional. |
|
||||
| test.cpp:115:9:115:18 | call to _strncpy_l | Return Value of _strncpy_l used as boolean. |
|
||||
| test.cpp:115:9:115:18 | call to _strncpy_l | Return Value of _strncpy_l used in a conditional. |
|
||||
| test.cpp:119:9:119:18 | call to _wcsncpy_l | Return Value of _wcsncpy_l used as boolean. |
|
||||
| test.cpp:119:9:119:18 | call to _wcsncpy_l | Return Value of _wcsncpy_l used in a conditional. |
|
||||
| test.cpp:123:9:123:18 | call to _mbsncpy_l | Return Value of _mbsncpy_l used as boolean. |
|
||||
| test.cpp:123:9:123:18 | call to _mbsncpy_l | Return Value of _mbsncpy_l used in a conditional. |
|
||||
| test.cpp:127:6:127:34 | ! ... | Return Value of strncpy used in a conditional. |
|
||||
| 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. |
|
||||
@@ -0,0 +1 @@
|
||||
Likely Bugs/Likely Typos/UsingStrcpyAsBoolean.ql
|
||||
@@ -49,6 +49,10 @@ void PositiveCases()
|
||||
if (strncpy(szbuf1, "test", 100)) // Bug
|
||||
{
|
||||
}
|
||||
|
||||
if (!strncpy(szbuf1, "test", 100)) // Bug
|
||||
{
|
||||
}
|
||||
}
|
||||
|
||||
void NegativeCases()
|
||||
@@ -124,6 +124,11 @@ void PositiveCases()
|
||||
{
|
||||
}
|
||||
|
||||
if (!strncpy(szbuf1, "test", 100)) // Bug
|
||||
{
|
||||
}
|
||||
|
||||
bool b = strncpy(szbuf1, "test", 100);
|
||||
}
|
||||
|
||||
void NegativeCases()
|
||||
@@ -1,18 +0,0 @@
|
||||
| test.c:33:9:33:14 | call to strcpy | Incorrect use of function strcpy. Verify the logic and replace with a secure string copy function, or a string comparison function. |
|
||||
| test.c:37:10:37:15 | call to strcpy | Incorrect use of function strcpy. Verify the logic and replace with a secure string copy function, or a string comparison function. |
|
||||
| test.c:41:9:41:14 | call to strcpy | Incorrect use of function strcpy. Verify the logic and replace with a secure string copy function, or a string comparison function. |
|
||||
| test.c:45:27:45:32 | call to strcpy | Incorrect use of function strcpy. Verify the logic and replace with a secure string copy function, or a string comparison function. |
|
||||
| test.c:49:9:49:15 | call to strncpy | Incorrect use of function strncpy. Verify the logic and replace with a secure string copy function, or a string comparison function. |
|
||||
| test.cpp:75:9:75:14 | call to strcpy | Incorrect use of function strcpy. Verify the logic and replace with a secure string copy function, or a string comparison function. |
|
||||
| test.cpp:79:10:79:15 | call to strcpy | Incorrect use of function strcpy. Verify the logic and replace with a secure string copy function, or a string comparison function. |
|
||||
| test.cpp:83:9:83:14 | call to strcpy | Incorrect use of function strcpy. Verify the logic and replace with a secure string copy function, or a string comparison function. |
|
||||
| test.cpp:87:27:87:32 | call to strcpy | Incorrect use of function strcpy. Verify the logic and replace with a secure string copy function, or a string comparison function. |
|
||||
| test.cpp:91:9:91:37 | call to wcscpy | Incorrect use of function wcscpy. Verify the logic and replace with a secure string copy function, or a string comparison function. |
|
||||
| test.cpp:95:9:95:14 | call to wcscpy | Incorrect use of function wcscpy. Verify the logic and replace with a secure string copy function, or a string comparison function. |
|
||||
| test.cpp:99:9:99:15 | call to _mbscpy | Incorrect use of function _mbscpy. Verify the logic and replace with a secure string copy function, or a string comparison function. |
|
||||
| test.cpp:103:9:103:15 | call to strncpy | Incorrect use of function strncpy. Verify the logic and replace with a secure string copy function, or a string comparison function. |
|
||||
| test.cpp:107:9:107:15 | call to wcsncpy | Incorrect use of function wcsncpy. Verify the logic and replace with a secure string copy function, or a string comparison function. |
|
||||
| test.cpp:111:9:111:16 | call to _mbsncpy | Incorrect use of function _mbsncpy. Verify the logic and replace with a secure string copy function, or a string comparison function. |
|
||||
| test.cpp:115:9:115:18 | call to _strncpy_l | Incorrect use of function _strncpy_l. Verify the logic and replace with a secure string copy function, or a string comparison function. |
|
||||
| test.cpp:119:9:119:18 | call to _wcsncpy_l | Incorrect use of function _wcsncpy_l. Verify the logic and replace with a secure string copy function, or a string comparison function. |
|
||||
| test.cpp:123:9:123:18 | call to _mbsncpy_l | Incorrect use of function _mbsncpy_l. Verify the logic and replace with a secure string copy function, or a string comparison function. |
|
||||
@@ -1 +0,0 @@
|
||||
Likely Bugs/Likely Typos/UsingStrcpyInConditional.ql
|
||||
Reference in New Issue
Block a user