Skip to content

Commit d36a4df

Browse files
bdrodesbdrodes
committed
Misc. FP fixes.
1 parent 6c103a9 commit d36a4df

4 files changed

Lines changed: 203 additions & 35 deletions

File tree

‎cpp/ql/src/Likely Bugs/Leap Year/LeapYear.qll‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -535,7 +535,9 @@ class TimeConversionFunction extends Function {
535535
"SystemTimeToTzSpecificLocalTimeEx", "TzSpecificLocalTimeToSystemTime",
536536
"TzSpecificLocalTimeToSystemTimeEx", "RtlLocalTimeToSystemTime",
537537
"RtlTimeToSecondsSince1970", "_mkgmtime", "SetSystemTime", "SystemTimeToVariantTime",
538-
"VariantTimeToSystemTime", "mktime", "_mktime32", "_mktime64"
538+
"VariantTimeToSystemTime", "mktime", "_mktime32", "_mktime64", "VarUdateFromDate"
539539
]
540+
or
541+
this.getQualifiedName().matches("GetDateFormat%")
540542
}
541543
}

‎cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification.ql‎

Lines changed: 106 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,21 @@ class IgnorableFunction extends Function {
5757
// A postgres function for local time conversions
5858
// conversion operations (from one time structure to another) are generally ignorable
5959
this.getName() = "localsub"
60+
or
61+
// Hijri years are not applicable for gregorian leap year checks
62+
this.getName().toLowerCase().matches("%hijri%")
63+
or
64+
this.getFile().getBaseName().toLowerCase().matches("%hijri%")
65+
or
66+
// Persian calendar conversions are not applicable for gregorian leap year checks
67+
this.getName().toLowerCase().matches("%persian%")
68+
or
69+
this.getFile().getBaseName().toLowerCase().matches("%persian%")
70+
// or
71+
// // Functions with "convert" in the name are often conversion functions
72+
// // This is a very broad heuristic to handle a few observed false positives
73+
// // involving conversions with Hijri years.
74+
// this.getName().toLowerCase().matches("%convert%")
6075
}
6176
}
6277

@@ -123,17 +138,21 @@ predicate isLikelyConversionConstant(int c) {
123138
i = 2400000 or // MJD → JDN conversion
124139
i = 2400001 or // alt MJD → JDN conversion
125140
i = 2141 or // fixed‑point month/day extraction
126-
i = 2000 or // observed in some conversions
127141
i = 65536 or // observed in some conversions
128142
i = 7834 or // observed in some conversions
129143
i = 256 or // observed in some conversions
130-
i = 1900 or // struct tm base‑year offset; harmless
131-
i = 1899 or // Observed in uses with 1900 to address off by one scenarios
132144
i = 292275056 or // qdatetime.h Qt Core year range first year constant
133145
i = 292278994 or // qdatetime.h Qt Core year range last year constant
134146
i = 1601 or // Windows FILETIME epoch start year
135147
i = 1970 or // Unix epoch start year
136148
i = 70 or // Unix epoch start year short form
149+
i = 1899 or // Observed in uses with 1900 to address off by one scenarios
150+
i = 1900 or // Used when converting a 2 digit year
151+
i = 2000 or // Used when converting a 2 digit year
152+
i = 1400 or // Hijri base year, used when converting a 2 digit year
153+
i = 1980 or // FAT filesystem epoch start year
154+
i = 227013 or // constant observed for Hirji year conversion, and Hirji years are not applicable for gregorian leap year
155+
i = 10631 or // constant observed for Hirji year conversion, and Hirji years are not applicable for gregorian leap year
137156
i = 0
138157
)
139158
}
@@ -375,6 +394,12 @@ module OperationToYearAssignmentConfig implements DataFlow::ConfigSig {
375394
or
376395
n.asExpr() instanceof IgnorableOperation
377396
or
397+
// Hijri or Persian years are not applicable for gregorian leap year checks
398+
exists(Variable v |
399+
v.getName().toLowerCase().matches(["%hijri%, %persian%"]) and
400+
v.getAnAccess() = [n.asIndirectExpr(), n.asExpr()]
401+
)
402+
or
378403
isLeapYearCheckSink(n)
379404
}
380405

