Add analyzers for leap year readiness CA2260&CA2261 #3152

kwilkins · 2020-01-06T22:26:40Z

Changes associated with Proposed Analyzer: Leap Year Readiness #2937

Leap year date analyzers added

DoNotIncrementYearValueInDateConstructor : CA2260
DoNotUserYearValueFromDifferentDateInDateConstructor : CA2261

added DoNotUseYearValueFromDifferentDateInDateConstructor

disabled leap year analyzers by default

dnfclas · 2020-01-06T22:26:53Z

All CLA requirements met.

codecov · 2020-01-07T00:54:10Z

Codecov Report

Merging #3152 into master will increase coverage by <.01%.
The diff coverage is 99.89%.

@@            Coverage Diff             @@
##           master    #3152      +/-   ##
==========================================
+ Coverage   94.13%   94.14%   +<.01%     
==========================================
  Files         967      971       +4     
  Lines      209646   212660    +3014     
  Branches    10451    12108    +1657     
==========================================
+ Hits       197353   200210    +2857     
+ Misses      11407    11174     -233     
- Partials      886     1276     +390

mavasani · 2020-01-08T01:15:33Z

FYI: You will need to move the added analyzer files to a different folder and project, Microsoft.CodeAnalysis.NetAnalyzers, once #3165 is merged. See #3139 (comment) for details.

I will try to review this PR tomorrow. Thanks!

mattjohnsonpint · 2020-01-08T18:54:37Z

Thanks for the great work on this @kwilkins !

@mavasani - Would it be possible to somehow mark the rest of CA2262 - CA2269 range (or larger) as "reserved for future use"? I can see that we might want to submit additional analyzers in this space in the future. It would be good to keep them grouped sequentially if possible. Thanks.

mavasani · 2020-01-08T19:18:59Z

Would it be possible to somehow mark the rest of CA2262 - CA2269 range (or larger) as "reserved for future use"?

@mj1856 I would recommend updating the ranges in

roslyn-analyzers/src/Utilities/Compiler/DiagnosticCategoryAndIdRanges.txt

Lines 5 to 15 in cf0425b

    
           # FxCop rules 
        
           Design: CA1000-CA1099, CA2210 
        
           Globalization: CA1300-CA1399, CA2101 
        
           Mobility: CA1600-CA1699 
        
           Performance: CA1800-CA1899, HA 
        
           Security: CA2100-CA2199, CA2300-CA2399, CA3000-CA3147, CA5300-CA5499 
        
           Usage: CA1801, CA1806, CA1816, CA2200-CA2209, CA2211-CA2299 
        
           Naming: CA1700-CA1799 
        
           Maintainability: CA1500-CA1599 
        
           Reliability: CA2000-CA2099, CA9999 
        
           Documentation: CA1200-CA1299

to explicitly introduce a gap for the IDs you want to reserve, say Usage: CA1801, CA1806, CA1816, CA2200-CA2209, CA2211-CA2262, CA2269-CA2299. Please also add a comment about the rules that need that reserved range. Note that this text file follows a specific format and we have an analyzer that enforces the rule category to rule ID range mapping specified in this file. Let me know if you need help in updating this file appropriately to get your desired reservation.

mavasani · 2020-01-10T18:55:04Z

src/Microsoft.NetCore.Analyzers/Core/LeapYear/DoNotIncrementYearValueInDateConstructor.cs

+        {
+            context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.Analyze);
+            context.EnableConcurrentExecution();
+            context.RegisterSemanticModelAction(semanticModelAnalysisContext =>


Please don't register this action - there are performance issues associated with it. I think you need to move to using IOperation API and RegisterOperationAction for all semantic analysis of executable code.

mavasani · 2020-01-10T19:05:08Z

...rosoft.NetCore.Analyzers/UnitTests/LeapYear/DoNotIncrementYearValueInDateConstructorTests.cs

+        }
+
+        [Fact]
+        public async Task YearIncrementVariableCases()


Tagging @dotpaul

Looking at this test case, it seems like you essentially want to do a version of TaintedDataFlowAnalysis, where an expression is considered tainted if it is a binary operation with one of the operands as a year/month/day of a DateTime variable and other operand is a literal. We already have large number of Taint analysis based rules in the repo, and I think you should build on top of that framework instead of attempting to write your pseudo-dataflow analysis (done in DateKindSemanticModel.cs and DateKindContext.cs at start of this PR).

@dotpaul Your thoughts? If you agree, might be good for you guys to sync in person.

Note that if you don't want to handle the DFA cases in this test, and just want the cases in prior tests to work, which might be a reasonable place to start, then you can write an extremely simple IOperation based analyzer (possible 20-30 lines) that can handle those cases. I'd let you guys decide if you want the simpler analyzer that just analyzes arguments or a dataflow analysis based analyzer.

As an example, consider the below SQL injection test based on TaintedDataFlowAnalysis:

roslyn-analyzers/src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Security/ReviewCodeForSqlInjectionVulnerabilitiesTests.cs

Lines 1924 to 1962 in 79c2638

