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

Merge pull request #1490 from geoffw0/leapyeararith

Browse Source 
CPP: Improvements to LeapYear.qll
This commit is contained in:
Jonas Jensen
2019-06-25 10:46:12 +02:00
committed by GitHub
parent 0fa41f7a21 562141759a
commit de65dc5501
16 changed files with 197 additions and 137 deletions
Download Patch File Download Diff File Expand all files Collapse all files

0
cpp/ql/src/Likely Bugs/Leap Year/Adding365daysPerYear.qhelp → cpp/ql/src/Likely Bugs/Leap Year/Adding365DaysPerYear.qhelp
View File

0
cpp/ql/src/Likely Bugs/Leap Year/Adding365daysPerYear.ql → cpp/ql/src/Likely Bugs/Leap Year/Adding365DaysPerYear.ql
View File

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

@@ -1,6 +1,7 @@
/**
* Provides a library for helping create leap year related queries
*/
import cpp
import semmle.code.cpp.dataflow.DataFlow
import semmle.code.cpp.controlflow.Guards
@@ -9,41 +10,37 @@ import semmle.code.cpp.commons.DateTime
/**
* Get the top-level BinaryOperation enclosing the expression e
*/
BinaryOperation getATopLevelBinaryOperationExpression(Expr e)
{
BinaryOperation getATopLevelBinaryOperationExpression(Expr e) {
result = e.getEnclosingElement().(BinaryOperation)
or
result = getATopLevelBinaryOperationExpression( e.getEnclosingElement())
result = getATopLevelBinaryOperationExpression(e.getEnclosingElement())
}
/**
* Holds if the top-level binary operation for expression `e` includes the operator specified in `operator` with an operand specified by `valueToCheck`
*/
predicate additionalLogicalCheck( Expr e, string operation, int valueToCheck) {
exists(BinaryLogicalOperation bo |
bo = getATopLevelBinaryOperationExpression(e) |
exists( BinaryArithmeticOperation bao |
bao = bo.getAChild*() |
bao.getAnOperand().getValue().toInt() = valueToCheck
and bao.getOperator() = operation
)
)
predicate additionalLogicalCheck(Expr e, string operation, int valueToCheck) {
exists(BinaryLogicalOperation bo | bo = getATopLevelBinaryOperationExpression(e) |
exists(BinaryArithmeticOperation bao | bao = bo.getAChild*() |
bao.getAnOperand().getValue().toInt() = valueToCheck and
bao.getOperator() = operation
)
)
}
/**
* Operation that seems to be checking for leap year
* Operation that seems to be checking for leap year
*/
class CheckForLeapYearOperation extends Operation {
CheckForLeapYearOperation() {
exists( BinaryArithmeticOperation bo |
bo = this |
bo.getAnOperand().getValue().toInt() = 4
and bo.getOperator().toString() = "%"
and additionalLogicalCheck( this.getEnclosingElement(), "%", 100)
and additionalLogicalCheck( this.getEnclosingElement(), "%", 400)
exists(BinaryArithmeticOperation bo | bo = this |
bo.getAnOperand().getValue().toInt() = 4 and
bo.getOperator().toString() = "%" and
additionalLogicalCheck(this.getEnclosingElement(), "%", 100) and
additionalLogicalCheck(this.getEnclosingElement(), "%", 400)
)
}
override string getOperator() { result = "LeapYearCheck" }
}
@@ -52,103 +49,121 @@ class CheckForLeapYearOperation extends Operation {
*/
abstract class LeapYearFieldAccess extends YearFieldAccess {
/**
* Holds if the field access is a modification,
* and it involves an arithmetic operation
* Holds if the field access is a modification,
* and it involves an arithmetic operation
*/
predicate isModifiedByArithmeticOperation() {
this.isModified()
and exists( Operation op |
op.getAnOperand() = this
and ( op instanceof AssignArithmeticOperation
or exists( BinaryArithmeticOperation bao |
bao = op.getAnOperand())
or op instanceof PostfixCrementOperation
or op instanceof PrefixCrementOperation
this.isModified() and
exists(Operation op |
op.getAnOperand() = this and
(
op instanceof AssignArithmeticOperation or
exists(BinaryArithmeticOperation bao | bao = op.getAnOperand()) or
op instanceof PostfixCrementOperation or
op instanceof PrefixCrementOperation
)
)
}
/**
* Holds if the field access is a modification,
* and it involves an arithmetic operation.
* 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
*/
predicate isModifiedByArithmeticOperationNotForNormalization() {
this.isModified()
and exists( Operation op |
op.getAnOperand() = this
and ( (op instanceof AssignArithmeticOperation
and not ( op.getAChild().getValue().toInt() = 1900
or op.getAChild().getValue().toInt() = 2000
or op.getAChild().getValue().toInt() = 1980
or op.getAChild().getValue().toInt() = 80
// Special case for transforming marshaled 2-digit year date:
this.isModified() and
exists(Operation op |
op.getAnOperand() = this and
(
op instanceof AssignArithmeticOperation and
not (
op.getAChild().getValue().toInt() = 1900
or
op.getAChild().getValue().toInt() = 2000
or
op.getAChild().getValue().toInt() = 1980
or
op.getAChild().getValue().toInt() = 80
or
// Special case for transforming marshaled 2-digit year date:
// theTime.wYear += 100*value;
or exists( MulExpr mulBy100 | mulBy100 = op.getAChild() |
mulBy100.getAChild().getValue().toInt() = 100 )))
or exists( BinaryArithmeticOperation bao |
bao = op.getAnOperand()
and not ( bao.getAChild().getValue().toInt() = 1900
or bao.getAChild().getValue().toInt() = 2000
or bao.getAChild().getValue().toInt() = 1980
or bao.getAChild().getValue().toInt() = 80
// Special case for transforming marshaled 2-digit year date:
// theTime.wYear += 100*value;
or exists( MulExpr mulBy100 | mulBy100 = op.getAChild() |
mulBy100.getAChild().getValue().toInt() = 100 ))
exists(MulExpr mulBy100 | mulBy100 = op.getAChild() |
mulBy100.getAChild().getValue().toInt() = 100
)
)
or op instanceof PostfixCrementOperation
or op instanceof PrefixCrementOperation
or
exists(BinaryArithmeticOperation bao |
bao = op.getAnOperand() and
// we're specifically interested in calculations that update the existing
// value (like `x = x + 1`), so look for a child `YearFieldAccess`.
bao.getAChild*() instanceof YearFieldAccess and
not (
bao.getAChild().getValue().toInt() = 1900
or
bao.getAChild().getValue().toInt() = 2000
or
bao.getAChild().getValue().toInt() = 1980
or
bao.getAChild().getValue().toInt() = 80
or
// Special case for transforming marshaled 2-digit year date:
// theTime.wYear += 100*value;
exists(MulExpr mulBy100 | mulBy100 = op.getAChild() |
mulBy100.getAChild().getValue().toInt() = 100
)
)
)
or
op instanceof PostfixCrementOperation
or
op instanceof PrefixCrementOperation
)
)
}
/**
* Holds if the top-level binary operation includes a modulus operator with an operand specified by `valueToCheck`
*/
predicate additionalModulusCheckForLeapYear( int valueToCheck) {
predicate additionalModulusCheckForLeapYear(int valueToCheck) {
additionalLogicalCheck(this, "%", valueToCheck)
}
/**
* Holds if the top-level binary operation includes an addition or subtraction operator with an operand specified by `valueToCheck`
*/
predicate additionalAdditionOrSubstractionCheckForLeapYear( int valueToCheck) {
additionalLogicalCheck(this, "+", valueToCheck)
or additionalLogicalCheck(this, "-", valueToCheck)
predicate additionalAdditionOrSubstractionCheckForLeapYear(int valueToCheck) {
additionalLogicalCheck(this, "+", valueToCheck) or
additionalLogicalCheck(this, "-", valueToCheck)
}
/**
* Holds true if this object is used on a modulus 4 operation, which would likely indicate the start of a leap year check
*/
predicate isUsedInMod4Operation()
{
predicate isUsedInMod4Operation() {
not this.isModified() and
exists( BinaryArithmeticOperation bo |
bo.getAnOperand() = this
and bo.getAnOperand().getValue().toInt() = 4
and bo.getOperator().toString() = "%"
exists(BinaryArithmeticOperation bo |
bo.getAnOperand() = this and
bo.getAnOperand().getValue().toInt() = 4 and
bo.getOperator().toString() = "%"
)
}
/**
* Holds true if this object seems to be used in a valid gregorian calendar leap year check
*/
predicate isUsedInCorrectLeapYearCheck()
{
// The Gregorian 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,
predicate isUsedInCorrectLeapYearCheck() {
// The Gregorian 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
//
// https://aa.usno.navy.mil/faq/docs/calendars.php
this.isUsedInMod4Operation()
and additionalModulusCheckForLeapYear(400)
and additionalModulusCheckForLeapYear(100)
this.isUsedInMod4Operation() and
additionalModulusCheckForLeapYear(400) and
additionalModulusCheckForLeapYear(100)
}
}
@@ -156,30 +171,29 @@ abstract class LeapYearFieldAccess extends YearFieldAccess {
* YearFieldAccess for SYSTEMTIME struct
*/
class StructSystemTimeLeapYearFieldAccess extends LeapYearFieldAccess {
StructSystemTimeLeapYearFieldAccess() {
this.toString().matches("wYear")
}
StructSystemTimeLeapYearFieldAccess() { this.toString().matches("wYear") }
}
/**
* YearFieldAccess for struct tm
*/
class StructTmLeapYearFieldAccess extends LeapYearFieldAccess {
StructTmLeapYearFieldAccess() {
this.toString().matches("tm_year")
}
StructTmLeapYearFieldAccess() { this.toString().matches("tm_year") }
override predicate isUsedInCorrectLeapYearCheck()
{
this.isUsedInMod4Operation()
and additionalModulusCheckForLeapYear(400)
and additionalModulusCheckForLeapYear(100)
override predicate isUsedInCorrectLeapYearCheck() {
this.isUsedInMod4Operation() and
additionalModulusCheckForLeapYear(400) and
additionalModulusCheckForLeapYear(100) and
// tm_year represents years since 1900
and ( additionalAdditionOrSubstractionCheckForLeapYear(1900)
// some systems may use 2000 for 2-digit year conversions
or additionalAdditionOrSubstractionCheckForLeapYear(2000)
// converting from/to Unix epoch
or additionalAdditionOrSubstractionCheckForLeapYear(1970)
)
(
additionalAdditionOrSubstractionCheckForLeapYear(1900)
or
// some systems may use 2000 for 2-digit year conversions
additionalAdditionOrSubstractionCheckForLeapYear(2000)
or
// converting from/to Unix epoch
additionalAdditionOrSubstractionCheckForLeapYear(1970)
)
}
}
@@ -188,36 +202,28 @@ class StructTmLeapYearFieldAccess extends LeapYearFieldAccess {
*/
class ChecksForLeapYearFunctionCall extends FunctionCall {
ChecksForLeapYearFunctionCall() {
exists( Function f, CheckForLeapYearOperation clyo |
f.getACallToThisFunction() = this |
exists(Function f, CheckForLeapYearOperation clyo | f.getACallToThisFunction() = this |
clyo.getEnclosingFunction() = f
)
}
}
/**
* DataFlow::Configuration for finding a variable access that would flow into
* DataFlow::Configuration for finding a variable access that would flow into
* a function call that includes an operation to check for leap year
*/
class LeapYearCheckConfiguration extends DataFlow::Configuration {
LeapYearCheckConfiguration() {
this = "LeapYearCheckConfiguration"
}
LeapYearCheckConfiguration() { this = "LeapYearCheckConfiguration" }
override predicate isSource(DataFlow::Node source) {
exists( VariableAccess va |
va = source.asExpr()
)
exists(VariableAccess va | va = source.asExpr())
}
override predicate isSink(DataFlow::Node sink) {
exists( ChecksForLeapYearFunctionCall fc |
sink.asExpr() = fc.getAnArgument()
)
exists(ChecksForLeapYearFunctionCall fc | sink.asExpr() = fc.getAnArgument())
}
}
/**
* DataFlow::Configuration for finding an operation w/hardcoded 365 that will flow into a FILEINFO field
*/
@@ -227,21 +233,19 @@ class FiletimeYearArithmeticOperationCheckConfiguration extends DataFlow::Config
}
override predicate isSource(DataFlow::Node source) {
exists( Expr e, Operation op |
e = source.asExpr() |
op.getAChild*().getValue().toInt()=365
and op.getAChild*() = e
)
exists(Expr e, Operation op | e = source.asExpr() |
op.getAChild*().getValue().toInt() = 365 and
op.getAChild*() = e
)
}
override predicate isSink(DataFlow::Node sink) {
exists( StructLikeClass dds, FieldAccess fa, AssignExpr aexpr, Expr e |
e = sink.asExpr() |
dds instanceof FileTimeStruct
and fa.getQualifier().getUnderlyingType() = dds
and fa.isModified()
and aexpr.getAChild() = fa
and aexpr.getChild(1).getAChild*() = e
exists(StructLikeClass dds, FieldAccess fa, AssignExpr aexpr, Expr e | e = sink.asExpr() |
dds instanceof FileTimeStruct and
fa.getQualifier().getUnderlyingType() = dds and
fa.isModified() and
aexpr.getAChild() = fa and
aexpr.getChild(1).getAChild*() = e
)
}
}
@@ -255,22 +259,32 @@ class PossibleYearArithmeticOperationCheckConfiguration extends DataFlow::Config
}
override predicate isSource(DataFlow::Node source) {
exists( Expr e, Operation op |
e = source.asExpr() |
op.getAChild*().getValue().toInt()=365
and op.getAChild*() = e
)
exists(Operation op | op = source.asExpr() |
op.getAChild*().getValue().toInt() = 365 and
not op.getParent() instanceof Expr
)
}
override predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
// flow from anything on the RHS of an assignment to a time/date structure to that
// assignment.
exists(StructLikeClass dds, FieldAccess fa, AssignExpr aexpr, Expr e |
e = node1.asExpr() and
aexpr = node2.asExpr()
|
(dds instanceof FileTimeStruct or dds instanceof DateDataStruct) and
fa.getQualifier().getUnderlyingType() = dds and
aexpr.getLValue() = fa and
aexpr.getRValue().getAChild*() = e
)
}
override predicate isSink(DataFlow::Node sink) {
exists( StructLikeClass dds, FieldAccess fa, AssignExpr aexpr, Expr e |
e = sink.asExpr() |
(dds instanceof FileTimeStruct
or dds instanceof DateDataStruct)
and fa.getQualifier().getUnderlyingType() = dds
and fa.isModified()
and aexpr.getAChild() = fa
and aexpr.getChild(1).getAChild*() = e
exists(StructLikeClass dds, FieldAccess fa, AssignExpr aexpr | aexpr = sink.asExpr() |
(dds instanceof FileTimeStruct or dds instanceof DateDataStruct) and
fa.getQualifier().getUnderlyingType() = dds and
fa.isModified() and
aexpr.getLValue() = fa
)
}
}

3
cpp/ql/test/query-tests/Likely Bugs/Leap Year/Adding365DaysPerYear/Adding365daysPerYear.expected Normal file
View File

@@ -0,0 +1,3 @@
| test.cpp:173:2:173:52 | ... = ... | This arithmetic operation $@ uses a constant value of 365 ends up modifying the date/time located at $@, without considering leap year scenarios. | test.cpp:170:2:170:47 | ... += ... | ... += ... | test.cpp:173:2:173:52 | ... = ... | ... = ... |
| test.cpp:174:2:174:46 | ... = ... | This arithmetic operation $@ uses a constant value of 365 ends up modifying the date/time located at $@, without considering leap year scenarios. | test.cpp:170:2:170:47 | ... += ... | ... += ... | test.cpp:174:2:174:46 | ... = ... | ... = ... |
| test.cpp:193:2:193:24 | ... = ... | This arithmetic operation $@ uses a constant value of 365 ends up modifying the date/time located at $@, without considering leap year scenarios. | test.cpp:193:2:193:24 | ... = ... | ... = ... | test.cpp:193:2:193:24 | ... = ... | ... = ... |

1
cpp/ql/test/query-tests/Likely Bugs/Leap Year/Adding365DaysPerYear/Adding365daysPerYear.qlref Normal file
View File

@@ -0,0 +1 @@
Likely Bugs/Leap Year/Adding365DaysPerYear.ql

21
cpp/ql/test/query-tests/Likely Bugs/Leap Year/AntiPattern2/test.cpp → cpp/ql/test/query-tests/Likely Bugs/Leap Year/Adding365DaysPerYear/test.cpp
View File

@@ -176,3 +176,24 @@ void antipattern2()
// convert back to SYSTEMTIME for display or other usage
FileTimeToSystemTime(&ft, &st);
}
time_t mktime(struct tm *timeptr);
struct tm *gmtime(const time_t *timer);
time_t mkTime(int days)
{
struct tm tm;
time_t t;
tm.tm_sec = 0;
tm.tm_min = 0;
tm.tm_hour = 0;
tm.tm_mday = 0;
tm.tm_mon = 0;
tm.tm_year = days / 365; // BAD
// ...
t = mktime(&tm); // convert tm -> time_t
return t;
}

2
cpp/ql/test/query-tests/Likely Bugs/Leap Year/AntiPattern2/Adding365daysPerYear.expected
View File

@@ -1,2 +0,0 @@
| test.cpp:173:29:173:38 | qwLongTime | This arithmetic operation $@ uses a constant value of 365 ends up modifying the date/time located at $@, without considering leap year scenarios. | test.cpp:170:2:170:47 | ... += ... | ... += ... | test.cpp:173:29:173:38 | qwLongTime | qwLongTime |
| test.cpp:174:30:174:39 | qwLongTime | This arithmetic operation $@ uses a constant value of 365 ends up modifying the date/time located at $@, without considering leap year scenarios. | test.cpp:170:2:170:47 | ... += ... | ... += ... | test.cpp:174:30:174:39 | qwLongTime | qwLongTime |

1
cpp/ql/test/query-tests/Likely Bugs/Leap Year/AntiPattern2/Adding365daysPerYear.qlref
View File

@@ -1 +0,0 @@
Likely Bugs/Leap Year/Adding365daysPerYear.ql

0
cpp/ql/test/query-tests/Likely Bugs/Leap Year/AntiPattern1/UncheckedLeapYearAfterYearModification.expected → cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/UncheckedLeapYearAfterYearModification.expected
View File

0
cpp/ql/test/query-tests/Likely Bugs/Leap Year/AntiPattern1/UncheckedLeapYearAfterYearModification.qlref → cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/UncheckedLeapYearAfterYearModification.qlref
View File

0
cpp/ql/test/query-tests/Likely Bugs/Leap Year/AntiPattern1/UncheckedReturnValueForTimeFunctions.expected → cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/UncheckedReturnValueForTimeFunctions.expected
View File

0
cpp/ql/test/query-tests/Likely Bugs/Leap Year/AntiPattern1/UncheckedReturnValueForTimeFunctions.qlref → cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/UncheckedReturnValueForTimeFunctions.qlref
View File

24
cpp/ql/test/query-tests/Likely Bugs/Leap Year/AntiPattern1/test.cpp → cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/test.cpp
View File

@@ -656,3 +656,27 @@ IncrementMonth(LPSYSTEMTIME pst)
pst->wYear++;
}
}
/////////////////////////////////////////////////////////
void mkDateTest(int year)
{
struct tm t;
t.tm_sec = 0;
t.tm_min = 0;
t.tm_hour = 0;
t.tm_mday = 1; // day of the month - [1, 31]
t.tm_mon = 0; // months since January - [0, 11]
if (year >= 1900)
{
// 4-digit year
t.tm_year = year - 1900; // GOOD
} else if ((year >= 0) && (year < 100)) {
// 2-digit year assumed in the range 2000 - 2099
t.tm_year = year + 100; // GOOD [FALSE POSITIVE]
} else {
// fail
}
// ...
}

0
cpp/ql/test/query-tests/Likely Bugs/Leap Year/AntiPattern4/UnsafeArrayForDaysOfYear.expected → cpp/ql/test/query-tests/Likely Bugs/Leap Year/UnsafeArrayForDaysOfYear/UnsafeArrayForDaysOfYear.expected
View File

0
cpp/ql/test/query-tests/Likely Bugs/Leap Year/AntiPattern4/UnsafeArrayForDaysOfYear.qlref → cpp/ql/test/query-tests/Likely Bugs/Leap Year/UnsafeArrayForDaysOfYear/UnsafeArrayForDaysOfYear.qlref
View File

0
cpp/ql/test/query-tests/Likely Bugs/Leap Year/AntiPattern4/test.cpp → cpp/ql/test/query-tests/Likely Bugs/Leap Year/UnsafeArrayForDaysOfYear/test.cpp
View File