diff --git a/cpp/ql/src/Likely Bugs/JapaneseEra/ConstructorOrMethodWithExactEraDate.ql b/cpp/ql/src/Likely Bugs/JapaneseEra/ConstructorOrMethodWithExactEraDate.ql index 923f22a845c..2a6a0f52874 100644 --- a/cpp/ql/src/Likely Bugs/JapaneseEra/ConstructorOrMethodWithExactEraDate.ql +++ b/cpp/ql/src/Likely Bugs/JapaneseEra/ConstructorOrMethodWithExactEraDate.ql @@ -10,8 +10,10 @@ */ import cpp + from Call cc, int i -where cc.getArgument(i).getValue().toInt() = 1989 and - cc.getArgument(i+1).getValue().toInt() = 1 and - cc.getArgument(i+2).getValue().toInt() = 8 -select cc, "Call that appears to have hard-coded Japanese era start date as parameter." +where + cc.getArgument(i).getValue().toInt() = 1989 and + cc.getArgument(i + 1).getValue().toInt() = 1 and + cc.getArgument(i + 2).getValue().toInt() = 8 +select cc, "Call that appears to have hard-coded Japanese era start date as parameter." diff --git a/cpp/ql/src/Likely Bugs/JapaneseEra/StructWithExactEraDate.ql b/cpp/ql/src/Likely Bugs/JapaneseEra/StructWithExactEraDate.ql index 19debd200bd..a6c9f0c2f82 100644 --- a/cpp/ql/src/Likely Bugs/JapaneseEra/StructWithExactEraDate.ql +++ b/cpp/ql/src/Likely Bugs/JapaneseEra/StructWithExactEraDate.ql @@ -10,11 +10,19 @@ */ import cpp - import semmle.code.cpp.commons.DateTime -from StructLikeClass s, YearFieldAccess year, MonthFieldAccess month, DayFieldAccess day, Operation yearAssignment, Operation monthAssignment, Operation dayAssignment -where s.getAField().getAnAccess () = year and yearAssignment.getAnOperand() = year and yearAssignment.getAnOperand().getValue().toInt() = 1989 and - s.getAField().getAnAccess () = month and monthAssignment.getAnOperand() = month and monthAssignment.getAnOperand().getValue().toInt() = 1 and - s.getAField().getAnAccess () = day and dayAssignment.getAnOperand() = day and dayAssignment.getAnOperand().getValue().toInt() = 8 +from + StructLikeClass s, YearFieldAccess year, MonthFieldAccess month, DayFieldAccess day, + Operation yearAssignment, Operation monthAssignment, Operation dayAssignment +where + s.getAField().getAnAccess() = year and + yearAssignment.getAnOperand() = year and + yearAssignment.getAnOperand().getValue().toInt() = 1989 and + s.getAField().getAnAccess() = month and + monthAssignment.getAnOperand() = month and + monthAssignment.getAnOperand().getValue().toInt() = 1 and + s.getAField().getAnAccess() = day and + dayAssignment.getAnOperand() = day and + dayAssignment.getAnOperand().getValue().toInt() = 8 select year, "A time struct that is initialized with exact Japanese calendar era start date." diff --git a/cpp/ql/src/Likely Bugs/Leap Year/Adding365daysPerYear.ql b/cpp/ql/src/Likely Bugs/Leap Year/Adding365daysPerYear.ql index cbacb15c0e0..364e9d4b998 100644 --- a/cpp/ql/src/Likely Bugs/Leap Year/Adding365daysPerYear.ql +++ b/cpp/ql/src/Likely Bugs/Leap Year/Adding365daysPerYear.ql @@ -15,6 +15,6 @@ import semmle.code.cpp.dataflow.DataFlow from Expr source, Expr sink, PossibleYearArithmeticOperationCheckConfiguration config where config.hasFlow(DataFlow::exprNode(source), DataFlow::exprNode(sink)) -select sink, "This arithmetic operation $@ uses a constant value of 365 ends up modifying the date/time located at $@, without considering leap year scenarios." - , source, source.toString() - , sink, sink.toString() +select sink, + "This arithmetic operation $@ uses a constant value of 365 ends up modifying the date/time located at $@, without considering leap year scenarios.", + source, source.toString(), sink, sink.toString() diff --git a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification.ql b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification.ql index 7d4339daa5d..0ce89934d75 100644 --- a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification.ql +++ b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification.ql @@ -12,47 +12,53 @@ import cpp import LeapYear -from Variable var, LeapYearFieldAccess yfa +from Variable var, LeapYearFieldAccess yfa where - exists(VariableAccess va | - yfa.getQualifier() = va - and var.getAnAccess() = va - // The year is modified with an arithmetic operation. Avoid values that are likely false positives - and yfa.isModifiedByArithmeticOperationNotForNormalization() - // Avoid false positives - and not ( - // If there is a local check for leap year after the modification - exists( LeapYearFieldAccess yfacheck | - yfacheck.getQualifier() = var.getAnAccess() - and yfacheck.isUsedInCorrectLeapYearCheck() - and yfacheck = yfa.getASuccessor*() - ) - // If there is a data flow from the variable that was modified to a function that seems to check for leap year - or exists(VariableAccess source, - ChecksForLeapYearFunctionCall fc, - LeapYearCheckConfiguration config | - source = var.getAnAccess() - and config.hasFlow( DataFlow::exprNode(source), DataFlow::exprNode(fc.getAnArgument())) - ) - // If there is a data flow from the field that was modified to a function that seems to check for leap year - or exists(VariableAccess vacheck, - YearFieldAccess yfacheck, - ChecksForLeapYearFunctionCall fc, - LeapYearCheckConfiguration config | - vacheck = var.getAnAccess() - and yfacheck.getQualifier() = vacheck - and config.hasFlow( DataFlow::exprNode(yfacheck), DataFlow::exprNode(fc.getAnArgument())) - ) - // If there is a successor or predecessor that sets the month = 1 - or exists(MonthFieldAccess mfa, AssignExpr ae | - mfa.getQualifier() = var.getAnAccess() - and mfa.isModified() - and (mfa = yfa.getASuccessor*() - or yfa = mfa.getASuccessor*()) - and ae = mfa.getEnclosingElement() - and ae.getAnOperand().getValue().toInt() = 1 - ) - ) - ) -select yfa - , "Field $@ on variable $@ has been modified, but no appropriate check for LeapYear was found.", yfa.getTarget(), yfa.getTarget().toString(), var, var.toString() + exists(VariableAccess va | + yfa.getQualifier() = va and + var.getAnAccess() = va and + // The year is modified with an arithmetic operation. Avoid values that are likely false positives + yfa.isModifiedByArithmeticOperationNotForNormalization() and + // Avoid false positives + not ( + // If there is a local check for leap year after the modification + exists(LeapYearFieldAccess yfacheck | + yfacheck.getQualifier() = var.getAnAccess() and + yfacheck.isUsedInCorrectLeapYearCheck() and + yfacheck = yfa.getASuccessor*() + ) + or + // If there is a data flow from the variable that was modified to a function that seems to check for leap year + exists( + VariableAccess source, ChecksForLeapYearFunctionCall fc, LeapYearCheckConfiguration config + | + source = var.getAnAccess() and + config.hasFlow(DataFlow::exprNode(source), DataFlow::exprNode(fc.getAnArgument())) + ) + or + // If there is a data flow from the field that was modified to a function that seems to check for leap year + exists( + VariableAccess vacheck, YearFieldAccess yfacheck, ChecksForLeapYearFunctionCall fc, + LeapYearCheckConfiguration config + | + vacheck = var.getAnAccess() and + yfacheck.getQualifier() = vacheck and + config.hasFlow(DataFlow::exprNode(yfacheck), DataFlow::exprNode(fc.getAnArgument())) + ) + or + // If there is a successor or predecessor that sets the month = 1 + exists(MonthFieldAccess mfa, AssignExpr ae | + mfa.getQualifier() = var.getAnAccess() and + mfa.isModified() and + ( + mfa = yfa.getASuccessor*() or + yfa = mfa.getASuccessor*() + ) and + ae = mfa.getEnclosingElement() and + ae.getAnOperand().getValue().toInt() = 1 + ) + ) + ) +select yfa, + "Field $@ on variable $@ has been modified, but no appropriate check for LeapYear was found.", + yfa.getTarget(), yfa.getTarget().toString(), var, var.toString() diff --git a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedReturnValueForTimeFunctions.ql b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedReturnValueForTimeFunctions.ql index 9f6b307b93b..2bc3e24d1ac 100644 --- a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedReturnValueForTimeFunctions.ql +++ b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedReturnValueForTimeFunctions.ql @@ -14,23 +14,22 @@ import LeapYear /** * A YearFieldAccess that is modifying the year by any arithmetic operation - * + * * NOTE: * To change this class to work for general purpose date transformations that do not check the return value, * make the following changes: - * -> extends FieldAccess (line 27) + * -> extends FieldAccess (line 27) * -> this.isModified (line 33) * Expect a lower precision for a general purpose version. */ - class DateStructModifiedFieldAccess extends LeapYearFieldAccess { - DateStructModifiedFieldAccess() { - exists( Field f, StructLikeClass struct | - f.getAnAccess() = this - and struct.getAField() = f - and struct.getUnderlyingType() instanceof DateDataStruct - and this.isModifiedByArithmeticOperation() - ) + DateStructModifiedFieldAccess() { + exists(Field f, StructLikeClass struct | + f.getAnAccess() = this and + struct.getAField() = f and + struct.getUnderlyingType() instanceof DateDataStruct and + this.isModifiedByArithmeticOperation() + ) } } @@ -39,9 +38,9 @@ class DateStructModifiedFieldAccess extends LeapYearFieldAccess { */ class SafeTimeGatheringFunction extends Function { SafeTimeGatheringFunction() { - this.getQualifiedName().matches("GetFileTime") - or this.getQualifiedName().matches("GetSystemTime") - or this.getQualifiedName().matches("NtQuerySystemTime") + this.getQualifiedName().matches("GetFileTime") or + this.getQualifiedName().matches("GetSystemTime") or + this.getQualifiedName().matches("NtQuerySystemTime") } } @@ -50,58 +49,62 @@ class SafeTimeGatheringFunction extends Function { */ class TimeConversionFunction extends Function { TimeConversionFunction() { - this.getQualifiedName().matches("FileTimeToSystemTime") - or this.getQualifiedName().matches("SystemTimeToFileTime") - or this.getQualifiedName().matches("SystemTimeToTzSpecificLocalTime") - or this.getQualifiedName().matches("SystemTimeToTzSpecificLocalTimeEx") - or this.getQualifiedName().matches("TzSpecificLocalTimeToSystemTime") - or this.getQualifiedName().matches("TzSpecificLocalTimeToSystemTimeEx") - or this.getQualifiedName().matches("RtlLocalTimeToSystemTime") - or this.getQualifiedName().matches("RtlTimeToSecondsSince1970") - or this.getQualifiedName().matches("_mkgmtime") + this.getQualifiedName().matches("FileTimeToSystemTime") or + this.getQualifiedName().matches("SystemTimeToFileTime") or + this.getQualifiedName().matches("SystemTimeToTzSpecificLocalTime") or + this.getQualifiedName().matches("SystemTimeToTzSpecificLocalTimeEx") or + this.getQualifiedName().matches("TzSpecificLocalTimeToSystemTime") or + this.getQualifiedName().matches("TzSpecificLocalTimeToSystemTimeEx") or + this.getQualifiedName().matches("RtlLocalTimeToSystemTime") or + this.getQualifiedName().matches("RtlTimeToSecondsSince1970") or + this.getQualifiedName().matches("_mkgmtime") } } -from FunctionCall fcall, TimeConversionFunction trf - , Variable var -where fcall = trf.getACallToThisFunction() - and fcall instanceof ExprInVoidContext - and var.getUnderlyingType() instanceof DateDataStruct - and (exists(AddressOfExpr aoe | - aoe = fcall.getAnArgument() - and aoe.getAddressable() = var - ) or exists(VariableAccess va | - fcall.getAnArgument() = va - and var.getAnAccess() = va +from FunctionCall fcall, TimeConversionFunction trf, Variable var +where + fcall = trf.getACallToThisFunction() and + fcall instanceof ExprInVoidContext and + var.getUnderlyingType() instanceof DateDataStruct and + ( + exists(AddressOfExpr aoe | + aoe = fcall.getAnArgument() and + aoe.getAddressable() = var ) - ) - and exists(DateStructModifiedFieldAccess dsmfa, VariableAccess modifiedVarAccess | - modifiedVarAccess = var.getAnAccess() - and modifiedVarAccess = dsmfa.getQualifier() - and modifiedVarAccess = fcall.getAPredecessor*() - ) + or + exists(VariableAccess va | + fcall.getAnArgument() = va and + var.getAnAccess() = va + ) + ) and + exists(DateStructModifiedFieldAccess dsmfa, VariableAccess modifiedVarAccess | + modifiedVarAccess = var.getAnAccess() and + modifiedVarAccess = dsmfa.getQualifier() and + modifiedVarAccess = fcall.getAPredecessor*() + ) and // Remove false positives - and not ( + not ( // Remove any instance where the predecessor is a SafeTimeGatheringFunction and no change to the data happened in between exists(FunctionCall pred | - pred = fcall.getAPredecessor*() - and exists( SafeTimeGatheringFunction stgf | - pred = stgf.getACallToThisFunction() + pred = fcall.getAPredecessor*() and + exists(SafeTimeGatheringFunction stgf | pred = stgf.getACallToThisFunction()) and + not exists(DateStructModifiedFieldAccess dsmfa, VariableAccess modifiedVarAccess | + modifiedVarAccess = var.getAnAccess() and + modifiedVarAccess = dsmfa.getQualifier() and + modifiedVarAccess = fcall.getAPredecessor*() and + modifiedVarAccess = pred.getASuccessor*() + ) ) - and not exists(DateStructModifiedFieldAccess dsmfa, VariableAccess modifiedVarAccess | - modifiedVarAccess = var.getAnAccess() - and modifiedVarAccess = dsmfa.getQualifier() - and modifiedVarAccess = fcall.getAPredecessor*() - and modifiedVarAccess = pred.getASuccessor*() + or + // Remove any instance where the year is changed, but the month is set to 1 (year wrapping) + exists(MonthFieldAccess mfa, AssignExpr ae | + mfa.getQualifier() = var.getAnAccess() and + mfa.isModified() and + mfa = fcall.getAPredecessor*() and + ae = mfa.getEnclosingElement() and + ae.getAnOperand().getValue().toInt() = 1 ) ) - // Remove any instance where the year is changed, but the month is set to 1 (year wrapping) - or exists(MonthFieldAccess mfa, AssignExpr ae | - mfa.getQualifier() = var.getAnAccess() - and mfa.isModified() - and mfa = fcall.getAPredecessor*() - and ae = mfa.getEnclosingElement() - and ae.getAnOperand().getValue().toInt() = 1 - ) - ) -select fcall, "Return value of $@ function should be verified to check for any error because variable $@ is not guaranteed to be safe.", trf, trf.getQualifiedName().toString(), var, var.getName() +select fcall, + "Return value of $@ function should be verified to check for any error because variable $@ is not guaranteed to be safe.", + trf, trf.getQualifiedName().toString(), var, var.getName() diff --git a/cpp/ql/src/Likely Bugs/Leap Year/UnsafeArrayForDaysOfYear.ql b/cpp/ql/src/Likely Bugs/Leap Year/UnsafeArrayForDaysOfYear.ql index fff99c18804..e734f8cb4b2 100644 --- a/cpp/ql/src/Likely Bugs/Leap Year/UnsafeArrayForDaysOfYear.ql +++ b/cpp/ql/src/Likely Bugs/Leap Year/UnsafeArrayForDaysOfYear.ql @@ -1,6 +1,6 @@ /** - * @name Unsafe array for days of the year - * @description An array of 365 items typically indicates one entry per day of the year, but without considering leap years, which would be 366 days. + * @name Unsafe array for days of the year + * @description An array of 365 items typically indicates one entry per day of the year, but without considering leap years, which would be 366 days. * An access on a leap year could result in buffer overflow bugs. * @kind problem * @problem.severity error @@ -13,24 +13,25 @@ import cpp class LeapYearUnsafeDaysOfTheYearArrayType extends ArrayType { - LeapYearUnsafeDaysOfTheYearArrayType() { - this.getArraySize() = 365 - } + LeapYearUnsafeDaysOfTheYearArrayType() { this.getArraySize() = 365 } } from Element element where - exists( NewArrayExpr nae | - element = nae - and nae.getAllocatedType() instanceof LeapYearUnsafeDaysOfTheYearArrayType - ) - or exists( Variable var | - var = element - and var.getType() instanceof LeapYearUnsafeDaysOfTheYearArrayType - ) - or exists( ConstructorCall cc | - element = cc - and cc.getTarget().hasName("vector") - and cc.getArgument(0).getValue().toInt() = 365 - ) -select element, "There is an array or std::vector allocation with a hard-coded set of 365 elements, which may indicate the number of days in a year without considering leap year scenarios." + exists(NewArrayExpr nae | + element = nae and + nae.getAllocatedType() instanceof LeapYearUnsafeDaysOfTheYearArrayType + ) + or + exists(Variable var | + var = element and + var.getType() instanceof LeapYearUnsafeDaysOfTheYearArrayType + ) + or + exists(ConstructorCall cc | + element = cc and + cc.getTarget().hasName("vector") and + cc.getArgument(0).getValue().toInt() = 365 + ) +select element, + "There is an array or std::vector allocation with a hard-coded set of 365 elements, which may indicate the number of days in a year without considering leap year scenarios."