From aba87c5124c706a4953fff7795cfc58b0a62c565 Mon Sep 17 00:00:00 2001 From: Josh Brown Date: Thu, 18 Dec 2025 12:30:12 -0800 Subject: [PATCH 01/34] 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/34] 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/34] 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/34] 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/34] 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/34] 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/34] 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/34] 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/34] 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/34] 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/34] 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/34] 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/34] 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/34] 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/34] 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/34] 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/34] 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/34] 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/34] 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/34] 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/34] 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/34] 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/34] 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/34] "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/34] 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/34] 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/34] 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/34] 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 From 63639eaf647983d42b4550a2a6467dd97347a784 Mon Sep 17 00:00:00 2001 From: "REDMOND\\brodes" Date: Tue, 20 Jan 2026 12:00:09 -0500 Subject: [PATCH 29/34] More FP tweaks. --- cpp/ql/src/Likely Bugs/Leap Year/LeapYear.qll | 3 +- .../UncheckedLeapYearAfterYearModification.ql | 20 +++++++- ...ckedLeapYearAfterYearModification.expected | 5 ++ .../test.cpp | 48 +++++++++++++++++++ 4 files changed, 73 insertions(+), 3 deletions(-) diff --git a/cpp/ql/src/Likely Bugs/Leap Year/LeapYear.qll b/cpp/ql/src/Likely Bugs/Leap Year/LeapYear.qll index 39e56627db2a..0f388fb4eeb3 100644 --- a/cpp/ql/src/Likely Bugs/Leap Year/LeapYear.qll +++ b/cpp/ql/src/Likely Bugs/Leap Year/LeapYear.qll @@ -534,7 +534,8 @@ class TimeConversionFunction extends Function { "FileTimeToSystemTime", "SystemTimeToFileTime", "SystemTimeToTzSpecificLocalTime", "SystemTimeToTzSpecificLocalTimeEx", "TzSpecificLocalTimeToSystemTime", "TzSpecificLocalTimeToSystemTimeEx", "RtlLocalTimeToSystemTime", - "RtlTimeToSecondsSince1970", "_mkgmtime", "SetSystemTime" + "RtlTimeToSecondsSince1970", "_mkgmtime", "SetSystemTime", "SystemTimeToVariantTime", + "VariantTimeToSystemTime" ] } } diff --git a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification.ql b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification.ql index 21b6403f2c13..17c0a22c9867 100644 --- a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification.ql +++ b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification.ql @@ -16,9 +16,19 @@ import semmle.code.cpp.controlflow.IRGuards /** * Functions whose operations should never be considered a * source or sink of a dangerous leap year operation. + * The general concept is to add conversion functions + * that convert one time type to another. Often + * other ignorable operation heuristics will filter these, + * but some cases, the simplest approach is to simply filter + * the function entirely. + * Note that flow through these functions should still be allowed + * we just cannot start or end flow from an operation to a + * year assignment in one of these functions. */ class IgnorableFunction extends Function { IgnorableFunction() { + this instanceof TimeConversionFunction + or // Helper utility in postgres with string time conversions this.getName() = "DecodeISO8601Interval" or @@ -119,6 +129,9 @@ predicate isLikelyConversionConstant(int c) { 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 = 1601 or // Windows FILETIME epoch start year + i = 1970 or // Unix epoch start year + i = 70 or // Unix epoch start year short form i = 0 ) } @@ -351,9 +364,12 @@ module OperationToYearAssignmentConfig implements DataFlow::ConfigSig { predicate isBarrier(DataFlow::Node n) { exists(ArrayExpr arr | arr.getArrayOffset() = n.asExpr()) or - n.asExpr().getUnspecifiedType() instanceof PointerType + n.getType().getUnspecifiedType() instanceof PointerType + or + n.getType().getUnspecifiedType() instanceof CharType or - n.asExpr().getUnspecifiedType() instanceof CharType + // If a type resembles "string" ignore flow (likely string conversion, currently ignored) + n.getType().getUnspecifiedType().stripType().getName().toLowerCase().matches("%string%") or n.asExpr() instanceof IgnorableOperation or 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 aa5e527e0759..913545791be7 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 @@ -22,6 +22,8 @@ edges | 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 | | +| test.cpp:1183:2:1183:15 | ... += ... | test.cpp:1185:16:1185:19 | year | provenance | | +| test.cpp:1183:10:1183:15 | offset | test.cpp:1183:2:1183:15 | ... += ... | provenance | | nodes | test.cpp:422:14:422:14 | 2 | semmle.label | 2 | | test.cpp:440:2:440:11 | ... ++ | semmle.label | ... ++ | @@ -56,6 +58,9 @@ nodes | 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 | ... - ... | +| test.cpp:1183:2:1183:15 | ... += ... | semmle.label | ... += ... | +| test.cpp:1183:10:1183:15 | offset | semmle.label | offset | +| test.cpp:1185:16:1185:19 | year | semmle.label | year | subpaths testFailures | test.cpp:1075:2:1075:11 | ... ++ | Unexpected result: Alert | 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 3bd830121d03..595ca809d1ca 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 @@ -1151,4 +1151,52 @@ typedef struct _TIME_FIELDS { void tp_ptime(PTIME_FIELDS ptm){ ptm->Year = ptm->Year - 1; // $ Alert[cpp/microsoft/public/leap-year/unchecked-after-arithmetic-year-modification] +} + + +bool isLeapYearRaw(WORD year) +{ + return year % 4 == 0 && (year % 100 != 0 || year % 400 == 0); +} + +void leap_year_checked_raw_false_positive1(WORD year, WORD offset, WORD day){ + struct tm tmp; + + year += offset; + + if (isLeapYearRaw(year)){ + // in this simplified example, assume the logic of this function + // can assume a day is 28 by default + // this check is more to establish the leap year guard is present + day += 1; + } + + // Assume the check handled leap year correctly + tmp.tm_year = year; // GOOD + tmp.tm_mday = day; +} + + +void leap_year_checked_raw_false_positive2(WORD year, WORD offset, WORD day){ + struct tm tmp; + + year += offset; + + tmp.tm_year = year; // GOOD, check performed immediately after on raw year + + // Adding some additional checks to resemble cases observed in the wild + if ( day > 0) + { + if (isLeapYearRaw(year)){ + // Assume logic that would adjust the day correctly + } + } + else{ + if (isLeapYearRaw(year)){ + // Assume logic that would adjust the day correctly + } + } + + tmp.tm_mday = day; + } \ No newline at end of file From e55ee18f8799ac21b07fbcaac285e106c17cf95b Mon Sep 17 00:00:00 2001 From: "REDMOND\\brodes" Date: Tue, 20 Jan 2026 15:11:27 -0500 Subject: [PATCH 30/34] Updating test for UncheckedReturnValueForTImeFunctions. That query needs to be generally reassessed but recent test changes alter the expected results. --- .../UncheckedReturnValueForTimeFunctions.expected | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/UncheckedReturnValueForTimeFunctions.expected b/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/UncheckedReturnValueForTimeFunctions.expected index ae8a55449daf..1b3fe76f2390 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/UncheckedReturnValueForTimeFunctions.expected +++ b/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/UncheckedReturnValueForTimeFunctions.expected @@ -1,5 +1,6 @@ -| test.cpp:395:2:395:21 | call to SystemTimeToFileTime | $@: Return value of $@ function should be verified to check for any error because variable $@ is not guaranteed to be safe. | test.cpp:385:6:385:48 | AntiPattern_unchecked_filetime_conversion2a | AntiPattern_unchecked_filetime_conversion2a | test.cpp:75:1:75:20 | SystemTimeToFileTime | SystemTimeToFileTime | test.cpp:387:13:387:14 | st | st | -| test.cpp:413:2:413:21 | call to SystemTimeToFileTime | $@: Return value of $@ function should be verified to check for any error because variable $@ is not guaranteed to be safe. | test.cpp:403:6:403:48 | AntiPattern_unchecked_filetime_conversion2b | AntiPattern_unchecked_filetime_conversion2b | test.cpp:75:1:75:20 | SystemTimeToFileTime | SystemTimeToFileTime | test.cpp:405:13:405:14 | st | st | -| test.cpp:429:2:429:21 | call to SystemTimeToFileTime | $@: Return value of $@ function should be verified to check for any error because variable $@ is not guaranteed to be safe. | test.cpp:421:6:421:48 | AntiPattern_unchecked_filetime_conversion2b | AntiPattern_unchecked_filetime_conversion2b | test.cpp:75:1:75:20 | SystemTimeToFileTime | SystemTimeToFileTime | test.cpp:421:62:421:63 | st | st | -| test.cpp:948:3:948:22 | call to SystemTimeToFileTime | $@: Return value of $@ function should be verified to check for any error because variable $@ is not guaranteed to be safe. | test.cpp:938:7:938:15 | modified3 | modified3 | test.cpp:75:1:75:20 | SystemTimeToFileTime | SystemTimeToFileTime | test.cpp:940:14:940:15 | st | st | -| test.cpp:965:3:965:22 | call to SystemTimeToFileTime | $@: Return value of $@ function should be verified to check for any error because variable $@ is not guaranteed to be safe. | test.cpp:955:7:955:15 | modified4 | modified4 | test.cpp:75:1:75:20 | SystemTimeToFileTime | SystemTimeToFileTime | test.cpp:957:14:957:15 | st | st | +| test.cpp:425:2:425:21 | call to SystemTimeToFileTime | $@: Return value of $@ function should be verified to check for any error because variable $@ is not guaranteed to be safe. | test.cpp:415:6:415:48 | AntiPattern_unchecked_filetime_conversion2a | AntiPattern_unchecked_filetime_conversion2a | test.cpp:101:1:101:20 | SystemTimeToFileTime | SystemTimeToFileTime | test.cpp:417:13:417:14 | st | st | +| test.cpp:443:2:443:21 | call to SystemTimeToFileTime | $@: Return value of $@ function should be verified to check for any error because variable $@ is not guaranteed to be safe. | test.cpp:433:6:433:48 | AntiPattern_unchecked_filetime_conversion2b | AntiPattern_unchecked_filetime_conversion2b | test.cpp:101:1:101:20 | SystemTimeToFileTime | SystemTimeToFileTime | test.cpp:435:13:435:14 | st | st | +| test.cpp:459:2:459:21 | call to SystemTimeToFileTime | $@: Return value of $@ function should be verified to check for any error because variable $@ is not guaranteed to be safe. | test.cpp:451:6:451:48 | AntiPattern_unchecked_filetime_conversion2b | AntiPattern_unchecked_filetime_conversion2b | test.cpp:101:1:101:20 | SystemTimeToFileTime | SystemTimeToFileTime | test.cpp:451:62:451:63 | st | st | +| test.cpp:956:3:956:22 | call to SystemTimeToFileTime | $@: Return value of $@ function should be verified to check for any error because variable $@ is not guaranteed to be safe. | test.cpp:945:7:945:15 | modified3 | modified3 | test.cpp:101:1:101:20 | SystemTimeToFileTime | SystemTimeToFileTime | test.cpp:947:14:947:15 | st | st | +| test.cpp:974:3:974:22 | call to SystemTimeToFileTime | $@: Return value of $@ function should be verified to check for any error because variable $@ is not guaranteed to be safe. | test.cpp:963:7:963:15 | modified4 | modified4 | test.cpp:101:1:101:20 | SystemTimeToFileTime | SystemTimeToFileTime | test.cpp:965:14:965:15 | st | st | +| test.cpp:1079:2:1079:21 | call to SystemTimeToFileTime | $@: Return value of $@ function should be verified to check for any error because variable $@ is not guaranteed to be safe. | test.cpp:1070:6:1070:22 | fp_daymonth_guard | fp_daymonth_guard | test.cpp:101:1:101:20 | SystemTimeToFileTime | SystemTimeToFileTime | test.cpp:1071:13:1071:14 | st | st | From f507239e243ea53e10e3ed69fc8d38435ab93297 Mon Sep 17 00:00:00 2001 From: "REDMOND\\brodes" Date: Tue, 20 Jan 2026 16:11:07 -0500 Subject: [PATCH 31/34] New additions to the set of TimeConversionFunctions. --- cpp/ql/src/Likely Bugs/Leap Year/LeapYear.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/ql/src/Likely Bugs/Leap Year/LeapYear.qll b/cpp/ql/src/Likely Bugs/Leap Year/LeapYear.qll index 0f388fb4eeb3..018aa5caa1b2 100644 --- a/cpp/ql/src/Likely Bugs/Leap Year/LeapYear.qll +++ b/cpp/ql/src/Likely Bugs/Leap Year/LeapYear.qll @@ -535,7 +535,7 @@ class TimeConversionFunction extends Function { "SystemTimeToTzSpecificLocalTimeEx", "TzSpecificLocalTimeToSystemTime", "TzSpecificLocalTimeToSystemTimeEx", "RtlLocalTimeToSystemTime", "RtlTimeToSecondsSince1970", "_mkgmtime", "SetSystemTime", "SystemTimeToVariantTime", - "VariantTimeToSystemTime" + "VariantTimeToSystemTime", "mktime", "_mktime32", "_mktime64" ] } } From e485d98105a9f38eb5c900c7f3db67d70878c914 Mon Sep 17 00:00:00 2001 From: "REDMOND\\brodes" Date: Wed, 21 Jan 2026 12:32:11 -0500 Subject: [PATCH 32/34] Created a new leap year check guard condition. Found that the prior definitions had gaps resulting in false positives and inconsistencies (inconsistent as to what is a guard and what is a function that does a leap year check). --- .../UncheckedLeapYearAfterYearModification.ql | 160 ++++++++++++++++-- ...ckedLeapYearAfterYearModification.expected | 14 ++ .../test.cpp | 61 ++++++- 3 files changed, 218 insertions(+), 17 deletions(-) diff --git a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification.ql b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification.ql index 17c0a22c9867..bbae02d37783 100644 --- a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification.ql +++ b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification.ql @@ -385,22 +385,11 @@ 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 - // 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() - ) + exists(LeapYearGuardCondition lgc | + lgc.checkedYearAccess() = [sink.asExpr(), sink.asIndirectExpr()] ) + or + isLeapYearCheckCall(_, [sink.asExpr(), sink.asIndirectExpr()]) } /** @@ -513,6 +502,147 @@ module TimeStructToCheckedTimeConversionConfig implements DataFlow::StateConfigS module TimeStructToCheckedTimeConversionFlow = DataFlow::GlobalWithState; +/** + * Finds flow from a parameter of a function to a leap year check. + * This is necessary to handle for scenarios like this: + * + * year = DANGEROUS_OP // source + * isLeap = isLeapYear(year); + * // logic based on isLeap + * struct.year = year; // sink + * + * In this case, we may flow a dangerous op to a year assignment, failing + * to barrier the flow through a leap year check, as the leap year check + * is nested, and dataflow does not progress down into the check and out. + * Instead, the point of this flow is to detect isLeapYear's argument + * is checked for leap year, making the isLeapYear call a barrier for + * the dangerous flow if we flow through the parameter identified to + * be checked. + */ +module ParameterToLeapYearCheckConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node source) { exists(source.asParameter()) } + + predicate isSink(DataFlow::Node sink) { + exists(LeapYearGuardCondition lgc | + lgc.checkedYearAccess() = [sink.asExpr(), sink.asIndirectExpr()] + ) + } + + predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) { + // flow from a YearFieldAccess to the qualifier + node2.asExpr() = node1.asExpr().(YearFieldAccess).getQualifier() + or + // flow from a year access qualifier to a year field + exists(YearFieldAccess yfa | node2.asExpr() = yfa and node1.asExpr() = yfa.getQualifier()) + } + + /** + * 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 } +} + +// NOTE: I do not believe taint flow is necessary here as we should +// be flowing directyly from some parameter to a leap year check. +module ParameterToLeapYearCheckFlow = DataFlow::Global; + +predicate isLeapYearCheckCall(Call c, Expr arg) { + exists(ParameterToLeapYearCheckFlow::PathNode src, Function f, int i | + src.isSource() and + f.getParameter(i) = src.getNode().asParameter() and + c.getTarget() = f and + c.getArgument(i) = arg + ) +} + +class LeapYearGuardCondition extends GuardCondition { + Expr yearSinkDiv4; + Expr yearSinkDiv100; + Expr yearSinkDiv400; + + LeapYearGuardCondition() { + exists( + LogicalAndExpr andExpr, LogicalOrExpr orExpr, GuardCondition div4Check, + GuardCondition div100Check, GuardCondition div400Check, GuardValue gv + | + // Cannonical case: + // form: `(year % 4 == 0) && (year % 100 != 0 || year % 400 == 0)` + // or : `!(year % 4 == 0) && (year % 100 != 0 || year % 400 == 0)` + this = andExpr and + andExpr.hasOperands(div4Check, orExpr) and + orExpr.hasOperands(div100Check, div400Check) and + exists(RemExpr e | + div4Check.comparesEq(e, 0, true, gv) and + e.getRightOperand().getValue().toInt() = 4 and + yearSinkDiv4 = e.getLeftOperand() + ) and + exists(RemExpr e | + div100Check.comparesEq(e, 0, false, gv) and + e.getRightOperand().getValue().toInt() = 100 and + yearSinkDiv100 = e.getLeftOperand() + ) and + exists(RemExpr e | + div400Check.comparesEq(e, 0, true, gv) and + e.getRightOperand().getValue().toInt() = 400 and + yearSinkDiv400 = e.getLeftOperand() + ) + or + // Inverted logic case: + // `year % 400 == 0 || (year % 100 != 0 && year % 4 == 0)` + this = orExpr and + orExpr.hasOperands(div4Check, andExpr) and + andExpr.hasOperands(div100Check, div400Check) and + exists(RemExpr e | + div4Check.comparesEq(e, 0, false, gv) and + e.getRightOperand().getValue().toInt() = 4 and + yearSinkDiv4 = e.getLeftOperand() + ) and + exists(RemExpr e | + div100Check.comparesEq(e, 0, true, gv) and + e.getRightOperand().getValue().toInt() = 100 and + yearSinkDiv100 = e.getLeftOperand() + ) and + exists(RemExpr e | + div400Check.comparesEq(e, 0, false, gv) and + e.getRightOperand().getValue().toInt() = 400 and + yearSinkDiv400 = e.getLeftOperand() + ) + ) + } + + Expr getYearSinkDiv4() { result = yearSinkDiv4 } + + Expr getYearSinkDiv100() { result = yearSinkDiv100 } + + Expr getYearSinkDiv400() { result = yearSinkDiv400 } + + /** + * The variable access that is used in all 3 components of the leap year check + * e.g., see getYearSinkDiv4/100/400.. + * If a field access is used, the qualifier and the field access are both returned + * in checked condition. + * NOTE: if the year is not checked using the same access in all 3 components, no result is returned. + * The typical case observed is a consistent variable access is used. If not, this may indicate a bug. + * We could check more accurately with a dataflow analysis, but this is likely sufficient for now. + */ + VariableAccess checkedYearAccess() { + exists(Variable var | + ( + this.getYearSinkDiv4().getAChild*() = var.getAnAccess() and + this.getYearSinkDiv100().getAChild*() = var.getAnAccess() and + this.getYearSinkDiv400().getAChild*() = var.getAnAccess() and + result = var.getAnAccess() and + ( + result = this.getYearSinkDiv4().getAChild*() or + result = this.getYearSinkDiv100().getAChild*() or + result = this.getYearSinkDiv400().getAChild*() + ) + ) + ) + } +} + import OperationToYearAssignmentFlow::PathGraph from OperationToYearAssignmentFlow::PathNode src, OperationToYearAssignmentFlow::PathNode sink 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 913545791be7..375414e6f019 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 @@ -14,6 +14,9 @@ | 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. | +| test.cpp:1204:16:1204:19 | year | test.cpp:1202:10:1202:15 | offset | test.cpp:1204:16:1204:19 | year | Year field has been modified, but no appropriate check for LeapYear was found. | +| test.cpp:1243:16:1243:28 | ... + ... | test.cpp:1243:16:1243:28 | ... + ... | test.cpp:1243:16:1243:28 | ... + ... | Year field has been modified, but no appropriate check for LeapYear was found. | +| test.cpp:1257:16:1257:28 | ... + ... | test.cpp:1257:16:1257:28 | ... + ... | test.cpp:1257:16:1257:28 | ... + ... | 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 | | @@ -24,6 +27,8 @@ edges | test.cpp:1087:2:1087:7 | ... ++ | test.cpp:1086:37:1086:37 | *x | provenance | | | test.cpp:1183:2:1183:15 | ... += ... | test.cpp:1185:16:1185:19 | year | provenance | | | test.cpp:1183:10:1183:15 | offset | test.cpp:1183:2:1183:15 | ... += ... | provenance | | +| test.cpp:1202:2:1202:15 | ... += ... | test.cpp:1204:16:1204:19 | year | provenance | | +| test.cpp:1202:10:1202:15 | offset | test.cpp:1202:2:1202:15 | ... += ... | provenance | | nodes | test.cpp:422:14:422:14 | 2 | semmle.label | 2 | | test.cpp:440:2:440:11 | ... ++ | semmle.label | ... ++ | @@ -61,6 +66,15 @@ nodes | test.cpp:1183:2:1183:15 | ... += ... | semmle.label | ... += ... | | test.cpp:1183:10:1183:15 | offset | semmle.label | offset | | test.cpp:1185:16:1185:19 | year | semmle.label | year | +| test.cpp:1202:2:1202:15 | ... += ... | semmle.label | ... += ... | +| test.cpp:1202:10:1202:15 | offset | semmle.label | offset | +| test.cpp:1204:16:1204:19 | year | semmle.label | year | +| test.cpp:1223:16:1223:28 | ... + ... | semmle.label | ... + ... | +| test.cpp:1229:16:1229:28 | ... + ... | semmle.label | ... + ... | +| test.cpp:1236:16:1236:28 | ... + ... | semmle.label | ... + ... | +| test.cpp:1243:16:1243:28 | ... + ... | semmle.label | ... + ... | +| test.cpp:1250:16:1250:28 | ... + ... | semmle.label | ... + ... | +| test.cpp:1257:16:1257:28 | ... + ... | 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/test.cpp b/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/test.cpp index 595ca809d1ca..a1efe1dcff7b 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 @@ -1198,5 +1198,62 @@ void leap_year_checked_raw_false_positive2(WORD year, WORD offset, WORD day){ } tmp.tm_mday = day; - -} \ No newline at end of file + + year += offset; // $ Source + + tmp.tm_year = year; // $ Alert[cpp/microsoft/public/leap-year/unchecked-after-arithmetic-year-modification] + +} + + +bool isNotLeapYear(struct tm tm) +{ + return !(tm.tm_year % 4 == 0 && (tm.tm_year % 100 != 0 || tm.tm_year % 400 == 0)); +} + +bool isNotLeapYear2(struct tm tm) +{ + return (tm.tm_year % 4 != 0 || (tm.tm_year % 100 == 0 && tm.tm_year % 400 != 0)); +} + + +void inverted_leap_year_check(WORD year, WORD offset, WORD day){ + struct tm tmp; + + tmp.tm_year = year + offset; + + if (isNotLeapYear(tmp)){ + day = 28; + } + + tmp.tm_year = year + offset; + + if(isNotLeapYear2(tmp)){ + day = 28; + } + + + tmp.tm_year = year + offset; + bool isNotLeapYear = (tmp.tm_year % 4 != 0 || (tmp.tm_year % 100 == 0 && tmp.tm_year % 400 != 0)); + + if(isNotLeapYear){ + day = 28; + } + + tmp.tm_year = year + offset; // $ Alert[cpp/microsoft/public/leap-year/unchecked-after-arithmetic-year-modification] +} + + +void simplified_leap_year_check(WORD year, WORD offset){ + struct tm tmp; + + tmp.tm_year = year + offset; + + bool isLeap = !(tmp.tm_year % 4) && (tmp.tm_year % 100 || !(tmp.tm_year % 400)); + if(isLeap){ + // do something + } + + tmp.tm_year = year + offset; // $ Alert[cpp/microsoft/public/leap-year/unchecked-after-arithmetic-year-modification] +} + From 8e7ff2d102c8b8ec774367fb215c83599b0a41b5 Mon Sep 17 00:00:00 2001 From: "REDMOND\\brodes" Date: Wed, 21 Jan 2026 13:03:37 -0500 Subject: [PATCH 33/34] Removing most dependencies from UncheckedLeapYearAfterYearMOdification on LeapYear.qll. We may need to end up moving new capabilities for this query into LeapYear.qll for other queries in the future. For now, focusing just on improvements to the one query. The only remaining dependency is the TimeConversionFunction class, which is currently used by another query. --- .../Leap Year/UncheckedLeapYearAfterYearModification.ql | 2 ++ 1 file changed, 2 insertions(+) diff --git a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification.ql b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification.ql index bbae02d37783..c54fb3185fa7 100644 --- a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification.ql +++ b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification.ql @@ -12,6 +12,8 @@ import cpp import LeapYear import semmle.code.cpp.controlflow.IRGuards +import semmle.code.cpp.dataflow.new.TaintTracking +import semmle.code.cpp.commons.DateTime /** * Functions whose operations should never be considered a From 6c103a98450fa7e7dbd7fc290c8683817d338571 Mon Sep 17 00:00:00 2001 From: "REDMOND\\brodes" Date: Wed, 21 Jan 2026 15:38:44 -0500 Subject: [PATCH 34/34] Time conversion results that are checked in a ternary operator conditions are now assumed to be a check of the result, i.e., a valid leap year check. --- .../UncheckedLeapYearAfterYearModification.ql | 13 ++++++++- ...ckedLeapYearAfterYearModification.expected | 2 ++ .../test.cpp | 29 +++++++++++++++++++ 3 files changed, 43 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 c54fb3185fa7..ef56fd2f7658 100644 --- a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification.ql +++ b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification.ql @@ -473,6 +473,13 @@ predicate isControlledByMonthEqualityCheckNonFebruary(Expr e) { ) } +/** + * Flow from a year field access through a time conversion function + * where the call's result is used to check error. The result must + * be used as a guard for an if or ternary operator. If so, + * assume some sort of error handling is occurring that could be used + * to detect bad dates due to leap year. + */ module TimeStructToCheckedTimeConversionConfig implements DataFlow::StateConfigSig { class FlowState = boolean; @@ -483,7 +490,11 @@ module TimeStructToCheckedTimeConversionConfig implements DataFlow::StateConfigS predicate isSink(DataFlow::Node sink, FlowState state) { state = true and - exists(IfStmt ifs | ifs.getCondition().getAChild*() = sink.asExpr()) + ( + exists(IfStmt ifs | ifs.getCondition().getAChild*() = sink.asExpr()) + or + exists(ConditionalExpr ce | ce.getCondition().getAChild*() = sink.asExpr()) + ) } predicate isAdditionalFlowStep( 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 375414e6f019..b82a5ff6b671 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 @@ -75,6 +75,8 @@ nodes | test.cpp:1243:16:1243:28 | ... + ... | semmle.label | ... + ... | | test.cpp:1250:16:1250:28 | ... + ... | semmle.label | ... + ... | | test.cpp:1257:16:1257:28 | ... + ... | semmle.label | ... + ... | +| test.cpp:1263:16:1263:28 | ... + ... | semmle.label | ... + ... | +| test.cpp:1277:14:1277: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/test.cpp b/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/test.cpp index a1efe1dcff7b..c9f974c3653a 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 @@ -1257,3 +1257,32 @@ void simplified_leap_year_check(WORD year, WORD offset){ tmp.tm_year = year + offset; // $ Alert[cpp/microsoft/public/leap-year/unchecked-after-arithmetic-year-modification] } +void compound_leap_year_check(WORD year, WORD offset, WORD month, WORD day){ + struct tm tmp; + + tmp.tm_year = year + offset; + + bool isLeap = tmp.tm_year % 4 == 0 && (tmp.tm_year % 100 != 0 || tmp.tm_year % 400 == 0) && (month == 2 && day == 29); + + if(isLeap){ + // do something + } + tmp.tm_mday = day; + tmp.tm_mon = month; +} + +void indirect_time_conversion_check(WORD year, WORD offset){ + SYSTEMTIME tmp; + + tmp.wYear = year + offset; + + FILETIME ft; + + // (out-of-scope) GeneralBug: Unchecked call to SystemTimeToFileTime. this may have failed, but we didn't check the return value! + BOOL res = SystemTimeToFileTime(&tmp, &ft); + + // Assume this check of the result is sufficient as an implicit leap year check. + bool x = (res == 0) ? true : false; +} + +