Merge pull request #1467 from geoffw0/dates-cleanup1

CPP: Follow-up for Mishandling Japanese Era and Leap Year in calculations
This commit is contained in:
Jonas Jensen
2019-06-18 20:13:33 +02:00
committed by GitHub
13 changed files with 175 additions and 158 deletions

View File

@@ -11,7 +11,7 @@
<references>
<li>
<a href="https://blogs.msdn.microsoft.com/shawnste/2018/04/12/the-japanese-calendars-y2k-moment/">The Japanese Calendar<EFBFBD>s Y2K Moment</a>.
<a href="https://blogs.msdn.microsoft.com/shawnste/2018/04/12/the-japanese-calendars-y2k-moment/">The Japanese Calendar's Y2K Moment</a>.
</li>
</references>
</qhelp>

View File

@@ -10,8 +10,10 @@
*/
import cpp
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."
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."

View File

@@ -11,7 +11,7 @@
<references>
<li>
<a href="https://blogs.msdn.microsoft.com/shawnste/2018/04/12/the-japanese-calendars-y2k-moment/">The Japanese Calendar<EFBFBD>s Y2K Moment</a>.
<a href="https://blogs.msdn.microsoft.com/shawnste/2018/04/12/the-japanese-calendars-y2k-moment/">The Japanese Calendar's Y2K Moment</a>.
</li>
</references>
</qhelp>

View File

@@ -4,17 +4,24 @@
* @kind problem
* @problem.severity warning
* @id cpp/japanese-era/struct-with-exact-era-date
* @precision medium
* @tags reliability
* japanese-era
*/
import cpp
import semmle.code.cpp.commons.DateTime
from StructLikeClass s, YearFieldAccess year, MonthFieldAccess month, DayFieldAccess day, Operation yearAssignment, Operation monthAssignment, Operation dayAssignment
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
from
StructLikeClass s, YearFieldAccess year, MonthFieldAccess month, DayFieldAccess day,
Operation yearAssignment, Operation monthAssignment, Operation dayAssignment
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."

View File

@@ -15,6 +15,8 @@ of days. Alternatively, use an established library routine that already contain
</recommendation>
<references>
<include src="LeapYearReferences.qhelp" />
<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>

View File

@@ -2,9 +2,9 @@
* @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
* @problem.severity warning
* @id cpp/leap-year/adding-365-days-per-year
* @precision high
* @precision medium
* @tags security
* leap-year
*/
@@ -15,6 +15,6 @@ 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()
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()

View File

@@ -1,10 +0,0 @@
<!DOCTYPE qhelp PUBLIC
"-//Semmle//qhelp//EN"
"qhelp.dtd">
<qhelp>
<fragment>
<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>
</fragment>
</qhelp>

View File

@@ -22,6 +22,8 @@
</example>
<references>
<include src="LeapYearReferences.qhelp" />
<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>

View File

@@ -2,9 +2,9 @@
* @name Year field changed using an arithmetic operation without checking for leap year
* @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
* @problem.severity warning
* @id cpp/leap-year/unchecked-after-arithmetic-year-modification
* @precision high
* @precision medium
* @tags security
* leap-year
*/
@@ -12,47 +12,53 @@
import cpp
import LeapYear
from Variable var, LeapYearFieldAccess yfa
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
)
)
)
select yfa
, "Field $@ on variable $@ has been modified, but no appropriate check for LeapYear was found.", yfa.getTarget(), yfa.getTarget().toString(), var, var.toString()
exists(VariableAccess va |
yfa.getQualifier() = va and
var.getAnAccess() = va and
// The year is modified with an arithmetic operation. Avoid values that are likely false positives
yfa.isModifiedByArithmeticOperationNotForNormalization() and
// Avoid false positives
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*()
)
or
// If there is a data flow from the variable that was modified to a function that seems to check for leap year
exists(
VariableAccess source, ChecksForLeapYearFunctionCall fc, LeapYearCheckConfiguration config
|
source = var.getAnAccess() and
config.hasFlow(DataFlow::exprNode(source), DataFlow::exprNode(fc.getAnArgument()))
)
or
// If there is a data flow from the field that was modified to a function that seems to check for leap year
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()))
)
or
// If there is a successor or predecessor that sets the month = 1
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()

View File