@@ -404,7 +429,17 @@ module YearFieldAccessToLeapYearCheckConfig implements DataFlow::ConfigSig {
404429

405430
predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
406431
// flow from a YearFieldAccess to the qualifier
407-
node2.asExpr() = node1.asExpr().(YearFieldAccess).getQualifier()
432+
node2.asExpr() = node1.asExpr().(YearFieldAccess).getQualifier*()
433+
or
434+
// Pass through any intermediate struct
435+
exists(Assignment a, DataFlow::PostUpdateNode pun |
436+
a.getLValue().(YearFieldAccess).getQualifier*() = pun.getPreUpdateNode().asExpr() and
437+
a.getRValue() = node1.asExpr() and
438+
node2.asExpr() = a.getLValue().(YearFieldAccess).getQualifier*()
439+
)
440+
or
441+
// flow from a year access qualifier to a year field
442+
exists(YearFieldAccess yfa | node2.asExpr() = yfa and node1.asExpr() = yfa.getQualifier())
408443
}
409444

410445
/**
@@ -428,7 +463,7 @@ predicate isYearModifiedWithCheck(YearFieldAccess fa) {
428463
// assume the leap year is being handled.
429464
exists(TimeStructToCheckedTimeConversionFlow::PathNode timeQualSrc |
430465
timeQualSrc.isSource() and
431-
timeQualSrc.getNode().asExpr() = fa.getQualifier()
466+
timeQualSrc.getNode().asExpr() = fa.getQualifier*()
432467
)
433468
// or
434469
// isUsedInFeb29Check(fa)
@@ -491,9 +526,13 @@ module TimeStructToCheckedTimeConversionConfig implements DataFlow::StateConfigS
491526
predicate isSink(DataFlow::Node sink, FlowState state) {
492527
state = true and
493528
(
494-
exists(IfStmt ifs | ifs.getCondition().getAChild*() = sink.asExpr())
529+
exists(IfStmt ifs | ifs.getCondition().getAChild*() = [sink.asExpr(), sink.asIndirectExpr()])
530+
or
531+
exists(ConditionalExpr ce |
532+
ce.getCondition().getAChild*() = [sink.asExpr(), sink.asIndirectExpr()]
533+
)
495534
or
496-
exists(ConditionalExpr ce | ce.getCondition().getAChild*() = sink.asExpr())
535+
exists(Loop l | l.getCondition().getAChild*() = [sink.asExpr(), sink.asIndirectExpr()])
497536
)
498537
}
499538

@@ -504,11 +543,26 @@ module TimeStructToCheckedTimeConversionConfig implements DataFlow::StateConfigS
504543
state2 = true and
505544
exists(Call c |
506545
c.getTarget() instanceof TimeConversionFunction and
507-
c.getAnArgument().getAChild*() = node1.asExpr() and
546+
c.getAnArgument().getAChild*() = [node1.asExpr(), node1.asIndirectExpr()] and
508547
node2.asExpr() = c
509548
)
510549
}
511550

551+
predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
552+
// flow from a YearFieldAccess to the qualifier
553+
node2.asExpr() = node1.asExpr().(YearFieldAccess).getQualifier*()
554+
or
555+
// Pass through any intermediate struct
556+
exists(Assignment a, DataFlow::PostUpdateNode pun |
557+
a.getLValue().(YearFieldAccess).getQualifier*() = pun.getPreUpdateNode().asExpr() and
558+
a.getRValue() = node1.asExpr() and
559+
node2.asExpr() = a.getLValue().(YearFieldAccess).getQualifier*()
560+
)
561+
or
562+
// flow from a year access qualifier to a year field
563+
exists(YearFieldAccess yfa | node2.asExpr() = yfa and node1.asExpr() = yfa.getQualifier())
564+
}
565+
512566
DataFlow::FlowFeature getAFeature() { result instanceof DataFlow::FeatureHasSourceCallContext }
513567
}
514568

@@ -543,7 +597,7 @@ module ParameterToLeapYearCheckConfig implements DataFlow::ConfigSig {
543597

544598
predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
545599
// flow from a YearFieldAccess to the qualifier
546-
node2.asExpr() = node1.asExpr().(YearFieldAccess).getQualifier()
600+
node2.asExpr() = node1.asExpr().(YearFieldAccess).getQualifier*()
547601
or
548602
// flow from a year access qualifier to a year field
549603
exists(YearFieldAccess yfa | node2.asExpr() = yfa and node1.asExpr() = yfa.getQualifier())
@@ -656,35 +710,56 @@ class LeapYearGuardCondition extends GuardCondition {
656710
}
657711
}
658712

713+
/**
714+
* A difficult case to detect is if a year modification is tied to a month modification
715+
* and the month is safe for leap year.
716+
* e.g.,
717+
* year++;
718+
* month = 1;
719+
* ... values eventually used in the same time struct
720+
* If this is even more challenging if the struct the values end up in are not
721+
* local (set inter-procedurally).
722+
* This flow flows constants 1-12 (but not 2 as this likely means february) to a month assignment.
723+
* It is assumed a user of this flow will check if the month/day source and month/day sink
724+
* are in the same basic blocks as a year modification source and a year modification sink.
725+
* It is also assumed a user will check if the constant source is a value that is ignorable
726+
* e.g., if it is 2 and the sink is a month assignment, then it isn't ignorable.
727+
*
728+
* Obviously this does not handle all conditions (e.g., the month set in another block).
729+
* It is meant to capture the most common cases of false positives.
730+
*/
731+
module NonFebConstantToMonthAssignmentConfig implements DataFlow::ConfigSig {
732+
predicate isSource(DataFlow::Node source) {
733+
source.asExpr().getValue().toInt() in [1 .. 12] and
734+
source.asExpr().getValue().toInt() != 2
735+
}
736+
737+
predicate isSink(DataFlow::Node sink) {
738+
exists(Assignment a |
739+
a.getLValue() instanceof MonthFieldAccess and
740+
a.getRValue() = sink.asExpr()
741+
)
742+
}
743+
}
744+
745+
// NOTE: only data flow here (no taint tracking) as we want the exact
746+
// constant flowing to the month assignment
747+
module NonFebConstantToMonthAssignmentFlow =
748+
DataFlow::Global<NonFebConstantToMonthAssignmentConfig>;
749+
659750
import OperationToYearAssignmentFlow::PathGraph
660751