[Fact]

public void CrossBinary_TaintedObject_Property_ConstructedInput()

{

VerifyCSharpWithDependencies(@"

namespace VulnerableWebApp

{

using System;

using System.Data;

using System.Data.SqlClient;

using System.Linq;

using System.Web;

using System.Web.UI;

using OtherDll;

public partial class WebForm : System.Web.UI.Page

{

protected void Page_Load(object sender, EventArgs e)

{

string taintedInput = this.Request[""input""];

OtherDllClass<string> otherDllObj = new OtherDllClass<string>(taintedInput);

string sqlCommandText = ""SELECT * FROM users WHERE username = '"" + otherDllObj.ConstructedInput + ""'"";

ExecuteSql(sqlCommandText);

}

protected void ExecuteSql(string sqlCommandText)

{

SqlCommand sqlCommand = new SqlCommand()

{

CommandText = sqlCommandText,

CommandType = CommandType.Text,

};

}

}

}",

GetCSharpResultAt(29, 17, 16, 35, "string SqlCommand.CommandText", "void WebForm.ExecuteSql(string sqlCommandText)", "string HttpRequest.this[string key]", "void WebForm.Page_Load(object sender, EventArgs e)"));

}

It has a variable holding tainted data below as the string has a concatenation from an unknown source:

roslyn-analyzers/src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Security/ReviewCodeForSqlInjectionVulnerabilitiesTests.cs

Line 1946 in 79c2638

string sqlCommandText = ""SELECT * FROM users WHERE username = '"" + otherDllObj.ConstructedInput + ""'"";

This tainted value gets passed to a method call as argument:

roslyn-analyzers/src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Security/ReviewCodeForSqlInjectionVulnerabilitiesTests.cs

Line 1948 in 79c2638

ExecuteSql(sqlCommandText);

Then it gets passed to the SqlCommand constructor in the invoked method:

roslyn-analyzers/src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Security/ReviewCodeForSqlInjectionVulnerabilitiesTests.cs

Lines 1953 to 1957 in 79c2638

SqlCommand sqlCommand = new SqlCommand()

{

CommandText = sqlCommandText,

CommandType = CommandType.Text,

};

The constructor gets flagged by this unit test.

IMO, this test shows how the analysis required for this analyzer closely matches the one done for SQL injection analyzer and can re-use almost all of the taint analysis dataflow framework.

TaintedDataAnalysis would need a bit of modification to treat integer values that get modified as a source.

I think the larger question is, are there other cases would we want to handle that dataflow analysis would help with it? E.g. dunno if something like this would be a case worth trying to find:

int y = originalDateTime.Year + 1; int year = y; DateTime newDateTime = new DateTime(year, originalDateTime.Month, originalDateTime.Day);

(There's also tracking the month and day values for determining if it's February 29, so that's another place where dataflow analysis might be helpful [but even more work].)

But all that being said, dataflow analysis isn't easy, so unless there are harder cases you really want to catch and you have time to work on it, I'd avoid it for now.

@dotpaul - are you suggesting that the analyzer be currently written as a simple IOperation analyzer that just flags violations where arithmetic is done in the callsite argument and avoid any DFA based cases? I think that is indeed a reasonable starting point and I am fine with that approach. I am lot more concerned with us attempting to write another pseudo-dataflow analysis solution, which should be avoided.

I'm saying I'll defer to @kwilkins as what to do for DFA. :) I dunno if tainted data analysis will meet all the needs. Agree it'd be better to avoid pseudo-DFA. I'd probably consider starting an entirely new analysis myself, given I had the time, but I haven't looked at all the cases.

mavasani

I think this analyzer should be rewritten using the tainted dataflow analysis written by @dotpaul

mattjohnsonpint · 2020-02-07T20:20:27Z

@mavasani - Thanks for the feedback. While we agree, unfortunately we do not have resources to rewrite this to use tainted dataflow analysis in time for it to be released and usable before leap day.

We will still consider working on this to prevent leap year bugs over the long term, as we will have another leap year in 2024, 2028, etc.

mavasani · 2020-02-07T20:29:42Z

@mj1856 That sounds reasonable. Would you be willing to simplify the analyzer as per https://github.com/dotnet/roslyn-analyzers/pull/3152/files#r365386559, so you don't perform any dataflow analysis at all, i.e. the first version of analyzer only supports analyzing values at arguments to call sites, but not flowing data values across statements? If so, you can delete large portion of the code being added in this PR, and then when you are ready to enhance the analyzer with dataflow analysis, you can work with @dotpaul to come with an appropriate DFA implementation.

mavasani · 2020-02-07T20:46:22Z

Note about the merge conflicts: You will need to move your code to under https://github.com/dotnet/roslyn-analyzers/tree/master/src/NetAnalyzers/Core. The repo has been refactored since this PR was created.

mavasani · 2020-03-28T00:52:22Z

FYI: You will need to fix a new RS diagnostic once #3444 goes in.

mavasani · 2021-05-24T07:06:36Z