@@ -8,7 +8,7 @@
<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 functions that this query covers:</p>
<list>
<ul>
<li><code>FileTimeToSystemTime</code></li>
<li><code>SystemTimeToFileTime</code></li>
<li><code>SystemTimeToTzSpecificLocalTime</code></li>
@@ -18,7 +18,7 @@
<li><code>RtlLocalTimeToSystemTime</code></li>
<li><code>RtlTimeToSecondsSince1970</code></li>
<li><code>_mkgmtime</code></li>
</list>
</ul>
</overview>
<recommendation>
@@ -34,6 +34,8 @@
</example>
<references>
<include src="LeapYearReferences.qhelp" />
<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>

View File

@@ -2,9 +2,9 @@
* @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
* @problem.severity warning
* @id cpp/leap-year/unchecked-return-value-for-time-conversion-function
* @precision high
* @precision medium
* @tags security
* leap-year
*/
@@ -14,23 +14,22 @@ 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,
* make the following changes:
* -> extends FieldAccess (line 27)
* -> 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
and this.isModifiedByArithmeticOperation()
)
DateStructModifiedFieldAccess() {
exists(Field f, StructLikeClass struct |
f.getAnAccess() = this and
struct.getAField() = f and
struct.getUnderlyingType() instanceof DateDataStruct and
this.isModifiedByArithmeticOperation()
)
}
}
@@ -39,9 +38,9 @@ class DateStructModifiedFieldAccess extends LeapYearFieldAccess {
*/
class SafeTimeGatheringFunction extends Function {
SafeTimeGatheringFunction() {
this.getQualifiedName().matches("GetFileTime")
or this.getQualifiedName().matches("GetSystemTime")
or this.getQualifiedName().matches("NtQuerySystemTime")
this.getQualifiedName().matches("GetFileTime") or
this.getQualifiedName().matches("GetSystemTime") or
this.getQualifiedName().matches("NtQuerySystemTime")
}
}
@@ -50,58 +49,62 @@ class SafeTimeGatheringFunction extends Function {
*/
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")
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
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
from FunctionCall fcall, TimeConversionFunction trf, 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
)
)
and exists(DateStructModifiedFieldAccess dsmfa, VariableAccess modifiedVarAccess |
modifiedVarAccess = var.getAnAccess()
and modifiedVarAccess = dsmfa.getQualifier()
and modifiedVarAccess = fcall.getAPredecessor*()
)
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*()
) and
// Remove false positives
and not (
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()
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*()
)
)
and not exists(DateStructModifiedFieldAccess dsmfa, VariableAccess modifiedVarAccess |
modifiedVarAccess = var.getAnAccess()
and modifiedVarAccess = dsmfa.getQualifier()
and modifiedVarAccess = fcall.getAPredecessor*()
and modifiedVarAccess = pred.getASuccessor*()
or
// Remove any instance where the year is changed, but the month is set to 1 (year wrapping)
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
)
)
// 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()
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()

View File

@@ -23,6 +23,8 @@
</example>
<references>
<include src="LeapYearReferences.qhelp" />
<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>

View File

@@ -1,9 +1,9 @@
/**
* @name Unsafe array for days of the year
* @description An array of 365 items typically indicates one entry per day of the year, but without considering leap years, which would be 366 days.
* @name Unsafe array for days of the year
* @description An array of 365 items typically indicates one entry per day of the year, but without considering leap years, which would be 366 days.
* An access on a leap year could result in buffer overflow bugs.
* @kind problem
* @problem.severity error
* @problem.severity warning
* @id cpp/leap-year/unsafe-array-for-days-of-the-year
* @precision medium
* @tags security
@@ -13,24 +13,25 @@
import cpp
class LeapYearUnsafeDaysOfTheYearArrayType extends ArrayType {
LeapYearUnsafeDaysOfTheYearArrayType() {
this.getArraySize() = 365
}
LeapYearUnsafeDaysOfTheYearArrayType() { this.getArraySize() = 365 }
}
from Element element
where
exists( NewArrayExpr nae |
element = nae
and nae.getAllocatedType() instanceof LeapYearUnsafeDaysOfTheYearArrayType
)
or exists( Variable var |
var = element
and var.getType() instanceof LeapYearUnsafeDaysOfTheYearArrayType
)
or exists( ConstructorCall cc |
element = cc
and cc.getTarget().hasName("vector")
and cc.getArgument(0).getValue().toInt() = 365
)
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."
exists(NewArrayExpr nae |
element = nae and
nae.getAllocatedType() instanceof LeapYearUnsafeDaysOfTheYearArrayType
)
or
exists(Variable var |
var = element and
var.getType() instanceof LeapYearUnsafeDaysOfTheYearArrayType
)
or
exists(ConstructorCall cc |
element = cc and
cc.getTarget().hasName("vector") and
cc.getArgument(0).getValue().toInt() = 365
)
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."