hohn/codeql
1
0
Fork 0
mirror of https://github.com/github/codeql.git synced 2026-05-02 04:05:14 +02:00
Code Issues Packages Projects Releases Wiki Activity

Corrections related to the review comments.

Browse Source
This commit is contained in:
Denis Levin
2019-06-13 13:04:42 -07:00
parent 1b8117ba3a
commit ad489db815
13 changed files with 142 additions and 146 deletions
Download Patch File Download Diff File Expand all files Collapse all files

3
cpp/ql/src/Likely Bugs/JapaneseEra/ConstructorOrMethodWithExactEraDate.qhelp
View File

@@ -4,7 +4,8 @@
<qhelp>
<overview>
<p>
When eras change, date and time conversions that rely on hard-coded era start date need to be reviewed. Conversions relying on Japanese dates in the current era can produce an ambiguous date.
When eras change, date and time conversions that rely on a hard-coded era start date need to be reviewed. Conversions relying on Japanese dates in the current era can produce an ambiguous date.
The values for the current Japanese era dates should be read from a source that will be updated, such as the Windows registry.
</p>
</overview>

8
cpp/ql/src/Likely Bugs/JapaneseEra/ConstructorOrMethodWithExactEraDate.ql
View File

@@ -1,9 +1,9 @@
/**
* @name ConstructingJapaneseEraStartDate
* @description Japanese era changes can lead to code behaving differently. Aviod hard-coding Japanese era start dates. The values should be read from registry.
* @name Hard-coded Japanese era start date
* @description Japanese era changes can lead to code behaving differently. Avoid hard-coding Japanese era start dates.
* @kind problem
* @problem.severity warning
* @id cpp/JapaneseEra/Constructor-Or-Method-With-Exact-Era-Date
* @id cpp/japanese-era/constructor-or-method-with-exact-era-date
* @precision medium
* @tags reliability
* japanese-era
@@ -14,4 +14,4 @@ from Call cc, int i
where cc.getArgument(i).getValue().toInt() = 1989 and
cc.getArgument(i+1).getValue().toInt() = 1 and
cc.getArgument(i+2).getValue().toInt() = 8
select cc, "Call that appears to have hard-coded Japanese era start date as parameter"
select cc, "Call that appears to have hard-coded Japanese era start date as parameter."

3
cpp/ql/src/Likely Bugs/JapaneseEra/StructWithExactEraDate.qhelp
View File

@@ -4,7 +4,8 @@
<qhelp>
<overview>
<p>
When eras change, date and time conversions that rely on hard-coded era start date need to be reviewed. Conversions relying on Japanese dates in the current era can produce an ambiguous date.
When eras change, date and time conversions that rely on a hard-coded era start date need to be reviewed. Conversions relying on Japanese dates in the current era can produce an ambiguous date.
The values for the current Japanese era dates should be read from a source that will be updated, such as the Windows registry.
</p>
</overview>

8
cpp/ql/src/Likely Bugs/JapaneseEra/StructWithExactEraDate.ql
View File

@@ -1,9 +1,9 @@
/**
* @name StructWithJapaneseEraStartDate
* @description Japanese era changes can lead to code behaving differently. Aviod hard-coding Japanese era start dates. The values should be read from registry.
* @name Hard-coded Japanese era start date
* @description Japanese era changes can lead to code behaving differently. Avoid hard-coding Japanese era start dates.
* @kind problem
* @problem.severity warning
* @id cpp/JapaneseEra/Struct-With-Exact-Era-Date
* @id cpp/japanese-era/struct-with-exact-era-date
* @precision medium
* @tags reliability
* japanese-era
@@ -17,4 +17,4 @@ from StructLikeClass s, YearFieldAccess year, MonthFieldAccess month, DayFieldAc
where s.getAField().getAnAccess () = year and yearAssignment.getAnOperand() = year and yearAssignment.getAnOperand().getValue().toInt() = 1989 and
s.getAField().getAnAccess () = month and monthAssignment.getAnOperand() = month and monthAssignment.getAnOperand().getValue().toInt() = 1 and
s.getAField().getAnAccess () = day and dayAssignment.getAnOperand() = day and dayAssignment.getAnOperand().getValue().toInt() = 8
select year, "A time struct that is initialized with exact Japanese calendar era start date"
select year, "A time struct that is initialized with exact Japanese calendar era start date."

9
cpp/ql/src/Likely Bugs/Leap Year/Adding365daysPerYear.ql
View File

@@ -1,6 +1,6 @@
/**
* @name Year field changed using an arithmetic operation is used on an unchekced time conversion function
* @description A year field changed using an arithmetic operation is used on a time conversion function, but the return value of the function is not check for success or failure
* @name Year field changed using an arithmetic operation is used on an unchecked time conversion function
* @description A year field changed using an arithmetic operation is used on a time conversion function, but the return value of the function is not checked for success or failure.
* @kind problem
* @problem.severity error
* @id cpp/leap-year/adding-365-days-per-year
@@ -13,9 +13,8 @@ import cpp
import LeapYear
import semmle.code.cpp.dataflow.DataFlow
from Expr source, Expr sink, PossibleYearArithmeticOperationCheckConfiguration config
where config.hasFlow(DataFlow::exprNode(source), DataFlow::exprNode(sink))
select sink, "This arithmetic operation $@ uses a constant value of 365 ends up modifying the date/time located at $@, without considering leap year scenarios."
, source, source.toString()
, sink, sink.toString()
, source, source.toString()
, sink, sink.toString()

21
cpp/ql/src/Likely Bugs/Leap Year/LeapYear.qhelp
View File

@@ -2,14 +2,15 @@
"-//Semmle//qhelp//EN"
"qhelp.dtd">
<qhelp>
<overview>
<p>The Gregorian Calendar, which has become the internationally accepted civil calendar, the leap year rule is: Every year that is exactly divisible by four is a leap year, except for years that are exactly divisible by 100, but these centurial years are leap years if they are exactly divisible by 400.</p>
<p>A leap year bug occurs when software (in any language) is written without consideration of leap year logic, or with flawed logic to calculate leap years; which typically results in incorrect results.</p>
<p>The impact of these bugs may range from almost unnoticeable bugs such as an incorrect date, to severe bugs that affect reliability, availability or even the security of the affected system.</p>
</overview>
<overview>
<p>The leap year rule for the Gregorian calendar, which has become the internationally accepted civil calendar, is: every year that is exactly divisible by four is a leap year, except for years that are exactly divisible by 100, but these centurial years are leap years if they are exactly divisible by 400.</p>
<p>A leap year bug occurs when software (in any language) is written without consideration of leap year logic, or with flawed logic to calculate leap years; which typically results in incorrect results.</p>
<p>The impact of these bugs may range from almost unnoticeable bugs such as an incorrect date, to severe bugs that affect reliability, availability or even the security of the affected system.</p>
</overview>
<references>
<li>U.S. Naval Observatory Website - <a href="https://aa.usno.navy.mil/faq/docs/calendars.php"> Introduction to Calendars</a></li>
<li>Wikipedia - <a href="https://en.wikipedia.org/wiki/Leap_year_bug"> Leap year bug</a> </li>
<li>Microsoft Azure blog - <a href="https://azure.microsoft.com/en-us/blog/is-your-code-ready-for-the-leap-year/"> Is your code ready for the leap year?</a> </li>
</references>
<references>
<li>U.S. Naval Observatory Website - <a href="https://aa.usno.navy.mil/faq/docs/calendars.php"> Introduction to Calendars</a></li>
<li>Wikipedia - <a href="https://en.wikipedia.org/wiki/Leap_year_bug"> Leap year bug</a> </li>
<li>Microsoft Azure blog - <a href="https://azure.microsoft.com/en-us/blog/is-your-code-ready-for-the-leap-year/"> Is your code ready for the leap year?</a> </li>
</references>
</qhelp>

5
cpp/ql/src/Likely Bugs/Leap Year/LeapYear.qll
View File

@@ -1,5 +1,5 @@
/**
* Provides a library for helping create leap year realted queries
* Provides a library for helping create leap year related queries
*/
import cpp
import semmle.code.cpp.dataflow.DataFlow
@@ -8,7 +8,6 @@ import semmle.code.cpp.commons.DateTime
/**
* Get the top-level BinaryOperation enclosing the expression e
* Not
*/
BinaryOperation getATopLevelBinaryOperationExpression(Expr e)
{
@@ -73,7 +72,7 @@ abstract class LeapYearFieldAccess extends YearFieldAccess {
* Holds if the field access is a modification,
* and it involves an arithmetic operation.
* In order to avoid false positives, the operation does not includes values that are normal for year normalization.
*
*
* 1900 - struct tm counts years since 1900
* 1980/80 - FAT32 epoch
*/

83
cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification.ql
View File

@@ -1,6 +1,6 @@
/**
* @name Year field changed using an arithmetic operation without checking for leap year
* @description A that field that represents a year is being modified by an arithmetic operation, but no proper check for leap year can be detected afterwards.
* @description A field that represents a year is being modified by an arithmetic operation, but no proper check for leap years can be detected afterwards.
* @kind problem
* @problem.severity error
* @id cpp/leap-year/unchecked-after-arithmetic-year-modification
@@ -14,48 +14,45 @@ import LeapYear
from Variable var, LeapYearFieldAccess yfa
where
exists( VariableAccess va |
yfa.getQualifier() = va
and var.getAnAccess() = va
// The year is modified with an arithmetic operation. Avoid values that are likely false positives
and yfa.isModifiedByArithmeticOperationNotForNormalization()
// Avoid false positives
and not (
// If there is a local check for leap year after the modification
exists( LeapYearFieldAccess yfacheck |
yfacheck.getQualifier() = var.getAnAccess()
and yfacheck.isUsedInCorrectLeapYearCheck()
and yfacheck = yfa.getASuccessor*()
)
// If there is a data flow from the variable that was modified to a function that seems to check for leap year
or exists(
VariableAccess source,
ChecksForLeapYearFunctionCall fc,
LeapYearCheckConfiguration config |
source = var.getAnAccess()
and config.hasFlow( DataFlow::exprNode(source), DataFlow::exprNode(fc.getAnArgument()))
)
// If there is a data flow from the field that was modified to a function that seems to check for leap year
or exists(
VariableAccess vacheck,
YearFieldAccess yfacheck,
ChecksForLeapYearFunctionCall fc,
LeapYearCheckConfiguration config |
vacheck = var.getAnAccess()
and yfacheck.getQualifier() = vacheck
and config.hasFlow( DataFlow::exprNode(yfacheck), DataFlow::exprNode(fc.getAnArgument()))
)
// If there is a successor or predecessor that sets the month = 1
or exists(
MonthFieldAccess mfa, AssignExpr ae |
mfa.getQualifier() = var.getAnAccess()
and mfa.isModified()
and ( mfa = yfa.getASuccessor*()
or yfa = mfa.getASuccessor*())
and ae = mfa.getEnclosingElement()
and ae.getAnOperand().getValue().toInt() = 1
)
exists(VariableAccess va |
yfa.getQualifier() = va
and var.getAnAccess() = va
// The year is modified with an arithmetic operation. Avoid values that are likely false positives
and yfa.isModifiedByArithmeticOperationNotForNormalization()
// Avoid false positives
and not (
// If there is a local check for leap year after the modification
exists( LeapYearFieldAccess yfacheck |
yfacheck.getQualifier() = var.getAnAccess()
and yfacheck.isUsedInCorrectLeapYearCheck()
and yfacheck = yfa.getASuccessor*()
)
)
// If there is a data flow from the variable that was modified to a function that seems to check for leap year
or exists(VariableAccess source,
ChecksForLeapYearFunctionCall fc,
LeapYearCheckConfiguration config |
source = var.getAnAccess()
and config.hasFlow( DataFlow::exprNode(source), DataFlow::exprNode(fc.getAnArgument()))
)
// If there is a data flow from the field that was modified to a function that seems to check for leap year
or exists(VariableAccess vacheck,
YearFieldAccess yfacheck,
ChecksForLeapYearFunctionCall fc,
LeapYearCheckConfiguration config |
vacheck = var.getAnAccess()
and yfacheck.getQualifier() = vacheck
and config.hasFlow( DataFlow::exprNode(yfacheck), DataFlow::exprNode(fc.getAnArgument()))
)
// If there is a successor or predecessor that sets the month = 1
or exists(MonthFieldAccess mfa, AssignExpr ae |
mfa.getQualifier() = var.getAnAccess()
and mfa.isModified()
and (mfa = yfa.getASuccessor*()
or yfa = mfa.getASuccessor*())
and ae = mfa.getEnclosingElement()
and ae.getAnOperand().getValue().toInt() = 1
)
)
)
select yfa
, "Field $@ on variable $@ has been modified, but no appropriate check for LeapYear was found.", yfa.getTarget(), yfa.getTarget().toString(), var, var.toString()

2
cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationBad.c
View File

@@ -2,7 +2,7 @@ SYSTEMTIME st;
FILETIME ft;
GetSystemTime(&st);
// Flawed logic will result in invalid date
// Flawed logic may result in invalid date
st.wYear++;
// The following code may fail

8
cpp/ql/src/Likely Bugs/Leap Year/UncheckedReturnValueForTimeFunctions.qhelp
View File

@@ -5,9 +5,9 @@
<overview>
<include src="LeapYear.qhelp" />
<p>When using a function that transform a date structure, and the year on the input argument for teh API has been manipulated, it is important to check for the return value of the function to make sure it succeeded.</p>
<p>When using a function that transforms a date structure, and the year on the input argument for the API has been manipulated, it is important to check for the return value of the function to make sure it succeeded.</p>
<p>Otherwise, the function may have failed, and the output parameter may contain invalid data that can cause any number of problems on the affected system.</p>
<p>The following is a list of the function that this query covers:</p>
<p>The following is a list of the functions that this query covers:</p>
<list>
<li><code>FileTimeToSystemTime</code></li>
<li><code>SystemTimeToFileTime</code></li>
@@ -19,10 +19,10 @@
<li><code>RtlTimeToSecondsSince1970</code></li>
<li><code>_mkgmtime</code></li>
</list>
</overview>
<recommendation>
<p>When calling an API that transforms a date variable that was manipulated, always check for the return value to verify if the API call succeeded or failed.</p>
<p>When calling an API that transforms a date variable that was manipulated, always check for the return value to verify that the API call succeeded.</p>
</recommendation>
<example>

130
cpp/ql/src/Likely Bugs/Leap Year/UncheckedReturnValueForTimeFunctions.ql
View File

@@ -1,6 +1,6 @@
/**
* @name Year field changed using an arithmetic operation is used on an unchecked time conversion function
* @description A year field changed using an arithmetic operation is used on a time conversion function, but the return value of the function is not check for success or failure
* @description A year field changed using an arithmetic operation is used on a time conversion function, but the return value of the function is not checked for success or failure
* @kind problem
* @problem.severity error
* @id cpp/leap-year/unchecked-return-value-for-time-conversion-function
@@ -16,21 +16,21 @@ import LeapYear
* A YearFieldAccess that is modifying the year by any arithmetic operation
*
* NOTE:
* To change this class to work for general purpose date transformations that do not check the return value
* To change this class to work for general purpose date transformations that do not check the return value,
* make the following changes:
* -> extends FieldAccess (line 22)
* -> this.isModified (line 28)
* ALthough you must expect a lower precision for a general purpose version.
* -> extends FieldAccess (line 27)
* -> this.isModified (line 33)
* Expect a lower precision for a general purpose version.
*/
class DateStructModifiedFieldAccess extends LeapYearFieldAccess {
DateStructModifiedFieldAccess() {
exists( Field f, StructLikeClass struct |
f.getAnAccess() = this
and struct.getAField() = f
and struct.getUnderlyingType() instanceof DateDataStruct
exists( Field f, StructLikeClass struct |
f.getAnAccess() = this
and struct.getAField() = f
and struct.getUnderlyingType() instanceof DateDataStruct
and this.isModifiedByArithmeticOperation()
)
)
}
}
@@ -38,72 +38,70 @@ class DateStructModifiedFieldAccess extends LeapYearFieldAccess {
* This is a list of APIs that will get the system time, and therefore guarantee that the value is valid
*/
class SafeTimeGatheringFunction extends Function {
SafeTimeGatheringFunction() {
this.getQualifiedName().matches("GetFileTime")
or this.getQualifiedName().matches("GetSystemTime")
or this.getQualifiedName().matches("NtQuerySystemTime")
}
SafeTimeGatheringFunction() {
this.getQualifiedName().matches("GetFileTime")
or this.getQualifiedName().matches("GetSystemTime")
or this.getQualifiedName().matches("NtQuerySystemTime")
}
}
/**
* This list of APIs should check for the return value to detect problems durin gthe conversion
* This list of APIs should check for the return value to detect problems during the conversion
*/
class TimeConversionFunction extends Function {
TimeConversionFunction() {
this.getQualifiedName().matches("FileTimeToSystemTime")
or this.getQualifiedName().matches("SystemTimeToFileTime")
or this.getQualifiedName().matches("SystemTimeToTzSpecificLocalTime")
or this.getQualifiedName().matches("SystemTimeToTzSpecificLocalTimeEx")
or this.getQualifiedName().matches("TzSpecificLocalTimeToSystemTime")
or this.getQualifiedName().matches("TzSpecificLocalTimeToSystemTimeEx")
or this.getQualifiedName().matches("RtlLocalTimeToSystemTime")
or this.getQualifiedName().matches("RtlTimeToSecondsSince1970")
or this.getQualifiedName().matches("_mkgmtime")
}
TimeConversionFunction() {
this.getQualifiedName().matches("FileTimeToSystemTime")
or this.getQualifiedName().matches("SystemTimeToFileTime")
or this.getQualifiedName().matches("SystemTimeToTzSpecificLocalTime")
or this.getQualifiedName().matches("SystemTimeToTzSpecificLocalTimeEx")
or this.getQualifiedName().matches("TzSpecificLocalTimeToSystemTime")
or this.getQualifiedName().matches("TzSpecificLocalTimeToSystemTimeEx")
or this.getQualifiedName().matches("RtlLocalTimeToSystemTime")
or this.getQualifiedName().matches("RtlTimeToSecondsSince1970")
or this.getQualifiedName().matches("_mkgmtime")
}
}
from FunctionCall fcall, TimeConversionFunction trf
, Variable var
, Variable var
where fcall = trf.getACallToThisFunction()
and fcall instanceof ExprInVoidContext
and var.getUnderlyingType() instanceof DateDataStruct
and ( exists( AddressOfExpr aoe |
aoe = fcall.getAnArgument()
and aoe.getAddressable() = var
) or exists( VariableAccess va |
fcall.getAnArgument() = va
and var.getAnAccess() = va
)
and fcall instanceof ExprInVoidContext
and var.getUnderlyingType() instanceof DateDataStruct
and (exists(AddressOfExpr aoe |
aoe = fcall.getAnArgument()
and aoe.getAddressable() = var
) or exists(VariableAccess va |
fcall.getAnArgument() = va
and var.getAnAccess() = va
)
and exists( DateStructModifiedFieldAccess dsmfa, VariableAccess modifiedVarAccess |
modifiedVarAccess = var.getAnAccess()
and modifiedVarAccess = dsmfa.getQualifier()
and modifiedVarAccess = fcall.getAPredecessor*()
)
// Remove false positives
and not
(
)
and exists(DateStructModifiedFieldAccess dsmfa, VariableAccess modifiedVarAccess |
modifiedVarAccess = var.getAnAccess()
and modifiedVarAccess = dsmfa.getQualifier()
and modifiedVarAccess = fcall.getAPredecessor*()
)
// Remove false positives
and not (
// Remove any instance where the predecessor is a SafeTimeGatheringFunction and no change to the data happened in between
exists( FunctionCall pred |
pred = fcall.getAPredecessor*()
and exists( SafeTimeGatheringFunction stgf |
pred = stgf.getACallToThisFunction()
)
and not exists( DateStructModifiedFieldAccess dsmfa, VariableAccess modifiedVarAccess |
modifiedVarAccess = var.getAnAccess()
and modifiedVarAccess = dsmfa.getQualifier()
and modifiedVarAccess = fcall.getAPredecessor*()
and modifiedVarAccess = pred.getASuccessor*()
)
exists(FunctionCall pred |
pred = fcall.getAPredecessor*()
and exists( SafeTimeGatheringFunction stgf |
pred = stgf.getACallToThisFunction()
)
// Remove any instance where the year is changed, but the month is set to 1 (year wrapping)
or
exists(MonthFieldAccess mfa, AssignExpr ae |
mfa.getQualifier() = var.getAnAccess()
and mfa.isModified()
and mfa = fcall.getAPredecessor*()
and ae = mfa.getEnclosingElement()
and ae.getAnOperand().getValue().toInt() = 1
)
and not exists(DateStructModifiedFieldAccess dsmfa, VariableAccess modifiedVarAccess |
modifiedVarAccess = var.getAnAccess()
and modifiedVarAccess = dsmfa.getQualifier()
and modifiedVarAccess = fcall.getAPredecessor*()
and modifiedVarAccess = pred.getASuccessor*()
)
select fcall, "Return value of $@ function should be checked to verified for any error because variable $@ is not guaranteed to be safe.", trf, trf.getQualifiedName().toString(), var, var.getName()
)
// Remove any instance where the year is changed, but the month is set to 1 (year wrapping)
or exists(MonthFieldAccess mfa, AssignExpr ae |
mfa.getQualifier() = var.getAnAccess()
and mfa.isModified()
and mfa = fcall.getAPredecessor*()
and ae = mfa.getEnclosingElement()
and ae.getAnOperand().getValue().toInt() = 1
)
)
select fcall, "Return value of $@ function should be verified to check for any error because variable $@ is not guaranteed to be safe.", trf, trf.getQualifiedName().toString(), var, var.getName()

6
cpp/ql/src/Likely Bugs/Leap Year/UnsafeArrayForDaysOfYear.qhelp
View File

@@ -6,12 +6,12 @@
<include src="LeapYear.qhelp" />
<p>This query helps to detect when a developer allocates an array or other fixed-length data structure such as <code>std::vector</code> with 365 elements – one for each day of the year.</p>
<p>Since leap years have 366 days, there will be no allocated element on December 31st at the end of a leap year; which will lead to a buffer overflow on a leap-year.</p>
<p>Since leap years have 366 days, there will be no allocated element on December 31st at the end of a leap year; which will lead to a buffer overflow on a leap year.</p>
</overview>
<recommendation>
<p>When allocating memory for storing elements for each day of the year, ensure that the correct amount of elements are being allocated</p>
<p>It is also highly recommended to compile teh code with array-bounds checking enabled whenever possible.</p>
<p>When allocating memory for storing elements for each day of the year, ensure that the correct number of elements are allocated.</p>
<p>It is also highly recommended to compile the code with array-bounds checking enabled whenever possible.</p>
</recommendation>
<example>

2
cpp/ql/src/Likely Bugs/Leap Year/UnsafeArrayForDaysOfYear.ql
View File

@@ -33,4 +33,4 @@ where
and cc.getTarget().hasName("vector")
and cc.getArgument(0).getValue().toInt() = 365
)
select element, "There is an array or std::vector allocation with a hardcoded set of 365 elements, which may indicate number of days of days in a year without considering leap year scenarios"
select element, "There is an array or std::vector allocation with a hard-coded set of 365 elements, which may indicate the number of days in a year without considering leap year scenarios."