diff --git a/cpp/ql/src/Likely Bugs/Leap Year/Adding365DaysPerYear.qhelp b/cpp/ql/src/Likely Bugs/Leap Year/Adding365DaysPerYear.qhelp index 186ec8079944..f0d303a05787 100644 --- a/cpp/ql/src/Likely Bugs/Leap Year/Adding365DaysPerYear.qhelp +++ b/cpp/ql/src/Likely Bugs/Leap Year/Adding365DaysPerYear.qhelp @@ -11,7 +11,7 @@ It is not safe to assume that a year is 365 days long.

Determine whether the time span in question contains a leap day, then perform the calculation using the correct number -of days. Alternatively, use an established library routine that already contains correct leap year logic.

+of days. Alternatively, use an established library routine that already contains correct leap year logic.

diff --git a/cpp/ql/src/Likely Bugs/Leap Year/Adding365DaysPerYear.ql b/cpp/ql/src/Likely Bugs/Leap Year/Adding365DaysPerYear.ql index 71aa97c0ae56..0b4f0f41e0ff 100644 --- a/cpp/ql/src/Likely Bugs/Leap Year/Adding365DaysPerYear.ql +++ b/cpp/ql/src/Likely Bugs/Leap Year/Adding365DaysPerYear.ql @@ -4,7 +4,7 @@ * value of 365, it may be a sign that leap years are not taken * into account. * @kind problem - * @problem.severity warning + * @problem.severity error * @id cpp/leap-year/adding-365-days-per-year * @precision medium * @tags leap-year @@ -13,11 +13,13 @@ import cpp import LeapYear +import semmle.code.cpp.dataflow.new.DataFlow from Expr source, Expr sink where PossibleYearArithmeticOperationCheckFlow::flow(DataFlow::exprNode(source), DataFlow::exprNode(sink)) select sink, - "An arithmetic operation $@ that uses a constant value of 365 ends up modifying this date/time, without considering leap year scenarios.", - source, source.toString() + "$@: This arithmetic operation $@ uses a constant value of 365 ends up modifying the date/time located at $@, without considering leap year scenarios.", + sink.getEnclosingFunction(), sink.getEnclosingFunction().toString(), source, source.toString(), + sink, sink.toString() diff --git a/cpp/ql/src/Likely Bugs/Leap Year/AntiPattern5InvalidLeapYearCheck.ql b/cpp/ql/src/Likely Bugs/Leap Year/AntiPattern5InvalidLeapYearCheck.ql new file mode 100644 index 000000000000..fd4366c376e0 --- /dev/null +++ b/cpp/ql/src/Likely Bugs/Leap Year/AntiPattern5InvalidLeapYearCheck.ql @@ -0,0 +1,17 @@ +/** + * @name Leap Year Invalid Check (AntiPattern 5) + * @description An expression is used to check a year is presumably a leap year, but the conditions used are insufficient. + * @kind problem + * @problem.severity warning + * @id cpp/leap-year/invalid-leap-year-check + * @precision medium + * @tags leap-year + * correctness + */ + +import cpp +import LeapYear + +from Mod4CheckedExpr exprMod4 +where not exists(ExprCheckLeapYear lyCheck | lyCheck.getAChild*() = exprMod4) +select exprMod4, "Possible Insufficient Leap Year check (AntiPattern 5)" diff --git a/cpp/ql/src/Likely Bugs/Leap Year/LeapYear.qll b/cpp/ql/src/Likely Bugs/Leap Year/LeapYear.qll index 3cff86412e49..d3b53bf8ff9b 100644 --- a/cpp/ql/src/Likely Bugs/Leap Year/LeapYear.qll +++ b/cpp/ql/src/Likely Bugs/Leap Year/LeapYear.qll @@ -3,7 +3,7 @@ */ import cpp -import semmle.code.cpp.ir.dataflow.TaintTracking +import semmle.code.cpp.dataflow.new.TaintTracking import semmle.code.cpp.commons.DateTime /** @@ -41,6 +41,271 @@ class CheckForLeapYearOperation extends Expr { } } +bindingset[modVal] +Expr moduloCheckEQ_0(EQExpr eq, int modVal) { + exists(RemExpr rem | rem = eq.getLeftOperand() | + result = rem.getLeftOperand() and + rem.getRightOperand().getValue().toInt() = modVal + ) and + eq.getRightOperand().getValue().toInt() = 0 +} + +bindingset[modVal] +Expr moduloCheckNEQ_0(NEExpr neq, int modVal) { + exists(RemExpr rem | rem = neq.getLeftOperand() | + result = rem.getLeftOperand() and + rem.getRightOperand().getValue().toInt() = modVal + ) and + 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. + */ +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() + ) +} + +/** + * An expression that is the subject of a mod-4 check. + * ie `expr % 4 == 0` + */ +class Mod4CheckedExpr extends Expr { + Mod4CheckedExpr() { exists(Expr e | e = moduloCheckEQ_0(this, 4)) } +} + +/** + * Year Div of 100 not equal to 0: + * - `year % 100 != 0` + * - `!(year % 100 == 0)` + */ +abstract class ExprCheckCenturyComponentDiv100 extends Expr { + abstract Expr getYearExpr(); +} + +/** + * The normal form of the expression `year % 100 != 0`. + */ +final class ExprCheckCenturyComponentDiv100Normative extends ExprCheckCenturyComponentDiv100 { + ExprCheckCenturyComponentDiv100Normative() { exists(moduloCheckNEQ_0(this, 100)) } + + override Expr getYearExpr() { result = moduloCheckNEQ_0(this, 100) } +} + +/** + * The inverted form of the expression `year % 100 != 0`, ie `!(year % 100 == 0)` + */ +final class ExprCheckCenturyComponentDiv100Inverted extends ExprCheckCenturyComponentDiv100, NotExpr +{ + ExprCheckCenturyComponentDiv100Inverted() { exists(moduloCheckEQ_0(this.getOperand(), 100)) } + + override Expr getYearExpr() { result = moduloCheckEQ_0(this.getOperand(), 100) } +} + +/** + * A check that an expression is divisible by 400 or not + * - `(year % 400 == 0)` + * - `!(year % 400 != 0)` + */ +abstract class ExprCheckCenturyComponentDiv400 extends Expr { + abstract Expr getYearExpr(); +} + +/** + * The normative form of expression is divisible by 400: + * ie `year % 400 == 0` + */ +final class ExprCheckCenturyComponentDiv400Normative extends ExprCheckCenturyComponentDiv400 { + ExprCheckCenturyComponentDiv400Normative() { exists(moduloCheckEQ_0(this, 400)) } + + override Expr getYearExpr() { + exists(Expr e | + e = moduloCheckEQ_0(this, 400) and + ( + if e instanceof ConvertedYearByOffset + then result = e.(ConvertedYearByOffset).getYearOperand() + else result = e + ) + ) + } +} + +/** + * An arithmetic operation that seemingly converts an operand between time formats. + */ +class ConvertedYearByOffset extends BinaryArithmeticOperation { + ConvertedYearByOffset() { + this.getAnOperand().getValue().toInt() instanceof TimeFormatConversionOffset + } + + Expr getYearOperand() { + this.getLeftOperand().getValue().toInt() instanceof TimeFormatConversionOffset and + result = this.getRightOperand() + or + this.getRightOperand().getValue().toInt() instanceof TimeFormatConversionOffset and + result = this.getLeftOperand() + } +} + +/** + * A flow configuration to track DataFlow from a `CovertedYearByOffset` to some `StructTmLeapYearFieldAccess`. + */ +module LocalConvertedYearByOffsetToLeapYearCheckFlowConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node n) { not n.asExpr() instanceof ConvertedYearByOffset } + + predicate isSink(DataFlow::Node n) { n.asExpr() instanceof StructTmLeapYearFieldAccess } +} + +module LocalConvertedYearByOffsetToLeapYearCheckFlow = + DataFlow::Global; + +/** + * The set of ints (or strings) which represent a value that is typically used to convert between time data types. + */ +final class TimeFormatConversionOffset extends int { + TimeFormatConversionOffset() { + this = + [ + 1900, // tm_year represents years since 1900 + 1970, // converting from/to Unix epoch + 2000, // some systems may use 2000 for 2-digit year conversions + ] + } +} + +/** + * The inverted form of expression is divisible by 400: + * ie `!(year % 400 != 0)` + */ +final class ExprCheckCenturyComponentDiv400Inverted extends ExprCheckCenturyComponentDiv400, NotExpr +{ + ExprCheckCenturyComponentDiv400Inverted() { exists(moduloCheckNEQ_0(this.getOperand(), 400)) } + + override Expr getYearExpr() { result = moduloCheckNEQ_0(this.getOperand(), 400) } +} + +/** + * The Century component of a Leap-Year guard + */ +class ExprCheckCenturyComponent extends LogicalOrExpr { + ExprCheckCenturyComponent() { + exists(ExprCheckCenturyComponentDiv400 exprDiv400, ExprCheckCenturyComponentDiv100 exprDiv100 | + this.getAnOperand() = exprDiv100 and + this.getAnOperand() = exprDiv400 and + exprEq_propertyPermissive(exprDiv100.getYearExpr(), exprDiv400.getYearExpr()) + ) + } + + Expr getYearExpr() { + exists(ExprCheckCenturyComponentDiv400 exprDiv400 | + this.getAnOperand() = exprDiv400 and + result = exprDiv400.getYearExpr() + ) + } +} + +/** + * A **Valid** Leap year check expression. + */ +abstract class ExprCheckLeapYear extends Expr { } + +/** + * A valid Leap-Year guard expression of the following form: + * `dt.Year % 4 == 0 && (dt.Year % 100 != 0 || dt.Year % 400 == 0)` + */ +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()) + ) + } +} + +/** + * A valid Leap-Year guard expression of the following forms: + * `year % 400 == 0 || (year % 100 != 0 && year % 4 == 0)` + * `(year + 1900) % 400 == 0 || (year % 100 != 0 && year % 4 == 0)` + */ +final class ExprCheckLeapYearFormB extends ExprCheckLeapYear, LogicalOrExpr { + ExprCheckLeapYearFormB() { + 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) + ) + ) + } +} + +Expr leapYearOpEnclosingElement(CheckForLeapYearOperation op) { result = op.getEnclosingElement() } + +/** + * A value that resolves as a constant integer that represents some normalization or conversion between date types. + */ +pragma[inline] +private predicate isNormalizationPrimitiveValue(Expr e) { + e.getValue().toInt() = [1900, 2000, 1980, 80] +} + +/** + * A normalization operation is an expression that is merely attempting to convert between two different datetime schemes, + * and does not apply any additional mutation to the represented value. + */ +pragma[inline] +predicate isNormalizationOperation(Expr e) { + isNormalizationPrimitiveValue([e, e.(Operation).getAChild()]) + or + // Special case for transforming marshaled 2-digit year date: + // theTime.wYear += 100*value; + e.(Operation).getAChild().(MulExpr).getValue().toInt() = 100 +} + +/** + * Get the field accesses used in a `ExprCheckLeapYear` expression. + */ +LeapYearFieldAccess leapYearCheckFieldAccess(ExprCheckLeapYear a) { result = a.getAChild*() } + /** * A `YearFieldAccess` that would represent an access to a year field on a struct and is used for arguing about leap year calculations. */ @@ -73,48 +338,7 @@ abstract class LeapYearFieldAccess extends YearFieldAccess { this.isModified() and exists(Operation op | op.getAnOperand() = this and - ( - op instanceof AssignArithmeticOperation and - not ( - op.getAChild().getValue().toInt() = 1900 - or - op.getAChild().getValue().toInt() = 2000 - or - op.getAChild().getValue().toInt() = 1980 - or - op.getAChild().getValue().toInt() = 80 - or - // Special case for transforming marshaled 2-digit year date: - // theTime.wYear += 100*value; - exists(MulExpr mulBy100 | mulBy100 = op.getAChild() | - mulBy100.getAChild().getValue().toInt() = 100 - ) - ) - or - exists(BinaryArithmeticOperation bao | - bao = op.getAnOperand() and - // we're specifically interested in calculations that update the existing - // value (like `x = x + 1`), so look for a child `YearFieldAccess`. - bao.getAChild*() instanceof YearFieldAccess and - not ( - bao.getAChild().getValue().toInt() = 1900 - or - bao.getAChild().getValue().toInt() = 2000 - or - bao.getAChild().getValue().toInt() = 1980 - or - bao.getAChild().getValue().toInt() = 80 - or - // Special case for transforming marshaled 2-digit year date: - // theTime.wYear += 100*value; - exists(MulExpr mulBy100 | mulBy100 = op.getAChild() | - mulBy100.getAChild().getValue().toInt() = 100 - ) - ) - ) - or - op instanceof CrementOperation - ) + not isNormalizationOperation(op) ) } @@ -155,9 +379,7 @@ abstract class LeapYearFieldAccess extends YearFieldAccess { // but these centurial years are leap years if they are exactly divisible by 400 // // https://aa.usno.navy.mil/faq/docs/calendars.php - this.isUsedInMod4Operation() and - this.additionalModulusCheckForLeapYear(400) and - this.additionalModulusCheckForLeapYear(100) + this = leapYearCheckFieldAccess(_) } } @@ -175,19 +397,9 @@ class StructTmLeapYearFieldAccess extends LeapYearFieldAccess { StructTmLeapYearFieldAccess() { this.getTarget().getName() = "tm_year" } override predicate isUsedInCorrectLeapYearCheck() { - this.isUsedInMod4Operation() and - this.additionalModulusCheckForLeapYear(400) and - this.additionalModulusCheckForLeapYear(100) and - // tm_year represents years since 1900 - ( - this.additionalAdditionOrSubstractionCheckForLeapYear(1900) - or - // some systems may use 2000 for 2-digit year conversions - this.additionalAdditionOrSubstractionCheckForLeapYear(2000) - or - // converting from/to Unix epoch - this.additionalAdditionOrSubstractionCheckForLeapYear(1970) - ) + this = leapYearCheckFieldAccess(_) and + /* There is some data flow from some conversion arithmetic to this expression. */ + LocalConvertedYearByOffsetToLeapYearCheckFlow::flow(_, DataFlow::exprNode(this)) } } @@ -206,10 +418,10 @@ class ChecksForLeapYearFunctionCall extends FunctionCall { } /** - * Data flow configuration for finding a variable access that would flow into + * A `DataFlow` configuraiton for finding a variable access that would flow into * a function call that includes an operation to check for leap year. */ -private module LeapYearCheckConfig implements DataFlow::ConfigSig { +private module LeapYearCheckFlowConfig implements DataFlow::ConfigSig { predicate isSource(DataFlow::Node source) { source.asExpr() instanceof VariableAccess } predicate isSink(DataFlow::Node sink) { @@ -217,11 +429,10 @@ private module LeapYearCheckConfig implements DataFlow::ConfigSig { } } -module LeapYearCheckFlow = DataFlow::Global; +module LeapYearCheckFlow = DataFlow::Global; /** - * Data flow configuration for finding an operation with hardcoded 365 that will flow into - * a `FILEINFO` field. + * A `DataFlow` configuration for finding an operation with hardcoded 365 that will flow into a `_FILETIME` field. */ private module FiletimeYearArithmeticOperationCheckConfig implements DataFlow::ConfigSig { predicate isSource(DataFlow::Node source) { @@ -246,46 +457,72 @@ module FiletimeYearArithmeticOperationCheckFlow = DataFlow::Global; /** - * Taint configuration for finding an operation with hardcoded 365 that will flow into any known date/time field. + * A `DataFlow` configuration for finding an operation with hardcoded 365 that will flow into any known date/time field. */ private module PossibleYearArithmeticOperationCheckConfig implements DataFlow::ConfigSig { predicate isSource(DataFlow::Node source) { - exists(Operation op | op = source.asExpr() | - op.getAChild*().getValue().toInt() = 365 and - ( - not op.getParent() instanceof Expr or - op.getParent() instanceof Assignment - ) - ) - } - - predicate isBarrierIn(DataFlow::Node node) { isSource(node) } - - predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) { - // flow from anything on the RHS of an assignment to a time/date structure to that - // assignment. - exists(StructLikeClass dds, FieldAccess fa, Assignment aexpr, Expr e | - e = node1.asExpr() and - fa = node2.asExpr() - | - (dds instanceof PackedTimeType or dds instanceof UnpackedTimeType) and - fa.getQualifier().getUnderlyingType() = dds and - aexpr.getLValue() = fa and - aexpr.getRValue().getAChild*() = e - ) + // NOTE: addressing current issue with new IR dataflow, where + // constant folding occurs before dataflow nodes are associated + // with the constituent literals. + source.asExpr().getAChild*().getValue().toInt() = 365 and + not exists(DataFlow::Node parent | parent.asExpr().getAChild+() = source.asExpr()) } predicate isSink(DataFlow::Node sink) { exists(StructLikeClass dds, FieldAccess fa, AssignExpr aexpr | - aexpr.getRValue() = sink.asExpr() - | (dds instanceof PackedTimeType or dds instanceof UnpackedTimeType) and fa.getQualifier().getUnderlyingType() = dds and fa.isModified() and - aexpr.getLValue() = fa + aexpr.getLValue() = fa and + sink.asExpr() = aexpr.getRValue() ) } } module PossibleYearArithmeticOperationCheckFlow = TaintTracking::Global; + +/** + * 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: + * - change `extends LeapYearFieldAccess` to `extends FieldAccess`. + * - change `this.isModifiedByArithmeticOperation()` to `this.isModified()`. + * 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 UnpackedTimeType and + this.isModifiedByArithmeticOperation() + ) + } +} + +/** + * This is a list of APIs that will get the system time, and therefore guarantee that the value is valid. + */ +class SafeTimeGatheringFunction extends Function { + SafeTimeGatheringFunction() { + this.getQualifiedName() = ["GetFileTime", "GetSystemTime", "NtQuerySystemTime"] + } +} + +/** + * This list of APIs should check for the return value to detect problems during the conversion. + */ +class TimeConversionFunction extends Function { + TimeConversionFunction() { + this.getQualifiedName() = + [ + "FileTimeToSystemTime", "SystemTimeToFileTime", "SystemTimeToTzSpecificLocalTime", + "SystemTimeToTzSpecificLocalTimeEx", "TzSpecificLocalTimeToSystemTime", + "TzSpecificLocalTimeToSystemTimeEx", "RtlLocalTimeToSystemTime", + "RtlTimeToSecondsSince1970", "_mkgmtime" + ] + } +} diff --git a/cpp/ql/src/Likely Bugs/Leap Year/LeapYearConditionalLogic.qhelp b/cpp/ql/src/Likely Bugs/Leap Year/LeapYearConditionalLogic.qhelp new file mode 100644 index 000000000000..4c94585477ae --- /dev/null +++ b/cpp/ql/src/Likely Bugs/Leap Year/LeapYearConditionalLogic.qhelp @@ -0,0 +1,26 @@ + + + + +

This anti-pattern occurs when a developer uses conditional logic to execute a different path of code for a leap year than for a common year, without fully testing both code paths.

+

Though using a framework or library's leap year function is better than manually calculating the leap year (as described in anti-pattern 5), it can still be a source of errors if the result is used to execute a different code path. The bug is especially easy to be masked if the year is derived from the current time of the system clock. See Prevention Measures for techniques to avoid this bug.

+
+ +
    +
  • Avoid using conditional logic that creates a separate branch in your code for leap year.
  • +
  • Ensure your code is testable, and test how it will behave when presented with leap year dates of February 29th and December 31st as inputs.
  • +
+
+ +

Note in the following examples, that year, month, and day might instead be .wYear, .wMonth, and .wDay fields of a SYSTEMTIME structure, or might be .tm_year, .tm_mon, and .tm_mday fields of a struct tm.

+ +
+ + +
  • NASA / Goddard Space Flight Center - Calendars
  • +
  • Wikipedia - Leap year bug
  • +
  • Microsoft Azure blog - Is your code ready for the leap year?
  • +
    +
    diff --git a/cpp/ql/src/Likely Bugs/Leap Year/LeapYearConditionalLogic.ql b/cpp/ql/src/Likely Bugs/Leap Year/LeapYearConditionalLogic.ql new file mode 100644 index 000000000000..075d8da84259 --- /dev/null +++ b/cpp/ql/src/Likely Bugs/Leap Year/LeapYearConditionalLogic.ql @@ -0,0 +1,28 @@ +/** + * @name Leap Year Conditional Logic (AntiPattern 7) + * @description Conditional logic is present for leap years and common years, potentially leading to untested code pathways. + * @kind problem + * @problem.severity warning + * @id cpp/leap-year/conditional-logic-branches + * @precision medium + * @tags leap-year + * correctness + */ + +import cpp +import LeapYear +import semmle.code.cpp.dataflow.new.DataFlow + +class IfStmtLeapYearCheck extends IfStmt { + IfStmtLeapYearCheck() { + this.hasElse() and + exists(ExprCheckLeapYear lyCheck, DataFlow::Node source, DataFlow::Node sink | + source.asExpr() = lyCheck and + sink.asExpr() = this.getCondition() and + DataFlow::localFlow(source, sink) + ) + } +} + +from IfStmtLeapYearCheck lyCheckIf +select lyCheckIf, "Leap Year conditional statement may have untested code paths" diff --git a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification.qhelp b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification.qhelp index 8fbe7933201b..b708e127767c 100644 --- a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification.qhelp +++ b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification.qhelp @@ -15,10 +15,10 @@

    In this example, we are adding 1 year to the current date. This may work most of the time, but on any given February 29th, the resulting value will be invalid.

    - +

    To fix this bug, check the result for leap year.

    - +
    diff --git a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification.ql b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification.ql index 03570b3611cd..6fd38bae01ee 100644 --- a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification.ql +++ b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification.ql @@ -1,5 +1,5 @@ /** - * @name Year field changed using an arithmetic operation without checking for leap year + * @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 * @problem.severity warning @@ -12,13 +12,16 @@ import cpp import LeapYear -from Variable var, LeapYearFieldAccess yfa -where - exists(VariableAccess va | +/** + * 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 - // 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 @@ -41,8 +44,10 @@ where LeapYearCheckFlow::flow(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 | + // 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 ( @@ -50,10 +55,87 @@ where yfa.getBasicBlock() = mfa.getBasicBlock().getASuccessor+() ) and ae = mfa.getEnclosingElement() and - ae.getAnOperand().getValue().toInt() = 1 + ae.getAnOperand().getValue().toInt() = val ) ) ) -select yfa, - "Field $@ on variable $@ has been modified, but no appropriate check for LeapYear was found.", - yfa.getTarget(), yfa.getTarget().toString(), var, var.toString() +} + +/** + * The set of all write operations to the Year field of a date struct. + */ +abstract class YearWriteOp extends Operation { + /** Extracts the access to the Year field */ + abstract YearFieldAccess getYearAccess(); + + /** Get the expression which represents the new value. */ + abstract Expr getMutationExpr(); +} + +/** + * A unary operation (Crement) performed on a Year field. + */ +class YearWriteOpUnary extends YearWriteOp, UnaryOperation { + YearWriteOpUnary() { this.getOperand() instanceof YearFieldAccess } + + override YearFieldAccess getYearAccess() { result = this.getOperand() } + + override Expr getMutationExpr() { result = this } +} + +/** + * An assignment operation or mutation on the Year field of a date object. + */ +class YearWriteOpAssignment extends YearWriteOp, Assignment { + YearWriteOpAssignment() { this.getLValue() instanceof YearFieldAccess } + + override YearFieldAccess getYearAccess() { result = this.getLValue() } + + 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 + } +} + +/** + * 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) { + not n.asExpr() instanceof Access and + not n.asExpr() instanceof Literal + } + + predicate isSink(DataFlow::Node n) { + exists(Assignment a | + a.getLValue() instanceof YearFieldAccess and + a.getRValue() = n.asExpr() + ) + } +} + +module OperationToYearAssignmentFlow = DataFlow::Global; + +from Variable var, YearWriteOp ywo, Expr mutationExpr +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 + ywo.getASuccessor*() = c + ) +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() diff --git a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedReturnValueForTimeFunctions.qhelp b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedReturnValueForTimeFunctions.qhelp index 6be0e091caf3..f3c4822632fb 100644 --- a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedReturnValueForTimeFunctions.qhelp +++ b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedReturnValueForTimeFunctions.qhelp @@ -27,10 +27,10 @@

    In this example, we are adding 1 year to the current date. This may work most of the time, but on any given February 29th, the resulting value will be invalid.

    - +

    To fix this bug, you must verify the return value for SystemTimeToFileTime and handle any potential error accordingly.

    - +
    diff --git a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedReturnValueForTimeFunctions.ql b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedReturnValueForTimeFunctions.ql index af02a2814a20..087aa8b77d1a 100644 --- a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedReturnValueForTimeFunctions.ql +++ b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedReturnValueForTimeFunctions.ql @@ -1,5 +1,5 @@ /** - * @name Unchecked return value for time conversion function + * @name Unchecked return value for time conversion function (AntiPattern 6) * @description When the return value of a fallible time conversion function is * not checked for failure, its output parameters may contain * invalid dates. @@ -14,51 +14,6 @@ import cpp 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: - * - change `extends LeapYearFieldAccess` to `extends FieldAccess`. - * - change `this.isModifiedByArithmeticOperation()` to `this.isModified()`. - * 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 UnpackedTimeType and - this.isModifiedByArithmeticOperation() - ) - } -} - -/** - * This is a list of APIs that will get the system time, and therefore guarantee that the value is valid. - */ -class SafeTimeGatheringFunction extends Function { - SafeTimeGatheringFunction() { - this.getQualifiedName() = ["GetFileTime", "GetSystemTime", "NtQuerySystemTime"] - } -} - -/** - * This list of APIs should check for the return value to detect problems during the conversion. - */ -class TimeConversionFunction extends Function { - TimeConversionFunction() { - this.getQualifiedName() = - [ - "FileTimeToSystemTime", "SystemTimeToFileTime", "SystemTimeToTzSpecificLocalTime", - "SystemTimeToTzSpecificLocalTimeEx", "TzSpecificLocalTimeToSystemTime", - "TzSpecificLocalTimeToSystemTimeEx", "RtlLocalTimeToSystemTime", - "RtlTimeToSecondsSince1970", "_mkgmtime" - ] - } -} - from FunctionCall fcall, TimeConversionFunction trf, Variable var where fcall = trf.getACallToThisFunction() and @@ -104,5 +59,6 @@ where ) ) 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() + "$@: Return value of $@ function should be verified to check for any error because variable $@ is not guaranteed to be safe.", + fcall.getEnclosingFunction(), fcall.getEnclosingFunction().toString(), trf, + trf.getQualifiedName().toString(), var, var.getName() diff --git a/cpp/ql/src/Likely Bugs/Leap Year/UnsafeArrayForDaysOfYear.qhelp b/cpp/ql/src/Likely Bugs/Leap Year/UnsafeArrayForDaysOfYear.qhelp index d2c0375f0afc..03a8ff6216bb 100644 --- a/cpp/ql/src/Likely Bugs/Leap Year/UnsafeArrayForDaysOfYear.qhelp +++ b/cpp/ql/src/Likely Bugs/Leap Year/UnsafeArrayForDaysOfYear.qhelp @@ -16,15 +16,15 @@

    In this example, we are allocating 365 integers, one for each day of the year. This code will fail on a leap year, when there are 366 days.

    - +

    When using arrays, allocate the correct number of elements to match the year.

    - +
  • NASA / Goddard Space Flight Center - Calendars
  • -
  • Wikipedia - Leap year bug
  • +
  • Wikipedia - Leap year bug
  • Microsoft Azure blog - Is your code ready for the leap year?
  • diff --git a/cpp/ql/src/Likely Bugs/Leap Year/UnsafeArrayForDaysOfYear.ql b/cpp/ql/src/Likely Bugs/Leap Year/UnsafeArrayForDaysOfYear.ql index b27db937b577..bb14b4efce54 100644 --- a/cpp/ql/src/Likely Bugs/Leap Year/UnsafeArrayForDaysOfYear.ql +++ b/cpp/ql/src/Likely Bugs/Leap Year/UnsafeArrayForDaysOfYear.ql @@ -1,41 +1,62 @@ /** - * @name Unsafe array for days of the year + * @name Unsafe array for days of the year (AntiPattern 4) * @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 warning * @id cpp/leap-year/unsafe-array-for-days-of-the-year * @precision low - * @tags security - * leap-year + * @tags leap-year + * correctness */ import cpp -class LeapYearUnsafeDaysOfTheYearArrayType extends ArrayType { - LeapYearUnsafeDaysOfTheYearArrayType() { this.getArraySize() = 365 } -} +/* Note: We used to have a `LeapYearUnsafeDaysOfTheYearArrayType` class which was the + set of ArrayType that had a fixed length of 365. However, to eliminate false positives, + we use `isElementAnArrayOfFixedSize` that *also* finds arrays of 366 items, where the programmer + has also catered for leap years. + So, instead of `instanceof` checks, for simplicity, we simply pass in 365/366 as integers as needed. +*/ -from Element element, string allocType -where +bindingset[size] +predicate isElementAnArrayOfFixedSize( + Element element, Type t, Declaration f, string allocType, int size +) { exists(NewArrayExpr nae | element = nae and - nae.getAllocatedType() instanceof LeapYearUnsafeDaysOfTheYearArrayType and - allocType = "an array allocation" + nae.getAllocatedType().(ArrayType).getArraySize() = size and + allocType = "an array allocation" and + f = nae.getEnclosingFunction() and + t = nae.getAllocatedType().(ArrayType).getBaseType() ) or exists(Variable var | var = element and - var.getType() instanceof LeapYearUnsafeDaysOfTheYearArrayType and - allocType = "an array allocation" + var.getType().(ArrayType).getArraySize() = size and + allocType = "an array allocation" and + f = var and + t = var.getType().(ArrayType).getBaseType() ) or exists(ConstructorCall cc | element = cc and cc.getTarget().hasName("vector") and - cc.getArgument(0).getValue().toInt() = 365 and - allocType = "a std::vector allocation" + cc.getArgument(0).getValue().toInt() = size and + allocType = "a std::vector allocation" and + f = cc.getEnclosingFunction() and + t = cc.getTarget().getDeclaringType() + ) +} + +from Element element, string allocType, Declaration f, Type t +where + isElementAnArrayOfFixedSize(element, t, f, allocType, 365) and + not exists(Element element2, Declaration f2 | + isElementAnArrayOfFixedSize(element2, t, f2, _, 366) and + if f instanceof Function then f = f2 else f.getParentScope() = f2.getParentScope() ) select element, - "There is " + allocType + - " with a hard-coded set of 365 elements, which may indicate the number of days in a year without considering leap year scenarios." + "$@: There is " + allocType + + " with a hard-coded set of 365 elements, which may indicate the number of days in a year without considering leap year scenarios.", + f, f.toString() diff --git a/cpp/ql/src/Likely Bugs/Leap Year/examples/LeapYearConditionalLogicBad.c b/cpp/ql/src/Likely Bugs/Leap Year/examples/LeapYearConditionalLogicBad.c new file mode 100644 index 000000000000..7751b9eb34b8 --- /dev/null +++ b/cpp/ql/src/Likely Bugs/Leap Year/examples/LeapYearConditionalLogicBad.c @@ -0,0 +1,21 @@ +// Checking for leap year +bool isLeapYear = year % 4 == 0 && (year % 100 != 0 || year % 400 == 0); +if (isLeapYear) +{ + // untested path +} +else +{ + // tested path +} + + +// Checking specifically for the leap day +if (month == 2 && day == 29) // (or 1 with a tm_mon value) +{ + // untested path +} +else +{ + // tested path +} \ No newline at end of file diff --git a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationBad.c b/cpp/ql/src/Likely Bugs/Leap Year/examples/UncheckedLeapYearAfterYearModificationBad.c similarity index 100% rename from cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationBad.c rename to cpp/ql/src/Likely Bugs/Leap Year/examples/UncheckedLeapYearAfterYearModificationBad.c diff --git a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationGood.c b/cpp/ql/src/Likely Bugs/Leap Year/examples/UncheckedLeapYearAfterYearModificationGood.c similarity index 100% rename from cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationGood.c rename to cpp/ql/src/Likely Bugs/Leap Year/examples/UncheckedLeapYearAfterYearModificationGood.c diff --git a/cpp/ql/src/Likely Bugs/Leap Year/UnsafeArrayForDaysOfYearBad.c b/cpp/ql/src/Likely Bugs/Leap Year/examples/UnsafeArrayForDaysOfYearBad.c similarity index 100% rename from cpp/ql/src/Likely Bugs/Leap Year/UnsafeArrayForDaysOfYearBad.c rename to cpp/ql/src/Likely Bugs/Leap Year/examples/UnsafeArrayForDaysOfYearBad.c diff --git a/cpp/ql/src/Likely Bugs/Leap Year/UnsafeArrayForDaysOfYearGood.c b/cpp/ql/src/Likely Bugs/Leap Year/examples/UnsafeArrayForDaysOfYearGood.c similarity index 100% rename from cpp/ql/src/Likely Bugs/Leap Year/UnsafeArrayForDaysOfYearGood.c rename to cpp/ql/src/Likely Bugs/Leap Year/examples/UnsafeArrayForDaysOfYearGood.c diff --git a/cpp/ql/src/Microsoft/Likely Bugs/Conversion/BadOverflowGuard.qhelp b/cpp/ql/src/Microsoft/Likely Bugs/Conversion/BadOverflowGuard.qhelp new file mode 100644 index 000000000000..1f27b051e8f8 --- /dev/null +++ b/cpp/ql/src/Microsoft/Likely Bugs/Conversion/BadOverflowGuard.qhelp @@ -0,0 +1,20 @@ + + + +

    Checking for overflow of an addition by comparing against one of the arguments of the addition fails if the size of all the argument types are smaller than 4 bytes. This is because the result of the addition is promoted to a 4 byte int.

    +
    + + +

    Check the overflow by comparing the addition against a value that is at least 4 bytes.

    +
    + + +

    In this example, the result of the comparison will result in an integer overflow.

    + + +

    To fix the bug, check the overflow by comparing the addition against a value that is at least 4 bytes.

    + +
    +
    diff --git a/cpp/ql/src/Microsoft/Likely Bugs/Conversion/BadOverflowGuard.ql b/cpp/ql/src/Microsoft/Likely Bugs/Conversion/BadOverflowGuard.ql new file mode 100644 index 000000000000..a0aaeed4f115 --- /dev/null +++ b/cpp/ql/src/Microsoft/Likely Bugs/Conversion/BadOverflowGuard.ql @@ -0,0 +1,31 @@ +/** + * @name Bad overflow check + * @description Checking for overflow of an addition by comparing against one + * of the arguments of the addition fails if the size of all the + * argument types are smaller than 4 bytes. This is because the + * result of the addition is promoted to a 4 byte int. + * @kind problem + * @problem.severity error + * @tags security + * external/cwe/cwe-190 + * external/cwe/cwe-191 + * @id cpp/badoverflowguard + */ + +import cpp + +/* + * Example: + * + * uint16 v, uint16 b + * if ((v + b < v) <-- bad check for overflow + */ + +from AddExpr a, Variable v, RelationalOperation cmp +where + a.getAnOperand() = v.getAnAccess() and + forall(Expr op | op = a.getAnOperand() | op.getType().getSize() < 4) and + cmp.getAnOperand() = a and + cmp.getAnOperand() = v.getAnAccess() and + not a.getExplicitlyConverted().getType().getSize() < 4 +select cmp, "Bad overflow check" diff --git a/cpp/ql/src/Microsoft/Likely Bugs/Conversion/BadOverflowGuardBadCode.c b/cpp/ql/src/Microsoft/Likely Bugs/Conversion/BadOverflowGuardBadCode.c new file mode 100644 index 000000000000..b7dc59a33785 --- /dev/null +++ b/cpp/ql/src/Microsoft/Likely Bugs/Conversion/BadOverflowGuardBadCode.c @@ -0,0 +1,9 @@ +unsigned short CheckForInt16OverflowBadCode(unsigned short v, unsigned short b) +{ + if (v + b < v) // BUG: "v + b" will be promoted to 32 bits + { + // ... do something + } + + return v + b; +} diff --git a/cpp/ql/src/Microsoft/Likely Bugs/Conversion/BadOverflowGuardGoodCode.c b/cpp/ql/src/Microsoft/Likely Bugs/Conversion/BadOverflowGuardGoodCode.c new file mode 100644 index 000000000000..f5cc5c2ed4f6 --- /dev/null +++ b/cpp/ql/src/Microsoft/Likely Bugs/Conversion/BadOverflowGuardGoodCode.c @@ -0,0 +1,9 @@ +unsigned short CheckForInt16OverflowCorrectCode(unsigned short v, unsigned short b) +{ + if (v + b > 0x00FFFF) + { + // ... do something + } + + return v + b; +} \ No newline at end of file diff --git a/cpp/ql/src/Microsoft/Likely Bugs/Drivers/IncorrectUsageOfRtlCompareMemory.qhelp b/cpp/ql/src/Microsoft/Likely Bugs/Drivers/IncorrectUsageOfRtlCompareMemory.qhelp new file mode 100644 index 000000000000..e7a85e353ed5 --- /dev/null +++ b/cpp/ql/src/Microsoft/Likely Bugs/Drivers/IncorrectUsageOfRtlCompareMemory.qhelp @@ -0,0 +1,29 @@ + + + +

    RtlCompareMemory routine compares two blocks of memory and returns the number of bytes that match, not a boolean value indicating a full comparison like RtlEqualMemory does.

    +

    This query detects the return value of RtlCompareMemory being handled as a boolean.

    +
    + + +

    Any findings from this rule may indicate that the return value of a call to RtlCompareMemory is being incorrectly interpreted as a boolean.

    +

    Review the logic of the call, and if necessary, replace the function call with RtlEqualMemory.

    +
    + + +

    The following example is a typical one where an identity comparison is intended, but the wrong API is being used.

    + + +

    In this example, the fix is to replace the call to RtlCompareMemory with RtlEqualMemory.

    + + + +
    + +
  • Books online RtlCompareMemory function (wdm.h)
  • +
  • Books online RtlEqualMemory macro (wdm.h)
  • +
    + +
    \ No newline at end of file diff --git a/cpp/ql/src/Microsoft/Likely Bugs/Drivers/IncorrectUsageOfRtlCompareMemory.ql b/cpp/ql/src/Microsoft/Likely Bugs/Drivers/IncorrectUsageOfRtlCompareMemory.ql new file mode 100644 index 000000000000..ee7d58cd3636 --- /dev/null +++ b/cpp/ql/src/Microsoft/Likely Bugs/Drivers/IncorrectUsageOfRtlCompareMemory.ql @@ -0,0 +1,69 @@ +/** + * @id cpp/drivers/incorrect-usage-of-rtlcomparememory + * @name Incorrect usage of RtlCompareMemory + * @description `RtlCompareMemory` routine compares two blocks of memory and returns the number of bytes that match, not a boolean value indicating a full comparison like `RtlEqualMemory` does. + * This query detects the return value of `RtlCompareMemory` being handled as a boolean. + * @security.severity Important + * @kind problem + * @problem.severity error + * @precision high + * @tags security + * kernel + */ + +import cpp + +predicate isLiteralABooleanMacro(Literal l) { + exists(MacroInvocation mi | mi.getExpr() = l | + mi.getMacroName() in ["true", "false", "TRUE", "FALSE"] + ) +} + +from FunctionCall fc, Function f, Expr e, string msg +where + f.getQualifiedName() = "RtlCompareMemory" and + f.getACallToThisFunction() = fc and + ( + exists(UnaryLogicalOperation ulo | e = ulo | + ulo.getAnOperand() = fc and + msg = "as an operand in an unary logical operation" + ) + or + exists(BinaryLogicalOperation blo | e = blo | + blo.getAnOperand() = fc and + msg = "as an operand in a binary logical operation" + ) + or + exists(Conversion conv | e = conv | + ( + conv.getType().hasName("bool") or + conv.getType().hasName("BOOLEAN") or + conv.getType().hasName("_Bool") + ) and + conv.getUnconverted() = fc and + msg = "as a boolean" + ) + or + exists(IfStmt s | e = s.getControllingExpr() | + s.getControllingExpr() = fc and + msg = "as the controlling expression in an If statement" + ) + or + exists(EqualityOperation bao, Expr e2 | e = bao | + bao.hasOperands(fc, e2) and + isLiteralABooleanMacro(e2) and + msg = + "as an operand in an equality operation where the other operand is a boolean value (high precision result)" + ) + or + exists(EqualityOperation bao, Expr e2 | e = bao | + bao.hasOperands(fc, e2) and + (e2.(Literal).getValue().toInt() = 1 or e2.(Literal).getValue().toInt() = 0) and + not isLiteralABooleanMacro(e2) and + msg = + "as an operand in an equality operation where the other operand is likely a boolean value (lower precision result, needs to be reviewed)" + ) + ) +select e, + "This $@ is being handled $@ instead of the number of matching bytes. Please review the usage of this function and consider replacing it with `RtlEqualMemory`.", + fc, "call to `RtlCompareMemory`", e, msg diff --git a/cpp/ql/src/Microsoft/Likely Bugs/Drivers/IncorrectUsageOfRtlCompareMemoryBad.c b/cpp/ql/src/Microsoft/Likely Bugs/Drivers/IncorrectUsageOfRtlCompareMemoryBad.c new file mode 100644 index 000000000000..34dd300663ca --- /dev/null +++ b/cpp/ql/src/Microsoft/Likely Bugs/Drivers/IncorrectUsageOfRtlCompareMemoryBad.c @@ -0,0 +1,5 @@ +//bug, the code assumes RtlCompareMemory is comparing for identical values & return false if not identical +if (!RtlCompareMemory(pBuffer, ptr, 16)) +{ + return FALSE; +} \ No newline at end of file diff --git a/cpp/ql/src/Microsoft/Likely Bugs/Drivers/IncorrectUsageOfRtlCompareMemoryGood.c b/cpp/ql/src/Microsoft/Likely Bugs/Drivers/IncorrectUsageOfRtlCompareMemoryGood.c new file mode 100644 index 000000000000..a8a5945a9e3c --- /dev/null +++ b/cpp/ql/src/Microsoft/Likely Bugs/Drivers/IncorrectUsageOfRtlCompareMemoryGood.c @@ -0,0 +1,5 @@ +//fixed +if (!RtlEqualMemory(pBuffer, ptr, 16)) +{ + return FALSE; +} \ No newline at end of file diff --git a/cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/ArgumentIsSizeofOrOperation.qhelp b/cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/ArgumentIsSizeofOrOperation.qhelp new file mode 100644 index 000000000000..5cca10d929ed --- /dev/null +++ b/cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/ArgumentIsSizeofOrOperation.qhelp @@ -0,0 +1,22 @@ + + + +

    If the argument for a sizeof call is a binary operation or a sizeof call, it is typically a sign that there is a confusion on the usage of the sizeof usage.

    +
    + + +

    Any findings from this rule may indicate that the sizeof is being used incorrectly.

    +

    Review the logic of the call.

    +
    + + +

    The following example shows a case where sizeof a binary operation by mistake.

    + + +

    In this example, the fix is to multiply the result of sizeof by the number of elements.

    + +
    + +
    \ No newline at end of file diff --git a/cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/ArgumentIsSizeofOrOperation.ql b/cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/ArgumentIsSizeofOrOperation.ql new file mode 100644 index 000000000000..51a40d0f0bf0 --- /dev/null +++ b/cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/ArgumentIsSizeofOrOperation.ql @@ -0,0 +1,59 @@ +/** + * @id cpp/sizeof/sizeof-or-operation-as-argument + * @name Usage of an expression that is a binary operation, or sizeof call passed as an argument to a sizeof call + * @description When the `expr` passed to `sizeof` is a binary operation, or a sizeof call, this is typically a sign that there is a confusion on the usage of sizeof. + * @tags security + */ + +import cpp +import SizeOfTypeUtils + +/** + * Windows SDK corecrt_math.h defines a macro _CLASS_ARG that + * intentionally misuses sizeof to determine the size of a floating point type. + * Explicitly ignoring any hit in this macro. + */ +predicate isPartOfCrtFloatingPointMacroExpansion(Expr e) { + exists(MacroInvocation mi | + mi.getMacroName() = "_CLASS_ARG" and + mi.getMacro().getFile().getBaseName() = "corecrt_math.h" and + mi.getAnExpandedElement() = e + ) +} + +/** + * Determines if the sizeOfExpr is ignorable. + */ +predicate ignorableSizeof(SizeofExprOperator sizeofExpr) { + // a common pattern found is to sizeof a binary operation to check a type + // to then perfomr an onperaiton for a 32 or 64 bit type. + // these cases often look like sizeof(x) >=4 + // more generally we see binary operations frequently used in different type + // checks, where the sizeof is part of some comparison operation of a switch statement guard. + // sizeof as an argument is also similarly used, but seemingly less frequently. + exists(ComparisonOperation comp | comp.getAnOperand() = sizeofExpr) + or + exists(ConditionalStmt s | s.getControllingExpr() = sizeofExpr) + or + // another common practice is to use bit-wise operations in sizeof to allow the compiler to + // 'pack' the size appropriate but get the size of the result out of a sizeof operation. + sizeofExpr.getExprOperand() instanceof BinaryBitwiseOperation +} + +from SizeofExprOperator sizeofExpr, string message, Expr op +where + exists(string tmpMsg | + ( + op instanceof BinaryOperation and tmpMsg = "binary operator" + or + op instanceof SizeofOperator and tmpMsg = "sizeof" + ) and + if sizeofExpr.isInMacroExpansion() + then message = tmpMsg + "(in a macro expansion)" + else message = tmpMsg + ) and + op = sizeofExpr.getExprOperand() and + not isPartOfCrtFloatingPointMacroExpansion(op) and + not ignorableSizeof(sizeofExpr) +select sizeofExpr, "$@: $@ of $@ inside sizeof.", sizeofExpr, message, + sizeofExpr.getEnclosingFunction(), "Usage", op, message diff --git a/cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/ArgumentIsSizeofOrOperationBad.c b/cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/ArgumentIsSizeofOrOperationBad.c new file mode 100644 index 000000000000..52b296b94016 --- /dev/null +++ b/cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/ArgumentIsSizeofOrOperationBad.c @@ -0,0 +1,5 @@ +#define SIZEOF_CHAR sizeof(char) + +char* buffer; +// bug - the code is really going to allocate sizeof(size_t) instead o fthe intended sizeof(char) * 10 +buffer = (char*) malloc(sizeof(SIZEOF_CHAR * 10)); diff --git a/cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/ArgumentIsSizeofOrOperationGood.c b/cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/ArgumentIsSizeofOrOperationGood.c new file mode 100644 index 000000000000..c61e019b41ef --- /dev/null +++ b/cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/ArgumentIsSizeofOrOperationGood.c @@ -0,0 +1,4 @@ +#define SIZEOF_CHAR sizeof(char) + +char* buffer; +buffer = (char*) malloc(SIZEOF_CHAR * 10); diff --git a/cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/SizeOfConstIntMacro.qhelp b/cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/SizeOfConstIntMacro.qhelp new file mode 100644 index 000000000000..ed473d234e4b --- /dev/null +++ b/cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/SizeOfConstIntMacro.qhelp @@ -0,0 +1,26 @@ + + + +

    If the argument for a sizeof call is a macro that expands to a constant integer type, it is a likely indication that the macro operation may be misused or that the argument was selected by mistake (i.e. typo).

    +

    This query detects if the argument for sizeof is a macro that expands to a constant integer value.

    +

    NOTE: This rule will ignore multicharacter literal values that are exactly 4 bytes long as it matches the length of int and may be expected.

    +
    + + +

    Any findings from this rule may indicate that the sizeof is being used incorrectly.

    +

    Review the logic of the call.

    +
    + + +

    The following example shows a case where sizeof a constant was used instead of the sizeof of a structure by mistake as the names are similar.

    + + +

    In this example, the fix is to replace the argument for sizeof with the structure name.

    + + + +
    + +
    \ No newline at end of file diff --git a/cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/SizeOfConstIntMacro.ql b/cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/SizeOfConstIntMacro.ql new file mode 100644 index 000000000000..10b6b80a3a86 --- /dev/null +++ b/cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/SizeOfConstIntMacro.ql @@ -0,0 +1,54 @@ +/** + * @id cpp/sizeof/const-int-argument + * @name Passing a constant integer macro to sizeof + * @description The expression passed to sizeof is a macro that expands to an integer constant. A data type was likely intended instead. + * @kind problem + * @problem.severity error + * @precision high + * @tags security + */ + +import cpp +import SizeOfTypeUtils + +predicate isExprAConstInteger(Expr e, MacroInvocation mi) { + exists(Type type | + type = e.getExplicitlyConverted().getType() and + isTypeDangerousForSizeof(type) and + // Special case for wide-char literals when the compiler doesn't recognize wchar_t (i.e. L'\\', L'\0') + // Accounting for parenthesis "()" around the value + not exists(Macro m | m = mi.getMacro() | + m.getBody().toString().regexpMatch("^[\\s(]*L'.+'[\\s)]*$") + ) and + // Special case for token pasting operator + not exists(Macro m | m = mi.getMacro() | m.getBody().toString().regexpMatch("^.*\\s*##\\s*.*$")) and + // Special case for multichar literal integers that are exactly 4 character long (i.e. 'val1') + not exists(Macro m | m = mi.getMacro() | + e.getType().toString() = "int" and + m.getBody().toString().regexpMatch("^'.{4}'$") + ) and + e.isConstant() + ) +} + +int countMacros(Expr e) { result = count(MacroInvocation mi | mi.getExpr() = e | mi) } + +predicate isSizeOfExprOperandMacroInvocationAConstInteger( + SizeofExprOperator sizeofExpr, MacroInvocation mi +) { + exists(Expr e | + e = mi.getExpr() and + e = sizeofExpr.getExprOperand() and + isExprAConstInteger(e, mi) and + // Special case for FPs that involve an inner macro that resolves to 0 such as _T('\0') + not exists(int macroCount | macroCount = countMacros(e) | + macroCount > 1 and e.(Literal).getValue().toInt() = 0 + ) + ) +} + +from SizeofExprOperator sizeofExpr, MacroInvocation mi +where isSizeOfExprOperandMacroInvocationAConstInteger(sizeofExpr, mi) +select sizeofExpr, + "$@: sizeof of integer macro $@ will always return the size of the underlying integer type.", + sizeofExpr, sizeofExpr.getEnclosingFunction().getName(), mi.getMacro(), mi.getMacro().getName() diff --git a/cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/SizeOfConstIntMacroBad.c b/cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/SizeOfConstIntMacroBad.c new file mode 100644 index 000000000000..63d73f4d349c --- /dev/null +++ b/cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/SizeOfConstIntMacroBad.c @@ -0,0 +1,12 @@ +#define SOMESTRUCT_ERRNO_THAT_MATTERS 0x8000000d + +typedef struct { + int a; + bool b; +} SOMESTRUCT_THAT_MATTERS; + +//bug, the code is using SOMESTRUCT_ERRNO_THAT_MATTERS by mistake instead of SOMESTRUCT_THAT_MATTERS +if (somedata.length >= sizeof(SOMESTRUCT_ERRNO_THAT_MATTERS)) +{ + /// Do something +} \ No newline at end of file diff --git a/cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/SizeOfConstIntMacroGood.c b/cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/SizeOfConstIntMacroGood.c new file mode 100644 index 000000000000..bfadb4d59892 --- /dev/null +++ b/cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/SizeOfConstIntMacroGood.c @@ -0,0 +1,11 @@ +#define SOMESTRUCT_ERRNO_THAT_MATTERS 0x8000000d + +typedef struct { + int a; + bool b; +} SOMESTRUCT_THAT_MATTERS; + +if (somedata.length >= sizeof(SOMESTRUCT_THAT_MATTERS)) +{ + /// Do something +} \ No newline at end of file diff --git a/cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/SizeOfTypeUtils.qll b/cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/SizeOfTypeUtils.qll new file mode 100644 index 000000000000..87e5b1fa0f4b --- /dev/null +++ b/cpp/ql/src/Microsoft/Likely Bugs/SizeOfMisuse/SizeOfTypeUtils.qll @@ -0,0 +1,45 @@ +import cpp + +/** + * Holds if `type` is a `Type` that typically should not be used for `sizeof` in macros or function return values. + */ +predicate isTypeDangerousForSizeof(Type type) { + ( + type instanceof IntegralOrEnumType and + // ignore string literals + not type instanceof WideCharType and + not type instanceof CharType + ) +} + +/** + * Holds if `type` is a `Type` that typically should not be used for `sizeof` in macros or function return values. + * This predicate extends the types detected in exchange of precision. + * For higher precision, please use `isTypeDangerousForSizeof` + */ +predicate isTypeDangerousForSizeofLowPrecision(Type type) { + ( + // UINT8/BYTE are typedefs to char, so we treat them separately. + // WCHAR is sometimes a typedef to UINT16, so we treat it separately too. + type.getName() = "UINT8" + or + type.getName() = "BYTE" + or + not type.getName() = "WCHAR" and + exists(Type ut | + ut = type.getUnderlyingType() and + ut instanceof IntegralOrEnumType and + not ut instanceof WideCharType and + not ut instanceof CharType + ) + ) +} + +/** + * Holds if the `Function` return type is dangerous as input for `sizeof`. + */ +class FunctionWithTypeDangerousForSizeofLowPrecision extends Function { + FunctionWithTypeDangerousForSizeofLowPrecision() { + exists(Type type | type = this.getType() | isTypeDangerousForSizeofLowPrecision(type)) + } +} diff --git a/cpp/ql/src/Microsoft/Security/Cryptography/BannedEncryption.qhelp b/cpp/ql/src/Microsoft/Security/Cryptography/BannedEncryption.qhelp new file mode 100644 index 000000000000..57ea002bd6a7 --- /dev/null +++ b/cpp/ql/src/Microsoft/Security/Cryptography/BannedEncryption.qhelp @@ -0,0 +1,46 @@ + + + + +

    + Finds explicit uses of symmetric encryption algorithms that are weak, obsolete, or otherwise unapproved. +

    +

    + Encryption algorithms such as DES, (uses keys of 56 bits only), RC2 (uses keys of 128 bits only), and TripleDES (provides at most 112 bits of security) are considered to be weak. +

    +

    + These cryptographic algorithms do not provide as much security assurance as more modern counterparts. +

    +
    + + +

    + For Microsoft internal security standards: +

    +

    + For WinCrypt, switch to ALG_SID_AES, ALG_SID_AES_128, ALG_SID_AES_192, or ALG_SID_AES_256. +

    +

    + For BCrypt, switch to AES or any algorithm other than RC2, RC4, DES, DESX, 3DES, 3DES_112. AES_GMAC and AES_CMAC require crypto board review. +

    +
    + + +

    Violations:

    + + + + +

    Solutions:

    + + + +
    + + +
  • Microsoft Docs: Microsoft SDL Cryptographic Recommendations.
  • +
    + +
    \ No newline at end of file diff --git a/cpp/ql/src/Microsoft/Security/Cryptography/BannedEncryption.ql b/cpp/ql/src/Microsoft/Security/Cryptography/BannedEncryption.ql new file mode 100644 index 000000000000..8e8579d3c67e --- /dev/null +++ b/cpp/ql/src/Microsoft/Security/Cryptography/BannedEncryption.ql @@ -0,0 +1,58 @@ +/** + * @name Weak cryptography + * @description Finds explicit uses of symmetric encryption algorithms that are weak, obsolete, or otherwise unapproved. + * @kind problem + * @id cpp/weak-crypto/banned-encryption-algorithms + * @problem.severity error + * @precision high + * @tags security + * external/cwe/cwe-327 + */ + +import cpp +import CryptoFilters +import CryptoDataflowCapi +import CryptoDataflowCng +import experimental.cryptography.Concepts + +predicate isCapiOrCNGBannedAlg(Expr e, string msg) { + exists(FunctionCall fc | + CapiCryptCreateEncryptionBanned::flow(DataFlow::exprNode(e), + DataFlow::exprNode(fc.getArgument(1))) + or + BCryptOpenAlgorithmProviderBannedEncryption::flow(DataFlow::exprNode(e), + DataFlow::exprNode(fc.getArgument(1))) + ) and + msg = + "Call to a cryptographic function with a banned symmetric encryption algorithm: " + + e.getValueText() +} + +predicate isGeneralBannedAlg(SymmetricEncryptionAlgorithm alg, Expr confSink, string msg) { + // Handle unknown cases in a separate query + not alg.getEncryptionName() = unknownAlgorithm() and + exists(string resMsg | + ( + not alg.getEncryptionName().matches("AES%") and + resMsg = "Use of banned symmetric encryption algorithm: " + alg.getEncryptionName() + "." + ) and + ( + if alg.hasConfigurationSink() and alg.configurationSink() != alg + then ( + confSink = alg.configurationSink() and msg = resMsg + " Algorithm used at sink: $@." + ) else ( + confSink = alg and msg = resMsg + ) + ) + ) +} + +from Expr sink, Expr confSink, string msg +where + ( + isCapiOrCNGBannedAlg(sink, msg) and confSink = sink + or + isGeneralBannedAlg(sink, confSink, msg) + ) and + not isSrcSinkFiltered(sink, confSink) +select sink, msg, confSink, confSink.toString() diff --git a/cpp/ql/src/Microsoft/Security/Cryptography/BannedModesCAPI.qhelp b/cpp/ql/src/Microsoft/Security/Cryptography/BannedModesCAPI.qhelp new file mode 100644 index 000000000000..e6e00a06cdb9 --- /dev/null +++ b/cpp/ql/src/Microsoft/Security/Cryptography/BannedModesCAPI.qhelp @@ -0,0 +1,29 @@ + + + + +

    Violation - Use of one of the following unsafe encryption modes that is not approved: ECB, OFB, CFB, CTR, CCM, or GCM.

    +

    These modes are vulnerable to attacks and may cause exposure of sensitive information. For example, using ECB to encrypt a plaintext block always produces a same cipher text, so it can easily tell if two encrypted messages are identical. Using approved modes can avoid these unnecessary risks.

    +
    + + +

    - Use only approved modes CBC, CTS and XTS.

    +
    + + +

    Violation:

    + + + +

    Solution:

    + + +
    + + +
  • Microsoft Docs: Microsoft SDL Cryptographic Recommendations.
  • +
    + +
    \ No newline at end of file diff --git a/cpp/ql/src/Microsoft/Security/Cryptography/BannedModesCAPI.ql b/cpp/ql/src/Microsoft/Security/Cryptography/BannedModesCAPI.ql new file mode 100644 index 000000000000..ccc999be56f0 --- /dev/null +++ b/cpp/ql/src/Microsoft/Security/Cryptography/BannedModesCAPI.ql @@ -0,0 +1,40 @@ +/** + * @name Weak cryptography + * @description Finds explicit uses of block cipher chaining mode algorithms that are not approved. (CAPI) + * @kind problem + * @id cpp/weak-crypto/capi/banned-modes + * @problem.severity error + * @precision high + * @tags security + * external/cwe/cwe-327 + */ + +import cpp +import semmle.code.cpp.dataflow.new.DataFlow +import CryptoDataflowCapi + +module CapiSetBlockCipherConfiguration implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node source) { + source.asExpr().isConstant() and + // KP_MODE + // CRYPT_MODE_CBC 1 - Cipher block chaining - Microsoft-Only: Only mode allowed by Crypto Board from this list (CBC-MAC) + // CRYPT_MODE_ECB 2 - Electronic code book - Generally not recommended for usage in cryptographic protocols at all + // CRYPT_MODE_OFB 3 - Output feedback mode - Microsoft-Only: Banned, usage requires Crypto Board review + // CRYPT_MODE_CFB 4 - Cipher feedback mode - Microsoft-Only: Banned, usage requires Crypto Board review + // CRYPT_MODE_CTS 5 - Ciphertext stealing mode - Microsoft-Only: CTS is approved by Crypto Board, but should probably use CNG and not CAPI + source.asExpr().getValue().toInt() != 1 + } + + predicate isSink(DataFlow::Node sink) { + exists(CapiCryptCryptSetKeyParamtoKPMODE call | sink.asIndirectExpr() = call.getArgument(2)) + } +} + +module CapiSetBlockCipherTrace = DataFlow::Global; + +from CapiCryptCryptSetKeyParamtoKPMODE call, DataFlow::Node src, DataFlow::Node sink +where + sink.asIndirectExpr() = call.getArgument(2) and + CapiSetBlockCipherTrace::flow(src, sink) +select call, + "Call to 'CryptSetKeyParam' function with argument dwParam = KP_MODE is setting up a banned block cipher mode." diff --git a/cpp/ql/src/Microsoft/Security/Cryptography/BannedModesCNG.qhelp b/cpp/ql/src/Microsoft/Security/Cryptography/BannedModesCNG.qhelp new file mode 100644 index 000000000000..4713ef9ff13c --- /dev/null +++ b/cpp/ql/src/Microsoft/Security/Cryptography/BannedModesCNG.qhelp @@ -0,0 +1,31 @@ + + + + +

    Violation - Use of one of the following unsafe encryption modes that is not approved: ECB, OFB, CFB, CTR, CCM, or GCM.

    +

    These modes are vulnerable to attacks and may cause exposure of sensitive information. For example, using ECB to encrypt a plaintext block always produces a same cipher text, so it can easily tell if two encrypted messages are identical. Using approved modes can avoid these unnecessary risks.

    +
    + + +

    - Use only approved modes CBC, CTS and XTS.

    +
    + + +

    Violation:

    + + + +

    Solution:

    + + +
    + + + +
  • Microsoft Docs: Microsoft SDL Cryptographic Recommendations.
  • +
    + + +
    \ No newline at end of file diff --git a/cpp/ql/src/Microsoft/Security/Cryptography/BannedModesCNG.ql b/cpp/ql/src/Microsoft/Security/Cryptography/BannedModesCNG.ql new file mode 100644 index 000000000000..430eb1c2784b --- /dev/null +++ b/cpp/ql/src/Microsoft/Security/Cryptography/BannedModesCNG.ql @@ -0,0 +1,23 @@ +/** + * @name Weak cryptography + * @description Finds explicit uses of block cipher chaining mode algorithms that are not approved. (CNG) + * @kind problem + * @id cpp/weak-crypto/cng/banned-modes + * @problem.severity error + * @precision high + * @tags security + * external/cwe/cwe-327 + */ + +import cpp +import semmle.code.cpp.dataflow.new.DataFlow +import CryptoDataflowCng + +from CngBCryptSetPropertyParamtoKChainingMode call, DataFlow::Node src, DataFlow::Node sink +where + sink.asIndirectArgument() = call.getArgument(2) and + CngBCryptSetPropertyChainingBannedModeIndirectParameter::flow(src, sink) + or + sink.asExpr() = call.getArgument(2) and CngBCryptSetPropertyChainingBannedMode::flow(src, sink) +select call, + "Call to 'BCryptSetProperty' function with argument pszProperty = \"ChainingMode\" is setting up a banned block cipher mode." diff --git a/cpp/ql/src/Microsoft/Security/Cryptography/CryptoDataflowCapi.qll b/cpp/ql/src/Microsoft/Security/Cryptography/CryptoDataflowCapi.qll new file mode 100644 index 000000000000..52c835ea9b89 --- /dev/null +++ b/cpp/ql/src/Microsoft/Security/Cryptography/CryptoDataflowCapi.qll @@ -0,0 +1,97 @@ +/** + * Provides classes and predicates for identifying expressions that are use Crypto API (CAPI). + */ + +import cpp +private import semmle.code.cpp.dataflow.new.DataFlow + +/** + * Dataflow that detects a call to CryptSetKeyParam dwParam = KP_MODE (CAPI) + */ +module CapiCryptCryptSetKeyParamtoKPMODEConfiguration implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node source) { + source.asExpr().getValue().toInt() = 4 // KP_MODE + } + + predicate isSink(DataFlow::Node sink) { + exists(FunctionCall call | + // CryptSetKeyParam 2nd argument specifies the key parameter to set + sink.asExpr() = call.getArgument(1) and + call.getTarget().hasGlobalName("CryptSetKeyParam") + ) + } +} + +module CapiCryptCryptSetKeyParamtoKPMODE = + DataFlow::Global; + +/** + * A function call to CryptSetKeyParam with dwParam = KP_MODE (CAPI) + */ +class CapiCryptCryptSetKeyParamtoKPMODE extends FunctionCall { + CapiCryptCryptSetKeyParamtoKPMODE() { + exists(Expr var | + CapiCryptCryptSetKeyParamtoKPMODE::flow(DataFlow::exprNode(var), + DataFlow::exprNode(this.getArgument(1))) + ) + } +} + +// CAPI-specific DataFlow configuration +module CapiCryptCreateHashBannedConfiguration implements DataFlow::ConfigSig { + // This mechnism will verify for approved set of values to call, rejecting anythign that is not in the list. + // NOTE: This mechanism is not guaranteed to work with CSPs that do not use the same algorithms defined in Wincrypt.h + // + predicate isSource(DataFlow::Node source) { + // Verify if source matched the mask for CAPI ALG_CLASS_HASH == 32768 + source.asExpr().getValue().toInt().bitShiftRight(13) = 4 and + // The following hash algorithms are safe to use, anything else is considered banned + not ( + source.asExpr().getValue().toInt().bitXor(32768) = 12 or // ALG_SID_SHA_256 + source.asExpr().getValue().toInt().bitXor(32768) = 13 or // ALG_SID_SHA_384 + source.asExpr().getValue().toInt().bitXor(32768) = 14 // ALG_SID_SHA_512 + ) + } + + predicate isSink(DataFlow::Node sink) { + exists(FunctionCall call | + // CryptCreateHash 2nd argument specifies the hash algorithm to be used. + sink.asExpr() = call.getArgument(1) and + call.getTarget().hasGlobalName("CryptCreateHash") + ) + } +} + +module CapiCryptCreateHashBanned = DataFlow::Global; + +// CAPI-specific DataFlow configuration +module CapiCryptCreateEncryptionBannedConfiguration implements DataFlow::ConfigSig { + // This mechanism will verify for approved set of values to call, rejecting anything that is not in the list. + // NOTE: This mechanism is not guaranteed to work with CSPs that do not use the same algorithms defined in Wincrypt.h + // + predicate isSource(DataFlow::Node source) { + // Verify if source matched the mask for CAPI ALG_CLASS_DATA_ENCRYPT == 24576 + source.asExpr().getValue().toInt().bitShiftRight(13) = 3 and + // The following algorithms are safe to use, anything else is considered banned + not ( + source.asExpr().getValue().toInt().bitXor(26112) = 14 or // ALG_SID_AES_128 + source.asExpr().getValue().toInt().bitXor(26112) = 15 or // ALG_SID_AES_192 + source.asExpr().getValue().toInt().bitXor(26112) = 16 or // ALG_SID_AES_256 + source.asExpr().getValue().toInt().bitXor(26112) = 17 // ALG_SID_AES + ) + } + + predicate isSink(DataFlow::Node sink) { + exists(FunctionCall call | + // CryptGenKey or CryptDeriveKey 2nd argument specifies the hash algorithm to be used. + sink.asExpr() = call.getArgument(1) and + ( + call.getTarget().hasGlobalName("CryptGenKey") or + call.getTarget().hasGlobalName("CryptDeriveKey") + ) + ) + } +} + +module CapiCryptCreateEncryptionBanned = + DataFlow::Global; diff --git a/cpp/ql/src/Microsoft/Security/Cryptography/CryptoDataflowCng.qll b/cpp/ql/src/Microsoft/Security/Cryptography/CryptoDataflowCng.qll new file mode 100644 index 000000000000..a54650692075 --- /dev/null +++ b/cpp/ql/src/Microsoft/Security/Cryptography/CryptoDataflowCng.qll @@ -0,0 +1,137 @@ +/** + * Provides classes and predicates for identifying expressions that are use crypto API Next Generation (CNG). + */ + +import cpp +private import semmle.code.cpp.dataflow.new.DataFlow + +/** + * Dataflow that detects a call to BCryptSetProperty pszProperty = ChainingMode (CNG) + */ +module CngBCryptSetPropertyParamtoKChainingModeConfiguration implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node source) { + source.asExpr().getValue().toString().matches("ChainingMode") + } + + predicate isSink(DataFlow::Node sink) { + exists(FunctionCall call | + // BCryptSetProperty 2nd argument specifies the key parameter to set + sink.asExpr() = call.getArgument(1) and + call.getTarget().hasGlobalName("BCryptSetProperty") + ) + } +} + +module CngBCryptSetPropertyParamtoKChainingMode = + DataFlow::Global; + +/** + * A function call to BCryptSetProperty pszProperty = ChainingMode (CNG) + */ +class CngBCryptSetPropertyParamtoKChainingMode extends FunctionCall { + CngBCryptSetPropertyParamtoKChainingMode() { + exists(Expr var | + CngBCryptSetPropertyParamtoKChainingMode::flow(DataFlow::exprNode(var), + DataFlow::exprNode(this.getArgument(1))) + ) + } +} + +predicate isChaniningModeCbc(DataFlow::Node source) { + // Verify if algorithm is in the approved list. + exists(string s | s = source.asExpr().getValue().toString() | + s.regexpMatch("ChainingMode[A-Za-z0-9/]+") and + // Property Strings + // BCRYPT_CHAIN_MODE_NA L"ChainingModeN/A" - The algorithm does not support chaining + // BCRYPT_CHAIN_MODE_CBC L"ChainingModeCBC" - Microsoft-Only: Only mode allowed by Crypto Board from this list (CBC-MAC) + // BCRYPT_CHAIN_MODE_ECB L"ChainingModeECB" - Generally not recommended for usage in cryptographic protocols at all + // BCRYPT_CHAIN_MODE_CFB L"ChainingModeCFB" - Microsoft-Only: Banned, usage requires Crypto Board review + // BCRYPT_CHAIN_MODE_CCM L"ChainingModeCCM" - Microsoft-Only: Banned, usage requires Crypto Board review + // BCRYPT_CHAIN_MODE_GCM L"ChainingModeGCM" - Microsoft-Only: Only for TLS, other usage requires Crypto Board review + not s.matches("ChainingModeCBC") + ) +} + +module CngBCryptSetPropertyChainingBannedModeConfiguration implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node source) { isChaniningModeCbc(source) } + + predicate isSink(DataFlow::Node sink) { + exists(CngBCryptSetPropertyParamtoKChainingMode call | + // BCryptOpenAlgorithmProvider 3rd argument sets the chaining mode value + sink.asExpr() = call.getArgument(2) + ) + } +} + +module CngBCryptSetPropertyChainingBannedMode = + DataFlow::Global; + +module CngBCryptSetPropertyChainingBannedModeIndirectParameterConfiguration implements + DataFlow::ConfigSig +{ + predicate isSource(DataFlow::Node source) { isChaniningModeCbc(source) } + + predicate isSink(DataFlow::Node sink) { + exists(CngBCryptSetPropertyParamtoKChainingMode call | + // CryptSetKeyParam 3rd argument specifies the mode (KP_MODE) + sink.asIndirectExpr() = call.getArgument(2) + ) + } +} + +module CngBCryptSetPropertyChainingBannedModeIndirectParameter = + DataFlow::Global; + +// CNG-specific DataFlow configuration +module BCryptOpenAlgorithmProviderBannedHashConfiguration implements DataFlow::ConfigSig { + // NOTE: Unlike the CAPI scenario, CNG will use this method to load and initialize + // a cryptographic provider for any type of algorithm,not only hash. + // Therefore, we have to take a banned-list instead of approved list approach. + // + predicate isSource(DataFlow::Node source) { + // Verify if algorithm is marked as banned. + source.asExpr().getValue().toString().matches("MD_") + or + source.asExpr().getValue().toString().matches("SHA1") + } + + predicate isSink(DataFlow::Node sink) { + exists(FunctionCall call | + // BCryptOpenAlgorithmProvider 2nd argument specifies the algorithm to be used + sink.asExpr() = call.getArgument(1) and + call.getTarget().hasGlobalName("BCryptOpenAlgorithmProvider") + ) + } +} + +module BCryptOpenAlgorithmProviderBannedHash = + DataFlow::Global; + +// CNG-specific DataFlow configuration +module BCryptOpenAlgorithmProviderBannedEncryptionConfiguration implements DataFlow::ConfigSig { + // NOTE: Unlike the CAPI scenario, CNG will use this method to load and initialize + // a cryptographic provider for any type of algorithm,not only encryption. + // Therefore, we have to take a banned-list instead of approved list approach. + // + predicate isSource(DataFlow::Node source) { + // Verify if algorithm is marked as banned. + source.asExpr().getValue().toString().matches("RC_") or + source.asExpr().getValue().toString().matches("DES") or + source.asExpr().getValue().toString().matches("DESX") or + source.asExpr().getValue().toString().matches("3DES") or + source.asExpr().getValue().toString().matches("3DES_112") or + source.asExpr().getValue().toString().matches("AES_GMAC") or // Microsoft Only: Requires Cryptoboard review + source.asExpr().getValue().toString().matches("AES_CMAC") // Microsoft Only: Requires Cryptoboard review + } + + predicate isSink(DataFlow::Node sink) { + exists(FunctionCall call | + // BCryptOpenAlgorithmProvider 2nd argument specifies the algorithm to be used + sink.asExpr() = call.getArgument(1) and + call.getTarget().hasGlobalName("BCryptOpenAlgorithmProvider") + ) + } +} + +module BCryptOpenAlgorithmProviderBannedEncryption = + DataFlow::Global; diff --git a/cpp/ql/src/Microsoft/Security/Cryptography/CryptoFilters.qll b/cpp/ql/src/Microsoft/Security/Cryptography/CryptoFilters.qll new file mode 100644 index 000000000000..65f9dde5f911 --- /dev/null +++ b/cpp/ql/src/Microsoft/Security/Cryptography/CryptoFilters.qll @@ -0,0 +1,45 @@ +import cpp + +/** + * Determines if an element should be filtered (ignored) + * from any result set. + * + * The current strategy is to determine if the element + * resides in a path that appears to be a library (in particular openssl). + * + * It is therefore important that the element being examined represents + * a use or configuration of cryptography in the user code. + * E.g., if a global variable were defined in an OpenSSL library + * representing a bad/vuln algorithm, and this global were assessed + * it would appear to be ignorable, as it exists in a a filtered library. + * The use of that global must be examined with this filter. + * + * ASSUMPTION/CAVEAT: note if an openssl library wraps a dangerous crypo use + * this filter approach will ignore the wrapper call, unless it is also flagged + * as dangerous. e.g., SomeWraper(){ ... ...} + * The wrapper if defined in openssl would result in ignoring + * the use of MD5 internally, since it's use is entirely in openssl. + * + * TODO: these caveats need to be reassessed in the future. + */ +predicate isUseFiltered(Element e) { + e.getFile().getAbsolutePath().toLowerCase().matches("%openssl%") +} + +/** + * Filtered only if both src and sink are considered filtered. + * + * This approach is meant to partially address some of the implications of + * `isUseFiltered`. Specifically, if an algorithm is specified by a user + * and some how passes to a user inside openssl, then this filter + * would not ignore that the user was specifying the use of something dangerous. + * + * e.g., if a wrapper in openssl existed of the form SomeWrapper(string alg, ...){ ... ...} + * and the user did something like SomeWrapper("MD5", ...), this would not be ignored. + * + * The source in the above example would the algorithm, and the sink is the configuration sink + * of the algorithm. + */ +predicate isSrcSinkFiltered(Element src, Element sink) { + isUseFiltered(src) and isUseFiltered(sink) +} diff --git a/cpp/ql/src/Microsoft/Security/Cryptography/HardcodedIVCNG.qhelp b/cpp/ql/src/Microsoft/Security/Cryptography/HardcodedIVCNG.qhelp new file mode 100644 index 000000000000..bd0b71f227e4 --- /dev/null +++ b/cpp/ql/src/Microsoft/Security/Cryptography/HardcodedIVCNG.qhelp @@ -0,0 +1,23 @@ + + + +

    An initialization vector (IV) is an input to a cryptographic primitive being used to provide the initial state. The IV is typically required to be random or pseudorandom (randomized scheme), but sometimes an IV only needs to be unpredictable or unique (stateful scheme).

    +

    Randomization is crucial for some encryption schemes to achieve semantic security, a property whereby repeated usage of the scheme under the same key does not allow an attacker to infer relationships between (potentially similar) segments of the encrypted message.

    +
    + + +

    All symmetric block ciphers must also be used with an appropriate initialization vector (IV) according to the mode of operation being used.

    +

    If using a randomized scheme such as CBC, it is recommended to use cryptographically secure pseudorandom number generator such as BCryptGenRandom.

    +
    + + +
  • + BCryptEncrypt function (bcrypt.h) + BCryptGenRandom function (bcrypt.h) + Initialization vector (Wikipedia) +
  • +
    + +
    \ No newline at end of file diff --git a/cpp/ql/src/Microsoft/Security/Cryptography/HardcodedIVCNG.ql b/cpp/ql/src/Microsoft/Security/Cryptography/HardcodedIVCNG.ql new file mode 100644 index 000000000000..26039e07d923 --- /dev/null +++ b/cpp/ql/src/Microsoft/Security/Cryptography/HardcodedIVCNG.ql @@ -0,0 +1,58 @@ +/** + * @name Weak cryptography + * @description Finds usage of a static (hardcoded) IV. (CNG) + * @kind problem + * @id cpp/weak-crypto/cng/hardcoded-iv + * @problem.severity error + * @precision high + * @tags security + * external/cwe/cwe-327 + */ + +import cpp +import semmle.code.cpp.dataflow.new.DataFlow + +/** + * Gets const element of `ArrayAggregateLiteral`. + */ +Expr getConstElement(ArrayAggregateLiteral lit) { + exists(int n | + result = lit.getElementExpr(n, _) and + result.isConstant() + ) +} + +/** + * Gets the last element in an `ArrayAggregateLiteral`. + */ +Expr getLastElement(ArrayAggregateLiteral lit) { + exists(int n | + result = lit.getElementExpr(n, _) and + not exists(lit.getElementExpr(n + 1, _)) + ) +} + +module CngBCryptEncryptHardcodedIVConfiguration implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node source) { + exists(AggregateLiteral lit | + getLastElement(lit) = source.asDefinition() and + exists(getConstElement(lit)) + ) + } + + predicate isSink(DataFlow::Node sink) { + exists(FunctionCall call | + // BCryptEncrypt 5h argument specifies the IV + sink.asIndirectExpr() = call.getArgument(4) and + call.getTarget().hasGlobalName("BCryptEncrypt") + ) + } +} + +module Flow = DataFlow::Global; + +from DataFlow::Node sl, DataFlow::Node fc, AggregateLiteral lit +where + Flow::flow(sl, fc) and + getLastElement(lit) = sl.asDefinition() +select lit, "Calling BCryptEncrypt with a hard-coded IV on function " diff --git a/cpp/ql/src/Microsoft/Security/Cryptography/WeakKDFBannedHashAlgorithm.qhelp b/cpp/ql/src/Microsoft/Security/Cryptography/WeakKDFBannedHashAlgorithm.qhelp new file mode 100644 index 000000000000..b61202a4dbc3 --- /dev/null +++ b/cpp/ql/src/Microsoft/Security/Cryptography/WeakKDFBannedHashAlgorithm.qhelp @@ -0,0 +1,14 @@ + + + + +

    Use of KDF algorithm BCryptDeriveKeyPBKDF2 uses insecure hash from BCryptOpenAlgorithmProvider.

    +
    + + +

    Use SHA 256, 384, or 512.

    +
    + +
    \ No newline at end of file diff --git a/cpp/ql/src/Microsoft/Security/Cryptography/WeakKDFBannedHashAlgorithm.ql b/cpp/ql/src/Microsoft/Security/Cryptography/WeakKDFBannedHashAlgorithm.ql new file mode 100644 index 000000000000..5c9c34caf185 --- /dev/null +++ b/cpp/ql/src/Microsoft/Security/Cryptography/WeakKDFBannedHashAlgorithm.ql @@ -0,0 +1,85 @@ +/** + * @name KDF may only use SHA256/384/512 in generating a key. + * @description KDF may only use SHA256/384/512 in generating a key. + * @kind problem + * @id cpp/kdf-insecure-hash + * @problem.severity error + * @precision high + * @tags security + */ + +import cpp +import semmle.code.cpp.dataflow.new.DataFlow + +module BannedHashAlgorithmConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node source) { + // Verify if algorithm is marked as banned. + not source.asExpr().getValue().toString().matches("SHA256") and + not source.asExpr().getValue().toString().matches("SHA384") and + not source.asExpr().getValue().toString().matches("SHA512") + } + + predicate isSink(DataFlow::Node sink) { + exists(FunctionCall call | + // Argument 1 (0-based) specified the algorithm ID. + // NTSTATUS BCryptOpenAlgorithmProvider( + // [out] BCRYPT_ALG_HANDLE *phAlgorithm, + // [in] LPCWSTR pszAlgId, + // [in] LPCWSTR pszImplementation, + // [in] ULONG dwFlags + // ); + sink.asExpr() = call.getArgument(1) and + call.getTarget().hasGlobalName("BCryptOpenAlgorithmProvider") + ) + } +} + +module BannedHashAlgorithmTrace = DataFlow::Global; + +module BCRYPT_ALG_HANDLE_Config implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node source) { + exists(FunctionCall call | + // Argument 0 (0-based) specified the algorithm handle + // NTSTATUS BCryptOpenAlgorithmProvider( + // [out] BCRYPT_ALG_HANDLE *phAlgorithm, + // [in] LPCWSTR pszAlgId, + // [in] LPCWSTR pszImplementation, + // [in] ULONG dwFlags + // ); + source.asDefiningArgument() = call.getArgument(0) and + call.getTarget().hasGlobalName("BCryptOpenAlgorithmProvider") + ) + } + + predicate isSink(DataFlow::Node sink) { + // Algorithm handle is the 0th (0-based) argument of the call + // NTSTATUS BCryptDeriveKeyPBKDF2( + // [in] BCRYPT_ALG_HANDLE hPrf, + // [in, optional] PUCHAR pbPassword, + // [in] ULONG cbPassword, + // [in, optional] PUCHAR pbSalt, + // [in] ULONG cbSalt, + // [in] ULONGLONG cIterations, + // [out] PUCHAR pbDerivedKey, + // [in] ULONG cbDerivedKey, + // [in] ULONG dwFlags + // ); + exists(Call c | c.getTarget().getName() = "BCryptDeriveKeyPBKDF2" | + c.getArgument(0) = sink.asExpr() + ) + } +} + +module BCRYPT_ALG_HANDLE_Trace = DataFlow::Global; + +from DataFlow::Node src1, DataFlow::Node src2, DataFlow::Node sink1, DataFlow::Node sink2 +where + BannedHashAlgorithmTrace::flow(src1, sink1) and + exists(Call c | + c.getAnArgument() = sink1.asExpr() and src2.asDefiningArgument() = c.getAnArgument() + | + BCRYPT_ALG_HANDLE_Trace::flow(src2, sink2) + ) +select sink2.asExpr(), + "BCRYPT_ALG_HANDLE is passed to this to KDF derived from insecure hashing function $@. Must use SHA256 or higher.", + src1.asExpr(), src1.asExpr().getValue() diff --git a/cpp/ql/src/Microsoft/Security/Cryptography/WeakKDFLowIterationCount.qhelp b/cpp/ql/src/Microsoft/Security/Cryptography/WeakKDFLowIterationCount.qhelp new file mode 100644 index 000000000000..84722ccec5cd --- /dev/null +++ b/cpp/ql/src/Microsoft/Security/Cryptography/WeakKDFLowIterationCount.qhelp @@ -0,0 +1,14 @@ + + + + +

    Use of KDF algorithm BCryptDeriveKeyPBKDF2 uses low iteration count (less than 100k).

    +
    + + +

    Use a minimum of 100,000 for iteration count.

    +
    + +
    \ No newline at end of file diff --git a/cpp/ql/src/Microsoft/Security/Cryptography/WeakKDFLowIterationCount.ql b/cpp/ql/src/Microsoft/Security/Cryptography/WeakKDFLowIterationCount.ql new file mode 100644 index 000000000000..e3aeba84f596 --- /dev/null +++ b/cpp/ql/src/Microsoft/Security/Cryptography/WeakKDFLowIterationCount.ql @@ -0,0 +1,51 @@ +/** + * @name Use iteration count at least 100k to prevent brute force attacks + * @description When deriving cryptographic keys from user-provided inputs such as password, use sufficient iteration count (at least 100k). + * This query traces constants of <100k to the iteration count parameter of CNG's BCryptDeriveKeyPBKDF2. + * This query traces constants of less than the min length to the target parameter. + * NOTE: if the constant is modified, or if a non-constant gets to the iteration count, this query will not flag these cases. + * The rationale currently is that this query is meant to validate common uses of key derivation. + * Non-common uses (modifying the iteration count somehow or getting the count from outside sources) are assumed to be intentional. + * @kind problem + * @id cpp/kdf-low-iteration-count + * @problem.severity error + * @precision high + * @tags security + */ + +import cpp +import semmle.code.cpp.dataflow.new.DataFlow + +module IterationCountDataFlowConfig implements DataFlow::ConfigSig { + /** + * Defines the source for iteration count when it's coming from a fixed value + * Any expression that has an assigned value < 100000 could be a source. + */ + predicate isSource(DataFlow::Node src) { src.asExpr().getValue().toInt() < 100000 } + + predicate isSink(DataFlow::Node sink) { + // iterations count is the 5th (0-based) argument of the call + // NTSTATUS BCryptDeriveKeyPBKDF2( + // [in] BCRYPT_ALG_HANDLE hPrf, + // [in, optional] PUCHAR pbPassword, + // [in] ULONG cbPassword, + // [in, optional] PUCHAR pbSalt, + // [in] ULONG cbSalt, + // [in] ULONGLONG cIterations, + // [out] PUCHAR pbDerivedKey, + // [in] ULONG cbDerivedKey, + // [in] ULONG dwFlags + // ); + exists(Call c | c.getTarget().getName() = "BCryptDeriveKeyPBKDF2" | + c.getArgument(5) = sink.asExpr() + ) + } +} + +module IterationCountDataFlow = DataFlow::Global; + +from DataFlow::Node src, DataFlow::Node sink +where IterationCountDataFlow::flow(src, sink) +select sink.asExpr(), + "Iteration count $@ is passed to this to KDF. Use at least 100000 iterations when deriving cryptographic key from password.", + src, src.asExpr().getValue() diff --git a/cpp/ql/src/Microsoft/Security/Cryptography/WeakKDFSmallKeyLength.qhelp b/cpp/ql/src/Microsoft/Security/Cryptography/WeakKDFSmallKeyLength.qhelp new file mode 100644 index 000000000000..6927cd16583c --- /dev/null +++ b/cpp/ql/src/Microsoft/Security/Cryptography/WeakKDFSmallKeyLength.qhelp @@ -0,0 +1,14 @@ + + + + +

    Use of KDF algorithm BCryptDeriveKeyPBKDF2 uses small key size (less than 16 bytes).

    +
    + + +

    Use a minimum of 16 bytes for key size.

    +
    + +
    \ No newline at end of file diff --git a/cpp/ql/src/Microsoft/Security/Cryptography/WeakKDFSmallKeyLength.ql b/cpp/ql/src/Microsoft/Security/Cryptography/WeakKDFSmallKeyLength.ql new file mode 100644 index 000000000000..6d7dd5e51605 --- /dev/null +++ b/cpp/ql/src/Microsoft/Security/Cryptography/WeakKDFSmallKeyLength.ql @@ -0,0 +1,46 @@ +/** + * @name Small KDF derived key length. + * @description KDF derived keys should be a minimum of 128 bits (16 bytes). + * This query traces constants of less than the min length to the target parameter. + * NOTE: if the constant is modified, or if a non-constant gets to the target, this query will not flag these cases. + * The rationale currently is that this query is meant to validate common uses of key derivation. + * Non-common uses (modifying the values somehow or getting the count from outside sources) are assumed to be intentional. + * @kind problem + * @id cpp/kdf-small-key-size + * @problem.severity error + * @precision high + * @tags security + */ + +import cpp +import semmle.code.cpp.dataflow.new.DataFlow + +module KeyLenConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node src) { src.asExpr().getValue().toInt() < 16 } + + predicate isSink(DataFlow::Node sink) { + // Key length size is the 7th (0-based) argument of the call + // NTSTATUS BCryptDeriveKeyPBKDF2( + // [in] BCRYPT_ALG_HANDLE hPrf, + // [in, optional] PUCHAR pbPassword, + // [in] ULONG cbPassword, + // [in, optional] PUCHAR pbSalt, + // [in] ULONG cbSalt, + // [in] ULONGLONG cIterations, + // [out] PUCHAR pbDerivedKey, + // [in] ULONG cbDerivedKey, + // [in] ULONG dwFlags + // ); + exists(Call c | c.getTarget().getName() = "BCryptDeriveKeyPBKDF2" | + c.getArgument(7) = sink.asExpr() + ) + } +} + +module KeyLenTrace = DataFlow::Global; + +from DataFlow::Node src, DataFlow::Node sink +where KeyLenTrace::flow(src, sink) +select sink.asExpr(), + "Key size $@ is passed to this to KDF. Use at least 16 bytes for key length when deriving cryptographic key from password.", + src.asExpr(), src.asExpr().getValue() diff --git a/cpp/ql/src/Microsoft/Security/Cryptography/WeakKDFSmallSaltSize.qhelp b/cpp/ql/src/Microsoft/Security/Cryptography/WeakKDFSmallSaltSize.qhelp new file mode 100644 index 000000000000..bf664be8d63c --- /dev/null +++ b/cpp/ql/src/Microsoft/Security/Cryptography/WeakKDFSmallSaltSize.qhelp @@ -0,0 +1,15 @@ + + + + +

    Use of KDF algorithm BCryptDeriveKeyPBKDF2 uses small salt size (less than 16 bytes).

    +
    + + +

    Use a minimum of 16 bytes for salt size.

    +
    + + +
    \ No newline at end of file diff --git a/cpp/ql/src/Microsoft/Security/Cryptography/WeakKDFSmallSaltSize.ql b/cpp/ql/src/Microsoft/Security/Cryptography/WeakKDFSmallSaltSize.ql new file mode 100644 index 000000000000..0c5cebda07c7 --- /dev/null +++ b/cpp/ql/src/Microsoft/Security/Cryptography/WeakKDFSmallSaltSize.ql @@ -0,0 +1,46 @@ +/** + * @name Small KDF salt length. + * @description KDF salts should be a minimum of 128 bits (16 bytes). + * This query traces constants of less than the min length to the target parameter. + * NOTE: if the constant is modified, or if a non-constant gets to the target, this query will not flag these cases. + * The rationale currently is that this query is meant to validate common uses of key derivation. + * Non-common uses (modifying the values somehow or getting the count from outside sources) are assumed to be intentional. + * @kind problem + * @id cpp/kdf-small-salt-size + * @problem.severity error + * @precision high + * @tags security + */ + +import cpp +import semmle.code.cpp.dataflow.new.DataFlow + +module SaltLenConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node src) { src.asExpr().getValue().toInt() < 16 } + + predicate isSink(DataFlow::Node sink) { + // Key length size is the 7th (0-based) argument of the call + // NTSTATUS BCryptDeriveKeyPBKDF2( + // [in] BCRYPT_ALG_HANDLE hPrf, + // [in, optional] PUCHAR pbPassword, + // [in] ULONG cbPassword, + // [in, optional] PUCHAR pbSalt, + // [in] ULONG cbSalt, + // [in] ULONGLONG cIterations, + // [out] PUCHAR pbDerivedKey, + // [in] ULONG cbDerivedKey, + // [in] ULONG dwFlags + // ); + exists(Call c | c.getTarget().getName() = "BCryptDeriveKeyPBKDF2" | + c.getArgument(4) = sink.asExpr() + ) + } +} + +module SaltLenTrace = DataFlow::Global; + +from DataFlow::Node src, DataFlow::Node sink +where SaltLenTrace::flow(src, sink) +select sink.asExpr(), + "Salt size $@ is passed to this to KDF. Use at least 16 bytes for salt size when deriving cryptographic key from password.", + src, src.asExpr().getValue() diff --git a/cpp/ql/src/Microsoft/Security/Cryptography/examples/BannedModesCAPI/BannedModesCAPI1.cpp b/cpp/ql/src/Microsoft/Security/Cryptography/examples/BannedModesCAPI/BannedModesCAPI1.cpp new file mode 100644 index 000000000000..6296e68499bf --- /dev/null +++ b/cpp/ql/src/Microsoft/Security/Cryptography/examples/BannedModesCAPI/BannedModesCAPI1.cpp @@ -0,0 +1,11 @@ +#include +#include +#include + +int main(){ + DWORD ivLen; + HCRYPTKEY hKey; + + //BAD + CryptGetKeyParam(hKey, CRYPT_MODE_ECB, NULL, &ivLen, 0); +} diff --git a/cpp/ql/src/Microsoft/Security/Cryptography/examples/BannedModesCAPI/BannedModesCAPI2.cpp b/cpp/ql/src/Microsoft/Security/Cryptography/examples/BannedModesCAPI/BannedModesCAPI2.cpp new file mode 100644 index 000000000000..d804432a1237 --- /dev/null +++ b/cpp/ql/src/Microsoft/Security/Cryptography/examples/BannedModesCAPI/BannedModesCAPI2.cpp @@ -0,0 +1,11 @@ +#include +#include +#include + +int main(){ + DWORD ivLen; + HCRYPTKEY hKey; + + //OKAY + CryptGetKeyParam(hKey, CRYPT_MODE_CBC, NULL, &ivLen, 0); +} diff --git a/cpp/ql/src/Microsoft/Security/Cryptography/examples/BannedModesCNG/BannedModesCNG1.cpp b/cpp/ql/src/Microsoft/Security/Cryptography/examples/BannedModesCNG/BannedModesCNG1.cpp new file mode 100644 index 000000000000..45b2e3607b55 --- /dev/null +++ b/cpp/ql/src/Microsoft/Security/Cryptography/examples/BannedModesCNG/BannedModesCNG1.cpp @@ -0,0 +1,14 @@ +#include +#include +#include + +int main(){ + BCRYPT_ALG_HANDLE aes; + + //BAD + status = BCryptSetProperty(aes, + BCRYPT_CHAINING_MODE, + (PBYTE)BCRYPT_CHAIN_MODE_ECB, + sizeof(BCRYPT_CHAIN_MODE_ECB), + 0); +} diff --git a/cpp/ql/src/Microsoft/Security/Cryptography/examples/BannedModesCNG/BannedModesCNG2.cpp b/cpp/ql/src/Microsoft/Security/Cryptography/examples/BannedModesCNG/BannedModesCNG2.cpp new file mode 100644 index 000000000000..5bf92ca87a47 --- /dev/null +++ b/cpp/ql/src/Microsoft/Security/Cryptography/examples/BannedModesCNG/BannedModesCNG2.cpp @@ -0,0 +1,14 @@ +#include +#include +#include + +int main(){ + BCRYPT_ALG_HANDLE aes; + + //OKAY + status = BCryptSetProperty(aes, + BCRYPT_CHAINING_MODE, + (PBYTE)BCRYPT_CHAIN_MODE_CBC, + sizeof(BCRYPT_CHAIN_MODE_CBC), + 0); +} diff --git a/cpp/ql/src/Microsoft/Security/Cryptography/examples/WeakEncryption/WeakEncryption1.cpp b/cpp/ql/src/Microsoft/Security/Cryptography/examples/WeakEncryption/WeakEncryption1.cpp new file mode 100644 index 000000000000..bdaaa7056ec5 --- /dev/null +++ b/cpp/ql/src/Microsoft/Security/Cryptography/examples/WeakEncryption/WeakEncryption1.cpp @@ -0,0 +1,14 @@ +#include +#include +#include + +int main(){ + HCRYPTPROV hCryptProv; + HCRYPTKEY hKey; + + //BAD + if(CryptGenKey( hCryptProv, CALG_DES_128, KEYLENGTH | CRYPT_EXPORTABLE, &hKey)) + { + printf("A session key has been created.\n"); + } +} diff --git a/cpp/ql/src/Microsoft/Security/Cryptography/examples/WeakEncryption/WeakEncryption2.cpp b/cpp/ql/src/Microsoft/Security/Cryptography/examples/WeakEncryption/WeakEncryption2.cpp new file mode 100644 index 000000000000..7e20995e8c95 --- /dev/null +++ b/cpp/ql/src/Microsoft/Security/Cryptography/examples/WeakEncryption/WeakEncryption2.cpp @@ -0,0 +1,14 @@ +#include +#include +#include + +int main(){ + HCRYPTPROV hCryptProv; + HCRYPTKEY hKey; + + //OKAY + if(CryptGenKey( hCryptProv, CALG_AES_128, KEYLENGTH | CRYPT_EXPORTABLE, &hKey)) + { + printf("A session key has been created.\n"); + } +} diff --git a/cpp/ql/src/Microsoft/Security/Cryptography/examples/WeakEncryption/WeakEncryption3.cpp b/cpp/ql/src/Microsoft/Security/Cryptography/examples/WeakEncryption/WeakEncryption3.cpp new file mode 100644 index 000000000000..e0ee60d830f9 --- /dev/null +++ b/cpp/ql/src/Microsoft/Security/Cryptography/examples/WeakEncryption/WeakEncryption3.cpp @@ -0,0 +1,12 @@ +#include +#include +#include + +int main(){ + BCRYPT_ALG_HANDLE hAlg; + NTSTATUS status; + //BAD + status = BCryptOpenAlgorithmProvider(&hAlg, BCRYPT_DES_ALGORITHM, MS_PRIMITIVE_PROVIDER, 0); +} + + diff --git a/cpp/ql/src/Microsoft/Security/Cryptography/examples/WeakEncryption/WeakEncryption4.cpp b/cpp/ql/src/Microsoft/Security/Cryptography/examples/WeakEncryption/WeakEncryption4.cpp new file mode 100644 index 000000000000..57d8e0bb9675 --- /dev/null +++ b/cpp/ql/src/Microsoft/Security/Cryptography/examples/WeakEncryption/WeakEncryption4.cpp @@ -0,0 +1,12 @@ +#include +#include +#include + +int main(){ + BCRYPT_ALG_HANDLE hAlg; + NTSTATUS status; + //OKAY + status = BCryptOpenAlgorithmProvider(&hAlg, BCRYPT_AES_ALGORITHM, MS_PRIMITIVE_PROVIDER, 0); +} + + diff --git a/cpp/ql/src/Microsoft/Security/MemoryAccess/EnumIndex/UncheckedBoundsEnumAsIndex.c b/cpp/ql/src/Microsoft/Security/MemoryAccess/EnumIndex/UncheckedBoundsEnumAsIndex.c new file mode 100644 index 000000000000..dead60efd5fd --- /dev/null +++ b/cpp/ql/src/Microsoft/Security/MemoryAccess/EnumIndex/UncheckedBoundsEnumAsIndex.c @@ -0,0 +1,15 @@ +typedef enum { + exampleSomeValue, + exampleSomeOtherValue, + exampleValueMax +} EXAMPLE_VALUES; + +/*...*/ + +int variable = someStructure->example; +if (variable >= exampleValueMax) +{ + /* ... Some action ... */ +} +// ... +Status = someArray[variable](/*...*/); \ No newline at end of file diff --git a/cpp/ql/src/Microsoft/Security/MemoryAccess/EnumIndex/UncheckedBoundsEnumAsIndex.qhelp b/cpp/ql/src/Microsoft/Security/MemoryAccess/EnumIndex/UncheckedBoundsEnumAsIndex.qhelp new file mode 100644 index 000000000000..3f3f6b8c4ab6 --- /dev/null +++ b/cpp/ql/src/Microsoft/Security/MemoryAccess/EnumIndex/UncheckedBoundsEnumAsIndex.qhelp @@ -0,0 +1,24 @@ + + + +

    This rule finds code where an enumerated type (enum) is used to check for an upper boundary, but not the lower boundary, and the value is used as an index to access an array.

    +

    By default an enum variable is signed, and therefore it is important to ensure that it cannot take on a negative value. When the enum is subsequently used to index an array, or worse still an array of function pointers, then a negative enum value would lead to potentially arbitrary memory being read, used and/or executed.

    +
    + +

    In the majority of cases the fix is simply to add the required lower bounds check to ensure that the enum has a positive value.

    +
    + + +

    The following example a value is passed and gets cast to an enumerated type and only partially bounds checked.

    + +

    In this example, the result of the out-of-bounds may allow for arbitrary code execution.

    +

    To fix the problem in this example, you need to add an additional check to the guarding if statement to verify that the index is a positive value.

    +
    + + + + + +
    diff --git a/cpp/ql/src/Microsoft/Security/MemoryAccess/EnumIndex/UncheckedBoundsEnumAsIndex.ql b/cpp/ql/src/Microsoft/Security/MemoryAccess/EnumIndex/UncheckedBoundsEnumAsIndex.ql new file mode 100644 index 000000000000..239ad149bc33 --- /dev/null +++ b/cpp/ql/src/Microsoft/Security/MemoryAccess/EnumIndex/UncheckedBoundsEnumAsIndex.ql @@ -0,0 +1,122 @@ +/** + * @name EnumIndex + * @description Code using enumerated types as indexes into arrays will often check for + * an upper bound to ensure the index is not out of range. + * By default an enum variable is signed, and therefore it is important to ensure + * that it cannot take on a negative value. When the enum is subsequently used + * to index an array, or worse still an array of function pointers, then a negative + * enum value would lead to potentially arbitrary memory being read, used and/or executed. + * @kind problem + * @problem.severity error + * @precision high + * @id cpp/enum-index + * @tags security + * external/cwe/cwe-125 + * external/microsoft/c33010 + */ + +import cpp +import semmle.code.cpp.controlflow.Guards +private import semmle.code.cpp.rangeanalysis.RangeAnalysisUtils +import semmle.code.cpp.rangeanalysis.SimpleRangeAnalysis + +/** + * Holds if `ec` is the upper bound of an enum + */ +predicate isUpperBoundEnumValue(EnumConstant ec) { + not exists(EnumConstant ec2, Enum enum | enum = ec2.getEnclosingElement() | + enum = ec.getEnclosingElement() and + ec2.getValue().toInt() > ec.getValue().toInt() + ) +} + +/** + * Holds if 'eca' is an access to the upper bound of an enum + */ +predicate isUpperBoundEnumAccess(EnumConstantAccess eca) { + exists(EnumConstant ec | + varbind(eca, ec) and + isUpperBoundEnumValue(ec) + ) +} + +/** + * Holds if the expression `e` is accessing the enum constant `ec` + */ +predicate isExpressionAccessingUpperboundEnum(Expr e, EnumConstantAccess ec) { + isExpressionAccessingUpperboundEnum(e.getAChild(), ec) + or + ec = e and + isUpperBoundEnumAccess(ec) +} + +/** + * Holds if `e` is a child of an If statement + */ +predicate isPartOfAnIfStatement(Expr e) { + isPartOfAnIfStatement(e.getAChild()) + or + exists(IfStmt ifs | ifs.getAChild() = e) +} + +/** + * Holds if the variable access `offsetExpr` upper bound is guarded by an If statement GuardCondition + * that is using the upper bound of an enum to check the upper bound of `offsetExpr` + */ +predicate hasUpperBoundDefinedByEnum(VariableAccess offsetExpr) { + exists(BasicBlock controlled, StackVariable offsetVar, SsaDefinition def | + controlled.contains(offsetExpr) and + linearBoundControlsEnum(controlled, def, offsetVar, Lesser()) and + offsetExpr = def.getAUse(offsetVar) + ) +} + +pragma[noinline] +predicate linearBoundControlsEnum( + BasicBlock controlled, SsaDefinition def, StackVariable offsetVar, RelationDirection direction +) { + exists(GuardCondition guard | + exists(boolean branch | + guard.controls(controlled, branch) and + cmpWithLinearBound(guard, def.getAUse(offsetVar), direction, branch) + ) and + exists(EnumConstantAccess enumca | isExpressionAccessingUpperboundEnum(guard, enumca)) and + isPartOfAnIfStatement(guard) + ) +} + +/** + * Holds if the variable access `offsetExpr` lower bound is guarded + */ +predicate hasLowerBound(VariableAccess offsetExpr) { + exists(BasicBlock controlled, StackVariable offsetVar, SsaDefinition def | + controlled.contains(offsetExpr) and + linearBoundControls(controlled, def, offsetVar, Greater()) and + offsetExpr = def.getAUse(offsetVar) + ) +} + +pragma[noinline] +predicate linearBoundControls( + BasicBlock controlled, SsaDefinition def, StackVariable offsetVar, RelationDirection direction +) { + exists(GuardCondition guard, boolean branch | + guard.controls(controlled, branch) and + cmpWithLinearBound(guard, def.getAUse(offsetVar), direction, branch) and + isPartOfAnIfStatement(guard) + ) +} + +from VariableAccess offset, ArrayExpr array +where + offset = array.getArrayOffset() and + hasUpperBoundDefinedByEnum(offset) and + not hasLowerBound(offset) and + exists(IntegralType t | + t = offset.getUnderlyingType() and + not t.isUnsigned() + ) and + lowerBound(offset.getFullyConverted()) < 0 +select offset, + "When accessing array " + array.getArrayBase() + " with index " + offset + + ", the upper bound of an enum is used to check the upper bound of the array, but the lower bound is not checked." diff --git a/cpp/ql/src/Microsoft/Security/Protocols/HardCodedSecurityProtocol.qhelp b/cpp/ql/src/Microsoft/Security/Protocols/HardCodedSecurityProtocol.qhelp new file mode 100644 index 000000000000..e34d26933823 --- /dev/null +++ b/cpp/ql/src/Microsoft/Security/Protocols/HardCodedSecurityProtocol.qhelp @@ -0,0 +1,29 @@ + + + + +

    Hard-coding security protocols rather than specifying the system default is risky because the protocol may become deprecated in future.

    +

    The grbitEnabledProtocols member of the SCHANNEL_CRED struct contains a bit string that represents the protocols supported by connections made with credentials acquired by using this structure. If this member is zero, Schannel selects the protocol. Applications should set grbitEnabledProtocols to zero and use the protocol versions enabled on the system by default.

    +
    + + +

    - Set the grbitEnabledProtocols member of the SCHANNEL_CRED struct to 0.

    +
    + + +

    Violation:

    + + + +

    Solution:

    + + +
    + + +
  • Microsoft Docs: SCHANNEL_CRED structure.
  • +
    + +
    \ No newline at end of file diff --git a/cpp/ql/src/Microsoft/Security/Protocols/HardCodedSecurityProtocol.ql b/cpp/ql/src/Microsoft/Security/Protocols/HardCodedSecurityProtocol.ql new file mode 100644 index 000000000000..96ab9509d5e6 --- /dev/null +++ b/cpp/ql/src/Microsoft/Security/Protocols/HardCodedSecurityProtocol.ql @@ -0,0 +1,19 @@ +/** + * @name Hard-coded use of a security protocol + * @description Hard-coding the security protocol used rather than specifying the system default is + * risky because the protocol may become deprecated in future. + * @kind problem + * @problem.severity warning + * @id cpp/hardcoded-security-protocol + */ + +import cpp +import HardCodedSecurityProtocol + +from ProtocolConstant constantValue, DataFlow::Node grbitEnabledProtocolsAssignment +where + GrbitEnabledConstantTace::flow(DataFlow::exprNode(constantValue), grbitEnabledProtocolsAssignment) and + constantValue.isHardcodedProtocol() +select constantValue, + "Hard-coded use of security protocol " + getConstantName(constantValue) + " set here $@.", + grbitEnabledProtocolsAssignment, grbitEnabledProtocolsAssignment.toString() diff --git a/cpp/ql/src/Microsoft/Security/Protocols/HardCodedSecurityProtocol.qll b/cpp/ql/src/Microsoft/Security/Protocols/HardCodedSecurityProtocol.qll new file mode 100644 index 000000000000..1cc71668ccbe --- /dev/null +++ b/cpp/ql/src/Microsoft/Security/Protocols/HardCodedSecurityProtocol.qll @@ -0,0 +1,140 @@ +import cpp +import semmle.code.cpp.dataflow.new.TaintTracking + +/** + * A constant representing one or more security protocols for the `grbitEnabledProtocols` field. + */ +class ProtocolConstant extends Expr { + ProtocolConstant() { + this.isConstant() and + GrbitEnabledConstantTace::flow(DataFlow::exprNode(this), _) and + ( + this instanceof Literal + or + this = any(ConstantMacroInvocation mi).getExpr() + or + // This is a workaround for folded constants, which currently have no + // dataflow node representation. Attach to the outermost dataflow node + // where a literal exists as a child that has no dataflow node representation. + exists(Literal l | + this.getAChild*() = l and + not exists(DataFlow::Node n | n.asExpr() = l) + ) + ) + } + + /** Gets the bitmask represented by this constant. */ + int getBitmask() { result = this.getValue().toInt() } + + /** Holds if this constant only represents TLS1.3 protocols. */ + predicate isTLS1_3Only() { + // Flags for TLS1.3 are 0x00001000 and 0x00002000 + // 12288 = 0x00001000 | 0x00002000 + this.getBitmask().bitAnd(12288.bitNot()) = 0 and + not this.isSystemDefault() + } + + /** Holds if this constant only represents TLS1.2 protocols. */ + predicate isTLS1_2Only() { + // Flags for TLS1.2 are 0x00000400 and 0x00000800 + // 3072 = 0x00000400 | 0x00000800 + this.getBitmask().bitAnd(3072.bitNot()) = 0 and + not this.isSystemDefault() + } + + /** Holds if this constant only represents TLS1.1 protocols. */ + predicate isTLS1_1Only() { + // Flags for TLS1.1 are 0x00000100 and 0x00000200 + // 768 = 0x00000100 | 0x00000200 + this.getBitmask().bitAnd(768.bitNot()) = 0 and + not this.isSystemDefault() + } + + /** Holds if this constant only represents TLS1.0 protocols. */ + predicate isTLS1_0Only() { + // Flags for TLS1.0 are 0x00000040 and 0x00000080 + // 192 = 0x00000040 | 0x00000080 + this.getBitmask().bitAnd(192.bitNot()) = 0 and + not this.isSystemDefault() + } + + /** Holds if this constant only represents TLS1.1 protocols. */ + predicate isSSL3Only() { + // Flags for SSL3 are 0x00000010 and 0x00000020 + // 48 = 0x00000010 | 0x00000020 + this.getBitmask().bitAnd(48.bitNot()) = 0 and + not this.isSystemDefault() + } + + /** Holds if this constant only represents SSL2 protocols. */ + predicate isSSL2Only() { + // Flags for TLS1.0 are 0x00000004 and 0x00000008 + // 12 = 0x00000004 | 0x00000008 + this.getBitmask().bitAnd(12.bitNot()) = 0 and + not this.isSystemDefault() + } + + /** Holds if this constant only represents PCT1 protocols. */ + predicate isPCT1Only() { + // Flags for PCT are 0x00000001 and 0x00000002 + // 3 = 0x00000001 | 0x00000002 + this.getBitmask().bitAnd(3.bitNot()) = 0 and + not this.isSystemDefault() + } + + /** Holds if this constant only represents any combination of TLS-related protocols. */ + predicate isHardcodedProtocol() { + // 16383 = SP_PROT_TLS1_3 | SP_PROT_TLS1_2 | SP_PROT_TLS1_1 | SP_PROT_TLS1_3 + // | SP_PROT_TLS1 | SP_PROT_SSL3 | SP_PROT_SSL2 | SP_PROT_PCT1 + this.getBitmask().bitAnd(16383.bitNot()) = 0 and + not this.isSystemDefault() + } + + /** Holds if this constant represents the system default protocol. */ + predicate isSystemDefault() { this.getBitmask() = 0 } +} + +/** + * A data flow configuration that tracks from constant values to assignments to the + * `grbitEnabledProtocols` field on the SCHANNEL_CRED structure. + */ +module GrbitEnabledConstantConfiguration implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node source) { source.asExpr().isConstant() } + + predicate isSink(DataFlow::Node sink) { + exists(Field grbitEnabledProtocols | + grbitEnabledProtocols.hasName("grbitEnabledProtocols") and + sink.asExpr() = grbitEnabledProtocols.getAnAssignedValue() + ) + } + + predicate isBarrier(DataFlow::Node node) { + // Do not flow through other macro invocations if they would, themselves, be represented + node.asExpr() = any(ConstantMacroInvocation mi).getExpr().getAChild+() + or + // Do not flow through complements, as they change the meaning + node.asExpr() instanceof ComplementExpr + } +} + +module GrbitEnabledConstantTace = TaintTracking::Global; + +/** + * A macro that represents a constant value. + */ +class ConstantMacroInvocation extends MacroInvocation { + ConstantMacroInvocation() { + exists(this.getExpr().getValue()) and + not this.getMacro().getHead().matches("%(%)%") + } +} + +/** + * Gets the name of the constant `val`, if it is a constant. + */ +string getConstantName(Expr val) { + exists(val.getValue()) and + if exists(ConstantMacroInvocation mi | mi.getExpr() = val) + then result = any(ConstantMacroInvocation mi | mi.getExpr() = val).getMacroName() + else result = val.toString() +} diff --git a/cpp/ql/src/Microsoft/Security/Protocols/UseOfDeprecatedSecurityProtocol.qhelp b/cpp/ql/src/Microsoft/Security/Protocols/UseOfDeprecatedSecurityProtocol.qhelp new file mode 100644 index 000000000000..0876a4e50439 --- /dev/null +++ b/cpp/ql/src/Microsoft/Security/Protocols/UseOfDeprecatedSecurityProtocol.qhelp @@ -0,0 +1,29 @@ + + + + +

    Older protocol versions of TLS are less secure than TLS 1.2 and TLS 1.3 and are more likely to have new vulnerabilities. Avoid older protocol versions to minimize risk.

    +

    The grbitEnabledProtocols member of the SCHANNEL_CRED struct contains a bit string that represents the protocols supported by connections made with credentials acquired by using this structure. If this member is zero, Schannel selects the protocol. Applications should set grbitEnabledProtocols to zero and use the protocol versions enabled on the system by default.

    +
    + + +

    - Set the grbitEnabledProtocols member of the SCHANNEL_CRED struct to 0.

    +
    + + +

    Violation:

    + + + +

    Solution:

    + + +
    + + +
  • Microsoft Docs: SCHANNEL_CRED structure.
  • +
    + +
    \ No newline at end of file diff --git a/cpp/ql/src/Microsoft/Security/Protocols/UseOfDeprecatedSecurityProtocol.ql b/cpp/ql/src/Microsoft/Security/Protocols/UseOfDeprecatedSecurityProtocol.ql new file mode 100644 index 000000000000..035bec3a425c --- /dev/null +++ b/cpp/ql/src/Microsoft/Security/Protocols/UseOfDeprecatedSecurityProtocol.ql @@ -0,0 +1,21 @@ +/** + * @name Hard-coded use of a deprecated security protocol + * @description Using a deprecated security protocol rather than the system default is risky. + * @kind problem + * @problem.severity error + * @id cpp/use-of-deprecated-security-protocol + */ + +import cpp +import HardCodedSecurityProtocol + +from ProtocolConstant constantValue, DataFlow::Node grbitEnabledProtocolsAssignment +where + GrbitEnabledConstantTace::flow(DataFlow::exprNode(constantValue), grbitEnabledProtocolsAssignment) and + // If the system default hasn't been specified, and TLS2 has not been specified, then this is a deprecated security protocol + not constantValue.isSystemDefault() and + not constantValue.isTLS1_2Only() and + not constantValue.isTLS1_3Only() +select constantValue, + "Hard-coded use of deprecated security protocol " + getConstantName(constantValue) + + " set here $@.", constantValue, getConstantName(constantValue) diff --git a/cpp/ql/src/Microsoft/Security/Protocols/examples/HardCodedSecurityProtocol/HardCodedSecurityProtocol1.cpp b/cpp/ql/src/Microsoft/Security/Protocols/examples/HardCodedSecurityProtocol/HardCodedSecurityProtocol1.cpp new file mode 100644 index 000000000000..3ad2eca405bb --- /dev/null +++ b/cpp/ql/src/Microsoft/Security/Protocols/examples/HardCodedSecurityProtocol/HardCodedSecurityProtocol1.cpp @@ -0,0 +1,18 @@ +#include +#include +#include +#include +#include + +void HardCodedSecurityProtocolGood() +{ + + SCHANNEL_CRED credData; + ZeroMemory(&credData, sizeof(credData)); + + // BAD: hardcoded protocols + credData.grbitEnabledProtocols = SP_PROT_TLS1_2; + credData.grbitEnabledProtocols = SP_PROT_TLS1_3; + + return; +} \ No newline at end of file diff --git a/cpp/ql/src/Microsoft/Security/Protocols/examples/HardCodedSecurityProtocol/HardCodedSecurityProtocol2.cpp b/cpp/ql/src/Microsoft/Security/Protocols/examples/HardCodedSecurityProtocol/HardCodedSecurityProtocol2.cpp new file mode 100644 index 000000000000..7f5318248ef1 --- /dev/null +++ b/cpp/ql/src/Microsoft/Security/Protocols/examples/HardCodedSecurityProtocol/HardCodedSecurityProtocol2.cpp @@ -0,0 +1,17 @@ +#include +#include +#include +#include +#include + +void HardCodedSecurityProtocolGood() +{ + + SCHANNEL_CRED credData; + ZeroMemory(&credData, sizeof(credData)); + + // GOOD: system default protocol + credData.grbitEnabledProtocols = 0; + + return; +} \ No newline at end of file diff --git a/cpp/ql/src/Microsoft/Security/Protocols/examples/UseOfDeprecatedSecurityProtocol/UseOfDeprecatedSecurityProtocol1.cpp b/cpp/ql/src/Microsoft/Security/Protocols/examples/UseOfDeprecatedSecurityProtocol/UseOfDeprecatedSecurityProtocol1.cpp new file mode 100644 index 000000000000..d9b0ddc4af74 --- /dev/null +++ b/cpp/ql/src/Microsoft/Security/Protocols/examples/UseOfDeprecatedSecurityProtocol/UseOfDeprecatedSecurityProtocol1.cpp @@ -0,0 +1,23 @@ +#include +#include +#include +#include +#include + +void UseOfDeprecatedSecurityProtocolGood() +{ + + SCHANNEL_CRED credData; + ZeroMemory(&credData, sizeof(credData)); + + // BAD: Deprecated protocols + credData.grbitEnabledProtocols = SP_PROT_PCT1_SERVER; + credData.grbitEnabledProtocols = SP_PROT_SSL2_SERVER; + credData.grbitEnabledProtocols = SP_PROT_SSL3_SERVER; + credData.grbitEnabledProtocols = SP_PROT_TLS1_1; + credData.grbitEnabledProtocols = SP_PROT_TLS1_1_SERVER; + credData.grbitEnabledProtocols = SP_PROT_TLS1_1_CLIENT; + credData.grbitEnabledProtocols = SP_PROT_SSL3TLS1; + + return; +} \ No newline at end of file diff --git a/cpp/ql/src/Microsoft/Security/Protocols/examples/UseOfDeprecatedSecurityProtocol/UseOfDeprecatedSecurityProtocol2.cpp b/cpp/ql/src/Microsoft/Security/Protocols/examples/UseOfDeprecatedSecurityProtocol/UseOfDeprecatedSecurityProtocol2.cpp new file mode 100644 index 000000000000..7f5318248ef1 --- /dev/null +++ b/cpp/ql/src/Microsoft/Security/Protocols/examples/UseOfDeprecatedSecurityProtocol/UseOfDeprecatedSecurityProtocol2.cpp @@ -0,0 +1,17 @@ +#include +#include +#include +#include +#include + +void HardCodedSecurityProtocolGood() +{ + + SCHANNEL_CRED credData; + ZeroMemory(&credData, sizeof(credData)); + + // GOOD: system default protocol + credData.grbitEnabledProtocols = 0; + + return; +} \ No newline at end of file diff --git a/cpp/ql/test/query-tests/Likely Bugs/Leap Year/Adding365DaysPerYear/Adding365daysPerYear.expected b/cpp/ql/test/query-tests/Likely Bugs/Leap Year/Adding365DaysPerYear/Adding365daysPerYear.expected index d9d9c4d3d338..094d84495ce4 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Leap Year/Adding365DaysPerYear/Adding365daysPerYear.expected +++ b/cpp/ql/test/query-tests/Likely Bugs/Leap Year/Adding365DaysPerYear/Adding365daysPerYear.expected @@ -1,5 +1,7 @@ -| test.cpp:173:29:173:51 | ... & ... | An arithmetic operation $@ that uses a constant value of 365 ends up modifying this date/time, without considering leap year scenarios. | test.cpp:170:2:170:47 | ... += ... | ... += ... | -| test.cpp:174:30:174:45 | ... >> ... | An arithmetic operation $@ that uses a constant value of 365 ends up modifying this date/time, without considering leap year scenarios. | test.cpp:170:2:170:47 | ... += ... | ... += ... | -| test.cpp:193:15:193:24 | ... / ... | An arithmetic operation $@ that uses a constant value of 365 ends up modifying this date/time, without considering leap year scenarios. | test.cpp:193:15:193:24 | ... / ... | ... / ... | -| test.cpp:217:29:217:51 | ... & ... | An arithmetic operation $@ that uses a constant value of 365 ends up modifying this date/time, without considering leap year scenarios. | test.cpp:214:2:214:47 | ... += ... | ... += ... | -| test.cpp:218:30:218:45 | ... >> ... | An arithmetic operation $@ that uses a constant value of 365 ends up modifying this date/time, without considering leap year scenarios. | test.cpp:214:2:214:47 | ... += ... | ... += ... | +| test.cpp:175:29:175:51 | ... & ... | $@: This arithmetic operation $@ uses a constant value of 365 ends up modifying the date/time located at $@, without considering leap year scenarios. | test.cpp:159:6:159:17 | antipattern2 | antipattern2 | test.cpp:172:2:172:47 | ... += ... | ... += ... | test.cpp:175:29:175:51 | ... & ... | ... & ... | +| test.cpp:176:30:176:45 | ... >> ... | $@: This arithmetic operation $@ uses a constant value of 365 ends up modifying the date/time located at $@, without considering leap year scenarios. | test.cpp:159:6:159:17 | antipattern2 | antipattern2 | test.cpp:172:2:172:47 | ... += ... | ... += ... | test.cpp:176:30:176:45 | ... >> ... | ... >> ... | +| test.cpp:195:15:195:24 | ... / ... | $@: This arithmetic operation $@ uses a constant value of 365 ends up modifying the date/time located at $@, without considering leap year scenarios. | test.cpp:185:8:185:13 | mkTime | mkTime | test.cpp:195:15:195:24 | ... / ... | ... / ... | test.cpp:195:15:195:24 | ... / ... | ... / ... | +| test.cpp:219:29:219:51 | ... & ... | $@: This arithmetic operation $@ uses a constant value of 365 ends up modifying the date/time located at $@, without considering leap year scenarios. | test.cpp:203:6:203:19 | checkedExample | checkedExample | test.cpp:216:2:216:47 | ... += ... | ... += ... | test.cpp:219:29:219:51 | ... & ... | ... & ... | +| test.cpp:220:30:220:45 | ... >> ... | $@: This arithmetic operation $@ uses a constant value of 365 ends up modifying the date/time located at $@, without considering leap year scenarios. | test.cpp:203:6:203:19 | checkedExample | checkedExample | test.cpp:216:2:216:47 | ... += ... | ... += ... | test.cpp:220:30:220:45 | ... >> ... | ... >> ... | +| test.cpp:247:29:247:51 | ... & ... | $@: This arithmetic operation $@ uses a constant value of 365 ends up modifying the date/time located at $@, without considering leap year scenarios. | test.cpp:230:6:230:18 | antipattern2A | antipattern2A | test.cpp:240:20:240:23 | 365 | 365 | test.cpp:247:29:247:51 | ... & ... | ... & ... | +| test.cpp:248:30:248:45 | ... >> ... | $@: This arithmetic operation $@ uses a constant value of 365 ends up modifying the date/time located at $@, without considering leap year scenarios. | test.cpp:230:6:230:18 | antipattern2A | antipattern2A | test.cpp:240:20:240:23 | 365 | 365 | test.cpp:248:30:248:45 | ... >> ... | ... >> ... | diff --git a/cpp/ql/test/query-tests/Likely Bugs/Leap Year/Adding365DaysPerYear/test.cpp b/cpp/ql/test/query-tests/Likely Bugs/Leap Year/Adding365DaysPerYear/test.cpp index a14667c75ca5..ccc9bf8446e0 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Leap Year/Adding365DaysPerYear/test.cpp +++ b/cpp/ql/test/query-tests/Likely Bugs/Leap Year/Adding365DaysPerYear/test.cpp @@ -153,7 +153,9 @@ GetFileTime( LPFILETIME lpLastWriteTime ); - +/** + * AntiPattern2 - datetime.AddDays(±365) +*/ void antipattern2() { // get the current time as a FILETIME @@ -223,3 +225,28 @@ void checkedExample() // handle error... } } + + +void antipattern2A() +{ + // get the current time as a FILETIME + SYSTEMTIME st; FILETIME ft; + GetSystemTime(&st); + SystemTimeToFileTime(&st, &ft); + + // convert to a quadword (64-bit integer) to do arithmetic + ULONGLONG qwLongTime; + qwLongTime = (((ULONGLONG)ft.dwHighDateTime) << 32) + ft.dwLowDateTime; + int days_in_year = 365; + + // add a year by calculating the ticks in 365 days + // (which may be incorrect when crossing a leap day) + qwLongTime += days_in_year * 24 * 60 * 60 * 10000000LLU; + + // copy back to a FILETIME + ft.dwLowDateTime = (DWORD)(qwLongTime & 0xFFFFFFFF); // BAD + ft.dwHighDateTime = (DWORD)(qwLongTime >> 32); // BAD + + // convert back to SYSTEMTIME for display or other usage + FileTimeToSystemTime(&ft, &st); +} diff --git a/cpp/ql/test/query-tests/Likely Bugs/Leap Year/AntiPattern5InvalidLeapYearCheck/AntiPattern5InvalidLeapYearCheck.expected b/cpp/ql/test/query-tests/Likely Bugs/Leap Year/AntiPattern5InvalidLeapYearCheck/AntiPattern5InvalidLeapYearCheck.expected new file mode 100644 index 000000000000..8e375a5ea5ce --- /dev/null +++ b/cpp/ql/test/query-tests/Likely Bugs/Leap Year/AntiPattern5InvalidLeapYearCheck/AntiPattern5InvalidLeapYearCheck.expected @@ -0,0 +1,3 @@ +| test.cpp:183:23:183:35 | ... == ... | Possible Insufficient Leap Year check (AntiPattern 5) | +| test.cpp:190:24:190:40 | ... == ... | Possible Insufficient Leap Year check (AntiPattern 5) | +| test.cpp:245:6:245:18 | ... == ... | Possible Insufficient Leap Year check (AntiPattern 5) | diff --git a/cpp/ql/test/query-tests/Likely Bugs/Leap Year/AntiPattern5InvalidLeapYearCheck/AntiPattern5InvalidLeapYearCheck.qlref b/cpp/ql/test/query-tests/Likely Bugs/Leap Year/AntiPattern5InvalidLeapYearCheck/AntiPattern5InvalidLeapYearCheck.qlref new file mode 100644 index 000000000000..70e3f8ba1029 --- /dev/null +++ b/cpp/ql/test/query-tests/Likely Bugs/Leap Year/AntiPattern5InvalidLeapYearCheck/AntiPattern5InvalidLeapYearCheck.qlref @@ -0,0 +1 @@ +Likely Bugs/Leap Year/AntiPattern5InvalidLeapYearCheck.ql diff --git a/cpp/ql/test/query-tests/Likely Bugs/Leap Year/AntiPattern5InvalidLeapYearCheck/test.cpp b/cpp/ql/test/query-tests/Likely Bugs/Leap Year/AntiPattern5InvalidLeapYearCheck/test.cpp new file mode 100644 index 000000000000..c0cd102321c1 --- /dev/null +++ b/cpp/ql/test/query-tests/Likely Bugs/Leap Year/AntiPattern5InvalidLeapYearCheck/test.cpp @@ -0,0 +1,255 @@ +typedef unsigned short WORD; +typedef unsigned long DWORD, HANDLE; +typedef int BOOL, BOOLEAN, errno_t; +typedef char CHAR; +typedef short SHORT; +typedef long LONG; +typedef unsigned short WCHAR; // wc, 16-bit UNICODE character +typedef long __time64_t, time_t; +#define NULL 0 + +typedef long long LONGLONG; +typedef unsigned long long ULONGLONG; + + +typedef struct _SYSTEMTIME { + WORD wYear; + WORD wMonth; + WORD wDayOfWeek; + WORD wDay; + WORD wHour; + WORD wMinute; + WORD wSecond; + WORD wMilliseconds; +} SYSTEMTIME, *PSYSTEMTIME, *LPSYSTEMTIME; + +typedef struct _FILETIME { + DWORD dwLowDateTime; + DWORD dwHighDateTime; +} FILETIME, *PFILETIME, *LPFILETIME; + +typedef struct _TIME_ZONE_INFORMATION { + LONG Bias; + WCHAR StandardName[32]; + SYSTEMTIME StandardDate; + LONG StandardBias; + WCHAR DaylightName[32]; + SYSTEMTIME DaylightDate; + LONG DaylightBias; +} TIME_ZONE_INFORMATION, *PTIME_ZONE_INFORMATION, *LPTIME_ZONE_INFORMATION; + +typedef struct _TIME_DYNAMIC_ZONE_INFORMATION { + LONG Bias; + WCHAR StandardName[32]; + SYSTEMTIME StandardDate; + LONG StandardBias; + WCHAR DaylightName[32]; + SYSTEMTIME DaylightDate; + LONG DaylightBias; + WCHAR TimeZoneKeyName[128]; + BOOLEAN DynamicDaylightTimeDisabled; +} DYNAMIC_TIME_ZONE_INFORMATION, *PDYNAMIC_TIME_ZONE_INFORMATION; + +struct tm +{ + int tm_sec; // seconds after the minute - [0, 60] including leap second + int tm_min; // minutes after the hour - [0, 59] + int tm_hour; // hours since midnight - [0, 23] + int tm_mday; // day of the month - [1, 31] + int tm_mon; // months since January - [0, 11] + int tm_year; // years since 1900 + int tm_wday; // days since Sunday - [0, 6] + int tm_yday; // days since January 1 - [0, 365] + int tm_isdst; // daylight savings time flag +}; + +BOOL +SystemTimeToFileTime( + const SYSTEMTIME* lpSystemTime, + LPFILETIME lpFileTime +); + +BOOL +FileTimeToSystemTime( + const FILETIME* lpFileTime, + LPSYSTEMTIME lpSystemTime +); + +BOOL +SystemTimeToTzSpecificLocalTime( + const TIME_ZONE_INFORMATION* lpTimeZoneInformation, + const SYSTEMTIME* lpUniversalTime, + LPSYSTEMTIME lpLocalTime +); + +BOOL +SystemTimeToTzSpecificLocalTimeEx( + const DYNAMIC_TIME_ZONE_INFORMATION* lpTimeZoneInformation, + const SYSTEMTIME* lpUniversalTime, + LPSYSTEMTIME lpLocalTime +); + +BOOL +TzSpecificLocalTimeToSystemTime( + const TIME_ZONE_INFORMATION* lpTimeZoneInformation, + const SYSTEMTIME* lpLocalTime, + LPSYSTEMTIME lpUniversalTime +); + +BOOL +TzSpecificLocalTimeToSystemTimeEx( + const DYNAMIC_TIME_ZONE_INFORMATION* lpTimeZoneInformation, + const SYSTEMTIME* lpLocalTime, + LPSYSTEMTIME lpUniversalTime +); + +void GetSystemTime( + LPSYSTEMTIME lpSystemTime +); + +void GetSystemTimeAsFileTime( + LPFILETIME lpSystemTimeAsFileTime +); + +__time64_t _mkgmtime64( + struct tm* _Tm +); + +__time64_t _mkgmtime( + struct tm* const _Tm +) +{ + return _mkgmtime64(_Tm); +} + +__time64_t mktime( + struct tm* const _Tm +) +{ + return _mkgmtime64(_Tm); +} + +__time64_t _time64( + __time64_t* _Time +); + +__time64_t time( + time_t* const _Time +) +{ + return _time64(_Time); +} + +int gmtime_s( + struct tm* _Tm, + __time64_t const* _Time +); + +BOOL +GetFileTime( + HANDLE hFile, + LPFILETIME lpCreationTime, + LPFILETIME lpLastAccessTime, + LPFILETIME lpLastWriteTime +); + +time_t mktime(struct tm *timeptr); +struct tm *gmtime(const time_t *timer); + +time_t mkTime(int days) +{ + struct tm tm; + time_t t; + + tm.tm_sec = 0; + tm.tm_min = 0; + tm.tm_hour = 0; + tm.tm_mday = 0; + tm.tm_mon = 0; + tm.tm_year = days / 365; // BAD + // ... + + t = mktime(&tm); // convert tm -> time_t + + return t; +} + +/** + * Positive AntiPattern 5 - year % 4 == 0 +*/ +void antipattern5() +{ + int year = 1; + bool isLeapYear = year % 4 == 0; + + // get the current time as a FILETIME + SYSTEMTIME st; FILETIME ft; + GetSystemTime(&st); + SystemTimeToFileTime(&st, &ft); + + bool isLeapYear2 = st.wYear % 4 == 0; +} + +/** + * Negative AntiPattern 5 - year % 4 == 0 +*/ +void antipattern5_negative() +{ + SYSTEMTIME st; FILETIME ft; + GetSystemTime(&st); + SystemTimeToFileTime(&st, &ft); + bool isLeapYear = st.wYear % 4 == 0 && (st.wYear % 100 != 0 || st.wYear % 400 == 0); + + int year = 1; + bool isLeapYear2 = year % 4 == 0 && (year % 100 != 0 || year % 400 == 0); +} + +/** +* Negative - Valid Leap year check (logically equivalent) (#1035) +*/ +bool ap5_negative_inverted_form(int year){ + return year % 400 == 0 || (year % 100 != 0 && year % 4 == 0); +} + +/** +* Negative - Valid Leap Year check (#1035) +* Century subexpression component is inverted `!(year % 100 == 0)` +*/ +bool ap5_negative_inverted_century_100(int year){ + return !((year % 4 == 0) && (!(year % 100 == 0) || (year % 400 == 0))); +} + +class SomeResultClass{ + public: + int GetYear() { + return 2000; + } +}; + +/** + * Negative - Valid Leap Year Check (#1038) + * Valid leap year check, but the expression is the result of a Call and thus breaks SSA. +*/ +bool ap5_fp_expr_call(SomeResultClass result){ + if (result.GetYear() % 4 == 0 && (result.GetYear() % 100 != 0 || result.GetYear() % 400 == 0)){ + return true; + } + return false; +} + +/** +* Positive - Invalid Leap Year check +* Components are split up and distributed across multiple if statements. +*/ +bool tp_leap_year_multiple_if_statements(int year){ + if (year % 4 == 0) { + if (year % 100 == 0) { + if (year % 400 == 0) { + return true; + } + }else{ + return true; + } + } + return false; +} \ No newline at end of file diff --git a/cpp/ql/test/query-tests/Likely Bugs/Leap Year/LeapYearConditionalLogic/LeapYearConditionalLogic.cpp b/cpp/ql/test/query-tests/Likely Bugs/Leap Year/LeapYearConditionalLogic/LeapYearConditionalLogic.cpp new file mode 100644 index 000000000000..3f9b61c68505 --- /dev/null +++ b/cpp/ql/test/query-tests/Likely Bugs/Leap Year/LeapYearConditionalLogic/LeapYearConditionalLogic.cpp @@ -0,0 +1,198 @@ + +typedef unsigned short WORD; +typedef unsigned long DWORD, HANDLE; +typedef int BOOL, BOOLEAN, errno_t; +typedef char CHAR; +typedef short SHORT; +typedef long LONG; +typedef unsigned short WCHAR; // wc, 16-bit UNICODE character +typedef long __time64_t, time_t; +#define NULL 0 + +typedef long long LONGLONG; +typedef unsigned long long ULONGLONG; + + +typedef struct _SYSTEMTIME { + WORD wYear; + WORD wMonth; + WORD wDayOfWeek; + WORD wDay; + WORD wHour; + WORD wMinute; + WORD wSecond; + WORD wMilliseconds; +} SYSTEMTIME, *PSYSTEMTIME, *LPSYSTEMTIME; + +typedef struct _FILETIME { + DWORD dwLowDateTime; + DWORD dwHighDateTime; +} FILETIME, *PFILETIME, *LPFILETIME; + +typedef struct _TIME_ZONE_INFORMATION { + LONG Bias; + WCHAR StandardName[32]; + SYSTEMTIME StandardDate; + LONG StandardBias; + WCHAR DaylightName[32]; + SYSTEMTIME DaylightDate; + LONG DaylightBias; +} TIME_ZONE_INFORMATION, *PTIME_ZONE_INFORMATION, *LPTIME_ZONE_INFORMATION; + +typedef struct _TIME_DYNAMIC_ZONE_INFORMATION { + LONG Bias; + WCHAR StandardName[32]; + SYSTEMTIME StandardDate; + LONG StandardBias; + WCHAR DaylightName[32]; + SYSTEMTIME DaylightDate; + LONG DaylightBias; + WCHAR TimeZoneKeyName[128]; + BOOLEAN DynamicDaylightTimeDisabled; +} DYNAMIC_TIME_ZONE_INFORMATION, *PDYNAMIC_TIME_ZONE_INFORMATION; + +struct tm +{ + int tm_sec; // seconds after the minute - [0, 60] including leap second + int tm_min; // minutes after the hour - [0, 59] + int tm_hour; // hours since midnight - [0, 23] + int tm_mday; // day of the month - [1, 31] + int tm_mon; // months since January - [0, 11] + int tm_year; // years since 1900 + int tm_wday; // days since Sunday - [0, 6] + int tm_yday; // days since January 1 - [0, 365] + int tm_isdst; // daylight savings time flag +}; + +BOOL +SystemTimeToFileTime( + const SYSTEMTIME* lpSystemTime, + LPFILETIME lpFileTime +); + +BOOL +FileTimeToSystemTime( + const FILETIME* lpFileTime, + LPSYSTEMTIME lpSystemTime +); + +BOOL +SystemTimeToTzSpecificLocalTime( + const TIME_ZONE_INFORMATION* lpTimeZoneInformation, + const SYSTEMTIME* lpUniversalTime, + LPSYSTEMTIME lpLocalTime +); + +BOOL +SystemTimeToTzSpecificLocalTimeEx( + const DYNAMIC_TIME_ZONE_INFORMATION* lpTimeZoneInformation, + const SYSTEMTIME* lpUniversalTime, + LPSYSTEMTIME lpLocalTime +); + +BOOL +TzSpecificLocalTimeToSystemTime( + const TIME_ZONE_INFORMATION* lpTimeZoneInformation, + const SYSTEMTIME* lpLocalTime, + LPSYSTEMTIME lpUniversalTime +); + +BOOL +TzSpecificLocalTimeToSystemTimeEx( + const DYNAMIC_TIME_ZONE_INFORMATION* lpTimeZoneInformation, + const SYSTEMTIME* lpLocalTime, + LPSYSTEMTIME lpUniversalTime +); + +void GetSystemTime( + LPSYSTEMTIME lpSystemTime +); + +void GetSystemTimeAsFileTime( + LPFILETIME lpSystemTimeAsFileTime +); + +__time64_t _mkgmtime64( + struct tm* _Tm +); + +__time64_t _mkgmtime( + struct tm* const _Tm +) +{ + return _mkgmtime64(_Tm); +} + +__time64_t mktime( + struct tm* const _Tm +) +{ + return _mkgmtime64(_Tm); +} + +__time64_t _time64( + __time64_t* _Time +); + +__time64_t time( + time_t* const _Time +) +{ + return _time64(_Time); +} + +int gmtime_s( + struct tm* _Tm, + __time64_t const* _Time +); + +BOOL +GetFileTime( + HANDLE hFile, + LPFILETIME lpCreationTime, + LPFILETIME lpLastAccessTime, + LPFILETIME lpLastWriteTime +); + +void print(const char* s); + +/** + * AntiPattern7 - isLeapYear Conditional +*/ +void antipattern7() +{ + // get the current time as a FILETIME + SYSTEMTIME st; FILETIME ft; + GetSystemTime(&st); + SystemTimeToFileTime(&st, &ft); + + bool isLeapYear = st.wYear % 4 == 0 && (st.wYear % 100 != 0 || st.wYear % 400 == 0); + if(isLeapYear){ + // do something to cater for a leap year.... + print("It was a leap year"); + }else{ + // do another (different) thing + print("It was **not** a leap year"); + } +} + +time_t mktime(struct tm *timeptr); +struct tm *gmtime(const time_t *timer); + +time_t mkTime(int days) +{ + struct tm tm; + time_t t; + + tm.tm_sec = 0; + tm.tm_min = 0; + tm.tm_hour = 0; + tm.tm_mday = 0; + tm.tm_mon = 0; + tm.tm_year = days / 365; // BAD + // ... + + t = mktime(&tm); // convert tm -> time_t + + return t; +} \ No newline at end of file diff --git a/cpp/ql/test/query-tests/Likely Bugs/Leap Year/LeapYearConditionalLogic/LeapYearConditionalLogic.expected b/cpp/ql/test/query-tests/Likely Bugs/Leap Year/LeapYearConditionalLogic/LeapYearConditionalLogic.expected new file mode 100644 index 000000000000..be5f31d55769 --- /dev/null +++ b/cpp/ql/test/query-tests/Likely Bugs/Leap Year/LeapYearConditionalLogic/LeapYearConditionalLogic.expected @@ -0,0 +1 @@ +| LeapYearConditionalLogic.cpp:170:5:176:5 | if (...) ... | Leap Year conditional statement may have untested code paths | diff --git a/cpp/ql/test/query-tests/Likely Bugs/Leap Year/LeapYearConditionalLogic/LeapYearConditionalLogic.qlref b/cpp/ql/test/query-tests/Likely Bugs/Leap Year/LeapYearConditionalLogic/LeapYearConditionalLogic.qlref new file mode 100644 index 000000000000..750ff8deb60a --- /dev/null +++ b/cpp/ql/test/query-tests/Likely Bugs/Leap Year/LeapYearConditionalLogic/LeapYearConditionalLogic.qlref @@ -0,0 +1 @@ +Likely Bugs/Leap Year/LeapYearConditionalLogic.ql 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 a9c1bc66c50f..555002b40867 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,15 +1,8 @@ -| test.cpp:314:5:314:9 | wYear | Field $@ on variable $@ has been modified, but no appropriate check for LeapYear was found. | test.cpp:12:7:12:11 | wYear | wYear | test.cpp:309:13:309:14 | st | st | -| test.cpp:327:5:327:9 | wYear | Field $@ on variable $@ has been modified, but no appropriate check for LeapYear was found. | test.cpp:12:7:12:11 | wYear | wYear | test.cpp:322:13:322:14 | st | st | -| test.cpp:338:6:338:10 | wYear | Field $@ on variable $@ has been modified, but no appropriate check for LeapYear was found. | test.cpp:12:7:12:11 | wYear | wYear | test.cpp:333:62:333:63 | st | st | -| test.cpp:484:5:484:9 | wYear | Field $@ on variable $@ has been modified, but no appropriate check for LeapYear was found. | test.cpp:12:7:12:11 | wYear | wYear | test.cpp:480:13:480:14 | st | st | -| test.cpp:497:5:497:9 | wYear | Field $@ on variable $@ has been modified, but no appropriate check for LeapYear was found. | test.cpp:12:7:12:11 | wYear | wYear | test.cpp:492:13:492:14 | st | st | -| test.cpp:509:5:509:9 | wYear | Field $@ on variable $@ has been modified, but no appropriate check for LeapYear was found. | test.cpp:12:7:12:11 | wYear | wYear | test.cpp:505:13:505:14 | st | st | -| test.cpp:606:11:606:17 | tm_year | Field $@ on variable $@ has been modified, but no appropriate check for LeapYear was found. | test.cpp:56:6:56:12 | tm_year | tm_year | test.cpp:602:12:602:19 | timeinfo | timeinfo | -| test.cpp:634:11:634:17 | tm_year | Field $@ on variable $@ has been modified, but no appropriate check for LeapYear was found. | test.cpp:56:6:56:12 | tm_year | tm_year | test.cpp:628:12:628:19 | timeinfo | timeinfo | -| test.cpp:636:11:636:17 | tm_year | Field $@ on variable $@ has been modified, but no appropriate check for LeapYear was found. | test.cpp:56:6:56:12 | tm_year | tm_year | test.cpp:628:12:628:19 | timeinfo | timeinfo | -| test.cpp:640:5:640:9 | wYear | Field $@ on variable $@ has been modified, but no appropriate check for LeapYear was found. | test.cpp:12:7:12:11 | wYear | wYear | test.cpp:629:13:629:14 | st | st | -| test.cpp:642:5:642:9 | wYear | Field $@ on variable $@ has been modified, but no appropriate check for LeapYear was found. | test.cpp:12:7:12:11 | wYear | wYear | test.cpp:629:13:629:14 | st | st | -| test.cpp:718:5:718:9 | wYear | Field $@ on variable $@ has been modified, but no appropriate check for LeapYear was found. | test.cpp:12:7:12:11 | wYear | wYear | test.cpp:712:13:712:14 | st | st | -| test.cpp:731:5:731:9 | wYear | Field $@ on variable $@ has been modified, but no appropriate check for LeapYear was found. | test.cpp:12:7:12:11 | wYear | wYear | test.cpp:725:13:725:14 | st | st | -| test.cpp:732:5:732:9 | wYear | Field $@ on variable $@ has been modified, but no appropriate check for LeapYear was found. | test.cpp:12:7:12:11 | wYear | wYear | test.cpp:725:13:725:14 | st | st | -| test.cpp:733:5:733:9 | wYear | Field $@ on variable $@ has been modified, but no appropriate check for LeapYear was found. | test.cpp:12:7:12:11 | wYear | wYear | test.cpp:725:13:725:14 | st | st | +| 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/UncheckedReturnValueForTimeFunctions.expected b/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/UncheckedReturnValueForTimeFunctions.expected index fb79592b7f2d..ae8a55449daf 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,5 @@ -| test.cpp:317:2:317: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:63:1:63:20 | SystemTimeToFileTime | SystemTimeToFileTime | test.cpp:309:13:309:14 | st | st | -| test.cpp:330:2:330: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:63:1:63:20 | SystemTimeToFileTime | SystemTimeToFileTime | test.cpp:322:13:322:14 | st | st | -| test.cpp:341:2:341: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:63:1:63:20 | SystemTimeToFileTime | SystemTimeToFileTime | test.cpp:333:62:333:63 | st | st | -| test.cpp:720:2:720: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:63:1:63:20 | SystemTimeToFileTime | SystemTimeToFileTime | test.cpp:712:13:712:14 | st | st | -| test.cpp:735:2:735: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:63:1:63:20 | SystemTimeToFileTime | SystemTimeToFileTime | test.cpp:725:13:725:14 | st | st | +| 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 | 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 3db9b61edd2b..beb2c4061496 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 @@ -59,6 +59,18 @@ struct tm int tm_isdst; // daylight savings time flag }; +struct timespec +{ + time_t tv_sec; + long tv_nsec; +}; + +/* Timestamps of log entries. */ +struct logtime { + struct tm tm; + long usec; +}; + BOOL SystemTimeToFileTime( const SYSTEMTIME* lpSystemTime, @@ -102,6 +114,9 @@ TzSpecificLocalTimeToSystemTimeEx( void GetSystemTime( LPSYSTEMTIME lpSystemTime ); +void GetLocalTime( + LPSYSTEMTIME lpSystemTime +); void GetSystemTimeAsFileTime( LPFILETIME lpSystemTimeAsFileTime @@ -149,6 +164,12 @@ GetFileTime( LPFILETIME lpLastWriteTime ); +struct tm *localtime_r( const time_t *timer, struct tm *buf ); + +/** + * Negative Case + * FileTimeToSystemTime is called and the return value is checked +*/ void Correct_FileTimeToSystemTime(const FILETIME* lpFileTime) { SYSTEMTIME systemTime; @@ -162,6 +183,10 @@ void Correct_FileTimeToSystemTime(const FILETIME* lpFileTime) /// Normal usage } +/** + * Positive (Out of Scope) Bug Case + * FileTimeToSystemTime is called but no check is conducted to verify the result of the operation +*/ void AntiPattern_FileTimeToSystemTime(const FILETIME* lpFileTime) { SYSTEMTIME systemTime; @@ -170,6 +195,10 @@ void AntiPattern_FileTimeToSystemTime(const FILETIME* lpFileTime) FileTimeToSystemTime(lpFileTime, &systemTime); } +/** + * Negative Case + * SystemTimeToTzSpecificLocalTime is called and the return value is verified +*/ void Correct_SystemTimeToTzSpecificLocalTime(const TIME_ZONE_INFORMATION *lpTimeZoneInformation, const SYSTEMTIME *lpUniversalTime) { SYSTEMTIME localTime; @@ -183,6 +212,10 @@ void Correct_SystemTimeToTzSpecificLocalTime(const TIME_ZONE_INFORMATION *lpTime /// Normal usage } +/** + * Positive (Out of Scope) Case + * AntiPattern_SystemTimeToTzSpecificLocalTime is called but the return value is not validated +*/ void AntiPattern_SystemTimeToTzSpecificLocalTime(const TIME_ZONE_INFORMATION *lpTimeZoneInformation, const SYSTEMTIME *lpUniversalTime) { SYSTEMTIME localTime; @@ -191,6 +224,10 @@ void AntiPattern_SystemTimeToTzSpecificLocalTime(const TIME_ZONE_INFORMATION *lp SystemTimeToTzSpecificLocalTime(lpTimeZoneInformation, lpUniversalTime, &localTime); } +/** + * Negative Case + * SystemTimeToTzSpecificLocalTimeEx is called and the return value is validated +*/ void Correct_SystemTimeToTzSpecificLocalTimeEx(const DYNAMIC_TIME_ZONE_INFORMATION *lpTimeZoneInformation, const SYSTEMTIME *lpUniversalTime) { SYSTEMTIME localTime; @@ -204,6 +241,10 @@ void Correct_SystemTimeToTzSpecificLocalTimeEx(const DYNAMIC_TIME_ZONE_INFORMATI /// Normal usage } +/** + * Positive Case + * SystemTimeToTzSpecificLocalTimeEx is called but the return value is not validated +*/ void AntiPattern_SystemTimeToTzSpecificLocalTimeEx(const DYNAMIC_TIME_ZONE_INFORMATION *lpTimeZoneInformation, const SYSTEMTIME *lpUniversalTime) { SYSTEMTIME localTime; @@ -212,6 +253,10 @@ void AntiPattern_SystemTimeToTzSpecificLocalTimeEx(const DYNAMIC_TIME_ZONE_INFOR SystemTimeToTzSpecificLocalTimeEx(lpTimeZoneInformation, lpUniversalTime, &localTime); } +/** + * Negative Case + * Correct use of TzSpecificLocalTimeToSystemTime, function is called and the return value is validated. +*/ void Correct_TzSpecificLocalTimeToSystemTime(const TIME_ZONE_INFORMATION *lpTimeZoneInformation, const SYSTEMTIME *lpLocalTime) { SYSTEMTIME universalTime; @@ -225,6 +270,10 @@ void Correct_TzSpecificLocalTimeToSystemTime(const TIME_ZONE_INFORMATION *lpTime /// Normal usage } +/** + * Positive (Out of Scope) Case + * TzSpecificLocalTimeToSystemTime is called however the return value is not validated +*/ void AntiPattern_TzSpecificLocalTimeToSystemTime(const TIME_ZONE_INFORMATION *lpTimeZoneInformation, const SYSTEMTIME *lpLocalTime) { SYSTEMTIME universalTime; @@ -233,6 +282,10 @@ void AntiPattern_TzSpecificLocalTimeToSystemTime(const TIME_ZONE_INFORMATION *lp TzSpecificLocalTimeToSystemTime(lpTimeZoneInformation, lpLocalTime, &universalTime); } +/** + * Negative Case + * TzSpecificLocalTimeToSystemTimeEx is called and the return value is correctly validated +*/ void Correct_TzSpecificLocalTimeToSystemTimeEx(const DYNAMIC_TIME_ZONE_INFORMATION *lpTimeZoneInformation, const SYSTEMTIME *lpLocalTime) { SYSTEMTIME universalTime; @@ -246,6 +299,10 @@ void Correct_TzSpecificLocalTimeToSystemTimeEx(const DYNAMIC_TIME_ZONE_INFORMATI /// Normal usage } +/** + * Positive (Out of Scope) Case + * TzSpecificLocalTimeToSystemTimeEx is called however the return value is not validated +*/ void AntiPattern_TzSpecificLocalTimeToSystemTimeEx(const DYNAMIC_TIME_ZONE_INFORMATION *lpTimeZoneInformation, const SYSTEMTIME *lpLocalTime) { SYSTEMTIME universalTime; @@ -258,6 +315,10 @@ void AntiPattern_TzSpecificLocalTimeToSystemTimeEx(const DYNAMIC_TIME_ZONE_INFOR SYSTEMTIME Cases *************************************************/ +/** + * Negative Case + * SystemTimeToFileTime is called and the return value is validated in a guard +*/ void Correct_filetime_conversion_check(SYSTEMTIME& st) { FILETIME ft; @@ -273,6 +334,10 @@ void Correct_filetime_conversion_check(SYSTEMTIME& st) ////////////////////////////////////////////// +/** + * Positive (Out of Scope) Case + * SystemTimeToFileTime is called but the return value is not validated in a guard +*/ void AntiPattern_unchecked_filetime_conversion(SYSTEMTIME& st) { FILETIME ft; @@ -281,6 +346,10 @@ void AntiPattern_unchecked_filetime_conversion(SYSTEMTIME& st) SystemTimeToFileTime(&st, &ft); } +/** + * Positive (Out of Scope) Case + * SystemTimeToFileTime is called but the return value is not validated in a guard +*/ void AntiPattern_unchecked_filetime_conversion2(SYSTEMTIME* st) { FILETIME ft; @@ -292,6 +361,10 @@ void AntiPattern_unchecked_filetime_conversion2(SYSTEMTIME* st) } } +/** + * Positive (Out of Scope) + * SYSTEMTIME.wDay is incremented by one (and no guard exists) +*/ void AntiPattern_unchecked_filetime_conversion2() { SYSTEMTIME st; @@ -304,6 +377,11 @@ void AntiPattern_unchecked_filetime_conversion2() SystemTimeToFileTime(&st, &ft); } +/** + * Positive Cases + * - Anti-pattern 1: [year ±n, month, day] + * - Generic (Out of Scope) - UncheckedReturnValueForTimeFunctions +*/ void AntiPattern_unchecked_filetime_conversion2a() { SYSTEMTIME st; @@ -317,6 +395,11 @@ void AntiPattern_unchecked_filetime_conversion2a() SystemTimeToFileTime(&st, &ft); } +/** + * Positive Cases + * - Anti-pattern 1: [year ±n, month, day] + * - Generic (Out of Scope) - UncheckedReturnValueForTimeFunctions +*/ void AntiPattern_unchecked_filetime_conversion2b() { SYSTEMTIME st; @@ -330,6 +413,11 @@ void AntiPattern_unchecked_filetime_conversion2b() SystemTimeToFileTime(&st, &ft); } +/** + * Positive Cases + * - Anti-pattern 1: [year ±n, month, day] + * - Generic (Out of Scope) - UncheckedReturnValueForTimeFunctions +*/ void AntiPattern_unchecked_filetime_conversion2b(SYSTEMTIME* st) { FILETIME ft; @@ -341,6 +429,11 @@ void AntiPattern_unchecked_filetime_conversion2b(SYSTEMTIME* st) SystemTimeToFileTime(st, &ft); } +/** + * Positive Cases + * - Anti-pattern 3: datetime.AddDays(±28) + * - Generic (Out of Scope) - UncheckedReturnValueForTimeFunctions +*/ void AntiPattern_unchecked_filetime_conversion3() { SYSTEMTIME st; @@ -349,11 +442,12 @@ void AntiPattern_unchecked_filetime_conversion3() if (st.wMonth < 12) { + // Anti-pattern 3: datetime.AddDays(±28) st.wMonth++; } else { - // Check for leap year, but... + // No check for leap year is required here, as the month is statically set to January. st.wMonth = 1; st.wYear++; } @@ -363,6 +457,11 @@ void AntiPattern_unchecked_filetime_conversion3() } ////////////////////////////////////////////// + +/** + * Negative Case - Anti-pattern 1: [year ±n, month, day] + * Year is incremented and if we are on Feb the 29th, set to the 28th if the new year is a common year. +*/ void CorrectPattern_check1() { SYSTEMTIME st; @@ -370,7 +469,7 @@ void CorrectPattern_check1() st.wYear++; - // Guard + // Guard against February the 29th if (st.wMonth == 2 && st.wDay == 29) { // move back a day when landing on Feb 29 in an non-leap year @@ -385,6 +484,10 @@ void CorrectPattern_check1() AntiPattern_unchecked_filetime_conversion(st); } +/** + * Negative Case - Anti-pattern 1: [year ±n, month, day] + * Years is incremented by some integer and then the leap year case is correctly guarded and handled. +*/ void CorrectPattern_check2(int yearsToAdd) { SYSTEMTIME st; @@ -400,11 +503,18 @@ void CorrectPattern_check2(int yearsToAdd) AntiPattern_unchecked_filetime_conversion(st); } +/** + * Could give rise to AntiPattern 7: IsLeapYear (Conditional Logic) +*/ bool isLeapYear(SYSTEMTIME& st) { return st.wYear % 4 == 0 && (st.wYear % 100 != 0 || st.wYear % 400 == 0); } +/** + * Negative Case - Anti-pattern 1: [year ±n, month, day] + * Years is incremented by some integer and then the leap year case is correctly guarded and handled. +*/ void CorrectPattern_check3() { SYSTEMTIME st; @@ -413,6 +523,9 @@ void CorrectPattern_check3() st.wYear++; // Guard + /** Negative Case - Anti-pattern 7: IsLeapYear + * Body of conditional statement is safe recommended code + */ if (st.wMonth == 2 && st.wDay == 29 && isLeapYear(st)) { // move back a day when landing on Feb 29 in an non-leap year @@ -423,6 +536,9 @@ void CorrectPattern_check3() AntiPattern_unchecked_filetime_conversion(st); } +/** + * Could give rise to AntiPattern 7: IsLeapYear (Conditional Logic) +*/ bool isLeapYear2(int year) { return year % 4 == 0 && (year % 100 != 0 || year % 400 == 0); @@ -433,6 +549,10 @@ bool fixDate(int day, int month, int year) return (month == 2 && day == 29 && isLeapYear2(year)); } +/** + * Negative Case - Anti-pattern 1: [year ±n, month, day] + * Years is incremented by some integer and then the leap year case is correctly guarded and handled. +*/ void CorrectPattern_check4() { SYSTEMTIME st; @@ -442,18 +562,23 @@ void CorrectPattern_check4() st.wYear++; // Guard + /** Negative Case - Anti-pattern 7: IsLeapYear + * Body of conditional statement is safe recommended code + */ if (fixDate(st.wDay, st.wMonth, st.wYear)) { // move back a day when landing on Feb 29 in an non-leap year - st.wDay = 28; // GOOD [FALSE POSITIVE] + st.wDay = 28; // GOOD [FALSE POSITIVE] Anti-pattern 7 } // Safe to use AntiPattern_unchecked_filetime_conversion(st); } - - +/** + * Negative Case - Generic + * No manipulation is conducted on struct populated from GetSystemTime. +*/ void CorrectPattern_NotManipulated_DateFromAPI_0() { SYSTEMTIME st; @@ -464,6 +589,10 @@ void CorrectPattern_NotManipulated_DateFromAPI_0() SystemTimeToFileTime(&st, &ft); } +/** + * Negative Case - Generic + * No manipulation is conducted on struct populated from GetFileTime. +*/ void CorrectPattern_NotManipulated_DateFromAPI_1(HANDLE hWatchdog) { SYSTEMTIME st; @@ -475,18 +604,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. +*/ void AntiPattern_1_year_addition() { SYSTEMTIME st; GetSystemTime(&st); - // BUG - UncheckedLeapYearAfterYearModification - st.wYear++; + // BUG - UncheckedLeapYearAfterYearModification + st.wYear++; // BUg V2 // 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. +*/ void AntiPattern_simple_addition(int yearAddition) { SYSTEMTIME st; @@ -494,12 +631,16 @@ void AntiPattern_simple_addition(int yearAddition) GetSystemTime(&st); // BUG - UncheckedLeapYearAfterYearModification - st.wYear += yearAddition; + st.wYear += yearAddition; // Bug V2 // 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 *correctly*. +*/ void AntiPattern_IncorrectGuard(int yearsToAdd) { SYSTEMTIME st; @@ -511,7 +652,7 @@ void AntiPattern_IncorrectGuard(int yearsToAdd) // Incorrect Guard if (st.wMonth == 2 && st.wDay == 29) { - // Part of a different anti-pattern. + // Part of a different anti-pattern (AntiPattern 5). // Make sure the guard includes the proper check bool isLeapYear = st.wYear % 4 == 0; if (!isLeapYear) @@ -539,6 +680,10 @@ void CorrectUsageOf_mkgmtime(struct tm& timeinfo) /// _mkgmtime succeeded } +/** + * Positive Case - General (Out of Scope) + * Must Check for return value of _mkgmtime +*/ void AntiPattern_uncheckedUsageOf_mkgmtime(struct tm& timeinfo) { // (out-of-scope) GeneralBug: Must check return value for _mkgmtime @@ -550,6 +695,10 @@ void AntiPattern_uncheckedUsageOf_mkgmtime(struct tm& timeinfo) ////////////////////////////////////////////////////////// +/** + * Negative Case - Anti-pattern 1: [year ±n, month, day] + * Years is incremented by some integer and leap year is not handled correctly. +*/ void Correct_year_addition_struct_tm() { time_t rawtime; @@ -575,6 +724,10 @@ void Correct_year_addition_struct_tm() AntiPattern_uncheckedUsageOf_mkgmtime(timeinfo); } +/** + * Negative Case - Anti-pattern 1: [year ±n, month, day] + * Years is incremented by some integer and leap year is not handled correctly. +*/ void Correct_LinuxPattern() { time_t rawtime; @@ -596,6 +749,10 @@ void Correct_LinuxPattern() ////////////////////////////////////////// +/** + * Negative Case - Anti-pattern 1: [year ±n, month, day] + * Years is incremented by some integer and leap year is not handled correctly. +*/ void AntiPattern_year_addition_struct_tm() { time_t rawtime; @@ -603,7 +760,7 @@ void AntiPattern_year_addition_struct_tm() time(&rawtime); gmtime_s(&timeinfo, &rawtime); // BUG - UncheckedLeapYearAfterYearModification - timeinfo.tm_year++; + timeinfo.tm_year++; // Bug V2 // Usage of potentially invalid date CorrectUsageOf_mkgmtime(timeinfo); @@ -611,6 +768,10 @@ void AntiPattern_year_addition_struct_tm() ///////////////////////////////////////////////////////// +/** + * 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; @@ -623,6 +784,10 @@ void FalsePositiveTests(int 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) { struct tm timeinfo; @@ -631,106 +796,211 @@ void FalseNegativeTests(int x) timeinfo.tm_year = x; // BUG - UncheckedLeapYearAfterYearModification - timeinfo.tm_year = x + timeinfo.tm_year; + // Positive Case - Anti-pattern 1: [year ±n, month, day] + timeinfo.tm_year = x + timeinfo.tm_year; // Bug V2 // BUG - UncheckedLeapYearAfterYearModification - timeinfo.tm_year = 1970 + timeinfo.tm_year; + // Positive Case - Anti-pattern 1: [year ±n, month, day] + timeinfo.tm_year = 1970 + timeinfo.tm_year; // Bug V2 st.wYear = x; // BUG - UncheckedLeapYearAfterYearModification - st.wYear = x + st.wYear; + // Positive Case - Anti-pattern 1: [year ±n, month, day] + st.wYear = x + st.wYear; // Bug V2 // BUG - UncheckedLeapYearAfterYearModification - st.wYear = (1986 + st.wYear) - 1; + // Positive Case - Anti-pattern 1: [year ±n, month, day] + st.wYear = (1986 + st.wYear) - 1; // Bug V2 +} + +/** + * Positive AntiPattern 1 + * Year field is modified but via an intermediary variable. +*/ +bool tp_intermediaryVar(struct timespec now, struct logtime ×tamp_remote) +{ + struct tm tm_parsed; + bool timestamp_found = false; + + struct tm tm_now; + time_t t_now; + int year; + + timestamp_found = true; + + /* + * As the timestamp does not contain the year + * number, daylight saving time information, nor + * a time zone, attempt to infer it. Due to + * clock skews, the timestamp may even be part + * of the next year. Use the last year for which + * the timestamp is at most one week in the + * future. + * + * This loop can only run for at most three + * iterations before terminating. + */ + t_now = now.tv_sec; + localtime_r(&t_now, &tm_now); + + 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) + { + // assert(year >= tm_now.tm_year - 1); + timestamp_remote.tm.tm_year = year; + if (mktime(×tamp_remote.tm) < t_now + 7 * 24 * 60 * 60) + break; + } } -// False positive -inline void -IncrementMonth(LPSYSTEMTIME pst) -{ - if (pst->wMonth < 12) + + // False positive + inline void + IncrementMonth(LPSYSTEMTIME pst) { - pst->wMonth++; + if (pst->wMonth < 12) + { + pst->wMonth++; + } + else + { + pst->wMonth = 1; + pst->wYear++; + } } - else + + ///////////////////////////////////////////////////////// + + void mkDateTest(int year) { - pst->wMonth = 1; - pst->wYear++; + struct tm t; + + t.tm_sec = 0; + t.tm_min = 0; + t.tm_hour = 0; + t.tm_mday = 1; // day of the month - [1, 31] + t.tm_mon = 0; // months since January - [0, 11] + if (year >= 1900) + { + // 4-digit year + t.tm_year = year - 1900; // GOOD + } + else if ((year >= 0) && (year < 100)) + { + // 2-digit year assumed in the range 2000 - 2099 + t.tm_year = year + 100; // GOOD [FALSE POSITIVE] + } + else + { + // fail + } + // ... } -} -///////////////////////////////////////////////////////// + /** + * Negative Case - Anti-pattern 1a: [a.year, b.month, b.day] + * False positive: No modification of SYSTEMTIME struct. + */ + void unmodified1() + { + SYSTEMTIME st; + FILETIME ft; + WORD w; -void mkDateTest(int year) -{ - struct tm t; + GetSystemTime(&st); - t.tm_sec = 0; - t.tm_min = 0; - t.tm_hour = 0; - t.tm_mday = 1; // day of the month - [1, 31] - t.tm_mon = 0; // months since January - [0, 11] - if (year >= 1900) - { - // 4-digit year - t.tm_year = year - 1900; // GOOD - } else if ((year >= 0) && (year < 100)) { - // 2-digit year assumed in the range 2000 - 2099 - t.tm_year = year + 100; // GOOD [FALSE POSITIVE] - } else { - // fail + w = st.wYear; + + SystemTimeToFileTime(&st, &ft); // GOOD - no modification } - // ... -} -void unmodified1() -{ - SYSTEMTIME st; - FILETIME ft; - WORD w; + /** + * Negative Case - Anti-pattern 1a: [a.year, b.month, b.day] + * False positive: No modification of SYSTEMTIME struct. + */ + void unmodified2() + { + SYSTEMTIME st; + FILETIME ft; + WORD *w_ptr; - GetSystemTime(&st); + GetSystemTime(&st); - w = st.wYear; + w_ptr = &(st.wYear); - SystemTimeToFileTime(&st, &ft); // GOOD - no modification -} + SystemTimeToFileTime(&st, &ft); // GOOD - no modification + } -void unmodified2() -{ - SYSTEMTIME st; - FILETIME ft; - WORD *w_ptr; + /** + * Positive Case - Anti-pattern 1: [year ±n, month, day] + * Modification of SYSTEMTIME struct adding to year but no leap year guard is conducted. + */ + void modified3() + { + SYSTEMTIME st; + FILETIME ft; + WORD *w_ptr; - GetSystemTime(&st); + GetSystemTime(&st); - w_ptr = &(st.wYear); + st.wYear = st.wYear + 1; // BAD - SystemTimeToFileTime(&st, &ft); // GOOD - no modification -} + SystemTimeToFileTime(&st, &ft); + } -void modified3() -{ - SYSTEMTIME st; - FILETIME ft; - WORD *w_ptr; + /** + * Positive Case - Anti-pattern 1: [year ±n, month, day] + * Modification of SYSTEMTIME struct adding to year but no leap year guard is conducted. + */ + void modified4() + { + SYSTEMTIME st; + FILETIME ft; + WORD *w_ptr; - GetSystemTime(&st); + GetSystemTime(&st); - st.wYear = st.wYear + 1; // BAD + st.wYear++; // BAD Positive Case - Anti-pattern 1: [year ±n, month, day] - SystemTimeToFileTime(&st, &ft); -} + SystemTimeToFileTime(&st, &ft); + } -void modified4() -{ - SYSTEMTIME st; - FILETIME ft; - WORD *w_ptr; + /** + * Negative Case - Anti-pattern 1: [year ±n, month, day] + * Modification of SYSTEMTIME struct adding to year but no leap year guard is conducted. + */ + void modified5() + { + SYSTEMTIME st; + FILETIME ft; + WORD *w_ptr; - GetSystemTime(&st); + GetSystemTime(&st); - st.wYear++; // BAD - st.wYear++; // BAD - st.wYear++; // BAD + st.wYear++; // Negative Case - Anti-pattern 1: [year ±n, month, day], guard condition below. - SystemTimeToFileTime(&st, &ft); -} + if (SystemTimeToFileTime(&st, &ft)) + { + ///... + } + } + + struct tm ltime(void) + { + SYSTEMTIME st; + struct tm tm; + bool isLeapYear; + + GetLocalTime(&st); + tm.tm_sec=st.wSecond; + tm.tm_min=st.wMinute; + tm.tm_hour=st.wHour; + tm.tm_mday=st.wDay; + tm.tm_mon=st.wMonth-1; + tm.tm_year=(st.wYear>=1900?st.wYear-1900:0); + + // Check for leap year, and adjust the date accordingly + isLeapYear = tm.tm_year % 4 == 0 && (tm.tm_year % 100 != 0 || tm.tm_year % 400 == 0); + tm.tm_mday = tm.tm_mon == 2 && tm.tm_mday == 29 && !isLeapYear ? 28 : tm.tm_mday; + return tm; + } diff --git a/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UnsafeArrayForDaysOfYear/GlobalFp.cpp b/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UnsafeArrayForDaysOfYear/GlobalFp.cpp new file mode 100644 index 000000000000..abed05719fa6 --- /dev/null +++ b/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UnsafeArrayForDaysOfYear/GlobalFp.cpp @@ -0,0 +1,2 @@ +int NormalYear[365]; +int LeapYear[366]; diff --git a/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UnsafeArrayForDaysOfYear/UnsafeArrayForDaysOfYear.expected b/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UnsafeArrayForDaysOfYear/UnsafeArrayForDaysOfYear.expected index 37dd8b1ae7d0..59a981aa3a8f 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UnsafeArrayForDaysOfYear/UnsafeArrayForDaysOfYear.expected +++ b/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UnsafeArrayForDaysOfYear/UnsafeArrayForDaysOfYear.expected @@ -1,3 +1,4 @@ -| test.cpp:17:6:17:10 | items | There is an array allocation with a hard-coded set of 365 elements, which may indicate the number of days in a year without considering leap year scenarios. | -| test.cpp:25:15:25:26 | new[] | There is an array allocation with a hard-coded set of 365 elements, which may indicate the number of days in a year without considering leap year scenarios. | -| test.cpp:52:20:52:23 | call to vector | There is a 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. | +| test.cpp:20:6:20:10 | items | $@: There is an array allocation with a hard-coded set of 365 elements, which may indicate the number of days in a year without considering leap year scenarios. | test.cpp:20:6:20:10 | items | items | +| test.cpp:31:15:31:26 | new[] | $@: There is an array allocation with a hard-coded set of 365 elements, which may indicate the number of days in a year without considering leap year scenarios. | test.cpp:28:6:28:21 | ArrayOfDays_Bug2 | ArrayOfDays_Bug2 | +| test.cpp:68:20:68:23 | call to vector | $@: There is a 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. | test.cpp:65:6:65:21 | VectorOfDays_Bug | VectorOfDays_Bug | +| test.cpp:115:7:115:15 | items_bad | $@: There is an array allocation with a hard-coded set of 365 elements, which may indicate the number of days in a year without considering leap year scenarios. | test.cpp:115:7:115:15 | items_bad | items_bad | diff --git a/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UnsafeArrayForDaysOfYear/test.cpp b/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UnsafeArrayForDaysOfYear/test.cpp index 7f6f2cfd3fe7..32a0f59ac6f8 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UnsafeArrayForDaysOfYear/test.cpp +++ b/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UnsafeArrayForDaysOfYear/test.cpp @@ -11,6 +11,9 @@ class vector { const T& operator[](int idx) const { return _x; } }; +/** + * AntiPattern 4 - Static allocation of 365 array items +*/ void ArrayOfDays_Bug(int dayOfYear, int x) { // BUG @@ -19,6 +22,9 @@ void ArrayOfDays_Bug(int dayOfYear, int x) items[dayOfYear - 1] = x; } +/** + * AntiPattern 4 - Static allocation of 365 array items +*/ void ArrayOfDays_Bug2(int dayOfYear, int x) { // BUG @@ -28,7 +34,10 @@ void ArrayOfDays_Bug2(int dayOfYear, int x) delete items; } - +/** + * True Negative + * Correct conditional allocation of array length +*/ void ArrayOfDays_Correct(unsigned long year, int dayOfYear, int x) { bool isLeapYear = year % 4 == 0 && (year % 100 != 0 || year % 400 == 0); @@ -39,6 +48,10 @@ void ArrayOfDays_Correct(unsigned long year, int dayOfYear, int x) delete[] items; } +/** + * True Negative + * Allocation of 366 items (Irregardless of common or leap year) +*/ void ArrayOfDays_FalsePositive(int dayOfYear, int x) { int items[366]; @@ -46,6 +59,9 @@ void ArrayOfDays_FalsePositive(int dayOfYear, int x) items[dayOfYear - 1] = x; } +/** + * AntiPattern 4 - Static allocation of 365 array items +*/ void VectorOfDays_Bug(int dayOfYear, int x) { // BUG @@ -54,6 +70,10 @@ void VectorOfDays_Bug(int dayOfYear, int x) items[dayOfYear - 1] = x; } +/** + * True Negative + * Conditional quantity allocation on the basis of common or leap year +*/ void VectorOfDays_Correct(unsigned long year, int dayOfYear, int x) { bool isLeapYear = year % 4 == 0 && (year % 100 != 0 || year % 400 == 0); @@ -62,9 +82,66 @@ void VectorOfDays_Correct(unsigned long year, int dayOfYear, int x) items[dayOfYear - 1] = x; } +/** + * True Negative + * Allocation of 366 items (Irregardless of common or leap year) +*/ void VectorOfDays_FalsePositive(int dayOfYear, int x) { vector items(366); items[dayOfYear - 1] = x; } + +/** + * AntiPattern 4 - Static allocation of 365 array items +*/ +void HandleBothCases(int dayOfYear, int x) +{ + vector items(365); + vector items_leap(366); + + items[dayOfYear - 1] = x; // BUG +} + +/** + * AntiPattern 4 - Static allocation of 365 array items +*/ +void HandleBothCases2(int dayOfYear, int x) +{ + int items[365]; + int items_leap[366]; + + char items_bad[365]; // BUG + + items[dayOfYear - 1] = x; // BUG +} + +const short LeapYearDayToMonth[366] = { + 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, // January + 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, // February + 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, // March + 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, // April + 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, // May + 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, // June + 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, // July + 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, // August + 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, // September + 9, 9, 9, 9, 9, 9, 9, 9, 9, 9, 9, 9, 9, 9, 9, 9, 9, 9, 9, 9, 9, 9, 9, 9, 9, 9, 9, 9, 9, 9, 9, // October + 10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10, // November + 11,11,11,11,11,11,11,11,11,11,11,11,11,11,11,11,11,11,11,11,11,11,11,11,11,11,11,11,11,11,11}; // December + +/* Negative - #947 Sibling definition above*/ +const short NormalYearDayToMonth[365] = { + 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, // January + 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, // February + 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, // March + 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, // April + 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, // May + 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, // June + 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, // July + 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, // August + 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, // September + 9, 9, 9, 9, 9, 9, 9, 9, 9, 9, 9, 9, 9, 9, 9, 9, 9, 9, 9, 9, 9, 9, 9, 9, 9, 9, 9, 9, 9, 9, 9, // October + 10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10, // November + 11,11,11,11,11,11,11,11,11,11,11,11,11,11,11,11,11,11,11,11,11,11,11,11,11,11,11,11,11,11,11}; // December \ No newline at end of file