From aba87c5124c706a4953fff7795cfc58b0a62c565 Mon Sep 17 00:00:00 2001 From: Josh Brown Date: Thu, 18 Dec 2025 12:30:12 -0800 Subject: [PATCH 01/28] Massaging cpp leap year AP1 --- .../UncheckedLeapYearAfterYearModification.ql | 141 +++++++++++------- .../test.cpp | 51 +++++++ 2 files changed, 136 insertions(+), 56 deletions(-) diff --git a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification.ql b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification.ql index 18ad003eb71f..452824f684e2 100644 --- a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification.ql +++ b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification.ql @@ -12,55 +12,6 @@ import cpp import LeapYear -/** - * Holds if there is no known leap-year verification for the given `YearWriteOp`. - * Binds the `var` argument to the qualifier of the `ywo` argument. - */ -bindingset[ywo] -predicate isYearModifedWithoutExplicitLeapYearCheck(Variable var, YearWriteOp ywo) { - exists(VariableAccess va, YearFieldAccess yfa | - yfa = ywo.getYearAccess() and - yfa.getQualifier() = va and - var.getAnAccess() = va 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.getBasicBlock() = yfa.getBasicBlock().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 | - source = var.getAnAccess() and - LeapYearCheckFlow::flow(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 | - vacheck = var.getAnAccess() and - yfacheck.getQualifier() = vacheck and - LeapYearCheckFlow::flow(DataFlow::exprNode(yfacheck), DataFlow::exprNode(fc.getAnArgument())) - ) - or - // If there is a successor or predecessor that sets the month or day to a fixed value - exists(FieldAccess mfa, AssignExpr ae, int val | - mfa instanceof MonthFieldAccess or mfa instanceof DayFieldAccess - | - mfa.getQualifier() = var.getAnAccess() and - mfa.isModified() and - ( - mfa.getBasicBlock() = yfa.getBasicBlock().getASuccessor*() or - yfa.getBasicBlock() = mfa.getBasicBlock().getASuccessor+() - ) and - ae = mfa.getEnclosingElement() and - ae.getAnOperand().getValue().toInt() = val - ) - ) - ) -} - /** * The set of all write operations to the Year field of a date struct. */ @@ -70,6 +21,61 @@ abstract class YearWriteOp extends Operation { /** Get the expression which represents the new value. */ abstract Expr getMutationExpr(); + + /** + * Checks if this Operation is a simple normalization, converting between formats. + */ + predicate isNormalizationOperation(){ + isNormalizationOperation(this.getMutationExpr()) + } + + /** + * Holds if there is no known leap-year verification for the `YearWriteOp`. + * Binds the `var` argument to the qualifier of the `ywo` argument. + */ + predicate isYearModifedWithoutExplicitLeapYearCheck(Variable var) { + exists(VariableAccess va, YearFieldAccess yfa | + yfa = this.getYearAccess() and + yfa.getQualifier() = va and + var.getAnAccess() = va 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.getBasicBlock() = yfa.getBasicBlock().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 | + source = var.getAnAccess() and + LeapYearCheckFlow::flow(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 | + vacheck = var.getAnAccess() and + yfacheck.getQualifier() = vacheck and + LeapYearCheckFlow::flow(DataFlow::exprNode(yfacheck), DataFlow::exprNode(fc.getAnArgument())) + ) + or + // If there is a successor or predecessor that sets the month or day to a fixed value + exists(FieldAccess mfa, AssignExpr ae, int val | + mfa instanceof MonthFieldAccess or mfa instanceof DayFieldAccess + | + mfa.getQualifier() = var.getAnAccess() and + mfa.isModified() and + ( + mfa.getBasicBlock() = yfa.getBasicBlock().getASuccessor*() or + yfa.getBasicBlock() = mfa.getBasicBlock().getASuccessor+() + ) and + ae = mfa.getEnclosingElement() and + ae.getAnOperand().getValue().toInt() = val + ) + ) + ) + } } /** @@ -125,14 +131,37 @@ module OperationToYearAssignmentConfig implements DataFlow::ConfigSig { module OperationToYearAssignmentFlow = DataFlow::Global; -from Variable var, YearWriteOp ywo, Expr mutationExpr +/** + * A Call to some known Time conversion function, which maps between time structs or scalars. + */ +class TimeConversionCall extends Call{ + TimeConversionCall(){ + this = any(TimeConversionFunction f).getACallToThisFunction() + } + + bindingset[var] + predicate converts(Variable var){ + this.getAnArgument().getAChild*() = var.getAnAccess() + } +} + +/** + * A YearWriteOp that is non-mutating (AddressOfExpr *could* mutate but doesnt) + */ +class NonMutatingYearWriteOp extends Operation instanceof YearWriteOp{ + NonMutatingYearWriteOp(){ + this instanceof AddressOfExpr + } +} + +from Variable var, YearWriteOp ywo where - mutationExpr = ywo.getMutationExpr() and - isYearModifedWithoutExplicitLeapYearCheck(var, ywo) and - not isNormalizationOperation(mutationExpr) and - not ywo instanceof AddressOfExpr and - not exists(Call c, TimeConversionFunction f | f.getACallToThisFunction() = c | - c.getAnArgument().getAChild*() = var.getAnAccess() and + not ywo.isNormalizationOperation() and + not ywo instanceof NonMutatingYearWriteOp and + ywo.isYearModifedWithoutExplicitLeapYearCheck(var) and + // If there is a Time conversion after, which utilizes the variable - could lead to false negatives maybe? + not exists(TimeConversionCall c | + c.converts(var) and ywo.getASuccessor*() = c ) select ywo, diff --git a/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/test.cpp b/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/test.cpp index beb2c4061496..9e7858a5faf0 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/test.cpp +++ b/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/test.cpp @@ -46,6 +46,8 @@ typedef struct _TIME_DYNAMIC_ZONE_INFORMATION { BOOLEAN DynamicDaylightTimeDisabled; } DYNAMIC_TIME_ZONE_INFORMATION, *PDYNAMIC_TIME_ZONE_INFORMATION; +typedef const wchar_t* LPCWSTR; + struct tm { int tm_sec; // seconds after the minute - [0, 60] including leap second @@ -111,6 +113,10 @@ TzSpecificLocalTimeToSystemTimeEx( LPSYSTEMTIME lpUniversalTime ); +int _wtoi( + const wchar_t *str +); + void GetSystemTime( LPSYSTEMTIME lpSystemTime ); @@ -1004,3 +1010,48 @@ bool tp_intermediaryVar(struct timespec now, struct logtime ×tamp_remote) tm.tm_mday = tm.tm_mon == 2 && tm.tm_mday == 29 && !isLeapYear ? 28 : tm.tm_mday; return tm; } + +/** +* Negative Case - Anti-pattern 1: [year ±n, month, day] +* Modification of SYSTEMTIME struct by copying from another struct, but no arithmetic is performed. +*/ +bool +FMAPITimeToSysTimeW(LPCWSTR wszTime, SYSTEMTIME *psystime) +{ + // if (!wszTime || SafeIsBadReadPtr(wszTime, 1) || lstrlenW(wszTime) != cchMAPITime) + // return false; + // AssertTag(!SafeIsBadWritePtr(psystime, sizeof(SYSTEMTIME)), 0x0004289a /* tag_abc80 */); + // memset(psystime, 0, sizeof(SYSTEMTIME)); + + psystime->wYear = (WORD)_wtoi(wszTime); + psystime->wMonth = (WORD)_wtoi(wszTime+5); + psystime->wDay = (WORD)_wtoi(wszTime+8); + psystime->wHour = (WORD)_wtoi(wszTime+11); + psystime->wMinute = (WORD)_wtoi(wszTime+14); + return true; +} + +/** +* Negative Case - Anti-pattern 1: [year ±n, month, day] +* Modification of SYSTEMTIME struct by copying from another struct, but no arithmetic is performed. +*/ +bool +ATime::HrGetSysTime( + SYSTEMTIME *pst + ) const +{ + // if (!FValidSysTime()) + // { + // TrapSzTag("ATime cannot be converted to SYSTEMTIME", 0x1e14f5c3 /* tag_4fpxd */); + // CORgTag(E_FAIL, 0x6c373230 /* tag_l720 */); + // } + + // pst->wYear = static_cast(m_lYear); + // pst->wMonth = static_cast(m_lMonth); + // //pst->wDayOfWeek = ???; + // pst->wDay = static_cast(m_lDay); + // pst->wHour = static_cast(m_lHour); + // pst->wMinute = static_cast(m_lMinute); + // pst->wSecond = static_cast(m_lSecond); + // pst->wMilliseconds = 0; +} \ No newline at end of file From e63e19b67eb434a168e43bc250dd0d322dba9c4a Mon Sep 17 00:00:00 2001 From: Josh Brown Date: Tue, 23 Dec 2025 05:34:46 -0800 Subject: [PATCH 02/28] Break out query into subcomponents, comments --- ...kedLeapYearAfterYearModificationPrecise.ql | 136 ++++++++++++++++++ .../test.cpp | 4 +- 2 files changed, 137 insertions(+), 3 deletions(-) create mode 100644 cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql diff --git a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql new file mode 100644 index 000000000000..9c74d56d5f7a --- /dev/null +++ b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql @@ -0,0 +1,136 @@ +/** +* @name Year field changed using an arithmetic operation without checking for leap year (AntiPattern 1) +* @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 path-problem +* @problem.severity warning +* @id cpp/microsoft/public/leap-year/unchecked-after-arithmetic-year-modification-precise +* @precision medium +* @tags leap-year +* correctness +*/ + +import cpp +import LeapYear + +/** + * The set of expressions which are ignorable; either because they seem to not be part of a year mutation, + * or because they seem to be a conversion pattern of mapping date scalars. + */ +abstract class IgnorableOperationSource extends Expr {} + +class IgnorableExprAssignRem extends IgnorableOperationSource instanceof AssignRemExpr{} +class IgnorableExprRem extends IgnorableOperationSource instanceof RemExpr{} +class IgnorableExprUnaryMinus extends IgnorableOperationSource instanceof UnaryMinusExpr{} +/** + * Ignore any operation that is nested inside a larger operation, assume the larger operation is the real source + * */ +class IgnorableExprNestedExpr extends IgnorableOperationSource instanceof BinaryArithmeticOperation{ + IgnorableExprNestedExpr(){ + exists(BinaryArithmeticOperation parentOp | + parentOp.getAChild+() = this + ) + } +} + +/** + * Anything involving a sub expression with char literal 48, ignore as a likely string conversion + */ +class IgnorableExpr48Mapping extends IgnorableOperationSource{ + IgnorableExpr48Mapping(){ + exists(SubExpr child | this.(Operation).getAChild*() = child | + child.getRightOperand().(Literal).getValue().toInt() = 48 + or + child.getRightOperand().(CharLiteral).getValue().toInt() = 48 + ) + } +} + +/** + * if doing any kind of operation involving multiplying 10, 100, 1000, etc., likely some kind of conversion + * and ignorable + */ +class IgnorableExpr10MulipleComponent extends IgnorableOperationSource{ + IgnorableExpr10MulipleComponent(){ + this.(Operation).getAChild*().(MulExpr).getAnOperand().(Literal).getValue().toInt() % 10 = 0 + or + this.(AssignMulExpr).getRValue().(Literal).getValue().toInt() % 10 = 0 + } +} + +/* + * linux time conversions expect the year to start from 1900, so subtracting or + * adding 1900 or anything involving 1900 as a generalization is probably + * a conversion that is ignorable + * */ +class IgnorableExprExpr1900Mapping extends IgnorableOperationSource instanceof Operation{ + IgnorableExprExpr1900Mapping() { + this.(Operation).getAnOperand().getAChild*().(Literal).getValue().toInt() = 1900 + } +} + +class OperationSource extends Expr { + OperationSource() { + ( + this instanceof BinaryArithmeticOperation + or + this instanceof UnaryArithmeticOperation + or + exists(AssignArithmeticOperation a | a.getRValue() = this) + ) and + not this instanceof IgnorableOperationSource + } +} + +/** + * An assignment to a year field, which will be a sink + */ +abstract class YearFieldAssignment extends Expr{} + +/** + * An assignment to x ie `x = a`. + */ +class YearFieldAssignmentAccess extends YearFieldAssignment{ + YearFieldAssignmentAccess(){ + // TODO: why can't I make this just the L value? Doesn't seem to work + exists(Assignment a | + a.getLValue() instanceof YearFieldAccess and + a.getRValue() = this + ) + } +} + +/** + * + */ +class YearFieldAssignmentUnary extends YearFieldAssignment instanceof UnaryArithmeticOperation{ + YearFieldAssignmentUnary(){ + this.getOperand() instanceof YearFieldAccess and + } +} + +/** +* A DataFlow configuration for identifying flows from some non trivial access or literal +* to the Year field of a date object. +*/ +module OperationToYearAssignmentConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node n) { n.asExpr() instanceof OperationSource } + + predicate isSink(DataFlow::Node n) { n.asExpr() instanceof YearFieldAssignment } + + predicate isBarrier(DataFlow::Node n) { + exists(ArrayExpr arr | arr.getArrayOffset() = n.asExpr()) + or + n.asExpr().getUnspecifiedType() instanceof PointerType + or + n.asExpr() instanceof IgnorableOperationSource + or + exists(ChecksForLeapYearFunctionCall fc | fc.getAnArgument() = n.asExpr()) + } +} + +module OperationToYearAssignmentFlow = TaintTracking::Global; +import OperationToYearAssignmentFlow::PathGraph + +from OperationToYearAssignmentFlow::PathNode src, OperationToYearAssignmentFlow::PathNode sink +where OperationToYearAssignmentFlow::flowPath(src, sink) +select sink, src, sink, "TEST" \ No newline at end of file diff --git a/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/test.cpp b/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/test.cpp index 9e7858a5faf0..fec991f36bfe 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/test.cpp +++ b/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/test.cpp @@ -1036,9 +1036,7 @@ FMAPITimeToSysTimeW(LPCWSTR wszTime, SYSTEMTIME *psystime) * Modification of SYSTEMTIME struct by copying from another struct, but no arithmetic is performed. */ bool -ATime::HrGetSysTime( - SYSTEMTIME *pst - ) const +ATime_HrGetSysTime(SYSTEMTIME *pst) { // if (!FValidSysTime()) // { From 7b5163c80898e057361b6fbbde0c10b1921ad1a1 Mon Sep 17 00:00:00 2001 From: Josh Brown Date: Tue, 23 Dec 2025 06:03:25 -0800 Subject: [PATCH 03/28] init precise version --- .../UncheckedLeapYearAfterYearModification.ql | 3 +- ...kedLeapYearAfterYearModificationPrecise.ql | 39 ++++++++++++++++++- 2 files changed, 38 insertions(+), 4 deletions(-) diff --git a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification.ql b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification.ql index 452824f684e2..38091cc23358 100644 --- a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification.ql +++ b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification.ql @@ -62,8 +62,7 @@ abstract class YearWriteOp extends Operation { or // If there is a successor or predecessor that sets the month or day to a fixed value exists(FieldAccess mfa, AssignExpr ae, int val | - mfa instanceof MonthFieldAccess or mfa instanceof DayFieldAccess - | + (mfa instanceof MonthFieldAccess or mfa instanceof DayFieldAccess) and mfa.getQualifier() = var.getAnAccess() and mfa.isModified() and ( diff --git a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql index 9c74d56d5f7a..4412556f6e58 100644 --- a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql +++ b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql @@ -100,11 +100,46 @@ class YearFieldAssignmentAccess extends YearFieldAssignment{ } /** - * + * A year field assignment, by a unary operator ie `x++`. */ class YearFieldAssignmentUnary extends YearFieldAssignment instanceof UnaryArithmeticOperation{ YearFieldAssignmentUnary(){ - this.getOperand() instanceof YearFieldAccess and + this.getOperand() instanceof YearFieldAccess + } +} + +class MonthOrDayFieldAccess extends FieldAccess{ + MonthOrDayFieldAccess(){ + this instanceof MonthFieldAccess + or + this instanceof DayFieldAccess + } +} + +/** + * A static assignment to the day or month access + * + * ``` + * a.day = 28; + * ``` + */ +class GoodExpr extends YearFieldAssignment{ + GoodExpr(){ + exists(Variable var, VariableAccess va, YearFieldAccess yfa | + // yfa = this.getYearAccess() and + yfa.getQualifier() = va and + var.getAnAccess() = va and + exists(MonthOrDayFieldAccess mfa, AssignExpr ae, int val | + mfa.getQualifier() = var.getAnAccess() and + mfa.isModified() and + ( + mfa.getBasicBlock() = yfa.getBasicBlock().getASuccessor*() or + yfa.getBasicBlock() = mfa.getBasicBlock().getASuccessor+() + ) and + ae = mfa.getEnclosingElement() and + ae.getAnOperand().getValue().toInt() = val + ) + ) } } From 7649370e5510f579ba24b05907d32fc3a8116cf9 Mon Sep 17 00:00:00 2001 From: Josh Brown Date: Tue, 23 Dec 2025 06:20:48 -0800 Subject: [PATCH 04/28] Test case qlref --- ...UncheckedLeapYearAfterYearModificationPrecise.expected | 8 ++++++++ .../UncheckedLeapYearAfterYearModificationPrecise.qlref | 1 + 2 files changed, 9 insertions(+) create mode 100644 cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/UncheckedLeapYearAfterYearModificationPrecise.expected create mode 100644 cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/UncheckedLeapYearAfterYearModificationPrecise.qlref diff --git a/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/UncheckedLeapYearAfterYearModificationPrecise.expected b/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/UncheckedLeapYearAfterYearModificationPrecise.expected new file mode 100644 index 000000000000..555002b40867 --- /dev/null +++ b/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/UncheckedLeapYearAfterYearModificationPrecise.expected @@ -0,0 +1,8 @@ +| test.cpp:617:2:617:11 | ... ++ | $@: Field $@ on variable $@ has been modified, but no appropriate check for LeapYear was found. | test.cpp:611:6:611:32 | AntiPattern_1_year_addition | AntiPattern_1_year_addition | test.cpp:12:7:12:11 | wYear | wYear | test.cpp:613:13:613:14 | st | st | +| test.cpp:634:2:634:25 | ... += ... | $@: Field $@ on variable $@ has been modified, but no appropriate check for LeapYear was found. | test.cpp:627:6:627:32 | AntiPattern_simple_addition | AntiPattern_simple_addition | test.cpp:12:7:12:11 | wYear | wYear | test.cpp:629:13:629:14 | st | st | +| test.cpp:763:2:763:19 | ... ++ | $@: Field $@ on variable $@ has been modified, but no appropriate check for LeapYear was found. | test.cpp:756:6:756:40 | AntiPattern_year_addition_struct_tm | AntiPattern_year_addition_struct_tm | test.cpp:56:6:56:12 | tm_year | tm_year | test.cpp:759:12:759:19 | timeinfo | timeinfo | +| test.cpp:800:2:800:40 | ... = ... | $@: Field $@ on variable $@ has been modified, but no appropriate check for LeapYear was found. | test.cpp:791:6:791:23 | FalseNegativeTests | FalseNegativeTests | test.cpp:56:6:56:12 | tm_year | tm_year | test.cpp:793:12:793:19 | timeinfo | timeinfo | +| test.cpp:803:2:803:43 | ... = ... | $@: Field $@ on variable $@ has been modified, but no appropriate check for LeapYear was found. | test.cpp:791:6:791:23 | FalseNegativeTests | FalseNegativeTests | test.cpp:56:6:56:12 | tm_year | tm_year | test.cpp:793:12:793:19 | timeinfo | timeinfo | +| test.cpp:808:2:808:24 | ... = ... | $@: Field $@ on variable $@ has been modified, but no appropriate check for LeapYear was found. | test.cpp:791:6:791:23 | FalseNegativeTests | FalseNegativeTests | test.cpp:12:7:12:11 | wYear | wYear | test.cpp:794:13:794:14 | st | st | +| test.cpp:811:2:811:33 | ... = ... | $@: Field $@ on variable $@ has been modified, but no appropriate check for LeapYear was found. | test.cpp:791:6:791:23 | FalseNegativeTests | FalseNegativeTests | test.cpp:12:7:12:11 | wYear | wYear | test.cpp:794:13:794:14 | st | st | +| test.cpp:850:3:850:36 | ... = ... | $@: Field $@ on variable $@ has been modified, but no appropriate check for LeapYear was found. | test.cpp:818:6:818:23 | tp_intermediaryVar | tp_intermediaryVar | test.cpp:56:6:56:12 | tm_year | tm_year | test.cpp:70:18:70:19 | tm | tm | diff --git a/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/UncheckedLeapYearAfterYearModificationPrecise.qlref b/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/UncheckedLeapYearAfterYearModificationPrecise.qlref new file mode 100644 index 000000000000..17b1bf722ba6 --- /dev/null +++ b/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/UncheckedLeapYearAfterYearModificationPrecise.qlref @@ -0,0 +1 @@ +Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql From c7a6543ed1b8d40d1de3e3ecc30cb601fd202359 Mon Sep 17 00:00:00 2001 From: Josh Brown Date: Fri, 26 Dec 2025 03:41:40 -0800 Subject: [PATCH 05/28] Use Bens version + Autoformat --- ...kedLeapYearAfterYearModificationPrecise.ql | 283 ++++++++++++------ 1 file changed, 198 insertions(+), 85 deletions(-) diff --git a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql index 4412556f6e58..f8ab6592e6e8 100644 --- a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql +++ b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql @@ -16,137 +16,215 @@ import LeapYear * The set of expressions which are ignorable; either because they seem to not be part of a year mutation, * or because they seem to be a conversion pattern of mapping date scalars. */ -abstract class IgnorableOperationSource extends Expr {} +abstract class IgnorableOperation extends Expr { } -class IgnorableExprAssignRem extends IgnorableOperationSource instanceof AssignRemExpr{} -class IgnorableExprRem extends IgnorableOperationSource instanceof RemExpr{} -class IgnorableExprUnaryMinus extends IgnorableOperationSource instanceof UnaryMinusExpr{} -/** - * Ignore any operation that is nested inside a larger operation, assume the larger operation is the real source - * */ -class IgnorableExprNestedExpr extends IgnorableOperationSource instanceof BinaryArithmeticOperation{ - IgnorableExprNestedExpr(){ - exists(BinaryArithmeticOperation parentOp | - parentOp.getAChild+() = this - ) - } +class IgnorableExprAssignRem extends IgnorableOperation instanceof AssignRemExpr { } + +class IgnorableExprRem extends IgnorableOperation instanceof RemExpr { } + +class IgnorableExprUnaryMinus extends IgnorableOperation instanceof UnaryMinusExpr { } + +class IgnorableNonPlusMinusOperation extends IgnorableOperation instanceof Operation { + IgnorableNonPlusMinusOperation() { not isOperationSourceCandidate(this) } +} + +class IgnorableNonPlusMinusAssignment extends IgnorableOperation instanceof AssignArithmeticOperation +{ + IgnorableNonPlusMinusAssignment() { not isOperationSourceCandidate(this) } } /** * Anything involving a sub expression with char literal 48, ignore as a likely string conversion */ -class IgnorableExpr48Mapping extends IgnorableOperationSource{ - IgnorableExpr48Mapping(){ - exists(SubExpr child | this.(Operation).getAChild*() = child | - child.getRightOperand().(Literal).getValue().toInt() = 48 - or - child.getRightOperand().(CharLiteral).getValue().toInt() = 48 - ) - } +class IgnorableExpr48Mapping extends IgnorableOperation { + IgnorableExpr48Mapping() { + exists(SubExpr child | this.(Operation).getAChild*() = child | + child.getRightOperand().(Literal).getValue().toInt() = 48 + or + child.getRightOperand().(CharLiteral).getValue().toInt() = 48 + ) + } } /** * if doing any kind of operation involving multiplying 10, 100, 1000, etc., likely some kind of conversion * and ignorable */ -class IgnorableExpr10MulipleComponent extends IgnorableOperationSource{ - IgnorableExpr10MulipleComponent(){ - this.(Operation).getAChild*().(MulExpr).getAnOperand().(Literal).getValue().toInt() % 10 = 0 - or - this.(AssignMulExpr).getRValue().(Literal).getValue().toInt() % 10 = 0 - } +class IgnorableExpr10MulipleComponent extends IgnorableOperation { + IgnorableExpr10MulipleComponent() { + this.(Operation).getAChild*().(MulExpr).getAnOperand().(Literal).getValue().toInt() in [ + 10, 100, 100 + ] + or + exists(AssignMulExpr a | this = a or a.getRValue() = this | + a.getRValue().getAChild*().(Literal).getValue().toInt() in [10, 100, 100] + ) + } } /* * linux time conversions expect the year to start from 1900, so subtracting or * adding 1900 or anything involving 1900 as a generalization is probably * a conversion that is ignorable - * */ -class IgnorableExprExpr1900Mapping extends IgnorableOperationSource instanceof Operation{ + */ + +class IgnorableExprExpr1900Mapping extends IgnorableOperation { IgnorableExprExpr1900Mapping() { this.(Operation).getAnOperand().getAChild*().(Literal).getValue().toInt() = 1900 + or + exists(AssignArithmeticOperation a | this = a or this = a.getRValue() | + a.getRValue().getAChild*().(Literal).getValue().toInt() = 1900 + ) + // or + // this.(Literal).getValue().toInt() = 1900 } } +predicate isOperationSourceCandidate(Expr e) { + e instanceof SubExpr + or + e instanceof AddExpr + or + e instanceof CrementOperation + or + exists(AssignSubExpr a | a.getRValue() = e) + or + exists(AssignAddExpr a | a.getRValue() = e) +} + class OperationSource extends Expr { OperationSource() { - ( - this instanceof BinaryArithmeticOperation - or - this instanceof UnaryArithmeticOperation - or - exists(AssignArithmeticOperation a | a.getRValue() = this) - ) and - not this instanceof IgnorableOperationSource + // operation source candidates that aren't nested inside other operation source candidates + isOperationSourceCandidate(this) and + not exists(Expr parent | isOperationSourceCandidate(parent) | parent.getAChild+() = this) and + not this instanceof IgnorableOperation and + not this.getEnclosingFunction() instanceof IgnorableFunction } } /** * An assignment to a year field, which will be a sink + * NOTE: could not make this abstract, it was binding to all expressions */ -abstract class YearFieldAssignment extends Expr{} +abstract class YearFieldAssignment extends Expr { + abstract YearFieldAccess getYearFieldAccess(); +} /** * An assignment to x ie `x = a`. */ -class YearFieldAssignmentAccess extends YearFieldAssignment{ - YearFieldAssignmentAccess(){ - // TODO: why can't I make this just the L value? Doesn't seem to work - exists(Assignment a | - a.getLValue() instanceof YearFieldAccess and - a.getRValue() = this - ) - } +class YearFieldAssignmentAccess extends YearFieldAssignment { + YearFieldAccess access; + + YearFieldAssignmentAccess() { + // TODO: why can't I make this just the L value? Doesn't seem to work + exists(Assignment a | + a.getLValue() = access and + a.getRValue() = this + ) + } + + override YearFieldAccess getYearFieldAccess() { result = access } } /** * A year field assignment, by a unary operator ie `x++`. */ -class YearFieldAssignmentUnary extends YearFieldAssignment instanceof UnaryArithmeticOperation{ - YearFieldAssignmentUnary(){ - this.getOperand() instanceof YearFieldAccess - } +class YearFieldAssignmentUnary extends YearFieldAssignment instanceof CrementOperation { + YearFieldAccess access; + + YearFieldAssignmentUnary() { this.getOperand() = access } + + override YearFieldAccess getYearFieldAccess() { result = access } } -class MonthOrDayFieldAccess extends FieldAccess{ - MonthOrDayFieldAccess(){ - this instanceof MonthFieldAccess - or - this instanceof DayFieldAccess - } +/** + * An access to either a Month or Day. + */ +class MonthOrDayFieldAccess extends FieldAccess { + MonthOrDayFieldAccess() { + this instanceof MonthFieldAccess + or + this instanceof DayFieldAccess + } } /** - * A static assignment to the day or month access - * - * ``` - * a.day = 28; - * ``` + * Flow for non operation candidate sources to year assignments to detect + * ignorable functions. + * If the ignorable operation flows to a year assignment and the source and sink + * are in the same function, we will consider that function ignorable. */ -class GoodExpr extends YearFieldAssignment{ - GoodExpr(){ - exists(Variable var, VariableAccess va, YearFieldAccess yfa | - // yfa = this.getYearAccess() and - yfa.getQualifier() = va and - var.getAnAccess() = va and - exists(MonthOrDayFieldAccess mfa, AssignExpr ae, int val | - mfa.getQualifier() = var.getAnAccess() and - mfa.isModified() and - ( - mfa.getBasicBlock() = yfa.getBasicBlock().getASuccessor*() or - yfa.getBasicBlock() = mfa.getBasicBlock().getASuccessor+() - ) and - ae = mfa.getEnclosingElement() and - ae.getAnOperand().getValue().toInt() = val - ) - ) - } +module IgnorableOperationToYearConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node n) { + ( + n.asExpr() instanceof Operation or + n.asExpr() instanceof AssignArithmeticOperation + ) and + not isOperationSourceCandidate(n.asExpr()) + } + + predicate isSink(DataFlow::Node n) { + n.asExpr() instanceof YearFieldAssignment + or + n.asExpr() instanceof YearFieldAccess + } + + // looking for sources and sinks in the same function + DataFlow::FlowFeature getAFeature() { + result instanceof DataFlow::FeatureEqualSourceSinkCallContext + } + + predicate isBarrierOut(DataFlow::Node n) { isSink(n) } +} + +module IgnorableOperationToYearFlow = TaintTracking::Global; + +module YearToIgnorableOperationConfig implements DataFlow::ConfigSig { + predicate isSink(DataFlow::Node n) { + ( + n.asExpr() instanceof Operation or + n.asExpr() instanceof AssignArithmeticOperation + ) and + not isOperationSourceCandidate(n.asExpr()) + } + + predicate isSource(DataFlow::Node n) { + n.asExpr() instanceof YearFieldAssignment + or + n.asExpr() instanceof YearFieldAccess + } + + // looking for sources and sinks in the same function + DataFlow::FlowFeature getAFeature() { + result instanceof DataFlow::FeatureEqualSourceSinkCallContext + } + + predicate isBarrierOut(DataFlow::Node n) { isSink(n) } +} + +module YearToIgnorableOperationFlow = TaintTracking::Global; + +class IgnorableFunction extends Function { + IgnorableFunction() { isIgnorableFunction(this) } +} + +predicate isIgnorableFunction(Function f) { + exists(IgnorableOperationToYearFlow::PathNode sink | + sink.getNode().asExpr().getEnclosingFunction() = f and + sink.isSink() + ) + or + exists(YearToIgnorableOperationFlow::PathNode sink | + sink.getNode().asExpr().getEnclosingFunction() = f and + sink.isSink() + ) } /** -* A DataFlow configuration for identifying flows from some non trivial access or literal -* to the Year field of a date object. -*/ + * A DataFlow configuration for identifying flows from some non trivial access or literal + * to the Year field of a date object. + */ module OperationToYearAssignmentConfig implements DataFlow::ConfigSig { predicate isSource(DataFlow::Node n) { n.asExpr() instanceof OperationSource } @@ -157,15 +235,50 @@ module OperationToYearAssignmentConfig implements DataFlow::ConfigSig { or n.asExpr().getUnspecifiedType() instanceof PointerType or - n.asExpr() instanceof IgnorableOperationSource + n.asExpr() instanceof IgnorableOperation or exists(ChecksForLeapYearFunctionCall fc | fc.getAnArgument() = n.asExpr()) + or + n.asExpr().getEnclosingFunction() instanceof IgnorableFunction } } module OperationToYearAssignmentFlow = TaintTracking::Global; + +predicate isYearModifiedWithCheck(YearFieldAccess fa) { + // If there is a local check for leap year after the modification + exists(LeapYearFieldAccess yfacheck, Variable var, YearFieldAccess yfa | + // TODO: cleanup + yfa = fa and + var.getAnAccess() = fa.getQualifier() and + yfacheck.getQualifier() = var.getAnAccess() and + yfacheck.isUsedInCorrectLeapYearCheck() and + yfacheck.getBasicBlock() = yfa.getBasicBlock().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, Variable var | + var.getAnAccess() = fa.getQualifier() and + source = var.getAnAccess() and + LeapYearCheckFlow::flow(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, Variable var + | + // TODO: cleanup + var.getAnAccess() = fa.getQualifier() and + vacheck = var.getAnAccess() and + yfacheck.getQualifier() = vacheck and + LeapYearCheckFlow::flow(DataFlow::exprNode(yfacheck), DataFlow::exprNode(fc.getAnArgument())) + ) +} + import OperationToYearAssignmentFlow::PathGraph from OperationToYearAssignmentFlow::PathNode src, OperationToYearAssignmentFlow::PathNode sink -where OperationToYearAssignmentFlow::flowPath(src, sink) -select sink, src, sink, "TEST" \ No newline at end of file +where + OperationToYearAssignmentFlow::flowPath(src, sink) and + not isYearModifiedWithCheck(sink.getNode().asExpr().(YearFieldAssignment).getYearFieldAccess()) +select sink, src, sink, "TEST" From f6f63cbd54d67e00383380f247fb0f86a2150618 Mon Sep 17 00:00:00 2001 From: Josh Brown Date: Fri, 26 Dec 2025 03:50:20 -0800 Subject: [PATCH 06/28] Refactoring common class between dataflow --- ...kedLeapYearAfterYearModificationPrecise.ql | 37 ++++++++++++------- 1 file changed, 23 insertions(+), 14 deletions(-) diff --git a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql index f8ab6592e6e8..1286cdc4c5dc 100644 --- a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql +++ b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql @@ -149,6 +149,25 @@ class MonthOrDayFieldAccess extends FieldAccess { } } +class OperationNode extends DataFlow::Node{ + OperationNode(){ + this.asExpr() instanceof Operation + or + this.asExpr() instanceof AssignArithmeticOperation + } +} + +/** + * An access or assignment to a Year field. + */ +class YearFieldAccessNode extends DataFlow::Node{ + YearFieldAccessNode(){ + this.asExpr() instanceof YearFieldAssignment + or + this.asExpr() instanceof YearFieldAccess + } +} + /** * Flow for non operation candidate sources to year assignments to detect * ignorable functions. @@ -157,17 +176,12 @@ class MonthOrDayFieldAccess extends FieldAccess { */ module IgnorableOperationToYearConfig implements DataFlow::ConfigSig { predicate isSource(DataFlow::Node n) { - ( - n.asExpr() instanceof Operation or - n.asExpr() instanceof AssignArithmeticOperation - ) and + n instanceof OperationNode and not isOperationSourceCandidate(n.asExpr()) } predicate isSink(DataFlow::Node n) { - n.asExpr() instanceof YearFieldAssignment - or - n.asExpr() instanceof YearFieldAccess + n instanceof YearFieldAccessNode } // looking for sources and sinks in the same function @@ -182,17 +196,12 @@ module IgnorableOperationToYearFlow = TaintTracking::Global Date: Fri, 26 Dec 2025 04:23:25 -0800 Subject: [PATCH 07/28] Hashcons definition of exprEq_propertyPermissive --- cpp/ql/src/Likely Bugs/Leap Year/LeapYear.qll | 38 ++----------------- ...kedLeapYearAfterYearModificationPrecise.ql | 21 +++++----- 2 files changed, 16 insertions(+), 43 deletions(-) diff --git a/cpp/ql/src/Likely Bugs/Leap Year/LeapYear.qll b/cpp/ql/src/Likely Bugs/Leap Year/LeapYear.qll index 06b6aff66abd..f100f63914cb 100644 --- a/cpp/ql/src/Likely Bugs/Leap Year/LeapYear.qll +++ b/cpp/ql/src/Likely Bugs/Leap Year/LeapYear.qll @@ -5,6 +5,7 @@ import cpp import semmle.code.cpp.dataflow.new.TaintTracking import semmle.code.cpp.commons.DateTime +import semmle.code.cpp.valuenumbering.HashCons /** * Get the top-level `BinaryOperation` enclosing the expression e. @@ -63,42 +64,11 @@ Expr moduloCheckNEQ_0(NEExpr neq, int modVal) { * Returns if the two expressions resolve to the same value, albeit it is a fuzzy attempt. * SSA is not fit for purpose here as calls break SSA equivalence. */ +bindingset[e1,e2] +pragma[inline_late] predicate exprEq_propertyPermissive(Expr e1, Expr e2) { not e1 = e2 and - ( - DataFlow::localFlow(DataFlow::exprNode(e1), DataFlow::exprNode(e2)) - or - if e1 instanceof ThisExpr and e2 instanceof ThisExpr - then any() - else - /* If it's a direct Access, check that the target is the same. */ - if e1 instanceof Access - then e1.(Access).getTarget() = e2.(Access).getTarget() - else - /* If it's a Call, compare qualifiers and only permit no-argument Calls. */ - if e1 instanceof Call - then - e1.(Call).getTarget() = e2.(Call).getTarget() and - e1.(Call).getNumberOfArguments() = 0 and - e2.(Call).getNumberOfArguments() = 0 and - if e1.(Call).hasQualifier() - then exprEq_propertyPermissive(e1.(Call).getQualifier(), e2.(Call).getQualifier()) - else any() - else - /* If it's a binaryOperation, compare op and recruse */ - if e1 instanceof BinaryOperation - then - e1.(BinaryOperation).getOperator() = e2.(BinaryOperation).getOperator() and - exprEq_propertyPermissive(e1.(BinaryOperation).getLeftOperand(), - e2.(BinaryOperation).getLeftOperand()) and - exprEq_propertyPermissive(e1.(BinaryOperation).getRightOperand(), - e2.(BinaryOperation).getRightOperand()) - else - // Otherwise fail (and permit the raising of a finding) - if e1 instanceof Literal - then e1.(Literal).getValue() = e2.(Literal).getValue() - else none() - ) + hashCons(e1) = hashCons(e2) } /** diff --git a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql index 1286cdc4c5dc..9735c141e8b5 100644 --- a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql +++ b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql @@ -138,17 +138,20 @@ class YearFieldAssignmentUnary extends YearFieldAssignment instanceof CrementOpe override YearFieldAccess getYearFieldAccess() { result = access } } +// * +// An access to either a Month or Day. +// / +// class MonthOrDayFieldAccess extends FieldAccess { +// MonthOrDayFieldAccess() { +// this instanceof MonthFieldAccess +// or +// this instanceof DayFieldAccess +// } +// } + /** - * An access to either a Month or Day. + * An operation of interest to source from */ -class MonthOrDayFieldAccess extends FieldAccess { - MonthOrDayFieldAccess() { - this instanceof MonthFieldAccess - or - this instanceof DayFieldAccess - } -} - class OperationNode extends DataFlow::Node{ OperationNode(){ this.asExpr() instanceof Operation From ca9f66c9f13b8967d1c734918d3cbefc5e0140cf Mon Sep 17 00:00:00 2001 From: "REDMOND\\brodes" Date: Mon, 29 Dec 2025 14:37:56 -0500 Subject: [PATCH 08/28] Misc. updates. Removed the ignorable function mechanic, and switched to a more precise ignorable operation analysis. Ignorable operations that flow to a possible source also invalidate that source. Also added a root source finder to get the earliest source if many exist. Modified the leap year checker finder to use a new dataflow mechanic that flows from a YearFieldAccess. --- cpp/ql/src/Likely Bugs/Leap Year/LeapYear.qll | 4 +- ...kedLeapYearAfterYearModificationPrecise.ql | 321 ++++++++---------- 2 files changed, 146 insertions(+), 179 deletions(-) diff --git a/cpp/ql/src/Likely Bugs/Leap Year/LeapYear.qll b/cpp/ql/src/Likely Bugs/Leap Year/LeapYear.qll index f100f63914cb..ce0e236f394a 100644 --- a/cpp/ql/src/Likely Bugs/Leap Year/LeapYear.qll +++ b/cpp/ql/src/Likely Bugs/Leap Year/LeapYear.qll @@ -64,7 +64,7 @@ Expr moduloCheckNEQ_0(NEExpr neq, int modVal) { * Returns if the two expressions resolve to the same value, albeit it is a fuzzy attempt. * SSA is not fit for purpose here as calls break SSA equivalence. */ -bindingset[e1,e2] +bindingset[e1, e2] pragma[inline_late] predicate exprEq_propertyPermissive(Expr e1, Expr e2) { not e1 = e2 and @@ -395,7 +395,7 @@ class ChecksForLeapYearFunctionCall extends FunctionCall { } /** - * A `DataFlow` configuraiton for finding a variable access that would flow into + * A `DataFlow` configuration for finding a variable access that would flow into * a function call that includes an operation to check for leap year. */ private module LeapYearCheckFlowConfig implements DataFlow::ConfigSig { diff --git a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql index 9735c141e8b5..b25f453a5f7c 100644 --- a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql +++ b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql @@ -1,13 +1,13 @@ /** -* @name Year field changed using an arithmetic operation without checking for leap year (AntiPattern 1) -* @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 path-problem -* @problem.severity warning -* @id cpp/microsoft/public/leap-year/unchecked-after-arithmetic-year-modification-precise -* @precision medium -* @tags leap-year -* correctness -*/ + * @name Year field changed using an arithmetic operation without checking for leap year (AntiPattern 1) + * @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 path-problem + * @problem.severity warning + * @id cpp/microsoft/public/leap-year/unchecked-after-arithmetic-year-modification-precise + * @precision medium + * @tags leap-year + * correctness + */ import cpp import LeapYear @@ -18,47 +18,30 @@ import LeapYear */ abstract class IgnorableOperation extends Expr { } -class IgnorableExprAssignRem extends IgnorableOperation instanceof AssignRemExpr { } - class IgnorableExprRem extends IgnorableOperation instanceof RemExpr { } -class IgnorableExprUnaryMinus extends IgnorableOperation instanceof UnaryMinusExpr { } - -class IgnorableNonPlusMinusOperation extends IgnorableOperation instanceof Operation { - IgnorableNonPlusMinusOperation() { not isOperationSourceCandidate(this) } -} - -class IgnorableNonPlusMinusAssignment extends IgnorableOperation instanceof AssignArithmeticOperation -{ - IgnorableNonPlusMinusAssignment() { not isOperationSourceCandidate(this) } -} - /** * Anything involving a sub expression with char literal 48, ignore as a likely string conversion */ -class IgnorableExpr48Mapping extends IgnorableOperation { - IgnorableExpr48Mapping() { - exists(SubExpr child | this.(Operation).getAChild*() = child | - child.getRightOperand().(Literal).getValue().toInt() = 48 - or - child.getRightOperand().(CharLiteral).getValue().toInt() = 48 +class IgnorableExpr10MulipleComponent extends IgnorableOperation { + IgnorableExpr10MulipleComponent() { + this.(MulExpr).getAnOperand().(Literal).getValue().toInt() in [10, 100, 100] + or + exists(AssignMulExpr a | a.getRValue() = this | + a.getRValue().(Literal).getValue().toInt() in [10, 100, 100] ) } } /** - * if doing any kind of operation involving multiplying 10, 100, 1000, etc., likely some kind of conversion - * and ignorable + * Anything involving a sub expression with char literal 48, ignore as a likely string conversion + * e.g., X - '0' */ -class IgnorableExpr10MulipleComponent extends IgnorableOperation { - IgnorableExpr10MulipleComponent() { - this.(Operation).getAChild*().(MulExpr).getAnOperand().(Literal).getValue().toInt() in [ - 10, 100, 100 - ] +class IgnorableExpr48Mapping extends IgnorableOperation { + IgnorableExpr48Mapping() { + this.(SubExpr).getRightOperand().(Literal).getValue().toInt() = 48 or - exists(AssignMulExpr a | this = a or a.getRValue() = this | - a.getRValue().getAChild*().(Literal).getValue().toInt() in [10, 100, 100] - ) + exists(AssignSubExpr e | e.getRValue() = this | e.getRValue().(Literal).getValue().toInt() = 48) } } @@ -70,35 +53,78 @@ class IgnorableExpr10MulipleComponent extends IgnorableOperation { class IgnorableExprExpr1900Mapping extends IgnorableOperation { IgnorableExprExpr1900Mapping() { - this.(Operation).getAnOperand().getAChild*().(Literal).getValue().toInt() = 1900 + this.(Operation).getAnOperand().(Literal).getValue().toInt() = 1900 or - exists(AssignArithmeticOperation a | this = a or this = a.getRValue() | - a.getRValue().getAChild*().(Literal).getValue().toInt() = 1900 + exists(AssignArithmeticOperation a | this = a.getRValue() | + a.getRValue().(Literal).getValue().toInt() = 1900 ) - // or - // this.(Literal).getValue().toInt() = 1900 } } +class IgnorableBinaryBitwiseOperation extends IgnorableOperation instanceof BinaryBitwiseOperation { +} + +class IgnorableUnaryBitwiseOperation extends IgnorableOperation instanceof UnaryBitwiseOperation { } + +class IgnorableAssignmentBitwiseOperation extends IgnorableOperation instanceof AssignBitwiseOperation +{ } + predicate isOperationSourceCandidate(Expr e) { - e instanceof SubExpr - or - e instanceof AddExpr - or - e instanceof CrementOperation - or - exists(AssignSubExpr a | a.getRValue() = e) - or - exists(AssignAddExpr a | a.getRValue() = e) + not e instanceof IgnorableOperation and + ( + e instanceof SubExpr + or + e instanceof AddExpr + or + e instanceof CrementOperation + or + exists(AssignSubExpr a | a.getRValue() = e) + or + exists(AssignAddExpr a | a.getRValue() = e) + ) } +module OperationSourceCandidateToIgnorableOperationConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node n) { isOperationSourceCandidate(n.asExpr()) } + + predicate isSink(DataFlow::Node n) { n.asExpr() instanceof IgnorableOperation } + + // looking for sources and sinks in the same function + DataFlow::FlowFeature getAFeature() { + result instanceof DataFlow::FeatureEqualSourceSinkCallContext + } + + predicate isBarrierOut(DataFlow::Node n) { isSink(n) } +} + +module OperationSourceCandidateToIgnorableOperationFlow = + TaintTracking::Global; + +module IgnorableOperationToOperationSourceCandidateConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node n) { n.asExpr() instanceof IgnorableOperation } + + predicate isSink(DataFlow::Node n) { isOperationSourceCandidate(n.asExpr()) } + + // looking for sources and sinks in the same function + DataFlow::FlowFeature getAFeature() { + result instanceof DataFlow::FeatureEqualSourceSinkCallContext + } +} + +module IgnorableOperationToOperationSourceCandidateFlow = + TaintTracking::Global; + class OperationSource extends Expr { OperationSource() { - // operation source candidates that aren't nested inside other operation source candidates isOperationSourceCandidate(this) and - not exists(Expr parent | isOperationSourceCandidate(parent) | parent.getAChild+() = this) and - not this instanceof IgnorableOperation and - not this.getEnclosingFunction() instanceof IgnorableFunction + not exists(OperationSourceCandidateToIgnorableOperationFlow::PathNode src | + src.getNode().asExpr() = this and + src.isSource() + ) and + not exists(IgnorableOperationToOperationSourceCandidateFlow::PathNode sink | + sink.getNode().asExpr() = this and + sink.isSink() + ) } } @@ -138,152 +164,92 @@ class YearFieldAssignmentUnary extends YearFieldAssignment instanceof CrementOpe override YearFieldAccess getYearFieldAccess() { result = access } } -// * -// An access to either a Month or Day. -// / -// class MonthOrDayFieldAccess extends FieldAccess { -// MonthOrDayFieldAccess() { -// this instanceof MonthFieldAccess -// or -// this instanceof DayFieldAccess -// } -// } - -/** - * An operation of interest to source from - */ -class OperationNode extends DataFlow::Node{ - OperationNode(){ - this.asExpr() instanceof Operation - or - this.asExpr() instanceof AssignArithmeticOperation - } -} - -/** - * An access or assignment to a Year field. - */ -class YearFieldAccessNode extends DataFlow::Node{ - YearFieldAccessNode(){ - this.asExpr() instanceof YearFieldAssignment - or - this.asExpr() instanceof YearFieldAccess - } -} - /** - * Flow for non operation candidate sources to year assignments to detect - * ignorable functions. - * If the ignorable operation flows to a year assignment and the source and sink - * are in the same function, we will consider that function ignorable. + * A DataFlow configuration for identifying flows from some non trivial access or literal + * to the Year field of a date object. */ -module IgnorableOperationToYearConfig implements DataFlow::ConfigSig { - predicate isSource(DataFlow::Node n) { - n instanceof OperationNode and - not isOperationSourceCandidate(n.asExpr()) - } +module OperationToYearAssignmentConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node n) { n.asExpr() instanceof OperationSource } - predicate isSink(DataFlow::Node n) { - n instanceof YearFieldAccessNode - } + predicate isSink(DataFlow::Node n) { n.asExpr() instanceof YearFieldAssignment } - // looking for sources and sinks in the same function - DataFlow::FlowFeature getAFeature() { - result instanceof DataFlow::FeatureEqualSourceSinkCallContext + predicate isBarrier(DataFlow::Node n) { + exists(ArrayExpr arr | arr.getArrayOffset() = n.asExpr()) + or + n.asExpr().getUnspecifiedType() instanceof PointerType + or + n.asExpr() instanceof IgnorableOperation } - - predicate isBarrierOut(DataFlow::Node n) { isSink(n) } } -module IgnorableOperationToYearFlow = TaintTracking::Global; - -module YearToIgnorableOperationConfig implements DataFlow::ConfigSig { - predicate isSink(DataFlow::Node n) { - n instanceof OperationNode and - not isOperationSourceCandidate(n.asExpr()) - } +module OperationToYearAssignmentFlow = TaintTracking::Global; +module KnownYearOpSourceToKnownYearOpSourceConfig implements DataFlow::ConfigSig { predicate isSource(DataFlow::Node n) { - n instanceof YearFieldAccessNode + exists(OperationToYearAssignmentFlow::PathNode src | + src.getNode().asExpr() = n.asExpr() and + src.isSource() + ) } - // looking for sources and sinks in the same function - DataFlow::FlowFeature getAFeature() { - result instanceof DataFlow::FeatureEqualSourceSinkCallContext + predicate isSink(DataFlow::Node n) { + exists(OperationToYearAssignmentFlow::PathNode src | + src.getNode().asExpr() = n.asExpr() and + src.isSource() + ) } - - predicate isBarrierOut(DataFlow::Node n) { isSink(n) } } -module YearToIgnorableOperationFlow = TaintTracking::Global; - -class IgnorableFunction extends Function { - IgnorableFunction() { isIgnorableFunction(this) } -} +module KnownYearOpSourceToKnownYearOpSourceFlow = + TaintTracking::Global; -predicate isIgnorableFunction(Function f) { - exists(IgnorableOperationToYearFlow::PathNode sink | - sink.getNode().asExpr().getEnclosingFunction() = f and - sink.isSink() - ) - or - exists(YearToIgnorableOperationFlow::PathNode sink | - sink.getNode().asExpr().getEnclosingFunction() = f and - sink.isSink() +predicate isRootOperationSource(OperationSource e) { + not exists(DataFlow::Node src, DataFlow::Node sink | + src != sink and + KnownYearOpSourceToKnownYearOpSourceFlow::flow(src, sink) and + sink.asExpr() = e ) } -/** - * A DataFlow configuration for identifying flows from some non trivial access or literal - * to the Year field of a date object. - */ -module OperationToYearAssignmentConfig implements DataFlow::ConfigSig { - predicate isSource(DataFlow::Node n) { n.asExpr() instanceof OperationSource } +module YearFieldAccessToLeapYearCheckConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node source) { source.asExpr() instanceof YearFieldAccess } - predicate isSink(DataFlow::Node n) { n.asExpr() instanceof YearFieldAssignment } - - predicate isBarrier(DataFlow::Node n) { - exists(ArrayExpr arr | arr.getArrayOffset() = n.asExpr()) - or - n.asExpr().getUnspecifiedType() instanceof PointerType + predicate isSink(DataFlow::Node sink) { + // Local leap year check + sink.asExpr().(LeapYearFieldAccess).isUsedInCorrectLeapYearCheck() or - n.asExpr() instanceof IgnorableOperation - or - exists(ChecksForLeapYearFunctionCall fc | fc.getAnArgument() = n.asExpr()) - or - n.asExpr().getEnclosingFunction() instanceof IgnorableFunction + // passed to function that seems to check for leap year + exists(ChecksForLeapYearFunctionCall fc | + sink.asExpr() = fc.getAnArgument() + or + sink.asExpr() = fc.getAnArgument().(FieldAccess).getQualifier() + or + exists(AddressOfExpr aoe | + fc.getAnArgument() = aoe and + aoe.getOperand() = sink.asExpr() + ) + ) + } + + predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) { + // flow from a YearFieldAccess to the qualifier + node1.asExpr() instanceof YearFieldAccess and + node2.asExpr() = node1.asExpr().(YearFieldAccess).getQualifier() } + // Use this to make the sink for leap year check intra-proc only + // i.e., the sink must be in the same scope (function) as the source. + // DataFlow::FlowFeature getAFeature() { + // result instanceof DataFlow::FeatureEqualSourceSinkCallContext + // } } -module OperationToYearAssignmentFlow = TaintTracking::Global; +module YearFieldAccessToLeapYearCheckFlow = + TaintTracking::Global; predicate isYearModifiedWithCheck(YearFieldAccess fa) { - // If there is a local check for leap year after the modification - exists(LeapYearFieldAccess yfacheck, Variable var, YearFieldAccess yfa | - // TODO: cleanup - yfa = fa and - var.getAnAccess() = fa.getQualifier() and - yfacheck.getQualifier() = var.getAnAccess() and - yfacheck.isUsedInCorrectLeapYearCheck() and - yfacheck.getBasicBlock() = yfa.getBasicBlock().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, Variable var | - var.getAnAccess() = fa.getQualifier() and - source = var.getAnAccess() and - LeapYearCheckFlow::flow(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, Variable var - | - // TODO: cleanup - var.getAnAccess() = fa.getQualifier() and - vacheck = var.getAnAccess() and - yfacheck.getQualifier() = vacheck and - LeapYearCheckFlow::flow(DataFlow::exprNode(yfacheck), DataFlow::exprNode(fc.getAnArgument())) + exists(YearFieldAccessToLeapYearCheckFlow::PathNode src | + src.isSource() and + src.getNode().asExpr() = fa ) } @@ -292,5 +258,6 @@ import OperationToYearAssignmentFlow::PathGraph from OperationToYearAssignmentFlow::PathNode src, OperationToYearAssignmentFlow::PathNode sink where OperationToYearAssignmentFlow::flowPath(src, sink) and + isRootOperationSource(src.getNode().asExpr()) and not isYearModifiedWithCheck(sink.getNode().asExpr().(YearFieldAssignment).getYearFieldAccess()) select sink, src, sink, "TEST" From ec2e5f786d98837eaa82d262a18b0e1a1916747a Mon Sep 17 00:00:00 2001 From: Josh Brown Date: Tue, 30 Dec 2025 02:18:45 -0800 Subject: [PATCH 09/28] Code Commenting --- ...kedLeapYearAfterYearModificationPrecise.ql | 35 ++++++++++++++++--- 1 file changed, 31 insertions(+), 4 deletions(-) diff --git a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql index b25f453a5f7c..cb33b8f017ab 100644 --- a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql +++ b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql @@ -61,14 +61,15 @@ class IgnorableExprExpr1900Mapping extends IgnorableOperation { } } -class IgnorableBinaryBitwiseOperation extends IgnorableOperation instanceof BinaryBitwiseOperation { -} +class IgnorableBinaryBitwiseOperation extends IgnorableOperation instanceof BinaryBitwiseOperation { } class IgnorableUnaryBitwiseOperation extends IgnorableOperation instanceof UnaryBitwiseOperation { } -class IgnorableAssignmentBitwiseOperation extends IgnorableOperation instanceof AssignBitwiseOperation -{ } +class IgnorableAssignmentBitwiseOperation extends IgnorableOperation instanceof AssignBitwiseOperation { } +/** + * An expression that is a candidate source for an dataflow configuration for an Operation that could flow to a Year field. + */ predicate isOperationSourceCandidate(Expr e) { not e instanceof IgnorableOperation and ( @@ -84,6 +85,9 @@ predicate isOperationSourceCandidate(Expr e) { ) } +/** + * A Dataflow that identifies flows from an Operation (addition, subtraction, etc) to some ignorable operation (bitwise operations for example) that disqualify it + */ module OperationSourceCandidateToIgnorableOperationConfig implements DataFlow::ConfigSig { predicate isSource(DataFlow::Node n) { isOperationSourceCandidate(n.asExpr()) } @@ -100,6 +104,9 @@ module OperationSourceCandidateToIgnorableOperationConfig implements DataFlow::C module OperationSourceCandidateToIgnorableOperationFlow = TaintTracking::Global; +/** + * A dataflow that tracks an ignorable operation (eg. bitwise op) to a operation source, so we may disqualify it. + */ module IgnorableOperationToOperationSourceCandidateConfig implements DataFlow::ConfigSig { predicate isSource(DataFlow::Node n) { n.asExpr() instanceof IgnorableOperation } @@ -114,6 +121,16 @@ module IgnorableOperationToOperationSourceCandidateConfig implements DataFlow::C module IgnorableOperationToOperationSourceCandidateFlow = TaintTracking::Global; +/** + * The set of all expressions which is a candidate expression and also does not flow from to to some ignorable expression (eg. bitwise op) + * ``` + * a = something <<< 2; + * myDate.year = a + 1; // invalid + * ... + * a = someDate.year + 1; + * myDate.year = a; // valid + * ``` + */ class OperationSource extends Expr { OperationSource() { isOperationSourceCandidate(this) and @@ -184,6 +201,9 @@ module OperationToYearAssignmentConfig implements DataFlow::ConfigSig { module OperationToYearAssignmentFlow = TaintTracking::Global; +/** + * A Dataflow configuration for tracing from one OperationToYearAssignmentFlow source to another OperationToYearAssignmentFlow source. + */ module KnownYearOpSourceToKnownYearOpSourceConfig implements DataFlow::ConfigSig { predicate isSource(DataFlow::Node n) { exists(OperationToYearAssignmentFlow::PathNode src | @@ -203,6 +223,9 @@ module KnownYearOpSourceToKnownYearOpSourceConfig implements DataFlow::ConfigSig module KnownYearOpSourceToKnownYearOpSourceFlow = TaintTracking::Global; +/** + * There does not exist an OperationSource that flows through this given OperationSource expression. + */ predicate isRootOperationSource(OperationSource e) { not exists(DataFlow::Node src, DataFlow::Node sink | src != sink and @@ -211,6 +234,9 @@ predicate isRootOperationSource(OperationSource e) { ) } +/** + * A flow configuration from a Year field access to some Leap year check or guard + */ module YearFieldAccessToLeapYearCheckConfig implements DataFlow::ConfigSig { predicate isSource(DataFlow::Node source) { source.asExpr() instanceof YearFieldAccess } @@ -246,6 +272,7 @@ module YearFieldAccessToLeapYearCheckConfig implements DataFlow::ConfigSig { module YearFieldAccessToLeapYearCheckFlow = TaintTracking::Global; +/** Does there exist a flow from the given YearFieldAccess to a Leap Year check or guard? */ predicate isYearModifiedWithCheck(YearFieldAccess fa) { exists(YearFieldAccessToLeapYearCheckFlow::PathNode src | src.isSource() and From fb7260270d03db88dbc87fe605c64ca44b3b8129 Mon Sep 17 00:00:00 2001 From: Josh Brown Date: Tue, 30 Dec 2025 02:19:17 -0800 Subject: [PATCH 10/28] Autoformat --- .../UncheckedLeapYearAfterYearModificationPrecise.ql | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql index cb33b8f017ab..e426a1f9e209 100644 --- a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql +++ b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql @@ -61,11 +61,13 @@ class IgnorableExprExpr1900Mapping extends IgnorableOperation { } } -class IgnorableBinaryBitwiseOperation extends IgnorableOperation instanceof BinaryBitwiseOperation { } +class IgnorableBinaryBitwiseOperation extends IgnorableOperation instanceof BinaryBitwiseOperation { +} class IgnorableUnaryBitwiseOperation extends IgnorableOperation instanceof UnaryBitwiseOperation { } -class IgnorableAssignmentBitwiseOperation extends IgnorableOperation instanceof AssignBitwiseOperation { } +class IgnorableAssignmentBitwiseOperation extends IgnorableOperation instanceof AssignBitwiseOperation +{ } /** * An expression that is a candidate source for an dataflow configuration for an Operation that could flow to a Year field. From c94edcf4bee92b38d65fb5d2f93b5fa5ff000cb8 Mon Sep 17 00:00:00 2001 From: Josh Brown Date: Tue, 30 Dec 2025 03:09:22 -0800 Subject: [PATCH 11/28] Add failing test case --- .../test.cpp | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/test.cpp b/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/test.cpp index fec991f36bfe..837e5327a2df 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/test.cpp +++ b/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/test.cpp @@ -1052,4 +1052,20 @@ ATime_HrGetSysTime(SYSTEMTIME *pst) // pst->wMinute = static_cast(m_lMinute); // pst->wSecond = static_cast(m_lSecond); // pst->wMilliseconds = 0; +} + +/** +* Negative Case - Anti-pattern 1: [year ±n, month, day] +* Modification of SYSTEMTIME struct by copying from another struct, but no arithmetic is performed. +*/ +void fp_daymonth_guard(){ + SYSTEMTIME st; + FILETIME ft; + GetSystemTime(&st); + + st.wYear++; + + st.wDay = st.wMonth == 2 && st.wDay == 29 ? 28 : st.wDay; + + SystemTimeToFileTime(&st, &ft); } \ No newline at end of file From 6d7bd97e94520e78898d4e641f98121a50c8c7a0 Mon Sep 17 00:00:00 2001 From: Josh Brown Date: Tue, 30 Dec 2025 04:20:50 -0800 Subject: [PATCH 12/28] Check for leap day --- cpp/ql/src/Likely Bugs/Leap Year/LeapYear.qll | 49 +++++++++++++++++++ ...kedLeapYearAfterYearModificationPrecise.ql | 13 +++++ 2 files changed, 62 insertions(+) diff --git a/cpp/ql/src/Likely Bugs/Leap Year/LeapYear.qll b/cpp/ql/src/Likely Bugs/Leap Year/LeapYear.qll index ce0e236f394a..4b36082bd451 100644 --- a/cpp/ql/src/Likely Bugs/Leap Year/LeapYear.qll +++ b/cpp/ql/src/Likely Bugs/Leap Year/LeapYear.qll @@ -380,6 +380,55 @@ class StructTmLeapYearFieldAccess extends LeapYearFieldAccess { } } +/** + * `stDate.wMonth == 2` + */ +class DateCheckMonthFebruary extends Operation { + DateCheckMonthFebruary(){ + this.getOperator() = "==" and + this.getAnOperand() instanceof MonthFieldAccess and + this.getAnOperand().(Literal).getValue() = "2" + } + + Expr getDateQualifier(){ + result = this.getAnOperand().(MonthFieldAccess).getQualifier() + } +} + +/** + * `stDate.wDay == 29` + */ +class DateCheckDay29 extends Operation { + DateCheckDay29(){ + this.getOperator() = "==" and + this.getAnOperand() instanceof DayFieldAccess and + this.getAnOperand().(Literal).getValue() = "29" + } + + Expr getDateQualifier(){ + result = this.getAnOperand().(DayFieldAccess).getQualifier() + } +} + +/** + * The combination of a February and Day 29 verification + * `stDate.wMonth == 2 && stDate.wDay == 29` + */ +class DateFebruary29Check extends Operation { + DateFebruary29Check(){ + this.getOperator() = "&&" and + exists(DateCheckMonthFebruary checkFeb, DateCheckDay29 check29 | + checkFeb = this.getAnOperand() and + check29 = this.getAnOperand() and + hashCons(checkFeb.getDateQualifier()) = hashCons(check29.getDateQualifier()) + ) + } + + Expr getDateQualifier(){ + result = this.getAnOperand().(DateCheckMonthFebruary).getDateQualifier() + } +} + /** * `Function` that includes an operation that is checking for leap year. */ diff --git a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql index e426a1f9e209..3a36ceaed899 100644 --- a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql +++ b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql @@ -280,6 +280,19 @@ predicate isYearModifiedWithCheck(YearFieldAccess fa) { src.isSource() and src.getNode().asExpr() = fa ) + or + isUsedInFeb29Check(fa) +} + +/** + * If there is a flow from a date struct year field access/assignment to a Feb 29 check + */ +predicate isUsedInFeb29Check(YearFieldAccess fa){ + exists(DateFebruary29Check check | + DataFlow::localExprFlow(fa.getQualifier(), check.getDateQualifier()) + or + DataFlow::localExprFlow(check.getDateQualifier(), fa.getQualifier()) + ) } import OperationToYearAssignmentFlow::PathGraph From 662119a52ff0f4b6a403edec8694e2d832e9e24f Mon Sep 17 00:00:00 2001 From: "REDMOND\\brodes" Date: Tue, 30 Dec 2025 14:25:20 -0500 Subject: [PATCH 13/28] Adding a test for setting a year field through a return arg. Misc. tweaks. --- ...kedLeapYearAfterYearModificationPrecise.ql | 105 ++++++++++++++++-- .../test.cpp | 20 ++++ 2 files changed, 115 insertions(+), 10 deletions(-) diff --git a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql index 3a36ceaed899..61c6f8ea9b7c 100644 --- a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql +++ b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql @@ -45,18 +45,52 @@ class IgnorableExpr48Mapping extends IgnorableOperation { } } +class IgnorableCharLiteralArithmetic extends IgnorableOperation { + IgnorableCharLiteralArithmetic() { + exists(this.(BinaryArithmeticOperation).getAnOperand().(TextLiteral).getValue()) + or + exists(AssignArithmeticOperation e | e.getRValue() = this | + exists(this.(TextLiteral).getValue()) + ) + } +} + /* * linux time conversions expect the year to start from 1900, so subtracting or * adding 1900 or anything involving 1900 as a generalization is probably * a conversion that is ignorable */ -class IgnorableExprExpr1900Mapping extends IgnorableOperation { - IgnorableExprExpr1900Mapping() { - this.(Operation).getAnOperand().(Literal).getValue().toInt() = 1900 - or - exists(AssignArithmeticOperation a | this = a.getRValue() | - a.getRValue().(Literal).getValue().toInt() = 1900 +predicate isLikelyConversionConstant(int c) { + c = 146097 or // days in 400-year Gregorian cycle + c = 36524 or // days in 100-year Gregorian subcycle + c = 1461 or // days in 4-year cycle (incl. 1 leap) + c = 32044 or // Fliegel–van Flandern JDN epoch shift + c = 1721425 or // JDN of 0001‑01‑01 (Gregorian) + c = 1721119 or // alt epoch offset + c = 2400000 or // MJD → JDN conversion + c = 2400001 or + c = 2400000 or + c = 2141 or // fixed‑point month/day extraction + c = 65536 or + c = 7834 or + c = 256 or + c = 1900 // struct tm base‑year offset; harmless +} + +/** + * Some constants indicate conversion that are ignorable, e.g., + * julian to gregorian conversion or conversions from linux time structs + * that start at 1900, etc. + */ +class IgnorableConstantArithmetic extends IgnorableOperation { + IgnorableConstantArithmetic() { + exists(int i | isLikelyConversionConstant(i) | + this.(Operation).getAnOperand().(Literal).getValue().toInt() = i + or + exists(AssignArithmeticOperation a | this = a.getRValue() | + a.getRValue().(Literal).getValue().toInt() = i + ) ) } } @@ -69,6 +103,26 @@ class IgnorableUnaryBitwiseOperation extends IgnorableOperation instanceof Unary class IgnorableAssignmentBitwiseOperation extends IgnorableOperation instanceof AssignBitwiseOperation { } +/** + * Any arithmetic operation where one of the operands is a pointer or char type, ignore it + */ +class IgnorablePointerOrCharArithmetic extends IgnorableOperation { + IgnorablePointerOrCharArithmetic() { + this instanceof BinaryArithmeticOperation and + ( + this.(BinaryArithmeticOperation).getAnOperand().getUnspecifiedType() instanceof PointerType + or + this.(BinaryArithmeticOperation).getAnOperand().getUnspecifiedType() instanceof CharType + ) + or + exists(AssignArithmeticOperation a | a.getRValue() = this | + a.getAnOperand().getUnspecifiedType() instanceof PointerType + or + a.getAnOperand().getUnspecifiedType() instanceof CharType + ) + } +} + /** * An expression that is a candidate source for an dataflow configuration for an Operation that could flow to a Year field. */ @@ -147,6 +201,27 @@ class OperationSource extends Expr { } } +class YearFieldAssignmentNode extends DataFlow::Node { + YearFieldAssignmentNode() { + this.asExpr() instanceof YearFieldAssignment + or + this.asDefiningArgument() instanceof YearFieldAccess + or + // TODO: is there a better way to do this? + // i.e., without having to be cognizant of the addressof + // occurring, especially if this occurs on a dataflow + exists(AddressOfExpr aoe | + aoe = this.asDefiningArgument() and + aoe.getOperand() instanceof YearFieldAccess + ) + } + + YearFieldAccess getYearFieldAccess() { + result = this.asDefiningArgument() or + result = this.asExpr().(YearFieldAssignment).getYearFieldAccess() + } +} + /** * An assignment to a year field, which will be a sink * NOTE: could not make this abstract, it was binding to all expressions @@ -190,15 +265,20 @@ class YearFieldAssignmentUnary extends YearFieldAssignment instanceof CrementOpe module OperationToYearAssignmentConfig implements DataFlow::ConfigSig { predicate isSource(DataFlow::Node n) { n.asExpr() instanceof OperationSource } - predicate isSink(DataFlow::Node n) { n.asExpr() instanceof YearFieldAssignment } + predicate isSink(DataFlow::Node n) { n instanceof YearFieldAssignmentNode } predicate isBarrier(DataFlow::Node n) { exists(ArrayExpr arr | arr.getArrayOffset() = n.asExpr()) or n.asExpr().getUnspecifiedType() instanceof PointerType or + n.asExpr().getUnspecifiedType() instanceof CharType + or n.asExpr() instanceof IgnorableOperation } + + predicate isBarrierOut(DataFlow::Node n) { isSink(n) } + // DataFlow::FlowFeature getAFeature() { result instanceof DataFlow::FeatureHasSourceCallContext } } module OperationToYearAssignmentFlow = TaintTracking::Global; @@ -240,7 +320,7 @@ predicate isRootOperationSource(OperationSource e) { * A flow configuration from a Year field access to some Leap year check or guard */ module YearFieldAccessToLeapYearCheckConfig implements DataFlow::ConfigSig { - predicate isSource(DataFlow::Node source) { source.asExpr() instanceof YearFieldAccess } + predicate isSource(DataFlow::Node source) { source instanceof YearFieldAssignmentNode } predicate isSink(DataFlow::Node sink) { // Local leap year check @@ -261,14 +341,19 @@ module YearFieldAccessToLeapYearCheckConfig implements DataFlow::ConfigSig { predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) { // flow from a YearFieldAccess to the qualifier - node1.asExpr() instanceof YearFieldAccess and node2.asExpr() = node1.asExpr().(YearFieldAccess).getQualifier() } + // Use this to make the sink for leap year check intra-proc only // i.e., the sink must be in the same scope (function) as the source. // DataFlow::FlowFeature getAFeature() { // result instanceof DataFlow::FeatureEqualSourceSinkCallContext // } + /** + * Enforcing the check must occur in the same call context as the source, + * i.e., do not return from the source function and check in a caller. + */ + DataFlow::FlowFeature getAFeature() { result instanceof DataFlow::FeatureHasSourceCallContext } } module YearFieldAccessToLeapYearCheckFlow = @@ -287,7 +372,7 @@ predicate isYearModifiedWithCheck(YearFieldAccess fa) { /** * If there is a flow from a date struct year field access/assignment to a Feb 29 check */ -predicate isUsedInFeb29Check(YearFieldAccess fa){ +predicate isUsedInFeb29Check(YearFieldAccess fa) { exists(DateFebruary29Check check | DataFlow::localExprFlow(fa.getQualifier(), check.getDateQualifier()) or diff --git a/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/test.cpp b/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/test.cpp index 837e5327a2df..4c310aeba777 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/test.cpp +++ b/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/test.cpp @@ -1068,4 +1068,24 @@ void fp_daymonth_guard(){ st.wDay = st.wMonth == 2 && st.wDay == 29 ? 28 : st.wDay; SystemTimeToFileTime(&st, &ft); +} + +void increment_arg(WORD &x){ + x++; +} + +void increment_arg_by_pointer(WORD *x){ + (*x)++; +} + + +void fn_year_set_through_out_arg(){ + SYSTEMTIME st; + GetSystemTime(&st); + // BAD, year incremented without check + increment_arg(st.wYear); + + // GetSystemTime(&st); + // Bad, year incremented without check + increment_arg_by_pointer(&st.wYear); } \ No newline at end of file From 1169f9e641c59253cfa32c1a452bd6db759a52f4 Mon Sep 17 00:00:00 2001 From: "REDMOND\\brodes" Date: Tue, 30 Dec 2025 14:59:32 -0500 Subject: [PATCH 14/28] Assignment through out arg is causing too many FPs. --- ...kedLeapYearAfterYearModificationPrecise.ql | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql index 61c6f8ea9b7c..4c9d3704a93c 100644 --- a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql +++ b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql @@ -204,16 +204,16 @@ class OperationSource extends Expr { class YearFieldAssignmentNode extends DataFlow::Node { YearFieldAssignmentNode() { this.asExpr() instanceof YearFieldAssignment - or - this.asDefiningArgument() instanceof YearFieldAccess - or - // TODO: is there a better way to do this? - // i.e., without having to be cognizant of the addressof - // occurring, especially if this occurs on a dataflow - exists(AddressOfExpr aoe | - aoe = this.asDefiningArgument() and - aoe.getOperand() instanceof YearFieldAccess - ) + // or + // this.asDefiningArgument() instanceof YearFieldAccess + // or + // // TODO: is there a better way to do this? + // // i.e., without having to be cognizant of the addressof + // // occurring, especially if this occurs on a dataflow + // exists(AddressOfExpr aoe | + // aoe = this.asDefiningArgument() and + // aoe.getOperand() instanceof YearFieldAccess + // ) } YearFieldAccess getYearFieldAccess() { From 800abae831c2a336a8f970eceb364890ac07de53 Mon Sep 17 00:00:00 2001 From: Josh Brown Date: Wed, 31 Dec 2025 01:12:44 -0800 Subject: [PATCH 15/28] Fix mislabeled test case --- .../UncheckedLeapYearAfterYearModification/test.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/test.cpp b/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/test.cpp index 4c310aeba777..d2e87da0959c 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/test.cpp +++ b/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/test.cpp @@ -731,10 +731,10 @@ void Correct_year_addition_struct_tm() } /** - * Negative Case - Anti-pattern 1: [year ±n, month, day] + * Positive Case - Anti-pattern 1: [year ±n, month, day] * Years is incremented by some integer and leap year is not handled correctly. */ -void Correct_LinuxPattern() +void Incorrect_LinuxPattern() { time_t rawtime; struct tm timeinfo; @@ -743,7 +743,7 @@ void Correct_LinuxPattern() errno_t err = gmtime_s(&timeinfo, &rawtime); /* from 1900 -> from 1980 */ - timeinfo.tm_year -= 80; + timeinfo.tm_year -= 80; // bug - no check for leap year /* 0~11 -> 1~12 */ timeinfo.tm_mon++; /* 0~59 -> 0~29(2sec counts) */ From ec3b350dc84a9dc5d2bab0041d5a2d885bbdb96e Mon Sep 17 00:00:00 2001 From: "REDMOND\\brodes" Date: Mon, 12 Jan 2026 14:00:48 -0500 Subject: [PATCH 16/28] Misc. tweaks addressing FPs and cases observed during auditing. --- ...kedLeapYearAfterYearModificationPrecise.ql | 71 +++++++------------ 1 file changed, 24 insertions(+), 47 deletions(-) diff --git a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql index 4c9d3704a93c..f64df8dc4f02 100644 --- a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql +++ b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql @@ -72,6 +72,7 @@ predicate isLikelyConversionConstant(int c) { c = 2400001 or c = 2400000 or c = 2141 or // fixed‑point month/day extraction + c = 2000 or c = 65536 or c = 7834 or c = 256 or @@ -95,6 +96,9 @@ class IgnorableConstantArithmetic extends IgnorableOperation { } } +// If a unary minus assume it is some sort of conversion +class IgnorableUnaryMinus extends IgnorableOperation instanceof UnaryMinusExpr { } + class IgnorableBinaryBitwiseOperation extends IgnorableOperation instanceof BinaryBitwiseOperation { } @@ -275,44 +279,33 @@ module OperationToYearAssignmentConfig implements DataFlow::ConfigSig { n.asExpr().getUnspecifiedType() instanceof CharType or n.asExpr() instanceof IgnorableOperation + or + isLeapYearCheckSink(n) } + // Block flow out of an operation source to get the "closest" operation to the sink + predicate isBarrierIn(DataFlow::Node n) { isSource(n) } + predicate isBarrierOut(DataFlow::Node n) { isSink(n) } // DataFlow::FlowFeature getAFeature() { result instanceof DataFlow::FeatureHasSourceCallContext } } module OperationToYearAssignmentFlow = TaintTracking::Global; -/** - * A Dataflow configuration for tracing from one OperationToYearAssignmentFlow source to another OperationToYearAssignmentFlow source. - */ -module KnownYearOpSourceToKnownYearOpSourceConfig implements DataFlow::ConfigSig { - predicate isSource(DataFlow::Node n) { - exists(OperationToYearAssignmentFlow::PathNode src | - src.getNode().asExpr() = n.asExpr() and - src.isSource() - ) - } - - predicate isSink(DataFlow::Node n) { - exists(OperationToYearAssignmentFlow::PathNode src | - src.getNode().asExpr() = n.asExpr() and - src.isSource() +predicate isLeapYearCheckSink(DataFlow::Node sink) { + // Local leap year check + sink.asExpr().(LeapYearFieldAccess).isUsedInCorrectLeapYearCheck() + or + // passed to function that seems to check for leap year + exists(ChecksForLeapYearFunctionCall fc | + sink.asExpr() = fc.getAnArgument() + or + sink.asExpr() = fc.getAnArgument().(FieldAccess).getQualifier() + or + exists(AddressOfExpr aoe | + fc.getAnArgument() = aoe and + aoe.getOperand() = sink.asExpr() ) - } -} - -module KnownYearOpSourceToKnownYearOpSourceFlow = - TaintTracking::Global; - -/** - * There does not exist an OperationSource that flows through this given OperationSource expression. - */ -predicate isRootOperationSource(OperationSource e) { - not exists(DataFlow::Node src, DataFlow::Node sink | - src != sink and - KnownYearOpSourceToKnownYearOpSourceFlow::flow(src, sink) and - sink.asExpr() = e ) } @@ -322,22 +315,7 @@ predicate isRootOperationSource(OperationSource e) { module YearFieldAccessToLeapYearCheckConfig implements DataFlow::ConfigSig { predicate isSource(DataFlow::Node source) { source instanceof YearFieldAssignmentNode } - predicate isSink(DataFlow::Node sink) { - // Local leap year check - sink.asExpr().(LeapYearFieldAccess).isUsedInCorrectLeapYearCheck() - or - // passed to function that seems to check for leap year - exists(ChecksForLeapYearFunctionCall fc | - sink.asExpr() = fc.getAnArgument() - or - sink.asExpr() = fc.getAnArgument().(FieldAccess).getQualifier() - or - exists(AddressOfExpr aoe | - fc.getAnArgument() = aoe and - aoe.getOperand() = sink.asExpr() - ) - ) - } + predicate isSink(DataFlow::Node sink) { isLeapYearCheckSink(sink) } predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) { // flow from a YearFieldAccess to the qualifier @@ -363,7 +341,7 @@ module YearFieldAccessToLeapYearCheckFlow = predicate isYearModifiedWithCheck(YearFieldAccess fa) { exists(YearFieldAccessToLeapYearCheckFlow::PathNode src | src.isSource() and - src.getNode().asExpr() = fa + src.getNode().asExpr().(YearFieldAssignment).getYearFieldAccess() = fa ) or isUsedInFeb29Check(fa) @@ -385,6 +363,5 @@ import OperationToYearAssignmentFlow::PathGraph from OperationToYearAssignmentFlow::PathNode src, OperationToYearAssignmentFlow::PathNode sink where OperationToYearAssignmentFlow::flowPath(src, sink) and - isRootOperationSource(src.getNode().asExpr()) and not isYearModifiedWithCheck(sink.getNode().asExpr().(YearFieldAssignment).getYearFieldAccess()) select sink, src, sink, "TEST" From 9b7f98622e7f140080d2d534fadcdb5b54f73051 Mon Sep 17 00:00:00 2001 From: "REDMOND\\brodes" Date: Mon, 12 Jan 2026 14:50:35 -0500 Subject: [PATCH 17/28] Fixed FP issue with ignorable constants, now no longer relying on the constant being a literal, but a known value variable or literal. --- .../UncheckedLeapYearAfterYearModificationPrecise.ql | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql index f64df8dc4f02..0c9c84063bc3 100644 --- a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql +++ b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql @@ -25,10 +25,10 @@ class IgnorableExprRem extends IgnorableOperation instanceof RemExpr { } */ class IgnorableExpr10MulipleComponent extends IgnorableOperation { IgnorableExpr10MulipleComponent() { - this.(MulExpr).getAnOperand().(Literal).getValue().toInt() in [10, 100, 100] + this.(MulExpr).getAnOperand().getValue().toInt() in [10, 100, 100] or exists(AssignMulExpr a | a.getRValue() = this | - a.getRValue().(Literal).getValue().toInt() in [10, 100, 100] + a.getRValue().getValue().toInt() in [10, 100, 100] ) } } @@ -39,9 +39,9 @@ class IgnorableExpr10MulipleComponent extends IgnorableOperation { */ class IgnorableExpr48Mapping extends IgnorableOperation { IgnorableExpr48Mapping() { - this.(SubExpr).getRightOperand().(Literal).getValue().toInt() = 48 + this.(SubExpr).getRightOperand().getValue().toInt() = 48 or - exists(AssignSubExpr e | e.getRValue() = this | e.getRValue().(Literal).getValue().toInt() = 48) + exists(AssignSubExpr e | e.getRValue() = this | e.getRValue().getValue().toInt() = 48) } } @@ -87,10 +87,10 @@ predicate isLikelyConversionConstant(int c) { class IgnorableConstantArithmetic extends IgnorableOperation { IgnorableConstantArithmetic() { exists(int i | isLikelyConversionConstant(i) | - this.(Operation).getAnOperand().(Literal).getValue().toInt() = i + this.(Operation).getAnOperand().getValue().toInt() = i or exists(AssignArithmeticOperation a | this = a.getRValue() | - a.getRValue().(Literal).getValue().toInt() = i + a.getRValue().getValue().toInt() = i ) ) } From 9b04b42ae3611e33a851e4e728668cdce341c664 Mon Sep 17 00:00:00 2001 From: "REDMOND\\brodes" Date: Mon, 12 Jan 2026 16:02:05 -0500 Subject: [PATCH 18/28] A leap year check sink can be a ExprCheckLeapYear component. --- .../Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql | 2 ++ 1 file changed, 2 insertions(+) diff --git a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql index 0c9c84063bc3..cebd6cfd8d10 100644 --- a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql +++ b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql @@ -293,6 +293,8 @@ module OperationToYearAssignmentConfig implements DataFlow::ConfigSig { module OperationToYearAssignmentFlow = TaintTracking::Global; predicate isLeapYearCheckSink(DataFlow::Node sink) { + exists(ExprCheckLeapYear e | sink.asExpr() = e.getAChild*()) + or // Local leap year check sink.asExpr().(LeapYearFieldAccess).isUsedInCorrectLeapYearCheck() or From 2f1a850ffaa781c5d7f1d37c0cdad61c267a37e7 Mon Sep 17 00:00:00 2001 From: Josh Brown Date: Tue, 13 Jan 2026 05:07:16 -0800 Subject: [PATCH 19/28] Check if there is a guard checking for a month that isnt a february value --- ...kedLeapYearAfterYearModificationPrecise.ql | 35 ++++++++++- .../test.cpp | 59 +++++++++++++++++++ 2 files changed, 93 insertions(+), 1 deletion(-) diff --git a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql index cebd6cfd8d10..1e4cda9b8842 100644 --- a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql +++ b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql @@ -11,6 +11,7 @@ import cpp import LeapYear +import semmle.code.cpp.controlflow.IRGuards /** * The set of expressions which are ignorable; either because they seem to not be part of a year mutation, @@ -360,10 +361,42 @@ predicate isUsedInFeb29Check(YearFieldAccess fa) { ) } +class MonthEqualityCheck extends EqualityOperation{ + MonthEqualityCheck(){ + this.getAnOperand() instanceof MonthFieldAccess + } + + Expr getExprCompared(){ + exists(Expr e | + e = this.getAnOperand() and + not e instanceof MonthFieldAccess and + result = e + ) + } + + MonthFieldAccess getMonthFieldAccess(){ + result = this.getAnOperand() + } +} + +class MonthEqualityCheckGuard extends GuardCondition instanceof MonthEqualityCheck{ + MonthEqualityCheckGuard(){ any() } +} + +bindingset[e] +pragma[inline_late] +predicate isControlledByMonthEqualityCheckNonFebruary(Expr e){ + exists(MonthEqualityCheckGuard monthGuard | + monthGuard.controls(e.getBasicBlock(), true) and + not monthGuard.(MonthEqualityCheck).getExprCompared().getValueText() = "2" + ) +} + import OperationToYearAssignmentFlow::PathGraph from OperationToYearAssignmentFlow::PathNode src, OperationToYearAssignmentFlow::PathNode sink where OperationToYearAssignmentFlow::flowPath(src, sink) and - not isYearModifiedWithCheck(sink.getNode().asExpr().(YearFieldAssignment).getYearFieldAccess()) + not isYearModifiedWithCheck(sink.getNode().asExpr().(YearFieldAssignment).getYearFieldAccess()) and + not isControlledByMonthEqualityCheckNonFebruary(sink.getNode().asExpr()) select sink, src, sink, "TEST" diff --git a/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/test.cpp b/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/test.cpp index d2e87da0959c..79269a9481a7 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/test.cpp +++ b/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/test.cpp @@ -73,6 +73,30 @@ struct logtime { long usec; }; +/* + * Data structure representing a broken-down timestamp. + * + * CAUTION: the IANA timezone library (src/timezone/) follows the POSIX + * convention that tm_mon counts from 0 and tm_year is relative to 1900. + * However, Postgres' datetime functions generally treat tm_mon as counting + * from 1 and tm_year as relative to 1 BC. Be sure to make the appropriate + * adjustments when moving from one code domain to the other. + */ +struct pg_tm +{ + int tm_sec; + int tm_min; + int tm_hour; + int tm_mday; + int tm_mon; /* see above */ + int tm_year; /* see above */ + int tm_wday; + int tm_yday; + int tm_isdst; + long int tm_gmtoff; + const char *tm_zone; +}; + BOOL SystemTimeToFileTime( const SYSTEMTIME* lpSystemTime, @@ -1088,4 +1112,39 @@ void fn_year_set_through_out_arg(){ // GetSystemTime(&st); // Bad, year incremented without check increment_arg_by_pointer(&st.wYear); +} + + +/* void +GetEpochTime(struct pg_tm *tm) +{ + struct pg_tm *t0; + pg_time_t epoch = 0; + + t0 = pg_gmtime(&epoch); + + tm->tm_year = t0->tm_year; + tm->tm_mon = t0->tm_mon; + tm->tm_mday = t0->tm_mday; + tm->tm_hour = t0->tm_hour; + tm->tm_min = t0->tm_min; + tm->tm_sec = t0->tm_sec; + + tm->tm_year += 1900; + tm->tm_mon++; +} */ + +void +fp_guarded_by_month(struct pg_tm *tm){ + int woy = 52; + int MONTHS_PER_YEAR = 12; + /* + * If it is week 52/53 and the month is January, then the + * week must belong to the previous year. Also, some + * December dates belong to the next year. + */ + if (woy >= 52 && tm->tm_mon == 1) + --tm->tm_year; // Negative Test Case + if (woy <= 1 && tm->tm_mon == MONTHS_PER_YEAR) + ++tm->tm_year; // Negative Test Case } \ No newline at end of file From b0130121ce29a7391d6286a9e9f406b8e8e7ec17 Mon Sep 17 00:00:00 2001 From: "REDMOND\\brodes" Date: Tue, 13 Jan 2026 10:23:30 -0500 Subject: [PATCH 20/28] Misc. updates. Specifically including how constant values are used to ignore certain opeartions. Also added an ignorable function class to be used to ignore operation sources. --- ...kedLeapYearAfterYearModificationPrecise.ql | 80 ++++++++++--------- 1 file changed, 44 insertions(+), 36 deletions(-) diff --git a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql index 1e4cda9b8842..d9179e090bc0 100644 --- a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql +++ b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql @@ -13,6 +13,17 @@ import cpp import LeapYear import semmle.code.cpp.controlflow.IRGuards +/** + * Functions whose operations should never be considered a + * source of a dangerous leap year operation. + */ +class IgnorableFunction extends Function { + IgnorableFunction() { + // Helper utility in postgres with string time conversions + this.getName() = "DecodeISO8601Interval" + } +} + /** * The set of expressions which are ignorable; either because they seem to not be part of a year mutation, * or because they seem to be a conversion pattern of mapping date scalars. @@ -22,14 +33,15 @@ abstract class IgnorableOperation extends Expr { } class IgnorableExprRem extends IgnorableOperation instanceof RemExpr { } /** - * Anything involving a sub expression with char literal 48, ignore as a likely string conversion + * Anything involving an operation with 10, 100, 1000, 10000 is often a sign of conversion + * or atoi. */ class IgnorableExpr10MulipleComponent extends IgnorableOperation { IgnorableExpr10MulipleComponent() { - this.(MulExpr).getAnOperand().getValue().toInt() in [10, 100, 100] + this.(Operation).getAnOperand().getValue().toInt() in [10, 100, 1000, 10000] or - exists(AssignMulExpr a | a.getRValue() = this | - a.getRValue().getValue().toInt() in [10, 100, 100] + exists(AssignOperation a | a.getRValue() = this | + a.getRValue().getValue().toInt() in [10, 100, 1000, 10000] ) } } @@ -56,28 +68,29 @@ class IgnorableCharLiteralArithmetic extends IgnorableOperation { } } -/* - * linux time conversions expect the year to start from 1900, so subtracting or - * adding 1900 or anything involving 1900 as a generalization is probably - * a conversion that is ignorable +/** + * Constants often used in date conversions (from one date data type to another) */ - +bindingset[c] predicate isLikelyConversionConstant(int c) { - c = 146097 or // days in 400-year Gregorian cycle - c = 36524 or // days in 100-year Gregorian subcycle - c = 1461 or // days in 4-year cycle (incl. 1 leap) - c = 32044 or // Fliegel–van Flandern JDN epoch shift - c = 1721425 or // JDN of 0001‑01‑01 (Gregorian) - c = 1721119 or // alt epoch offset - c = 2400000 or // MJD → JDN conversion - c = 2400001 or - c = 2400000 or - c = 2141 or // fixed‑point month/day extraction - c = 2000 or - c = 65536 or - c = 7834 or - c = 256 or - c = 1900 // struct tm base‑year offset; harmless + exists(int i | i = c.abs() | i >= 100) + // c = 146097 or // days in 400-year Gregorian cycle + // c = 36524 or // days in 100-year Gregorian subcycle + // c = 1461 or // days in 4-year cycle (incl. 1 leap) + // c = 32044 or // Fliegel–van Flandern JDN epoch shift + // c = 1721425 or // JDN of 0001‑01‑01 (Gregorian) + // c = 1721119 or // alt epoch offset + // c = 2400000 or // MJD → JDN conversion + // c = 2400001 or // alt MJD → JDN conversion + // c = 2141 or // fixed‑point month/day extraction + // c = 2000 or // observed in some conversions + // c = 65536 or // observed in some conversions + // c = 7834 or // observed in some conversions + // c = 256 or // observed in some conversions + // c = 1900 or // struct tm base‑year offset; harmless + // c = 1899 or // Observed in uses with 1900 to address off by one scenarios + // c = 292275056 or // qdatetime.h Qt Core year range first year constant + // c = 292278994 // qdatetime.h Qt Core year range last year constant } /** @@ -133,6 +146,7 @@ class IgnorablePointerOrCharArithmetic extends IgnorableOperation { */ predicate isOperationSourceCandidate(Expr e) { not e instanceof IgnorableOperation and + not e.getEnclosingFunction() instanceof IgnorableFunction and ( e instanceof SubExpr or @@ -361,12 +375,10 @@ predicate isUsedInFeb29Check(YearFieldAccess fa) { ) } -class MonthEqualityCheck extends EqualityOperation{ - MonthEqualityCheck(){ - this.getAnOperand() instanceof MonthFieldAccess - } +class MonthEqualityCheck extends EqualityOperation { + MonthEqualityCheck() { this.getAnOperand() instanceof MonthFieldAccess } - Expr getExprCompared(){ + Expr getExprCompared() { exists(Expr e | e = this.getAnOperand() and not e instanceof MonthFieldAccess and @@ -374,18 +386,14 @@ class MonthEqualityCheck extends EqualityOperation{ ) } - MonthFieldAccess getMonthFieldAccess(){ - result = this.getAnOperand() - } + MonthFieldAccess getMonthFieldAccess() { result = this.getAnOperand() } } -class MonthEqualityCheckGuard extends GuardCondition instanceof MonthEqualityCheck{ - MonthEqualityCheckGuard(){ any() } -} +class MonthEqualityCheckGuard extends GuardCondition instanceof MonthEqualityCheck { } bindingset[e] pragma[inline_late] -predicate isControlledByMonthEqualityCheckNonFebruary(Expr e){ +predicate isControlledByMonthEqualityCheckNonFebruary(Expr e) { exists(MonthEqualityCheckGuard monthGuard | monthGuard.controls(e.getBasicBlock(), true) and not monthGuard.(MonthEqualityCheck).getExprCompared().getValueText() = "2" From ce802bdd19d3e6db473e0833af616d342fd6660e Mon Sep 17 00:00:00 2001 From: "REDMOND\\brodes" Date: Tue, 13 Jan 2026 12:56:51 -0500 Subject: [PATCH 21/28] More ignorable functions, and adding 0 as an ignorable constant. --- ...ncheckedLeapYearAfterYearModificationPrecise.ql | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql index d9179e090bc0..6fed1fce1529 100644 --- a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql +++ b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql @@ -21,6 +21,18 @@ class IgnorableFunction extends Function { IgnorableFunction() { // Helper utility in postgres with string time conversions this.getName() = "DecodeISO8601Interval" + or + // helper utility for date conversions in qtbase + this.getName() = "adjacentDay" + or + // Windows API function that does timezone conversions + this.getName().matches("%SystemTimeToTzSpecificLocalTime%") + or + // Windows APIs that do time conversions + this.getName().matches("%localtime%\\_s%") + or + // Windows APIs that do time conversions + this.getName().matches("%SpecificLocalTimeToSystemTime%") } } @@ -74,6 +86,8 @@ class IgnorableCharLiteralArithmetic extends IgnorableOperation { bindingset[c] predicate isLikelyConversionConstant(int c) { exists(int i | i = c.abs() | i >= 100) + or + c = 0 // c = 146097 or // days in 400-year Gregorian cycle // c = 36524 or // days in 100-year Gregorian subcycle // c = 1461 or // days in 4-year cycle (incl. 1 leap) From 328271838e8d6f78d17cecf466272e2b4e7f70c4 Mon Sep 17 00:00:00 2001 From: Josh Brown Date: Thu, 15 Jan 2026 04:14:26 -0800 Subject: [PATCH 22/28] Add TIME_FIELDS struct and test case --- .../lib/semmle/code/cpp/commons/DateTime.qll | 23 ++++++++++++++++++- ...kedLeapYearAfterYearModificationPrecise.ql | 10 +++++++- .../test.cpp | 21 ++++++++++++++++- 3 files changed, 51 insertions(+), 3 deletions(-) diff --git a/cpp/ql/lib/semmle/code/cpp/commons/DateTime.qll b/cpp/ql/lib/semmle/code/cpp/commons/DateTime.qll index c67bf7cf96e3..b01c775e5535 100644 --- a/cpp/ql/lib/semmle/code/cpp/commons/DateTime.qll +++ b/cpp/ql/lib/semmle/code/cpp/commons/DateTime.qll @@ -14,7 +14,7 @@ class PackedTimeType extends Type { } } -private predicate timeType(string typeName) { typeName = ["_SYSTEMTIME", "SYSTEMTIME", "tm"] } +private predicate timeType(string typeName) { typeName = ["_SYSTEMTIME", "SYSTEMTIME", "tm", "TIME_FIELDS", "PTIME_FIELDS"] } /** * A type that is used to represent times and dates in an 'unpacked' form, that is, @@ -95,3 +95,24 @@ class StructTmMonthFieldAccess extends MonthFieldAccess { class StructTmYearFieldAccess extends YearFieldAccess { StructTmYearFieldAccess() { this.getTarget().getName() = "tm_year" } } + +/** + * A `DayFieldAccess` for the `TIME_FIELDS` struct. + */ +class TimeFieldsDayFieldAccess extends DayFieldAccess { + TimeFieldsDayFieldAccess() { this.getTarget().getName() = "Day" } +} + +/** + * A `MonthFieldAccess` for the `TIME_FIELDS` struct. + */ +class TimeFieldsMonthFieldAccess extends MonthFieldAccess { + TimeFieldsMonthFieldAccess() { this.getTarget().getName() = "Month" } +} + +/** + * A `YearFieldAccess` for the `TIME_FIELDS` struct. + */ +class TimeFieldsYearFieldAccess extends YearFieldAccess { + TimeFieldsYearFieldAccess() { this.getTarget().getName() = "Year" } +} \ No newline at end of file diff --git a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql index 6fed1fce1529..b980d5f9f045 100644 --- a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql +++ b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql @@ -312,10 +312,11 @@ module OperationToYearAssignmentConfig implements DataFlow::ConfigSig { isLeapYearCheckSink(n) } - // Block flow out of an operation source to get the "closest" operation to the sink + /** Block flow out of an operation source to get the "closest" operation to the sink */ predicate isBarrierIn(DataFlow::Node n) { isSource(n) } predicate isBarrierOut(DataFlow::Node n) { isSink(n) } + // DataFlow::FlowFeature getAFeature() { result instanceof DataFlow::FeatureHasSourceCallContext } } @@ -358,6 +359,7 @@ module YearFieldAccessToLeapYearCheckConfig implements DataFlow::ConfigSig { // DataFlow::FlowFeature getAFeature() { // result instanceof DataFlow::FeatureEqualSourceSinkCallContext // } + /** * Enforcing the check must occur in the same call context as the source, * i.e., do not return from the source function and check in a caller. @@ -389,6 +391,9 @@ predicate isUsedInFeb29Check(YearFieldAccess fa) { ) } +/** + * An expression which checks the value of a Month field `a->month == 1`. + */ class MonthEqualityCheck extends EqualityOperation { MonthEqualityCheck() { this.getAnOperand() instanceof MonthFieldAccess } @@ -405,6 +410,9 @@ class MonthEqualityCheck extends EqualityOperation { class MonthEqualityCheckGuard extends GuardCondition instanceof MonthEqualityCheck { } +/** + * Verifies if the expression is guarded by a check on the Month property of a date struct, that is NOT February. + */ bindingset[e] pragma[inline_late] predicate isControlledByMonthEqualityCheckNonFebruary(Expr e) { diff --git a/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/test.cpp b/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/test.cpp index 79269a9481a7..05f5ebf89053 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/test.cpp +++ b/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/test.cpp @@ -1115,7 +1115,8 @@ void fn_year_set_through_out_arg(){ } -/* void +/* TODO: don't alert on simple copies from another struct where all three {year,month,day} are copied +void GetEpochTime(struct pg_tm *tm) { struct pg_tm *t0; @@ -1147,4 +1148,22 @@ fp_guarded_by_month(struct pg_tm *tm){ --tm->tm_year; // Negative Test Case if (woy <= 1 && tm->tm_mon == MONTHS_PER_YEAR) ++tm->tm_year; // Negative Test Case +} + +typedef unsigned short CSHORT; + +typedef struct _TIME_FIELDS { + CSHORT Year; + CSHORT Month; + CSHORT Day; + CSHORT Hour; + CSHORT Minute; + CSHORT Second; + CSHORT Milliseconds; + CSHORT Weekday; +} TIME_FIELDS, *PTIME_FIELDS; + +void +tp_ptime(PTIME_FIELDS ptm){ + ptm->Year = ptm->Year - 1; // Positive Test Case } \ No newline at end of file From 938b42aca03a4015ebbb7b65754b89c016590908 Mon Sep 17 00:00:00 2001 From: Josh Brown Date: Thu, 15 Jan 2026 04:15:23 -0800 Subject: [PATCH 23/28] Removed unused predicates --- .../UncheckedLeapYearAfterYearModificationPrecise.ql | 7 ------- 1 file changed, 7 deletions(-) diff --git a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql index b980d5f9f045..c4a2f49feb31 100644 --- a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql +++ b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql @@ -248,11 +248,6 @@ class YearFieldAssignmentNode extends DataFlow::Node { // aoe.getOperand() instanceof YearFieldAccess // ) } - - YearFieldAccess getYearFieldAccess() { - result = this.asDefiningArgument() or - result = this.asExpr().(YearFieldAssignment).getYearFieldAccess() - } } /** @@ -404,8 +399,6 @@ class MonthEqualityCheck extends EqualityOperation { result = e ) } - - MonthFieldAccess getMonthFieldAccess() { result = this.getAnOperand() } } class MonthEqualityCheckGuard extends GuardCondition instanceof MonthEqualityCheck { } From 4aaad2381522c57b1002658012c43a72fc0514d1 Mon Sep 17 00:00:00 2001 From: Josh Brown Date: Thu, 15 Jan 2026 04:16:53 -0800 Subject: [PATCH 24/28] "Precise" query overwrites previous version --- .../UncheckedLeapYearAfterYearModification.ql | 474 ++++++++++++++---- ...kedLeapYearAfterYearModificationPrecise.ql | 425 ---------------- ...pYearAfterYearModificationPrecise.expected | 8 - ...LeapYearAfterYearModificationPrecise.qlref | 1 - 4 files changed, 365 insertions(+), 543 deletions(-) delete mode 100644 cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql delete mode 100644 cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/UncheckedLeapYearAfterYearModificationPrecise.expected delete mode 100644 cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/UncheckedLeapYearAfterYearModificationPrecise.qlref diff --git a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification.ql b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification.ql index 38091cc23358..8681ace34620 100644 --- a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification.ql +++ b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification.ql @@ -1,7 +1,7 @@ /** * @name Year field changed using an arithmetic operation without checking for leap year (AntiPattern 1) * @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 + * @kind path-problem * @problem.severity warning * @id cpp/microsoft/public/leap-year/unchecked-after-arithmetic-year-modification * @precision medium @@ -11,159 +11,415 @@ import cpp import LeapYear +import semmle.code.cpp.controlflow.IRGuards /** - * The set of all write operations to the Year field of a date struct. + * Functions whose operations should never be considered a + * source of a dangerous leap year operation. */ -abstract class YearWriteOp extends Operation { - /** Extracts the access to the Year field */ - abstract YearFieldAccess getYearAccess(); +class IgnorableFunction extends Function { + IgnorableFunction() { + // Helper utility in postgres with string time conversions + this.getName() = "DecodeISO8601Interval" + or + // helper utility for date conversions in qtbase + this.getName() = "adjacentDay" + or + // Windows API function that does timezone conversions + this.getName().matches("%SystemTimeToTzSpecificLocalTime%") + or + // Windows APIs that do time conversions + this.getName().matches("%localtime%\\_s%") + or + // Windows APIs that do time conversions + this.getName().matches("%SpecificLocalTimeToSystemTime%") + } +} - /** Get the expression which represents the new value. */ - abstract Expr getMutationExpr(); +/** + * The set of expressions which are ignorable; either because they seem to not be part of a year mutation, + * or because they seem to be a conversion pattern of mapping date scalars. + */ +abstract class IgnorableOperation extends Expr { } - /** - * Checks if this Operation is a simple normalization, converting between formats. - */ - predicate isNormalizationOperation(){ - isNormalizationOperation(this.getMutationExpr()) +class IgnorableExprRem extends IgnorableOperation instanceof RemExpr { } + +/** + * Anything involving an operation with 10, 100, 1000, 10000 is often a sign of conversion + * or atoi. + */ +class IgnorableExpr10MulipleComponent extends IgnorableOperation { + IgnorableExpr10MulipleComponent() { + this.(Operation).getAnOperand().getValue().toInt() in [10, 100, 1000, 10000] + or + exists(AssignOperation a | a.getRValue() = this | + a.getRValue().getValue().toInt() in [10, 100, 1000, 10000] + ) } +} - /** - * Holds if there is no known leap-year verification for the `YearWriteOp`. - * Binds the `var` argument to the qualifier of the `ywo` argument. - */ - predicate isYearModifedWithoutExplicitLeapYearCheck(Variable var) { - exists(VariableAccess va, YearFieldAccess yfa | - yfa = this.getYearAccess() and - yfa.getQualifier() = va and - var.getAnAccess() = va 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.getBasicBlock() = yfa.getBasicBlock().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 | - source = var.getAnAccess() and - LeapYearCheckFlow::flow(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 | - vacheck = var.getAnAccess() and - yfacheck.getQualifier() = vacheck and - LeapYearCheckFlow::flow(DataFlow::exprNode(yfacheck), DataFlow::exprNode(fc.getAnArgument())) - ) - or - // If there is a successor or predecessor that sets the month or day to a fixed value - exists(FieldAccess mfa, AssignExpr ae, int val | - (mfa instanceof MonthFieldAccess or mfa instanceof DayFieldAccess) and - mfa.getQualifier() = var.getAnAccess() and - mfa.isModified() and - ( - mfa.getBasicBlock() = yfa.getBasicBlock().getASuccessor*() or - yfa.getBasicBlock() = mfa.getBasicBlock().getASuccessor+() - ) and - ae = mfa.getEnclosingElement() and - ae.getAnOperand().getValue().toInt() = val - ) +/** + * Anything involving a sub expression with char literal 48, ignore as a likely string conversion + * e.g., X - '0' + */ +class IgnorableExpr48Mapping extends IgnorableOperation { + IgnorableExpr48Mapping() { + this.(SubExpr).getRightOperand().getValue().toInt() = 48 + or + exists(AssignSubExpr e | e.getRValue() = this | e.getRValue().getValue().toInt() = 48) + } +} + +class IgnorableCharLiteralArithmetic extends IgnorableOperation { + IgnorableCharLiteralArithmetic() { + exists(this.(BinaryArithmeticOperation).getAnOperand().(TextLiteral).getValue()) + or + exists(AssignArithmeticOperation e | e.getRValue() = this | + exists(this.(TextLiteral).getValue()) + ) + } +} + +/** + * Constants often used in date conversions (from one date data type to another) + */ +bindingset[c] +predicate isLikelyConversionConstant(int c) { + exists(int i | i = c.abs() | i >= 100) + or + c = 0 + // c = 146097 or // days in 400-year Gregorian cycle + // c = 36524 or // days in 100-year Gregorian subcycle + // c = 1461 or // days in 4-year cycle (incl. 1 leap) + // c = 32044 or // Fliegel–van Flandern JDN epoch shift + // c = 1721425 or // JDN of 0001‑01‑01 (Gregorian) + // c = 1721119 or // alt epoch offset + // c = 2400000 or // MJD → JDN conversion + // c = 2400001 or // alt MJD → JDN conversion + // c = 2141 or // fixed‑point month/day extraction + // c = 2000 or // observed in some conversions + // c = 65536 or // observed in some conversions + // c = 7834 or // observed in some conversions + // c = 256 or // observed in some conversions + // c = 1900 or // struct tm base‑year offset; harmless + // c = 1899 or // Observed in uses with 1900 to address off by one scenarios + // c = 292275056 or // qdatetime.h Qt Core year range first year constant + // c = 292278994 // qdatetime.h Qt Core year range last year constant +} + +/** + * Some constants indicate conversion that are ignorable, e.g., + * julian to gregorian conversion or conversions from linux time structs + * that start at 1900, etc. + */ +class IgnorableConstantArithmetic extends IgnorableOperation { + IgnorableConstantArithmetic() { + exists(int i | isLikelyConversionConstant(i) | + this.(Operation).getAnOperand().getValue().toInt() = i + or + exists(AssignArithmeticOperation a | this = a.getRValue() | + a.getRValue().getValue().toInt() = i ) ) } } +// If a unary minus assume it is some sort of conversion +class IgnorableUnaryMinus extends IgnorableOperation instanceof UnaryMinusExpr { } + +class IgnorableBinaryBitwiseOperation extends IgnorableOperation instanceof BinaryBitwiseOperation { +} + +class IgnorableUnaryBitwiseOperation extends IgnorableOperation instanceof UnaryBitwiseOperation { } + +class IgnorableAssignmentBitwiseOperation extends IgnorableOperation instanceof AssignBitwiseOperation +{ } + +/** + * Any arithmetic operation where one of the operands is a pointer or char type, ignore it + */ +class IgnorablePointerOrCharArithmetic extends IgnorableOperation { + IgnorablePointerOrCharArithmetic() { + this instanceof BinaryArithmeticOperation and + ( + this.(BinaryArithmeticOperation).getAnOperand().getUnspecifiedType() instanceof PointerType + or + this.(BinaryArithmeticOperation).getAnOperand().getUnspecifiedType() instanceof CharType + ) + or + exists(AssignArithmeticOperation a | a.getRValue() = this | + a.getAnOperand().getUnspecifiedType() instanceof PointerType + or + a.getAnOperand().getUnspecifiedType() instanceof CharType + ) + } +} + +/** + * An expression that is a candidate source for an dataflow configuration for an Operation that could flow to a Year field. + */ +predicate isOperationSourceCandidate(Expr e) { + not e instanceof IgnorableOperation and + not e.getEnclosingFunction() instanceof IgnorableFunction and + ( + e instanceof SubExpr + or + e instanceof AddExpr + or + e instanceof CrementOperation + or + exists(AssignSubExpr a | a.getRValue() = e) + or + exists(AssignAddExpr a | a.getRValue() = e) + ) +} + /** - * A unary operation (Crement) performed on a Year field. + * A Dataflow that identifies flows from an Operation (addition, subtraction, etc) to some ignorable operation (bitwise operations for example) that disqualify it */ -class YearWriteOpUnary extends YearWriteOp, UnaryOperation { - YearWriteOpUnary() { this.getOperand() instanceof YearFieldAccess } +module OperationSourceCandidateToIgnorableOperationConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node n) { isOperationSourceCandidate(n.asExpr()) } - override YearFieldAccess getYearAccess() { result = this.getOperand() } + predicate isSink(DataFlow::Node n) { n.asExpr() instanceof IgnorableOperation } - override Expr getMutationExpr() { result = this } + // looking for sources and sinks in the same function + DataFlow::FlowFeature getAFeature() { + result instanceof DataFlow::FeatureEqualSourceSinkCallContext + } + + predicate isBarrierOut(DataFlow::Node n) { isSink(n) } } +module OperationSourceCandidateToIgnorableOperationFlow = + TaintTracking::Global; + /** - * An assignment operation or mutation on the Year field of a date object. + * A dataflow that tracks an ignorable operation (eg. bitwise op) to a operation source, so we may disqualify it. */ -class YearWriteOpAssignment extends YearWriteOp, Assignment { - YearWriteOpAssignment() { this.getLValue() instanceof YearFieldAccess } +module IgnorableOperationToOperationSourceCandidateConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node n) { n.asExpr() instanceof IgnorableOperation } - override YearFieldAccess getYearAccess() { result = this.getLValue() } + predicate isSink(DataFlow::Node n) { isOperationSourceCandidate(n.asExpr()) } - override Expr getMutationExpr() { - // Note: may need to use DF analysis to pull out the original value, - // if there is excessive false positives. - if this.getOperator() = "=" - then - exists(DataFlow::Node source, DataFlow::Node sink | - sink.asExpr() = this.getRValue() and - OperationToYearAssignmentFlow::flow(source, sink) and - result = source.asExpr() - ) - else result = this + // looking for sources and sinks in the same function + DataFlow::FlowFeature getAFeature() { + result instanceof DataFlow::FeatureEqualSourceSinkCallContext } } +module IgnorableOperationToOperationSourceCandidateFlow = + TaintTracking::Global; + /** - * A DataFlow configuration for identifying flows from some non trivial access or literal - * to the Year field of a date object. + * The set of all expressions which is a candidate expression and also does not flow from to to some ignorable expression (eg. bitwise op) + * ``` + * a = something <<< 2; + * myDate.year = a + 1; // invalid + * ... + * a = someDate.year + 1; + * myDate.year = a; // valid + * ``` */ -module OperationToYearAssignmentConfig implements DataFlow::ConfigSig { - predicate isSource(DataFlow::Node n) { - not n.asExpr() instanceof Access and - not n.asExpr() instanceof Literal +class OperationSource extends Expr { + OperationSource() { + isOperationSourceCandidate(this) and + not exists(OperationSourceCandidateToIgnorableOperationFlow::PathNode src | + src.getNode().asExpr() = this and + src.isSource() + ) and + not exists(IgnorableOperationToOperationSourceCandidateFlow::PathNode sink | + sink.getNode().asExpr() = this and + sink.isSink() + ) + } +} + +class YearFieldAssignmentNode extends DataFlow::Node { + YearFieldAssignmentNode() { + this.asExpr() instanceof YearFieldAssignment + // or + // this.asDefiningArgument() instanceof YearFieldAccess + // or + // // TODO: is there a better way to do this? + // // i.e., without having to be cognizant of the addressof + // // occurring, especially if this occurs on a dataflow + // exists(AddressOfExpr aoe | + // aoe = this.asDefiningArgument() and + // aoe.getOperand() instanceof YearFieldAccess + // ) } +} + +/** + * An assignment to a year field, which will be a sink + * NOTE: could not make this abstract, it was binding to all expressions + */ +abstract class YearFieldAssignment extends Expr { + abstract YearFieldAccess getYearFieldAccess(); +} + +/** + * An assignment to x ie `x = a`. + */ +class YearFieldAssignmentAccess extends YearFieldAssignment { + YearFieldAccess access; - predicate isSink(DataFlow::Node n) { + YearFieldAssignmentAccess() { + // TODO: why can't I make this just the L value? Doesn't seem to work exists(Assignment a | - a.getLValue() instanceof YearFieldAccess and - a.getRValue() = n.asExpr() + a.getLValue() = access and + a.getRValue() = this ) } + + override YearFieldAccess getYearFieldAccess() { result = access } } -module OperationToYearAssignmentFlow = DataFlow::Global; +/** + * A year field assignment, by a unary operator ie `x++`. + */ +class YearFieldAssignmentUnary extends YearFieldAssignment instanceof CrementOperation { + YearFieldAccess access; + + YearFieldAssignmentUnary() { this.getOperand() = access } + + override YearFieldAccess getYearFieldAccess() { result = access } +} /** - * A Call to some known Time conversion function, which maps between time structs or scalars. + * A DataFlow configuration for identifying flows from some non trivial access or literal + * to the Year field of a date object. */ -class TimeConversionCall extends Call{ - TimeConversionCall(){ - this = any(TimeConversionFunction f).getACallToThisFunction() +module OperationToYearAssignmentConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node n) { n.asExpr() instanceof OperationSource } + + predicate isSink(DataFlow::Node n) { n instanceof YearFieldAssignmentNode } + + predicate isBarrier(DataFlow::Node n) { + exists(ArrayExpr arr | arr.getArrayOffset() = n.asExpr()) + or + n.asExpr().getUnspecifiedType() instanceof PointerType + or + n.asExpr().getUnspecifiedType() instanceof CharType + or + n.asExpr() instanceof IgnorableOperation + or + isLeapYearCheckSink(n) } - bindingset[var] - predicate converts(Variable var){ - this.getAnArgument().getAChild*() = var.getAnAccess() + /** Block flow out of an operation source to get the "closest" operation to the sink */ + predicate isBarrierIn(DataFlow::Node n) { isSource(n) } + + predicate isBarrierOut(DataFlow::Node n) { isSink(n) } + + // DataFlow::FlowFeature getAFeature() { result instanceof DataFlow::FeatureHasSourceCallContext } +} + +module OperationToYearAssignmentFlow = TaintTracking::Global; + +predicate isLeapYearCheckSink(DataFlow::Node sink) { + exists(ExprCheckLeapYear e | sink.asExpr() = e.getAChild*()) + or + // Local leap year check + sink.asExpr().(LeapYearFieldAccess).isUsedInCorrectLeapYearCheck() + or + // passed to function that seems to check for leap year + exists(ChecksForLeapYearFunctionCall fc | + sink.asExpr() = fc.getAnArgument() + or + sink.asExpr() = fc.getAnArgument().(FieldAccess).getQualifier() + or + exists(AddressOfExpr aoe | + fc.getAnArgument() = aoe and + aoe.getOperand() = sink.asExpr() + ) + ) +} + +/** + * A flow configuration from a Year field access to some Leap year check or guard + */ +module YearFieldAccessToLeapYearCheckConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node source) { source instanceof YearFieldAssignmentNode } + + predicate isSink(DataFlow::Node sink) { isLeapYearCheckSink(sink) } + + predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) { + // flow from a YearFieldAccess to the qualifier + node2.asExpr() = node1.asExpr().(YearFieldAccess).getQualifier() } + + // Use this to make the sink for leap year check intra-proc only + // i.e., the sink must be in the same scope (function) as the source. + // DataFlow::FlowFeature getAFeature() { + // result instanceof DataFlow::FeatureEqualSourceSinkCallContext + // } + + /** + * Enforcing the check must occur in the same call context as the source, + * i.e., do not return from the source function and check in a caller. + */ + DataFlow::FlowFeature getAFeature() { result instanceof DataFlow::FeatureHasSourceCallContext } +} + +module YearFieldAccessToLeapYearCheckFlow = + TaintTracking::Global; + +/** Does there exist a flow from the given YearFieldAccess to a Leap Year check or guard? */ +predicate isYearModifiedWithCheck(YearFieldAccess fa) { + exists(YearFieldAccessToLeapYearCheckFlow::PathNode src | + src.isSource() and + src.getNode().asExpr().(YearFieldAssignment).getYearFieldAccess() = fa + ) + or + isUsedInFeb29Check(fa) +} + +/** + * If there is a flow from a date struct year field access/assignment to a Feb 29 check + */ +predicate isUsedInFeb29Check(YearFieldAccess fa) { + exists(DateFebruary29Check check | + DataFlow::localExprFlow(fa.getQualifier(), check.getDateQualifier()) + or + DataFlow::localExprFlow(check.getDateQualifier(), fa.getQualifier()) + ) } /** - * A YearWriteOp that is non-mutating (AddressOfExpr *could* mutate but doesnt) + * An expression which checks the value of a Month field `a->month == 1`. */ -class NonMutatingYearWriteOp extends Operation instanceof YearWriteOp{ - NonMutatingYearWriteOp(){ - this instanceof AddressOfExpr +class MonthEqualityCheck extends EqualityOperation { + MonthEqualityCheck() { this.getAnOperand() instanceof MonthFieldAccess } + + Expr getExprCompared() { + exists(Expr e | + e = this.getAnOperand() and + not e instanceof MonthFieldAccess and + result = e + ) } } -from Variable var, YearWriteOp ywo -where - not ywo.isNormalizationOperation() and - not ywo instanceof NonMutatingYearWriteOp and - ywo.isYearModifedWithoutExplicitLeapYearCheck(var) and - // If there is a Time conversion after, which utilizes the variable - could lead to false negatives maybe? - not exists(TimeConversionCall c | - c.converts(var) and - ywo.getASuccessor*() = c +class MonthEqualityCheckGuard extends GuardCondition instanceof MonthEqualityCheck { } + +/** + * Verifies if the expression is guarded by a check on the Month property of a date struct, that is NOT February. + */ +bindingset[e] +pragma[inline_late] +predicate isControlledByMonthEqualityCheckNonFebruary(Expr e) { + exists(MonthEqualityCheckGuard monthGuard | + monthGuard.controls(e.getBasicBlock(), true) and + not monthGuard.(MonthEqualityCheck).getExprCompared().getValueText() = "2" ) -select ywo, - "$@: Field $@ on variable $@ has been modified, but no appropriate check for LeapYear was found.", - ywo.getEnclosingFunction(), ywo.getEnclosingFunction().toString(), - ywo.getYearAccess().getTarget(), ywo.getYearAccess().getTarget().toString(), var, var.toString() +} + +import OperationToYearAssignmentFlow::PathGraph + +from OperationToYearAssignmentFlow::PathNode src, OperationToYearAssignmentFlow::PathNode sink +where + OperationToYearAssignmentFlow::flowPath(src, sink) and + not isYearModifiedWithCheck(sink.getNode().asExpr().(YearFieldAssignment).getYearFieldAccess()) and + not isControlledByMonthEqualityCheckNonFebruary(sink.getNode().asExpr()) +select sink, src, sink, "TEST" diff --git a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql deleted file mode 100644 index c4a2f49feb31..000000000000 --- a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql +++ /dev/null @@ -1,425 +0,0 @@ -/** - * @name Year field changed using an arithmetic operation without checking for leap year (AntiPattern 1) - * @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 path-problem - * @problem.severity warning - * @id cpp/microsoft/public/leap-year/unchecked-after-arithmetic-year-modification-precise - * @precision medium - * @tags leap-year - * correctness - */ - -import cpp -import LeapYear -import semmle.code.cpp.controlflow.IRGuards - -/** - * Functions whose operations should never be considered a - * source of a dangerous leap year operation. - */ -class IgnorableFunction extends Function { - IgnorableFunction() { - // Helper utility in postgres with string time conversions - this.getName() = "DecodeISO8601Interval" - or - // helper utility for date conversions in qtbase - this.getName() = "adjacentDay" - or - // Windows API function that does timezone conversions - this.getName().matches("%SystemTimeToTzSpecificLocalTime%") - or - // Windows APIs that do time conversions - this.getName().matches("%localtime%\\_s%") - or - // Windows APIs that do time conversions - this.getName().matches("%SpecificLocalTimeToSystemTime%") - } -} - -/** - * The set of expressions which are ignorable; either because they seem to not be part of a year mutation, - * or because they seem to be a conversion pattern of mapping date scalars. - */ -abstract class IgnorableOperation extends Expr { } - -class IgnorableExprRem extends IgnorableOperation instanceof RemExpr { } - -/** - * Anything involving an operation with 10, 100, 1000, 10000 is often a sign of conversion - * or atoi. - */ -class IgnorableExpr10MulipleComponent extends IgnorableOperation { - IgnorableExpr10MulipleComponent() { - this.(Operation).getAnOperand().getValue().toInt() in [10, 100, 1000, 10000] - or - exists(AssignOperation a | a.getRValue() = this | - a.getRValue().getValue().toInt() in [10, 100, 1000, 10000] - ) - } -} - -/** - * Anything involving a sub expression with char literal 48, ignore as a likely string conversion - * e.g., X - '0' - */ -class IgnorableExpr48Mapping extends IgnorableOperation { - IgnorableExpr48Mapping() { - this.(SubExpr).getRightOperand().getValue().toInt() = 48 - or - exists(AssignSubExpr e | e.getRValue() = this | e.getRValue().getValue().toInt() = 48) - } -} - -class IgnorableCharLiteralArithmetic extends IgnorableOperation { - IgnorableCharLiteralArithmetic() { - exists(this.(BinaryArithmeticOperation).getAnOperand().(TextLiteral).getValue()) - or - exists(AssignArithmeticOperation e | e.getRValue() = this | - exists(this.(TextLiteral).getValue()) - ) - } -} - -/** - * Constants often used in date conversions (from one date data type to another) - */ -bindingset[c] -predicate isLikelyConversionConstant(int c) { - exists(int i | i = c.abs() | i >= 100) - or - c = 0 - // c = 146097 or // days in 400-year Gregorian cycle - // c = 36524 or // days in 100-year Gregorian subcycle - // c = 1461 or // days in 4-year cycle (incl. 1 leap) - // c = 32044 or // Fliegel–van Flandern JDN epoch shift - // c = 1721425 or // JDN of 0001‑01‑01 (Gregorian) - // c = 1721119 or // alt epoch offset - // c = 2400000 or // MJD → JDN conversion - // c = 2400001 or // alt MJD → JDN conversion - // c = 2141 or // fixed‑point month/day extraction - // c = 2000 or // observed in some conversions - // c = 65536 or // observed in some conversions - // c = 7834 or // observed in some conversions - // c = 256 or // observed in some conversions - // c = 1900 or // struct tm base‑year offset; harmless - // c = 1899 or // Observed in uses with 1900 to address off by one scenarios - // c = 292275056 or // qdatetime.h Qt Core year range first year constant - // c = 292278994 // qdatetime.h Qt Core year range last year constant -} - -/** - * Some constants indicate conversion that are ignorable, e.g., - * julian to gregorian conversion or conversions from linux time structs - * that start at 1900, etc. - */ -class IgnorableConstantArithmetic extends IgnorableOperation { - IgnorableConstantArithmetic() { - exists(int i | isLikelyConversionConstant(i) | - this.(Operation).getAnOperand().getValue().toInt() = i - or - exists(AssignArithmeticOperation a | this = a.getRValue() | - a.getRValue().getValue().toInt() = i - ) - ) - } -} - -// If a unary minus assume it is some sort of conversion -class IgnorableUnaryMinus extends IgnorableOperation instanceof UnaryMinusExpr { } - -class IgnorableBinaryBitwiseOperation extends IgnorableOperation instanceof BinaryBitwiseOperation { -} - -class IgnorableUnaryBitwiseOperation extends IgnorableOperation instanceof UnaryBitwiseOperation { } - -class IgnorableAssignmentBitwiseOperation extends IgnorableOperation instanceof AssignBitwiseOperation -{ } - -/** - * Any arithmetic operation where one of the operands is a pointer or char type, ignore it - */ -class IgnorablePointerOrCharArithmetic extends IgnorableOperation { - IgnorablePointerOrCharArithmetic() { - this instanceof BinaryArithmeticOperation and - ( - this.(BinaryArithmeticOperation).getAnOperand().getUnspecifiedType() instanceof PointerType - or - this.(BinaryArithmeticOperation).getAnOperand().getUnspecifiedType() instanceof CharType - ) - or - exists(AssignArithmeticOperation a | a.getRValue() = this | - a.getAnOperand().getUnspecifiedType() instanceof PointerType - or - a.getAnOperand().getUnspecifiedType() instanceof CharType - ) - } -} - -/** - * An expression that is a candidate source for an dataflow configuration for an Operation that could flow to a Year field. - */ -predicate isOperationSourceCandidate(Expr e) { - not e instanceof IgnorableOperation and - not e.getEnclosingFunction() instanceof IgnorableFunction and - ( - e instanceof SubExpr - or - e instanceof AddExpr - or - e instanceof CrementOperation - or - exists(AssignSubExpr a | a.getRValue() = e) - or - exists(AssignAddExpr a | a.getRValue() = e) - ) -} - -/** - * A Dataflow that identifies flows from an Operation (addition, subtraction, etc) to some ignorable operation (bitwise operations for example) that disqualify it - */ -module OperationSourceCandidateToIgnorableOperationConfig implements DataFlow::ConfigSig { - predicate isSource(DataFlow::Node n) { isOperationSourceCandidate(n.asExpr()) } - - predicate isSink(DataFlow::Node n) { n.asExpr() instanceof IgnorableOperation } - - // looking for sources and sinks in the same function - DataFlow::FlowFeature getAFeature() { - result instanceof DataFlow::FeatureEqualSourceSinkCallContext - } - - predicate isBarrierOut(DataFlow::Node n) { isSink(n) } -} - -module OperationSourceCandidateToIgnorableOperationFlow = - TaintTracking::Global; - -/** - * A dataflow that tracks an ignorable operation (eg. bitwise op) to a operation source, so we may disqualify it. - */ -module IgnorableOperationToOperationSourceCandidateConfig implements DataFlow::ConfigSig { - predicate isSource(DataFlow::Node n) { n.asExpr() instanceof IgnorableOperation } - - predicate isSink(DataFlow::Node n) { isOperationSourceCandidate(n.asExpr()) } - - // looking for sources and sinks in the same function - DataFlow::FlowFeature getAFeature() { - result instanceof DataFlow::FeatureEqualSourceSinkCallContext - } -} - -module IgnorableOperationToOperationSourceCandidateFlow = - TaintTracking::Global; - -/** - * The set of all expressions which is a candidate expression and also does not flow from to to some ignorable expression (eg. bitwise op) - * ``` - * a = something <<< 2; - * myDate.year = a + 1; // invalid - * ... - * a = someDate.year + 1; - * myDate.year = a; // valid - * ``` - */ -class OperationSource extends Expr { - OperationSource() { - isOperationSourceCandidate(this) and - not exists(OperationSourceCandidateToIgnorableOperationFlow::PathNode src | - src.getNode().asExpr() = this and - src.isSource() - ) and - not exists(IgnorableOperationToOperationSourceCandidateFlow::PathNode sink | - sink.getNode().asExpr() = this and - sink.isSink() - ) - } -} - -class YearFieldAssignmentNode extends DataFlow::Node { - YearFieldAssignmentNode() { - this.asExpr() instanceof YearFieldAssignment - // or - // this.asDefiningArgument() instanceof YearFieldAccess - // or - // // TODO: is there a better way to do this? - // // i.e., without having to be cognizant of the addressof - // // occurring, especially if this occurs on a dataflow - // exists(AddressOfExpr aoe | - // aoe = this.asDefiningArgument() and - // aoe.getOperand() instanceof YearFieldAccess - // ) - } -} - -/** - * An assignment to a year field, which will be a sink - * NOTE: could not make this abstract, it was binding to all expressions - */ -abstract class YearFieldAssignment extends Expr { - abstract YearFieldAccess getYearFieldAccess(); -} - -/** - * An assignment to x ie `x = a`. - */ -class YearFieldAssignmentAccess extends YearFieldAssignment { - YearFieldAccess access; - - YearFieldAssignmentAccess() { - // TODO: why can't I make this just the L value? Doesn't seem to work - exists(Assignment a | - a.getLValue() = access and - a.getRValue() = this - ) - } - - override YearFieldAccess getYearFieldAccess() { result = access } -} - -/** - * A year field assignment, by a unary operator ie `x++`. - */ -class YearFieldAssignmentUnary extends YearFieldAssignment instanceof CrementOperation { - YearFieldAccess access; - - YearFieldAssignmentUnary() { this.getOperand() = access } - - override YearFieldAccess getYearFieldAccess() { result = access } -} - -/** - * A DataFlow configuration for identifying flows from some non trivial access or literal - * to the Year field of a date object. - */ -module OperationToYearAssignmentConfig implements DataFlow::ConfigSig { - predicate isSource(DataFlow::Node n) { n.asExpr() instanceof OperationSource } - - predicate isSink(DataFlow::Node n) { n instanceof YearFieldAssignmentNode } - - predicate isBarrier(DataFlow::Node n) { - exists(ArrayExpr arr | arr.getArrayOffset() = n.asExpr()) - or - n.asExpr().getUnspecifiedType() instanceof PointerType - or - n.asExpr().getUnspecifiedType() instanceof CharType - or - n.asExpr() instanceof IgnorableOperation - or - isLeapYearCheckSink(n) - } - - /** Block flow out of an operation source to get the "closest" operation to the sink */ - predicate isBarrierIn(DataFlow::Node n) { isSource(n) } - - predicate isBarrierOut(DataFlow::Node n) { isSink(n) } - - // DataFlow::FlowFeature getAFeature() { result instanceof DataFlow::FeatureHasSourceCallContext } -} - -module OperationToYearAssignmentFlow = TaintTracking::Global; - -predicate isLeapYearCheckSink(DataFlow::Node sink) { - exists(ExprCheckLeapYear e | sink.asExpr() = e.getAChild*()) - or - // Local leap year check - sink.asExpr().(LeapYearFieldAccess).isUsedInCorrectLeapYearCheck() - or - // passed to function that seems to check for leap year - exists(ChecksForLeapYearFunctionCall fc | - sink.asExpr() = fc.getAnArgument() - or - sink.asExpr() = fc.getAnArgument().(FieldAccess).getQualifier() - or - exists(AddressOfExpr aoe | - fc.getAnArgument() = aoe and - aoe.getOperand() = sink.asExpr() - ) - ) -} - -/** - * A flow configuration from a Year field access to some Leap year check or guard - */ -module YearFieldAccessToLeapYearCheckConfig implements DataFlow::ConfigSig { - predicate isSource(DataFlow::Node source) { source instanceof YearFieldAssignmentNode } - - predicate isSink(DataFlow::Node sink) { isLeapYearCheckSink(sink) } - - predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) { - // flow from a YearFieldAccess to the qualifier - node2.asExpr() = node1.asExpr().(YearFieldAccess).getQualifier() - } - - // Use this to make the sink for leap year check intra-proc only - // i.e., the sink must be in the same scope (function) as the source. - // DataFlow::FlowFeature getAFeature() { - // result instanceof DataFlow::FeatureEqualSourceSinkCallContext - // } - - /** - * Enforcing the check must occur in the same call context as the source, - * i.e., do not return from the source function and check in a caller. - */ - DataFlow::FlowFeature getAFeature() { result instanceof DataFlow::FeatureHasSourceCallContext } -} - -module YearFieldAccessToLeapYearCheckFlow = - TaintTracking::Global; - -/** Does there exist a flow from the given YearFieldAccess to a Leap Year check or guard? */ -predicate isYearModifiedWithCheck(YearFieldAccess fa) { - exists(YearFieldAccessToLeapYearCheckFlow::PathNode src | - src.isSource() and - src.getNode().asExpr().(YearFieldAssignment).getYearFieldAccess() = fa - ) - or - isUsedInFeb29Check(fa) -} - -/** - * If there is a flow from a date struct year field access/assignment to a Feb 29 check - */ -predicate isUsedInFeb29Check(YearFieldAccess fa) { - exists(DateFebruary29Check check | - DataFlow::localExprFlow(fa.getQualifier(), check.getDateQualifier()) - or - DataFlow::localExprFlow(check.getDateQualifier(), fa.getQualifier()) - ) -} - -/** - * An expression which checks the value of a Month field `a->month == 1`. - */ -class MonthEqualityCheck extends EqualityOperation { - MonthEqualityCheck() { this.getAnOperand() instanceof MonthFieldAccess } - - Expr getExprCompared() { - exists(Expr e | - e = this.getAnOperand() and - not e instanceof MonthFieldAccess and - result = e - ) - } -} - -class MonthEqualityCheckGuard extends GuardCondition instanceof MonthEqualityCheck { } - -/** - * Verifies if the expression is guarded by a check on the Month property of a date struct, that is NOT February. - */ -bindingset[e] -pragma[inline_late] -predicate isControlledByMonthEqualityCheckNonFebruary(Expr e) { - exists(MonthEqualityCheckGuard monthGuard | - monthGuard.controls(e.getBasicBlock(), true) and - not monthGuard.(MonthEqualityCheck).getExprCompared().getValueText() = "2" - ) -} - -import OperationToYearAssignmentFlow::PathGraph - -from OperationToYearAssignmentFlow::PathNode src, OperationToYearAssignmentFlow::PathNode sink -where - OperationToYearAssignmentFlow::flowPath(src, sink) and - not isYearModifiedWithCheck(sink.getNode().asExpr().(YearFieldAssignment).getYearFieldAccess()) and - not isControlledByMonthEqualityCheckNonFebruary(sink.getNode().asExpr()) -select sink, src, sink, "TEST" diff --git a/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/UncheckedLeapYearAfterYearModificationPrecise.expected b/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/UncheckedLeapYearAfterYearModificationPrecise.expected deleted file mode 100644 index 555002b40867..000000000000 --- a/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/UncheckedLeapYearAfterYearModificationPrecise.expected +++ /dev/null @@ -1,8 +0,0 @@ -| test.cpp:617:2:617:11 | ... ++ | $@: Field $@ on variable $@ has been modified, but no appropriate check for LeapYear was found. | test.cpp:611:6:611:32 | AntiPattern_1_year_addition | AntiPattern_1_year_addition | test.cpp:12:7:12:11 | wYear | wYear | test.cpp:613:13:613:14 | st | st | -| test.cpp:634:2:634:25 | ... += ... | $@: Field $@ on variable $@ has been modified, but no appropriate check for LeapYear was found. | test.cpp:627:6:627:32 | AntiPattern_simple_addition | AntiPattern_simple_addition | test.cpp:12:7:12:11 | wYear | wYear | test.cpp:629:13:629:14 | st | st | -| test.cpp:763:2:763:19 | ... ++ | $@: Field $@ on variable $@ has been modified, but no appropriate check for LeapYear was found. | test.cpp:756:6:756:40 | AntiPattern_year_addition_struct_tm | AntiPattern_year_addition_struct_tm | test.cpp:56:6:56:12 | tm_year | tm_year | test.cpp:759:12:759:19 | timeinfo | timeinfo | -| test.cpp:800:2:800:40 | ... = ... | $@: Field $@ on variable $@ has been modified, but no appropriate check for LeapYear was found. | test.cpp:791:6:791:23 | FalseNegativeTests | FalseNegativeTests | test.cpp:56:6:56:12 | tm_year | tm_year | test.cpp:793:12:793:19 | timeinfo | timeinfo | -| test.cpp:803:2:803:43 | ... = ... | $@: Field $@ on variable $@ has been modified, but no appropriate check for LeapYear was found. | test.cpp:791:6:791:23 | FalseNegativeTests | FalseNegativeTests | test.cpp:56:6:56:12 | tm_year | tm_year | test.cpp:793:12:793:19 | timeinfo | timeinfo | -| test.cpp:808:2:808:24 | ... = ... | $@: Field $@ on variable $@ has been modified, but no appropriate check for LeapYear was found. | test.cpp:791:6:791:23 | FalseNegativeTests | FalseNegativeTests | test.cpp:12:7:12:11 | wYear | wYear | test.cpp:794:13:794:14 | st | st | -| test.cpp:811:2:811:33 | ... = ... | $@: Field $@ on variable $@ has been modified, but no appropriate check for LeapYear was found. | test.cpp:791:6:791:23 | FalseNegativeTests | FalseNegativeTests | test.cpp:12:7:12:11 | wYear | wYear | test.cpp:794:13:794:14 | st | st | -| test.cpp:850:3:850:36 | ... = ... | $@: Field $@ on variable $@ has been modified, but no appropriate check for LeapYear was found. | test.cpp:818:6:818:23 | tp_intermediaryVar | tp_intermediaryVar | test.cpp:56:6:56:12 | tm_year | tm_year | test.cpp:70:18:70:19 | tm | tm | diff --git a/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/UncheckedLeapYearAfterYearModificationPrecise.qlref b/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/UncheckedLeapYearAfterYearModificationPrecise.qlref deleted file mode 100644 index 17b1bf722ba6..000000000000 --- a/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/UncheckedLeapYearAfterYearModificationPrecise.qlref +++ /dev/null @@ -1 +0,0 @@ -Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationPrecise.ql From 87560891605dcdf2631a7f230957b93bb7800a56 Mon Sep 17 00:00:00 2001 From: "REDMOND\\brodes" Date: Thu, 15 Jan 2026 15:16:42 -0500 Subject: [PATCH 25/28] Removing hashcons usages from LeapYear.qll, inefficient and I'm not sure they are actually necessary or providing much utility. --- cpp/ql/src/Likely Bugs/Leap Year/LeapYear.qll | 48 +++++-------------- 1 file changed, 12 insertions(+), 36 deletions(-) diff --git a/cpp/ql/src/Likely Bugs/Leap Year/LeapYear.qll b/cpp/ql/src/Likely Bugs/Leap Year/LeapYear.qll index 4b36082bd451..0742ab2a71a0 100644 --- a/cpp/ql/src/Likely Bugs/Leap Year/LeapYear.qll +++ b/cpp/ql/src/Likely Bugs/Leap Year/LeapYear.qll @@ -5,7 +5,6 @@ import cpp import semmle.code.cpp.dataflow.new.TaintTracking import semmle.code.cpp.commons.DateTime -import semmle.code.cpp.valuenumbering.HashCons /** * Get the top-level `BinaryOperation` enclosing the expression e. @@ -42,7 +41,6 @@ class CheckForLeapYearOperation extends Expr { } } -bindingset[modVal] Expr moduloCheckEQ_0(EQExpr eq, int modVal) { exists(RemExpr rem | rem = eq.getLeftOperand() | result = rem.getLeftOperand() and @@ -51,7 +49,6 @@ Expr moduloCheckEQ_0(EQExpr eq, int modVal) { eq.getRightOperand().getValue().toInt() = 0 } -bindingset[modVal] Expr moduloCheckNEQ_0(NEExpr neq, int modVal) { exists(RemExpr rem | rem = neq.getLeftOperand() | result = rem.getLeftOperand() and @@ -60,17 +57,6 @@ Expr moduloCheckNEQ_0(NEExpr neq, int modVal) { neq.getRightOperand().getValue().toInt() = 0 } -/** - * Returns if the two expressions resolve to the same value, albeit it is a fuzzy attempt. - * SSA is not fit for purpose here as calls break SSA equivalence. - */ -bindingset[e1, e2] -pragma[inline_late] -predicate exprEq_propertyPermissive(Expr e1, Expr e2) { - not e1 = e2 and - hashCons(e1) = hashCons(e2) -} - /** * An expression that is the subject of a mod-4 check. * ie `expr % 4 == 0` @@ -196,8 +182,7 @@ class ExprCheckCenturyComponent extends LogicalOrExpr { ExprCheckCenturyComponent() { exists(ExprCheckCenturyComponentDiv400 exprDiv400, ExprCheckCenturyComponentDiv100 exprDiv100 | this.getAnOperand() = exprDiv100 and - this.getAnOperand() = exprDiv400 and - exprEq_propertyPermissive(exprDiv100.getYearExpr(), exprDiv400.getYearExpr()) + this.getAnOperand() = exprDiv400 ) } @@ -222,8 +207,7 @@ final class ExprCheckLeapYearFormA extends ExprCheckLeapYear, LogicalAndExpr { ExprCheckLeapYearFormA() { exists(Expr e, ExprCheckCenturyComponent centuryCheck | e = moduloCheckEQ_0(this.getLeftOperand(), 4) and - centuryCheck = this.getAnOperand().getAChild*() and - exprEq_propertyPermissive(e, centuryCheck.getYearExpr()) + centuryCheck = this.getAnOperand().getAChild*() ) } } @@ -238,12 +222,7 @@ final class ExprCheckLeapYearFormB extends ExprCheckLeapYear, LogicalOrExpr { exists(VariableAccess va1, VariableAccess va2, VariableAccess va3 | va1 = moduloCheckEQ_0(this.getAnOperand(), 400) and va2 = moduloCheckNEQ_0(this.getAnOperand().(LogicalAndExpr).getAnOperand(), 100) and - va3 = moduloCheckEQ_0(this.getAnOperand().(LogicalAndExpr).getAnOperand(), 4) and - // The 400-leap year check may be offset by [1900,1970,2000]. - exists(Expr va1_subExpr | va1_subExpr = va1.getAChild*() | - exprEq_propertyPermissive(va1_subExpr, va2) and - exprEq_propertyPermissive(va2, va3) - ) + va3 = moduloCheckEQ_0(this.getAnOperand().(LogicalAndExpr).getAnOperand(), 4) ) } } @@ -384,30 +363,26 @@ class StructTmLeapYearFieldAccess extends LeapYearFieldAccess { * `stDate.wMonth == 2` */ class DateCheckMonthFebruary extends Operation { - DateCheckMonthFebruary(){ + DateCheckMonthFebruary() { this.getOperator() = "==" and this.getAnOperand() instanceof MonthFieldAccess and this.getAnOperand().(Literal).getValue() = "2" } - Expr getDateQualifier(){ - result = this.getAnOperand().(MonthFieldAccess).getQualifier() - } + Expr getDateQualifier() { result = this.getAnOperand().(MonthFieldAccess).getQualifier() } } /** * `stDate.wDay == 29` */ class DateCheckDay29 extends Operation { - DateCheckDay29(){ + DateCheckDay29() { this.getOperator() = "==" and this.getAnOperand() instanceof DayFieldAccess and this.getAnOperand().(Literal).getValue() = "29" } - Expr getDateQualifier(){ - result = this.getAnOperand().(DayFieldAccess).getQualifier() - } + Expr getDateQualifier() { result = this.getAnOperand().(DayFieldAccess).getQualifier() } } /** @@ -415,16 +390,17 @@ class DateCheckDay29 extends Operation { * `stDate.wMonth == 2 && stDate.wDay == 29` */ class DateFebruary29Check extends Operation { - DateFebruary29Check(){ + DateFebruary29Check() { this.getOperator() = "&&" and exists(DateCheckMonthFebruary checkFeb, DateCheckDay29 check29 | checkFeb = this.getAnOperand() and - check29 = this.getAnOperand() and - hashCons(checkFeb.getDateQualifier()) = hashCons(check29.getDateQualifier()) + check29 = this.getAnOperand() + // and + // hashCons(checkFeb.getDateQualifier()) = hashCons(check29.getDateQualifier()) ) } - Expr getDateQualifier(){ + Expr getDateQualifier() { result = this.getAnOperand().(DateCheckMonthFebruary).getDateQualifier() } } From 2e7dba2a1750699215b0496d3305352e93685356 Mon Sep 17 00:00:00 2001 From: "REDMOND\\brodes" Date: Thu, 15 Jan 2026 15:23:14 -0500 Subject: [PATCH 26/28] Comment cleanup --- .../UncheckedLeapYearAfterYearModification.ql | 23 ++++--------------- 1 file changed, 4 insertions(+), 19 deletions(-) diff --git a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification.ql b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification.ql index 8681ace34620..88afb6137c5b 100644 --- a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification.ql +++ b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification.ql @@ -82,29 +82,16 @@ class IgnorableCharLiteralArithmetic extends IgnorableOperation { /** * Constants often used in date conversions (from one date data type to another) + * Numerous examples exist, like 1900 or 2000 that convert years from one + * representation to another. All examples found tend to be 100 or greater, + * so just using this as a heuristic for detecting such conversion constants. + * Also '0' is sometimes observed as an atoi style conversion. */ bindingset[c] predicate isLikelyConversionConstant(int c) { exists(int i | i = c.abs() | i >= 100) or c = 0 - // c = 146097 or // days in 400-year Gregorian cycle - // c = 36524 or // days in 100-year Gregorian subcycle - // c = 1461 or // days in 4-year cycle (incl. 1 leap) - // c = 32044 or // Fliegel–van Flandern JDN epoch shift - // c = 1721425 or // JDN of 0001‑01‑01 (Gregorian) - // c = 1721119 or // alt epoch offset - // c = 2400000 or // MJD → JDN conversion - // c = 2400001 or // alt MJD → JDN conversion - // c = 2141 or // fixed‑point month/day extraction - // c = 2000 or // observed in some conversions - // c = 65536 or // observed in some conversions - // c = 7834 or // observed in some conversions - // c = 256 or // observed in some conversions - // c = 1900 or // struct tm base‑year offset; harmless - // c = 1899 or // Observed in uses with 1900 to address off by one scenarios - // c = 292275056 or // qdatetime.h Qt Core year range first year constant - // c = 292278994 // qdatetime.h Qt Core year range last year constant } /** @@ -311,7 +298,6 @@ module OperationToYearAssignmentConfig implements DataFlow::ConfigSig { predicate isBarrierIn(DataFlow::Node n) { isSource(n) } predicate isBarrierOut(DataFlow::Node n) { isSink(n) } - // DataFlow::FlowFeature getAFeature() { result instanceof DataFlow::FeatureHasSourceCallContext } } @@ -354,7 +340,6 @@ module YearFieldAccessToLeapYearCheckConfig implements DataFlow::ConfigSig { // DataFlow::FlowFeature getAFeature() { // result instanceof DataFlow::FeatureEqualSourceSinkCallContext // } - /** * Enforcing the check must occur in the same call context as the source, * i.e., do not return from the source function and check in a caller. From 9446a6b8e555a8a5663deb8c19a114ce1404a0f0 Mon Sep 17 00:00:00 2001 From: "REDMOND\\brodes" Date: Thu, 15 Jan 2026 15:53:05 -0500 Subject: [PATCH 27/28] Updating query alert message. --- .../Leap Year/UncheckedLeapYearAfterYearModification.ql | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification.ql b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification.ql index 88afb6137c5b..d16cb1ce49aa 100644 --- a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification.ql +++ b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification.ql @@ -407,4 +407,5 @@ where OperationToYearAssignmentFlow::flowPath(src, sink) and not isYearModifiedWithCheck(sink.getNode().asExpr().(YearFieldAssignment).getYearFieldAccess()) and not isControlledByMonthEqualityCheckNonFebruary(sink.getNode().asExpr()) -select sink, src, sink, "TEST" +select sink, src, sink, + "Year field has been modified, but no appropriate check for LeapYear was found." From bc76018c40fdffa911b545fb32312551700bfbdd Mon Sep 17 00:00:00 2001 From: "REDMOND\\brodes" Date: Fri, 16 Jan 2026 14:48:48 -0500 Subject: [PATCH 28/28] Misc. false positive and false negative updates. as a response to reviewing the unit tests and conversations about how to handle some of the fp/fn cases observed. Updated the unit tests to use InlineExpectationsTestQuery.ql so it is easier to detect FP/FNs. --- cpp/ql/src/Likely Bugs/Leap Year/LeapYear.qll | 4 +- .../UncheckedLeapYearAfterYearModification.ql | 240 +++++++++++++----- ...ckedLeapYearAfterYearModification.expected | 69 ++++- ...checkedLeapYearAfterYearModification.qlref | 3 +- .../test.cpp | 97 +++---- 5 files changed, 285 insertions(+), 128 deletions(-) diff --git a/cpp/ql/src/Likely Bugs/Leap Year/LeapYear.qll b/cpp/ql/src/Likely Bugs/Leap Year/LeapYear.qll index 0742ab2a71a0..39e56627db2a 100644 --- a/cpp/ql/src/Likely Bugs/Leap Year/LeapYear.qll +++ b/cpp/ql/src/Likely Bugs/Leap Year/LeapYear.qll @@ -395,8 +395,6 @@ class DateFebruary29Check extends Operation { exists(DateCheckMonthFebruary checkFeb, DateCheckDay29 check29 | checkFeb = this.getAnOperand() and check29 = this.getAnOperand() - // and - // hashCons(checkFeb.getDateQualifier()) = hashCons(check29.getDateQualifier()) ) } @@ -536,7 +534,7 @@ class TimeConversionFunction extends Function { "FileTimeToSystemTime", "SystemTimeToFileTime", "SystemTimeToTzSpecificLocalTime", "SystemTimeToTzSpecificLocalTimeEx", "TzSpecificLocalTimeToSystemTime", "TzSpecificLocalTimeToSystemTimeEx", "RtlLocalTimeToSystemTime", - "RtlTimeToSecondsSince1970", "_mkgmtime" + "RtlTimeToSecondsSince1970", "_mkgmtime", "SetSystemTime" ] } } diff --git a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification.ql b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification.ql index d16cb1ce49aa..21b6403f2c13 100644 --- a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification.ql +++ b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification.ql @@ -15,7 +15,7 @@ import semmle.code.cpp.controlflow.IRGuards /** * Functions whose operations should never be considered a - * source of a dangerous leap year operation. + * source or sink of a dangerous leap year operation. */ class IgnorableFunction extends Function { IgnorableFunction() { @@ -33,6 +33,18 @@ class IgnorableFunction extends Function { or // Windows APIs that do time conversions this.getName().matches("%SpecificLocalTimeToSystemTime%") + or + // postgres function for diffing timestamps, date for leap year + // is not applicable. + this.getName().toLowerCase().matches("%timestamp%age%") + or + // Reading byte streams often involves operations of some base, but that's + // not a real source of leap year issues. + this.getName().toLowerCase().matches("%read%bytes%") + or + // A postgres function for local time conversions + // conversion operations (from one time structure to another) are generally ignorable + this.getName() = "localsub" } } @@ -83,15 +95,32 @@ class IgnorableCharLiteralArithmetic extends IgnorableOperation { /** * Constants often used in date conversions (from one date data type to another) * Numerous examples exist, like 1900 or 2000 that convert years from one - * representation to another. All examples found tend to be 100 or greater, - * so just using this as a heuristic for detecting such conversion constants. + * representation to another. * Also '0' is sometimes observed as an atoi style conversion. */ bindingset[c] predicate isLikelyConversionConstant(int c) { - exists(int i | i = c.abs() | i >= 100) - or - c = 0 + exists(int i | i = c.abs() | + //| i >= 100) + i = 146097 or // days in 400-year Gregorian cycle + i = 36524 or // days in 100-year Gregorian subcycle + i = 1461 or // days in 4-year cycle (incl. 1 leap) + i = 32044 or // Fliegel–van Flandern JDN epoch shift + i = 1721425 or // JDN of 0001‑01‑01 (Gregorian) + i = 1721119 or // alt epoch offset + i = 2400000 or // MJD → JDN conversion + i = 2400001 or // alt MJD → JDN conversion + i = 2141 or // fixed‑point month/day extraction + i = 2000 or // observed in some conversions + i = 65536 or // observed in some conversions + i = 7834 or // observed in some conversions + i = 256 or // observed in some conversions + i = 1900 or // struct tm base‑year offset; harmless + i = 1899 or // Observed in uses with 1900 to address off by one scenarios + i = 292275056 or // qdatetime.h Qt Core year range first year constant + i = 292278994 or // qdatetime.h Qt Core year range last year constant + i = 0 + ) } /** @@ -112,7 +141,35 @@ class IgnorableConstantArithmetic extends IgnorableOperation { } // If a unary minus assume it is some sort of conversion -class IgnorableUnaryMinus extends IgnorableOperation instanceof UnaryMinusExpr { } +class IgnorableUnaryMinus extends IgnorableOperation { + IgnorableUnaryMinus() { + this instanceof UnaryMinusExpr + or + this.(Operation).getAnOperand() instanceof UnaryMinusExpr + } +} + +class OperationAsArgToIgnorableFunction extends IgnorableOperation { + OperationAsArgToIgnorableFunction() { + exists(Call c | + c.getAnArgument().getAChild*() = this and + c.getTarget() instanceof IgnorableFunction + ) + } +} + +/** + * Literal OP literal means the result is constant/known + * and the operation is basically ignorable (it's not a real operation but + * probably one visual simplicity what it means). + */ +class ConstantBinaryArithmeticOperation extends IgnorableOperation { + ConstantBinaryArithmeticOperation() { + this instanceof BinaryArithmeticOperation and + this.(BinaryArithmeticOperation).getLeftOperand() instanceof Literal and + this.(BinaryArithmeticOperation).getRightOperand() instanceof Literal + } +} class IgnorableBinaryBitwiseOperation extends IgnorableOperation instanceof BinaryBitwiseOperation { } @@ -132,12 +189,31 @@ class IgnorablePointerOrCharArithmetic extends IgnorableOperation { this.(BinaryArithmeticOperation).getAnOperand().getUnspecifiedType() instanceof PointerType or this.(BinaryArithmeticOperation).getAnOperand().getUnspecifiedType() instanceof CharType + or + // Operations on calls to functions that accept char or char* + this.(BinaryArithmeticOperation) + .getAnOperand() + .(Call) + .getAnArgument() + .getUnspecifiedType() + .stripType() instanceof CharType + or + // Operations on calls to functions named like "strlen", "wcslen", etc + // NOTE: workaround for cases where the wchar_t type is not a char, but an unsigned short + // unclear if there is a best way to filter cases like these out based on type info. + this.(BinaryArithmeticOperation).getAnOperand().(Call).getTarget().getName().matches("%len%") ) or exists(AssignArithmeticOperation a | a.getRValue() = this | a.getAnOperand().getUnspecifiedType() instanceof PointerType or a.getAnOperand().getUnspecifiedType() instanceof CharType + or + // Operations on calls to functions that accept char or char* + a.getAnOperand().(Call).getAnArgument().getUnspecifiedType().stripType() instanceof CharType + or + // Operations on calls to functions named like "strlen", "wcslen", etc + this.(BinaryArithmeticOperation).getAnOperand().(Call).getTarget().getName().matches("%len%") ) } } @@ -161,25 +237,6 @@ predicate isOperationSourceCandidate(Expr e) { ) } -/** - * A Dataflow that identifies flows from an Operation (addition, subtraction, etc) to some ignorable operation (bitwise operations for example) that disqualify it - */ -module OperationSourceCandidateToIgnorableOperationConfig implements DataFlow::ConfigSig { - predicate isSource(DataFlow::Node n) { isOperationSourceCandidate(n.asExpr()) } - - predicate isSink(DataFlow::Node n) { n.asExpr() instanceof IgnorableOperation } - - // looking for sources and sinks in the same function - DataFlow::FlowFeature getAFeature() { - result instanceof DataFlow::FeatureEqualSourceSinkCallContext - } - - predicate isBarrierOut(DataFlow::Node n) { isSink(n) } -} - -module OperationSourceCandidateToIgnorableOperationFlow = - TaintTracking::Global; - /** * A dataflow that tracks an ignorable operation (eg. bitwise op) to a operation source, so we may disqualify it. */ @@ -210,10 +267,10 @@ module IgnorableOperationToOperationSourceCandidateFlow = class OperationSource extends Expr { OperationSource() { isOperationSourceCandidate(this) and - not exists(OperationSourceCandidateToIgnorableOperationFlow::PathNode src | - src.getNode().asExpr() = this and - src.isSource() - ) and + // If the candidate came from an ignorable operation, ignore the candidate + // NOTE: we cannot easily flow the candidate to an ignorable operation as that can + // be tricky in practice, e.g., a mod operation on a year would be part of a leap year check + // but a mod operation ending in a year is more indicative of something to ignore (a conversion) not exists(IgnorableOperationToOperationSourceCandidateFlow::PathNode sink | sink.getNode().asExpr() = this and sink.isSink() @@ -223,28 +280,37 @@ class OperationSource extends Expr { class YearFieldAssignmentNode extends DataFlow::Node { YearFieldAssignmentNode() { - this.asExpr() instanceof YearFieldAssignment - // or - // this.asDefiningArgument() instanceof YearFieldAccess - // or - // // TODO: is there a better way to do this? - // // i.e., without having to be cognizant of the addressof - // // occurring, especially if this occurs on a dataflow - // exists(AddressOfExpr aoe | - // aoe = this.asDefiningArgument() and - // aoe.getOperand() instanceof YearFieldAccess - // ) + not this.getEnclosingCallable().getUnderlyingCallable() instanceof IgnorableFunction and + ( + this.asExpr() instanceof YearFieldAssignment or + this.asDefiningArgument() instanceof YearFieldOutArgAssignment + ) } } /** * An assignment to a year field, which will be a sink - * NOTE: could not make this abstract, it was binding to all expressions */ abstract class YearFieldAssignment extends Expr { abstract YearFieldAccess getYearFieldAccess(); } +class YearFieldOutArgAssignment extends YearFieldAssignment { + YearFieldAccess access; + + YearFieldOutArgAssignment() { + exists(Call c | c.getAnArgument() = access and this = access) + or + exists(Call c, AddressOfExpr aoe | + c.getAnArgument() = aoe and + aoe.getOperand() = access and + this = aoe + ) + } + + override YearFieldAccess getYearFieldAccess() { result = access } +} + /** * An assignment to x ie `x = a`. */ @@ -298,7 +364,6 @@ module OperationToYearAssignmentConfig implements DataFlow::ConfigSig { predicate isBarrierIn(DataFlow::Node n) { isSource(n) } predicate isBarrierOut(DataFlow::Node n) { isSink(n) } - // DataFlow::FlowFeature getAFeature() { result instanceof DataFlow::FeatureHasSourceCallContext } } module OperationToYearAssignmentFlow = TaintTracking::Global; @@ -335,11 +400,6 @@ module YearFieldAccessToLeapYearCheckConfig implements DataFlow::ConfigSig { node2.asExpr() = node1.asExpr().(YearFieldAccess).getQualifier() } - // Use this to make the sink for leap year check intra-proc only - // i.e., the sink must be in the same scope (function) as the source. - // DataFlow::FlowFeature getAFeature() { - // result instanceof DataFlow::FeatureEqualSourceSinkCallContext - // } /** * Enforcing the check must occur in the same call context as the source, * i.e., do not return from the source function and check in a caller. @@ -357,20 +417,26 @@ predicate isYearModifiedWithCheck(YearFieldAccess fa) { src.getNode().asExpr().(YearFieldAssignment).getYearFieldAccess() = fa ) or - isUsedInFeb29Check(fa) -} - -/** - * If there is a flow from a date struct year field access/assignment to a Feb 29 check - */ -predicate isUsedInFeb29Check(YearFieldAccess fa) { - exists(DateFebruary29Check check | - DataFlow::localExprFlow(fa.getQualifier(), check.getDateQualifier()) - or - DataFlow::localExprFlow(check.getDateQualifier(), fa.getQualifier()) + // If the time flows to a time conversion whose value/result is checked, + // assume the leap year is being handled. + exists(TimeStructToCheckedTimeConversionFlow::PathNode timeQualSrc | + timeQualSrc.isSource() and + timeQualSrc.getNode().asExpr() = fa.getQualifier() ) + // or + // isUsedInFeb29Check(fa) } +// /** +// * If there is a flow from a date struct year field access/assignment to a Feb 29 check +// */ +// predicate isUsedInFeb29Check(YearFieldAccess fa) { +// exists(DateFebruary29Check check | +// DataFlow::localExprFlow(fa.getQualifier(), check.getDateQualifier()) +// or +// DataFlow::localExprFlow(check.getDateQualifier(), fa.getQualifier()) +// ) +// } /** * An expression which checks the value of a Month field `a->month == 1`. */ @@ -400,12 +466,66 @@ predicate isControlledByMonthEqualityCheckNonFebruary(Expr e) { ) } +module TimeStructToCheckedTimeConversionConfig implements DataFlow::StateConfigSig { + class FlowState = boolean; + + predicate isSource(DataFlow::Node source, FlowState state) { + exists(YearFieldAccess yfa | source.asExpr() = yfa.getQualifier()) and + state = false + } + + predicate isSink(DataFlow::Node sink, FlowState state) { + state = true and + exists(IfStmt ifs | ifs.getCondition().getAChild*() = sink.asExpr()) + } + + predicate isAdditionalFlowStep( + DataFlow::Node node1, FlowState state1, DataFlow::Node node2, FlowState state2 + ) { + state1 in [true, false] and + state2 = true and + exists(Call c | + c.getTarget() instanceof TimeConversionFunction and + c.getAnArgument().getAChild*() = node1.asExpr() and + node2.asExpr() = c + ) + } + + DataFlow::FlowFeature getAFeature() { result instanceof DataFlow::FeatureHasSourceCallContext } +} + +module TimeStructToCheckedTimeConversionFlow = + DataFlow::GlobalWithState; + import OperationToYearAssignmentFlow::PathGraph from OperationToYearAssignmentFlow::PathNode src, OperationToYearAssignmentFlow::PathNode sink where OperationToYearAssignmentFlow::flowPath(src, sink) and not isYearModifiedWithCheck(sink.getNode().asExpr().(YearFieldAssignment).getYearFieldAccess()) and - not isControlledByMonthEqualityCheckNonFebruary(sink.getNode().asExpr()) + not isControlledByMonthEqualityCheckNonFebruary(sink.getNode().asExpr()) and + // TODO: this is an older heuristic that should be reassessed. + // The heuristic stipulates that if a month or day is set to a constant in the local flow up to or after + // a modified year's sink, then assume the user is knowingly handling the conversion correctly. + // There is no interpretation of the assignment of the month/day or consideration for what branch the assignment is on. + // Merely if the assignment of a constant day/month occurs anywhere, the year modification can likely + // be ignored. + not exists(FieldAccess mfa, AssignExpr ae, YearFieldAccess yfa, int val, Variable var | + mfa instanceof MonthFieldAccess or mfa instanceof DayFieldAccess + | + yfa = sink.getNode().asExpr().(YearFieldAssignment).getYearFieldAccess() and + var.getAnAccess() = yfa.getQualifier() and + mfa.getQualifier() = var.getAnAccess() and + mfa.isModified() and + ( + mfa.getBasicBlock() = yfa.getBasicBlock().getASuccessor*() or + yfa.getBasicBlock() = mfa.getBasicBlock().getASuccessor+() + ) and + ae = mfa.getEnclosingElement() and + ae.getAnOperand().getValue().toInt() = val and + // If the constant looks like it relates to february, then there still might be an error + // so only look for any other constant. + not val in [2, 28, 29] + ) select sink, src, sink, "Year field has been modified, but no appropriate check for LeapYear was found." diff --git a/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/UncheckedLeapYearAfterYearModification.expected b/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/UncheckedLeapYearAfterYearModification.expected index 555002b40867..aa5e527e0759 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/UncheckedLeapYearAfterYearModification.expected +++ b/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/UncheckedLeapYearAfterYearModification.expected @@ -1,8 +1,61 @@ -| test.cpp:617:2:617:11 | ... ++ | $@: Field $@ on variable $@ has been modified, but no appropriate check for LeapYear was found. | test.cpp:611:6:611:32 | AntiPattern_1_year_addition | AntiPattern_1_year_addition | test.cpp:12:7:12:11 | wYear | wYear | test.cpp:613:13:613:14 | st | st | -| test.cpp:634:2:634:25 | ... += ... | $@: Field $@ on variable $@ has been modified, but no appropriate check for LeapYear was found. | test.cpp:627:6:627:32 | AntiPattern_simple_addition | AntiPattern_simple_addition | test.cpp:12:7:12:11 | wYear | wYear | test.cpp:629:13:629:14 | st | st | -| test.cpp:763:2:763:19 | ... ++ | $@: Field $@ on variable $@ has been modified, but no appropriate check for LeapYear was found. | test.cpp:756:6:756:40 | AntiPattern_year_addition_struct_tm | AntiPattern_year_addition_struct_tm | test.cpp:56:6:56:12 | tm_year | tm_year | test.cpp:759:12:759:19 | timeinfo | timeinfo | -| test.cpp:800:2:800:40 | ... = ... | $@: Field $@ on variable $@ has been modified, but no appropriate check for LeapYear was found. | test.cpp:791:6:791:23 | FalseNegativeTests | FalseNegativeTests | test.cpp:56:6:56:12 | tm_year | tm_year | test.cpp:793:12:793:19 | timeinfo | timeinfo | -| test.cpp:803:2:803:43 | ... = ... | $@: Field $@ on variable $@ has been modified, but no appropriate check for LeapYear was found. | test.cpp:791:6:791:23 | FalseNegativeTests | FalseNegativeTests | test.cpp:56:6:56:12 | tm_year | tm_year | test.cpp:793:12:793:19 | timeinfo | timeinfo | -| test.cpp:808:2:808:24 | ... = ... | $@: Field $@ on variable $@ has been modified, but no appropriate check for LeapYear was found. | test.cpp:791:6:791:23 | FalseNegativeTests | FalseNegativeTests | test.cpp:12:7:12:11 | wYear | wYear | test.cpp:794:13:794:14 | st | st | -| test.cpp:811:2:811:33 | ... = ... | $@: Field $@ on variable $@ has been modified, but no appropriate check for LeapYear was found. | test.cpp:791:6:791:23 | FalseNegativeTests | FalseNegativeTests | test.cpp:12:7:12:11 | wYear | wYear | test.cpp:794:13:794:14 | st | st | -| test.cpp:850:3:850:36 | ... = ... | $@: Field $@ on variable $@ has been modified, but no appropriate check for LeapYear was found. | test.cpp:818:6:818:23 | tp_intermediaryVar | tp_intermediaryVar | test.cpp:56:6:56:12 | tm_year | tm_year | test.cpp:70:18:70:19 | tm | tm | +#select +| test.cpp:422:14:422:14 | 2 | test.cpp:422:14:422:14 | 2 | test.cpp:422:14:422:14 | 2 | Year field has been modified, but no appropriate check for LeapYear was found. | +| test.cpp:440:2:440:11 | ... ++ | test.cpp:440:2:440:11 | ... ++ | test.cpp:440:2:440:11 | ... ++ | Year field has been modified, but no appropriate check for LeapYear was found. | +| test.cpp:456:2:456:12 | ... ++ | test.cpp:456:2:456:12 | ... ++ | test.cpp:456:2:456:12 | ... ++ | Year field has been modified, but no appropriate check for LeapYear was found. | +| test.cpp:681:14:681:23 | yearsToAdd | test.cpp:681:14:681:23 | yearsToAdd | test.cpp:681:14:681:23 | yearsToAdd | Year field has been modified, but no appropriate check for LeapYear was found. | +| test.cpp:769:22:769:23 | 80 | test.cpp:769:22:769:23 | 80 | test.cpp:769:22:769:23 | 80 | Year field has been modified, but no appropriate check for LeapYear was found. | +| test.cpp:813:21:813:40 | ... + ... | test.cpp:813:21:813:40 | ... + ... | test.cpp:813:21:813:40 | ... + ... | Year field has been modified, but no appropriate check for LeapYear was found. | +| test.cpp:818:13:818:24 | ... + ... | test.cpp:818:13:818:24 | ... + ... | test.cpp:818:13:818:24 | ... + ... | Year field has been modified, but no appropriate check for LeapYear was found. | +| test.cpp:857:33:857:36 | year | test.cpp:854:14:854:31 | ... + ... | test.cpp:857:33:857:36 | year | Year field has been modified, but no appropriate check for LeapYear was found. | +| test.cpp:857:33:857:36 | year | test.cpp:854:35:854:40 | -- ... | test.cpp:857:33:857:36 | year | Year field has been modified, but no appropriate check for LeapYear was found. | +| test.cpp:954:14:954:25 | ... + ... | test.cpp:954:14:954:25 | ... + ... | test.cpp:954:14:954:25 | ... + ... | Year field has been modified, but no appropriate check for LeapYear was found. | +| test.cpp:972:3:972:12 | ... ++ | test.cpp:972:3:972:12 | ... ++ | test.cpp:972:3:972:12 | ... ++ | Year field has been modified, but no appropriate check for LeapYear was found. | +| test.cpp:1075:2:1075:11 | ... ++ | test.cpp:1075:2:1075:11 | ... ++ | test.cpp:1075:2:1075:11 | ... ++ | Year field has been modified, but no appropriate check for LeapYear was found. | +| test.cpp:1095:16:1095:23 | increment_arg output argument | test.cpp:1083:2:1083:4 | ... ++ | test.cpp:1095:16:1095:23 | increment_arg output argument | Year field has been modified, but no appropriate check for LeapYear was found. | +| test.cpp:1099:27:1099:35 | increment_arg_by_pointer output argument | test.cpp:1087:2:1087:7 | ... ++ | test.cpp:1099:27:1099:35 | increment_arg_by_pointer output argument | Year field has been modified, but no appropriate check for LeapYear was found. | +| test.cpp:1153:14:1153:26 | ... - ... | test.cpp:1153:14:1153:26 | ... - ... | test.cpp:1153:14:1153:26 | ... - ... | Year field has been modified, but no appropriate check for LeapYear was found. | +edges +| test.cpp:854:7:854:31 | ... = ... | test.cpp:857:33:857:36 | year | provenance | | +| test.cpp:854:14:854:31 | ... + ... | test.cpp:854:7:854:31 | ... = ... | provenance | | +| test.cpp:854:35:854:40 | -- ... | test.cpp:857:33:857:36 | year | provenance | | +| test.cpp:1082:26:1082:26 | *x | test.cpp:1095:16:1095:23 | increment_arg output argument | provenance | | +| test.cpp:1083:2:1083:4 | ... ++ | test.cpp:1082:26:1082:26 | *x | provenance | | +| test.cpp:1086:37:1086:37 | *x | test.cpp:1099:27:1099:35 | increment_arg_by_pointer output argument | provenance | | +| test.cpp:1087:2:1087:7 | ... ++ | test.cpp:1086:37:1086:37 | *x | provenance | | +nodes +| test.cpp:422:14:422:14 | 2 | semmle.label | 2 | +| test.cpp:440:2:440:11 | ... ++ | semmle.label | ... ++ | +| test.cpp:456:2:456:12 | ... ++ | semmle.label | ... ++ | +| test.cpp:482:3:482:12 | ... ++ | semmle.label | ... ++ | +| test.cpp:500:2:500:11 | ... ++ | semmle.label | ... ++ | +| test.cpp:526:14:526:23 | yearsToAdd | semmle.label | yearsToAdd | +| test.cpp:553:2:553:11 | ... ++ | semmle.label | ... ++ | +| test.cpp:592:2:592:11 | ... ++ | semmle.label | ... ++ | +| test.cpp:647:2:647:11 | ... ++ | semmle.label | ... ++ | +| test.cpp:665:14:665:25 | yearAddition | semmle.label | yearAddition | +| test.cpp:681:14:681:23 | yearsToAdd | semmle.label | yearsToAdd | +| test.cpp:744:2:744:19 | ... ++ | semmle.label | ... ++ | +| test.cpp:769:22:769:23 | 80 | semmle.label | 80 | +| test.cpp:792:2:792:19 | ... ++ | semmle.label | ... ++ | +| test.cpp:813:21:813:40 | ... + ... | semmle.label | ... + ... | +| test.cpp:818:13:818:24 | ... + ... | semmle.label | ... + ... | +| test.cpp:854:7:854:31 | ... = ... | semmle.label | ... = ... | +| test.cpp:854:14:854:31 | ... + ... | semmle.label | ... + ... | +| test.cpp:854:35:854:40 | -- ... | semmle.label | -- ... | +| test.cpp:857:33:857:36 | year | semmle.label | year | +| test.cpp:875:4:875:15 | ... ++ | semmle.label | ... ++ | +| test.cpp:954:14:954:25 | ... + ... | semmle.label | ... + ... | +| test.cpp:972:3:972:12 | ... ++ | semmle.label | ... ++ | +| test.cpp:990:3:990:12 | ... ++ | semmle.label | ... ++ | +| test.cpp:1075:2:1075:11 | ... ++ | semmle.label | ... ++ | +| test.cpp:1082:26:1082:26 | *x | semmle.label | *x | +| test.cpp:1083:2:1083:4 | ... ++ | semmle.label | ... ++ | +| test.cpp:1086:37:1086:37 | *x | semmle.label | *x | +| test.cpp:1087:2:1087:7 | ... ++ | semmle.label | ... ++ | +| test.cpp:1095:16:1095:23 | increment_arg output argument | semmle.label | increment_arg output argument | +| test.cpp:1099:27:1099:35 | increment_arg_by_pointer output argument | semmle.label | increment_arg_by_pointer output argument | +| test.cpp:1133:3:1133:15 | -- ... | semmle.label | -- ... | +| test.cpp:1153:14:1153:26 | ... - ... | semmle.label | ... - ... | +subpaths +testFailures +| test.cpp:1075:2:1075:11 | ... ++ | Unexpected result: Alert | diff --git a/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/UncheckedLeapYearAfterYearModification.qlref b/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/UncheckedLeapYearAfterYearModification.qlref index 22a30d72b689..12bb5eb1e69b 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/UncheckedLeapYearAfterYearModification.qlref +++ b/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/UncheckedLeapYearAfterYearModification.qlref @@ -1 +1,2 @@ -Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification.ql +query: Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification.ql +postprocess: utils/test/InlineExpectationsTestQuery.ql \ No newline at end of file diff --git a/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/test.cpp b/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/test.cpp index 05f5ebf89053..3bd830121d03 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/test.cpp +++ b/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/test.cpp @@ -419,7 +419,7 @@ void AntiPattern_unchecked_filetime_conversion2a() GetSystemTime(&st); // BUG - UncheckedLeapYearAfterYearModification - st.wYear += 2; + st.wYear += 2; // $ Alert[cpp/microsoft/public/leap-year/unchecked-after-arithmetic-year-modification] // BUG - UncheckedReturnValueForTimeFunctions SystemTimeToFileTime(&st, &ft); @@ -437,7 +437,7 @@ void AntiPattern_unchecked_filetime_conversion2b() GetSystemTime(&st); // BUG - UncheckedLeapYearAfterYearModification - st.wYear++; + st.wYear++; // $ Alert[cpp/microsoft/public/leap-year/unchecked-after-arithmetic-year-modification] // BUG - UncheckedReturnValueForTimeFunctions SystemTimeToFileTime(&st, &ft); @@ -453,7 +453,7 @@ void AntiPattern_unchecked_filetime_conversion2b(SYSTEMTIME* st) FILETIME ft; // BUG - UncheckedLeapYearAfterYearModification - st->wYear++; + st->wYear++; // $ Alert[cpp/microsoft/public/leap-year/unchecked-after-arithmetic-year-modification] // BUG - UncheckedReturnValueForTimeFunctions SystemTimeToFileTime(st, &ft); @@ -635,24 +635,26 @@ void CorrectPattern_NotManipulated_DateFromAPI_1(HANDLE hWatchdog) ///////////////////////////////////////////////////////////////// /** - * Positive Case - Anti-pattern 1: [year ±n, month, day] - * Years is incremented by some integer but a leap year is not handled. + * Negative Case - Anti-pattern 1: [year ±n, month, day] + * Year is incremented by some integer and checked through a conversion through an inter procedural function check */ void AntiPattern_1_year_addition() { SYSTEMTIME st; GetSystemTime(&st); - // BUG - UncheckedLeapYearAfterYearModification - st.wYear++; // BUg V2 + // Safe, checked interprocedurally through Correct_filetime_conversion_check + st.wYear++; // Usage of potentially invalid date Correct_filetime_conversion_check(st); } + + /** - * Positive Case - Anti-pattern 1: [year ±n, month, day] - * Years is incremented by some integer but a leap year is not handled. + * Negative Case - Anti-pattern 1: [year ±n, month, day] + * Years is incremented by some integer and checked through a conversion through an inter procedural function check */ void AntiPattern_simple_addition(int yearAddition) { @@ -660,8 +662,7 @@ void AntiPattern_simple_addition(int yearAddition) GetSystemTime(&st); - // BUG - UncheckedLeapYearAfterYearModification - st.wYear += yearAddition; // Bug V2 + st.wYear += yearAddition; // Usage of potentially invalid date Correct_filetime_conversion_check(st); @@ -677,7 +678,7 @@ void AntiPattern_IncorrectGuard(int yearsToAdd) GetSystemTime(&st); // BUG - UncheckedLeapYearAfterYearModification - st.wYear += yearsToAdd; + st.wYear += yearsToAdd; // $ Alert[cpp/microsoft/public/leap-year/unchecked-after-arithmetic-year-modification] // Incorrect Guard if (st.wMonth == 2 && st.wDay == 29) @@ -690,9 +691,6 @@ void AntiPattern_IncorrectGuard(int yearsToAdd) st.wDay = 28; } } - - // Potentially Unsafe to use - Correct_filetime_conversion_check(st); } /************************************************* @@ -767,7 +765,8 @@ void Incorrect_LinuxPattern() errno_t err = gmtime_s(&timeinfo, &rawtime); /* from 1900 -> from 1980 */ - timeinfo.tm_year -= 80; // bug - no check for leap year + // BUG - UncheckedLeapYearAfterYearModification + timeinfo.tm_year -= 80; // $ Alert[cpp/microsoft/public/leap-year/unchecked-after-arithmetic-year-modification] /* 0~11 -> 1~12 */ timeinfo.tm_mon++; /* 0~59 -> 0~29(2sec counts) */ @@ -781,7 +780,8 @@ void Incorrect_LinuxPattern() /** * Negative Case - Anti-pattern 1: [year ±n, month, day] - * Years is incremented by some integer and leap year is not handled correctly. + * Years is incremented by some integer and leap year is assumed checked through + * check of a conversion functions return value. */ void AntiPattern_year_addition_struct_tm() { @@ -789,36 +789,19 @@ void AntiPattern_year_addition_struct_tm() struct tm timeinfo; time(&rawtime); gmtime_s(&timeinfo, &rawtime); - // BUG - UncheckedLeapYearAfterYearModification - timeinfo.tm_year++; // Bug V2 + timeinfo.tm_year++; - // Usage of potentially invalid date + // mkgmtime result checked in nested call here, assume leap year conversion is potentially handled CorrectUsageOf_mkgmtime(timeinfo); } ///////////////////////////////////////////////////////// -/** - * Negative Case - Anti-pattern 1: [year ±n, month, day] - * False positive: Years is initialized to or incremented by some integer (but never used). -*/ -void FalsePositiveTests(int x) -{ - struct tm timeinfo; - SYSTEMTIME st; - - timeinfo.tm_year = x; - timeinfo.tm_year = 1970; - - st.wYear = x; - st.wYear = 1900 + x; -} /** * Positive Case - Anti-pattern 1: [year ±n, month, day] - * False positive: Years is initialized to or incremented by some integer (but never used). */ -void FalseNegativeTests(int x) +void test(int x) { struct tm timeinfo; SYSTEMTIME st; @@ -827,18 +810,12 @@ void FalseNegativeTests(int x) // BUG - UncheckedLeapYearAfterYearModification // Positive Case - Anti-pattern 1: [year ±n, month, day] - timeinfo.tm_year = x + timeinfo.tm_year; // Bug V2 - // BUG - UncheckedLeapYearAfterYearModification - // Positive Case - Anti-pattern 1: [year ±n, month, day] - timeinfo.tm_year = 1970 + timeinfo.tm_year; // Bug V2 + timeinfo.tm_year = x + timeinfo.tm_year; // $ Alert[cpp/microsoft/public/leap-year/unchecked-after-arithmetic-year-modification] st.wYear = x; // BUG - UncheckedLeapYearAfterYearModification // Positive Case - Anti-pattern 1: [year ±n, month, day] - st.wYear = x + st.wYear; // Bug V2 - // BUG - UncheckedLeapYearAfterYearModification - // Positive Case - Anti-pattern 1: [year ±n, month, day] - st.wYear = (1986 + st.wYear) - 1; // Bug V2 + st.wYear = x + st.wYear; // $ Alert[cpp/microsoft/public/leap-year/unchecked-after-arithmetic-year-modification] } /** @@ -874,10 +851,10 @@ bool tp_intermediaryVar(struct timespec now, struct logtime ×tamp_remote) timestamp_remote.tm = tm_parsed; timestamp_remote.tm.tm_isdst = -1; timestamp_remote.usec = now.tv_nsec * 0.001; - for (year = tm_now.tm_year + 1;; --year) + for (year = tm_now.tm_year + 1;; --year) // $ Source { // assert(year >= tm_now.tm_year - 1); - timestamp_remote.tm.tm_year = year; + timestamp_remote.tm.tm_year = year; // $ Alert[cpp/microsoft/public/leap-year/unchecked-after-arithmetic-year-modification] if (mktime(×tamp_remote.tm) < t_now + 7 * 24 * 60 * 60) break; } @@ -973,7 +950,8 @@ bool tp_intermediaryVar(struct timespec now, struct logtime ×tamp_remote) GetSystemTime(&st); - st.wYear = st.wYear + 1; // BAD + // BUG - UncheckedLeapYearAfterYearModification + st.wYear = st.wYear + 1; // $ Alert[cpp/microsoft/public/leap-year/unchecked-after-arithmetic-year-modification] SystemTimeToFileTime(&st, &ft); } @@ -990,14 +968,16 @@ bool tp_intermediaryVar(struct timespec now, struct logtime ×tamp_remote) GetSystemTime(&st); - st.wYear++; // BAD Positive Case - Anti-pattern 1: [year ±n, month, day] + // BUG - UncheckedLeapYearAfterYearModification + st.wYear++; // $ Alert[cpp/microsoft/public/leap-year/unchecked-after-arithmetic-year-modification] SystemTimeToFileTime(&st, &ft); } /** * Negative Case - Anti-pattern 1: [year ±n, month, day] - * Modification of SYSTEMTIME struct adding to year but no leap year guard is conducted. + * Modification of SYSTEMTIME struct adding to year but value passed to a + * conversion function that can be checked for success, and the result is checked. */ void modified5() { @@ -1007,8 +987,13 @@ bool tp_intermediaryVar(struct timespec now, struct logtime ×tamp_remote) GetSystemTime(&st); - st.wYear++; // Negative Case - Anti-pattern 1: [year ±n, month, day], guard condition below. + st.wYear++; + // Presumed safe usage, as if the conversion is incorrect, a user can handle the error. + // NOTE: it doesn't mean the user actually does the correct conversion and it it also + // doesn't mean it will error our in all cases that may be invalid. + // For example, if a leap year and the date is 28, we may want 29 if the time is meant + // to capture the end of the month, but 28 is still valid and will not error out. if (SystemTimeToFileTime(&st, &ft)) { ///... @@ -1095,11 +1080,11 @@ void fp_daymonth_guard(){ } void increment_arg(WORD &x){ - x++; + x++; // $ Source } void increment_arg_by_pointer(WORD *x){ - (*x)++; + (*x)++; // $ Source } @@ -1107,11 +1092,11 @@ void fn_year_set_through_out_arg(){ SYSTEMTIME st; GetSystemTime(&st); // BAD, year incremented without check - increment_arg(st.wYear); + increment_arg(st.wYear); // $ Alert[cpp/microsoft/public/leap-year/unchecked-after-arithmetic-year-modification] // GetSystemTime(&st); // Bad, year incremented without check - increment_arg_by_pointer(&st.wYear); + increment_arg_by_pointer(&st.wYear); // $ Alert[cpp/microsoft/public/leap-year/unchecked-after-arithmetic-year-modification] } @@ -1165,5 +1150,5 @@ typedef struct _TIME_FIELDS { void tp_ptime(PTIME_FIELDS ptm){ - ptm->Year = ptm->Year - 1; // Positive Test Case + ptm->Year = ptm->Year - 1; // $ Alert[cpp/microsoft/public/leap-year/unchecked-after-arithmetic-year-modification] } \ No newline at end of file