Is this PR still actively being worked upon? If not, can we close it and reopen once ready?

mattjohnsonpint · 2021-05-24T15:32:27Z

It is not being worked on presently. The need still exists though.

You can close this. I hope to (at some point) make another one that does dataflow analysis.

kwilkins added 5 commits November 26, 2019 13:16

added DoNotIncrementYearValueInDateConstructor

3a49b38

added DoNotUseYearValueFromDifferentDateInDateConstructor

added initial basic tests

0b6c40e

added year increment pattern test cases

6923690

added tests for date part overflow pattern

fd69afc

changed rule ids

3963558

disabled leap year analyzers by default

fixed C# 8 nullable syntax

23f5215

kwilkins marked this pull request as ready for review January 7, 2020 19:28

kwilkins requested a review from a team as a code owner January 7, 2020 19:28

improved code coverage

d9733cf

mavasani self-assigned this Jan 8, 2020

mavasani reviewed Jan 10, 2020

View reviewed changes

mavasani suggested changes Jan 10, 2020

View reviewed changes

carlossanlop mentioned this pull request Oct 28, 2020

Do not do math with local DateTime dotnet/runtime#33764

Open

Base automatically changed from master to main March 5, 2021 02:20

mavasani closed this May 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add analyzers for leap year readiness CA2260&CA2261 #3152

Add analyzers for leap year readiness CA2260&CA2261 #3152

kwilkins commented Jan 6, 2020

dnfclas commented Jan 6, 2020 •

edited

Loading

codecov bot commented Jan 7, 2020 •

edited

Loading

mavasani commented Jan 8, 2020 •

edited

Loading

mattjohnsonpint commented Jan 8, 2020

mavasani commented Jan 8, 2020

mavasani Jan 10, 2020

mavasani Jan 10, 2020

mavasani Jan 10, 2020

mavasani Jan 10, 2020

dotpaul Jan 13, 2020

mavasani Jan 21, 2020

dotpaul Jan 22, 2020

mavasani left a comment

mattjohnsonpint commented Feb 7, 2020

mavasani commented Feb 7, 2020

mavasani commented Feb 7, 2020

mavasani commented Mar 28, 2020

mavasani commented May 24, 2021

mattjohnsonpint commented May 24, 2021

	[Fact]
	public void CrossBinary_TaintedObject_Property_ConstructedInput()
	{
	VerifyCSharpWithDependencies(@"
	namespace VulnerableWebApp
	{
	using System;
	using System.Data;
	using System.Data.SqlClient;
	using System.Linq;
	using System.Web;
	using System.Web.UI;
	using OtherDll;

	public partial class WebForm : System.Web.UI.Page
	{
	protected void Page_Load(object sender, EventArgs e)
	{
	string taintedInput = this.Request[""input""];

	OtherDllClass<string> otherDllObj = new OtherDllClass<string>(taintedInput);

	string sqlCommandText = ""SELECT * FROM users WHERE username = '"" + otherDllObj.ConstructedInput + ""'"";

	ExecuteSql(sqlCommandText);
	}

	protected void ExecuteSql(string sqlCommandText)
	{
	SqlCommand sqlCommand = new SqlCommand()
	{
	CommandText = sqlCommandText,
	CommandType = CommandType.Text,
	};
	}
	}
	}",
	GetCSharpResultAt(29, 17, 16, 35, "string SqlCommand.CommandText", "void WebForm.ExecuteSql(string sqlCommandText)", "string HttpRequest.this[string key]", "void WebForm.Page_Load(object sender, EventArgs e)"));
	}

Add analyzers for leap year readiness CA2260&CA2261 #3152

Add analyzers for leap year readiness CA2260&CA2261 #3152

Conversation

kwilkins commented Jan 6, 2020

dnfclas commented Jan 6, 2020 • edited Loading

codecov bot commented Jan 7, 2020 • edited Loading

Codecov Report

mavasani commented Jan 8, 2020 • edited Loading

mattjohnsonpint commented Jan 8, 2020

mavasani commented Jan 8, 2020

mavasani Jan 10, 2020

Choose a reason for hiding this comment

mavasani Jan 10, 2020

Choose a reason for hiding this comment

mavasani Jan 10, 2020

Choose a reason for hiding this comment

mavasani Jan 10, 2020

Choose a reason for hiding this comment

dotpaul Jan 13, 2020

Choose a reason for hiding this comment

mavasani Jan 21, 2020

Choose a reason for hiding this comment

dotpaul Jan 22, 2020

Choose a reason for hiding this comment

mavasani left a comment

Choose a reason for hiding this comment

mattjohnsonpint commented Feb 7, 2020

mavasani commented Feb 7, 2020

mavasani commented Feb 7, 2020

mavasani commented Mar 28, 2020

mavasani commented May 24, 2021

mattjohnsonpint commented May 24, 2021

dnfclas commented Jan 6, 2020 •

edited

Loading

codecov bot commented Jan 7, 2020 •

edited

Loading

mavasani commented Jan 8, 2020 •

edited

Loading