661752
from OperationToYearAssignmentFlow::PathNode src, OperationToYearAssignmentFlow::PathNode sink
662753
where
663754
OperationToYearAssignmentFlow::flowPath(src, sink) and
664755
not isYearModifiedWithCheck(sink.getNode().asExpr().(YearFieldAssignment).getYearFieldAccess()) and
665756
not isControlledByMonthEqualityCheckNonFebruary(sink.getNode().asExpr()) and
666-
// TODO: this is an older heuristic that should be reassessed.
667-
// The heuristic stipulates that if a month or day is set to a constant in the local flow up to or after
668-
// a modified year's sink, then assume the user is knowingly handling the conversion correctly.
669-
// There is no interpretation of the assignment of the month/day or consideration for what branch the assignment is on.
670-
// Merely if the assignment of a constant day/month occurs anywhere, the year modification can likely
671-
// be ignored.
672-
not exists(FieldAccess mfa, AssignExpr ae, YearFieldAccess yfa, int val, Variable var |
673-
mfa instanceof MonthFieldAccess or mfa instanceof DayFieldAccess
674-
|
675-
yfa = sink.getNode().asExpr().(YearFieldAssignment).getYearFieldAccess() and
676-
var.getAnAccess() = yfa.getQualifier() and
677-
mfa.getQualifier() = var.getAnAccess() and
678-
mfa.isModified() and
679-
(
680-
mfa.getBasicBlock() = yfa.getBasicBlock().getASuccessor*() or
681-
yfa.getBasicBlock() = mfa.getBasicBlock().getASuccessor+()
682-
) and
683-
ae = mfa.getEnclosingElement() and
684-
ae.getAnOperand().getValue().toInt() = val and
685-
// If the constant looks like it relates to february, then there still might be an error
686-
// so only look for any other constant.
687-
not val in [2, 28, 29]
757+
// Check if a month is set in the same block as the year operation source
758+
// to a month that would be considered safe.
759+
not exists(DataFlow::Node monthValSrc, DataFlow::Node monthValSink |
760+
NonFebConstantToMonthAssignmentFlow::flow(monthValSrc, monthValSink) and
761+
monthValSrc.asExpr().getBasicBlock() = src.getNode().asExpr().getBasicBlock() and
762+
monthValSink.asExpr().getBasicBlock() = sink.getNode().asExpr().getBasicBlock()
688763
)
689764
select sink, src, sink,
690765
"Year field has been modified, but no appropriate check for LeapYear was found."

‎cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/UncheckedLeapYearAfterYearModification.expected‎

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,8 +6,6 @@
66
| test.cpp:769:22:769:23 | 80 | test.cpp:769:22:769:23 | 80 | test.cpp:769:22:769:23 | 80 | Year field has been modified, but no appropriate check for LeapYear was found. |
77
| test.cpp:813:21:813:40 | ... + ... | test.cpp:813:21:813:40 | ... + ... | test.cpp:813:21:813:40 | ... + ... | Year field has been modified, but no appropriate check for LeapYear was found. |
88
| test.cpp:818:13:818:24 | ... + ... | test.cpp:818:13:818:24 | ... + ... | test.cpp:818:13:818:24 | ... + ... | Year field has been modified, but no appropriate check for LeapYear was found. |
9-
| test.cpp:857:33:857:36 | year | test.cpp:854:14:854:31 | ... + ... | test.cpp:857:33:857:36 | year | Year field has been modified, but no appropriate check for LeapYear was found. |
10-
| test.cpp:857:33:857:36 | year | test.cpp:854:35:854:40 | -- ... | test.cpp:857:33:857:36 | year | Year field has been modified, but no appropriate check for LeapYear was found. |
119
| test.cpp:954:14:954:25 | ... + ... | test.cpp:954:14:954:25 | ... + ... | test.cpp:954:14:954:25 | ... + ... | Year field has been modified, but no appropriate check for LeapYear was found. |
1210
| test.cpp:972:3:972:12 | ... ++ | test.cpp:972:3:972:12 | ... ++ | test.cpp:972:3:972:12 | ... ++ | Year field has been modified, but no appropriate check for LeapYear was found. |
1311
| test.cpp:1075:2:1075:11 | ... ++ | test.cpp:1075:2:1075:11 | ... ++ | test.cpp:1075:2:1075:11 | ... ++ | Year field has been modified, but no appropriate check for LeapYear was found. |
@@ -29,6 +27,11 @@ edges
2927
| test.cpp:1183:10:1183:15 | offset | test.cpp:1183:2:1183:15 | ... += ... | provenance | |
3028
| test.cpp:1202:2:1202:15 | ... += ... | test.cpp:1204:16:1204:19 | year | provenance | |
3129
| test.cpp:1202:10:1202:15 | offset | test.cpp:1202:2:1202:15 | ... += ... | provenance | |
30+
| test.cpp:1288:20:1288:23 | year | test.cpp:1291:14:1291:17 | year | provenance | |
31+
| test.cpp:1306:12:1306:17 | ... + ... | test.cpp:1288:20:1288:23 | year | provenance | |
32+
| test.cpp:1325:3:1325:20 | ... = ... | test.cpp:1327:12:1327:18 | yeartmp | provenance | |
33+
| test.cpp:1325:13:1325:20 | ... + ... | test.cpp:1325:3:1325:20 | ... = ... | provenance | |
34+
| test.cpp:1327:12:1327:18 | yeartmp | test.cpp:1288:20:1288:23 | year | provenance | |
3235
nodes
3336
| test.cpp:422:14:422:14 | 2 | semmle.label | 2 |
3437
| test.cpp:440:2:440:11 | ... ++ | semmle.label | ... ++ |
@@ -77,6 +80,18 @@ nodes
7780
| test.cpp:1257:16:1257:28 | ... + ... | semmle.label | ... + ... |
7881
| test.cpp:1263:16:1263:28 | ... + ... | semmle.label | ... + ... |
7982
| test.cpp:1277:14:1277:26 | ... + ... | semmle.label | ... + ... |
83+
| test.cpp:1288:20:1288:23 | year | semmle.label | year |
84+
| test.cpp:1291:14:1291:17 | year | semmle.label | year |
85+
| test.cpp:1301:15:1301:22 | ... + ... | semmle.label | ... + ... |
86+
| test.cpp:1306:12:1306:17 | ... + ... | semmle.label | ... + ... |
87+
| test.cpp:1315:15:1315:22 | ... + ... | semmle.label | ... + ... |
88+
| test.cpp:1325:3:1325:20 | ... = ... | semmle.label | ... = ... |
89+
| test.cpp:1325:13:1325:20 | ... + ... | semmle.label | ... + ... |
90+
| test.cpp:1327:12:1327:18 | yeartmp | semmle.label | yeartmp |
91+
| test.cpp:1340:15:1340:27 | ... + ... | semmle.label | ... + ... |
92+
| test.cpp:1353:13:1353:25 | ... + ... | semmle.label | ... + ... |
8093
subpaths
8194
testFailures
95+
| test.cpp:854:43:854:53 | // $ Source | Missing result: Source |
96+
| test.cpp:857:39:857:125 | // $ Alert[cpp/microsoft/public/leap-year/unchecked-after-arithmetic-year-modification] | Missing result: Alert[cpp/microsoft/public/leap-year/unchecked-after-arithmetic-year-modification] |
8297
| test.cpp:1075:2:1075:11 | ... ++ | Unexpected result: Alert |

‎cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/test.cpp‎

Lines changed: 77 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -819,7 +819,7 @@ void test(int x)
819819
}
820820

821821
/**
822-
* Positive AntiPattern 1
822+
* Positive AntiPattern 1 NOTE: historically considered positive but mktime checks year validity, needs re-assessment
823823
* Year field is modified but via an intermediary variable.
824824
*/
825825
bool tp_intermediaryVar(struct timespec now, struct logtime &timestamp_remote)
@@ -1285,4 +1285,80 @@ void indirect_time_conversion_check(WORD year, WORD offset){
12851285
bool x = (res == 0) ? true : false;
12861286
}
12871287

1288+
void set_time(WORD year, WORD month, WORD day){
1289+
SYSTEMTIME tmp;
1290+
1291+
tmp.wYear = year;
1292+
tmp.wMonth = month;
1293+
tmp.wDay = day;
1294+
}
1295+
1296+
void constant_month_on_year_modification1(WORD year, WORD offset, WORD month){
1297+
SYSTEMTIME tmp;
1298+
1299+
if(month++ > 12){
1300+
tmp.wMonth = 1;
1301+
tmp.wYear = year + 1;// OK since the year is incremented with a known non-leap year month change
1302+
}
1303+
1304+
if(month++ > 12){
1305+
1306+
set_time(year+1, 1, 1);// OK since the year is incremented with a known non-leap year month change
1307+
}
1308+
}
1309+
1310+
void constant_month_on_year_modification2(WORD year, WORD offset, WORD month){
1311+
SYSTEMTIME tmp;
1312+
1313+
if(month++ > 12){
1314+
tmp.wMonth = 1;
1315+
tmp.wYear = year + 1;// OK since the year is incremented with a known non-leap year month change
1316+
}
1317+
1318+
1319+
if(month++ > 12){
1320+
// some hueristics to detect a false positive here rely on variable names
1321+
// which is often consistent in the wild.
1322+
// This variant uses the variable names yeartmp and monthtmp
1323+
WORD yeartmp;
1324+
WORD monthtmp;
1325+
yeartmp = year + 1;
1326+
monthtmp = 1;
1327+
set_time(yeartmp, monthtmp, 1);// OK since the year is incremented with a known non-leap year month change
1328+
}
1329+
}
1330+
1331+
typedef struct parent_struct {
1332+
SYSTEMTIME t;
1333+
} PARENT_STRUCT;
1334+
1335+
1336+
1337+
void nested_time_struct(WORD year, WORD offset){
1338+
PARENT_STRUCT ps;
1339+
1340+
ps.t.wYear = year + offset; // OK, checked below
1341+
1342+
bool isLeap = isLeapYearRaw(ps.t.wYear);
1343+
1344+
if(isLeap){
1345+
// do something
1346+
}
1347+
}
1348+
1349+
void intermediate_time_struct(WORD year, WORD offset){
1350+
SYSTEMTIME tm, tm2;
1351+
FILETIME ftTime;
1352+
1353+
tm.wYear = year + offset;
1354+
1355+
tm2.wYear = tm.wYear;
1356+
1357+
1358+
while ( !SystemTimeToFileTime( &tm2, &ftTime ) )
1359+
{
1360+
/// handle error
1361+
}
1362+
1363+
}
12881364

0 commit comments

Comments
 (0)