Skip to content

Commit

Permalink
Merge remote-tracking branch 'upstream/2.9.x' into NullableReferenceT…
Browse files Browse the repository at this point in the history 
…ypes
  • Loading branch information
@mavasani
mavasani committed Nov 16, 2019
2 parents 9c1ee5b + 1f5001e commit 0339d37
Show file tree
Hide file tree
Showing 5 changed files with 120 additions and 70 deletions.
96 changes: 40 additions & 56 deletions src/Microsoft.NetCore.Analyzers/Core/Security/SourceTriggeredTaintedDataAnalyzerBase.cs
View file
Expand Up @@ -6,6 +6,7 @@
using System;
using System.Collections.Immutable;
using System.Linq;
using System.Threading;
using Analyzer.Utilities;
using Analyzer.Utilities.Extensions;
using Analyzer.Utilities.FlowAnalysis.Analysis.TaintedDataAnalysis;
Expand Down Expand Up @@ -53,7 +54,8 @@ public override void Initialize(AnalysisContext context)
context.RegisterCompilationStartAction(
(CompilationStartAnalysisContext compilationContext) =>
{
TaintedDataConfig taintedDataConfig = TaintedDataConfig.GetOrCreate(compilationContext.Compilation);
Compilation compilation = compilationContext.Compilation;
TaintedDataConfig taintedDataConfig = TaintedDataConfig.GetOrCreate(compilation);
TaintedDataSymbolMap<SourceInfo> sourceInfoSymbolMap = taintedDataConfig.GetSourceSymbolMap(this.SinkKind);
if (sourceInfoSymbolMap.IsEmpty)
{
Expand All @@ -70,16 +72,45 @@ public override void Initialize(AnalysisContext context)
operationBlockStartContext =>
{
ISymbol owningSymbol = operationBlockStartContext.OwningSymbol;
if (owningSymbol.IsConfiguredToSkipAnalysis(operationBlockStartContext.Options,
TaintedDataEnteringSinkDescriptor, operationBlockStartContext.Compilation, operationBlockStartContext.CancellationToken))
AnalyzerOptions options = operationBlockStartContext.Options;
CancellationToken cancellationToken = operationBlockStartContext.CancellationToken;
if (owningSymbol.IsConfiguredToSkipAnalysis(options, TaintedDataEnteringSinkDescriptor, compilation, cancellationToken))
{
return;
}
PointsToAnalysisResult pointsToResult = null;
bool pointsToComputed = false;
ValueContentAnalysisResult valueContentResult = null;
bool valueContentComputed = false;
ControlFlowGraph cfg = operationBlockStartContext.OperationBlocks.GetControlFlowGraph();
WellKnownTypeProvider wellKnownTypeProvider = WellKnownTypeProvider.GetOrCreate(compilation);
InterproceduralAnalysisConfiguration interproceduralAnalysisConfiguration = InterproceduralAnalysisConfiguration.Create(
options,
SupportedDiagnostics,
defaultInterproceduralAnalysisKind: InterproceduralAnalysisKind.ContextSensitive,
cancellationToken: cancellationToken);
Lazy<PointsToAnalysisResult> pointsToFactory = new Lazy<PointsToAnalysisResult>(
() =>
{
return PointsToAnalysis.TryGetOrComputeResult(
cfg,
owningSymbol,
options,
wellKnownTypeProvider,
interproceduralAnalysisConfiguration,
interproceduralAnalysisPredicateOpt: null);
});
Lazy<(PointsToAnalysisResult, ValueContentAnalysisResult)> valueContentFactory = new Lazy<(PointsToAnalysisResult, ValueContentAnalysisResult)>(
() =>
{
ValueContentAnalysisResult valuecontentAnalysisResult = ValueContentAnalysis.TryGetOrComputeResult(
cfg,
owningSymbol,
options,
wellKnownTypeProvider,
interproceduralAnalysisConfiguration,
out _,
out PointsToAnalysisResult p);
return (p, valuecontentAnalysisResult);
});
PooledHashSet<IOperation> rootOperationsNeedingAnalysis = PooledHashSet<IOperation>.GetInstance();
Expand Down Expand Up @@ -108,55 +139,8 @@ public override void Initialize(AnalysisContext context)
if (sourceInfoSymbolMap.IsSourceMethod(
invocationOperation.TargetMethod,
invocationOperation.Arguments,
new Lazy<PointsToAnalysisResult>(
() =>
{
if (!pointsToComputed)
{
pointsToResult = PointsToAnalysis.TryGetOrComputeResult(
cfg,
owningSymbol,
operationAnalysisContext.Options,
WellKnownTypeProvider.GetOrCreate(operationAnalysisContext.Compilation),
InterproceduralAnalysisConfiguration.Create(
operationAnalysisContext.Options,
SupportedDiagnostics,
defaultInterproceduralAnalysisKind: InterproceduralAnalysisKind.ContextSensitive,
cancellationToken: operationAnalysisContext.CancellationToken),
interproceduralAnalysisPredicateOpt: null);
pointsToComputed = true;
}
return pointsToResult;
}),
new Lazy<ValueContentAnalysisResult>(
() =>
{
if (!valueContentComputed)
{
valueContentResult = ValueContentAnalysis.TryGetOrComputeResult(
cfg,
owningSymbol,
operationAnalysisContext.Options,
WellKnownTypeProvider.GetOrCreate(operationAnalysisContext.Compilation),
InterproceduralAnalysisConfiguration.Create(
operationAnalysisContext.Options,
SupportedDiagnostics,
defaultInterproceduralAnalysisKind: InterproceduralAnalysisKind.ContextSensitive,
cancellationToken: operationAnalysisContext.CancellationToken),
out _,
out PointsToAnalysisResult p);
if (p != null && pointsToResult == null)
{
pointsToResult = p;
pointsToComputed = true;
}
valueContentComputed = true;
}
return valueContentResult;
}),
pointsToFactory,
valueContentFactory,
out _))
{
lock (rootOperationsNeedingAnalysis)
Expand Down
51 changes: 51 additions & 0 deletions src/Microsoft.NetCore.Analyzers/UnitTests/Security/DoNotHardCodeCertificateTests.cs
View file
Expand Up @@ -3,6 +3,7 @@
using System.Collections.Generic;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.Diagnostics;
using Test.Utilities;
using Xunit;
using Xunit.Abstractions;

Expand Down Expand Up @@ -383,6 +384,56 @@ public void TestMethod(byte[] bytes, string path)
}");
}

// Didn't find out what causes NRE.
[Fact, WorkItem(3012, "https://github.com/dotnet/roslyn-analyzers/issues/3012")]
public void Test_ExampleCodeFromTheIssue_NoDiagnostic()
{
VerifyCSharp(@"
using System;
using System.Globalization;
using System.IO;
using System.Security;
using System.Security.Cryptography;
using System.Security.Cryptography.X509Certificates;
using System.Text;
using System.Text.RegularExpressions;
class Constants
{
public static Regex UnhashedNameIdRegex = new Regex(@""^[a-zA-Z0-9]\d{2}[a-zA-Z0-9](-\d{3}){2}[A-Za-z0-9]$"");
}
class TestClass
{
public static string Calculate(string unhashedNameId)
{
if (string.IsNullOrWhiteSpace(unhashedNameId))
{
throw new ArgumentNullException(nameof(unhashedNameId), $""{ nameof(unhashedNameId)} must not be null, empty or whitespace."");
}
if (!Constants.UnhashedNameIdRegex.IsMatch(unhashedNameId))
{
throw new ArgumentException($""{ nameof(unhashedNameId)} does not match '{Constants.UnhashedNameIdRegex}'."", nameof(unhashedNameId));
}
using (var sha = new SHA256Managed())
{
byte[] textData = Encoding.UTF8.GetBytes(unhashedNameId);
byte[] crypto = sha.ComputeHash(textData);
var nameId = new StringBuilder();
foreach (byte hash in crypto)
{
nameId.Append(hash.ToString(""x2"", CultureInfo.InvariantCulture));
}
return nameId.ToString();
}
}
}");
}

protected override DiagnosticAnalyzer GetBasicDiagnosticAnalyzer()
{
return new DoNotHardCodeCertificate();
Expand Down
3 changes: 2 additions & 1 deletion src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/SourceInfo.cs
View file
Expand Up @@ -2,6 +2,7 @@

using System;
using System.Collections.Immutable;
using System.Linq;
using Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.PointsToAnalysis;
using Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.ValueContentAnalysis;
using Microsoft.CodeAnalysis.Operations;
Expand Down Expand Up @@ -140,7 +141,7 @@ internal class SourceInfo : ITaintedDataInfo, IEquatable<SourceInfo>
/// <summary>
/// Indicates that this <see cref="SourceInfo"/> uses <see cref="ValueContentAbstractValue"/>s.
/// </summary>
public bool RequiresValueContentAnalysis => this.TaintedMethodsNeedsValueContentAnalysis != null;
public bool RequiresValueContentAnalysis => this.TaintedMethodsNeedsValueContentAnalysis.Any();

public override int GetHashCode()
{
Expand Down
2 changes: 1 addition & 1 deletion ...wAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysis.TaintedDataOperationVisitor.cs
View file
Expand Up @@ -246,7 +246,7 @@ public override TaintedDataAbstractValue VisitObjectCreation(IObjectCreationOper
method,
visitedArguments,
new Lazy<PointsToAnalysisResult?>(() => DataFlowAnalysisContext.PointsToAnalysisResultOpt),
new Lazy<ValueContentAnalysisResult?>(() => DataFlowAnalysisContext.ValueContentAnalysisResultOpt),
new Lazy<(PointsToAnalysisResult?, ValueContentAnalysisResult?)>(() => (DataFlowAnalysisContext.PointsToAnalysisResultOpt, DataFlowAnalysisContext.ValueContentAnalysisResultOpt)),
out taintedTargets))
{
foreach (string taintedTarget in taintedTargets)
Expand Down
38 changes: 26 additions & 12 deletions .../FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataSymbolMapExtensions.cs
View file
Expand Up @@ -24,19 +24,21 @@ internal static class TaintedDataSymbolMapExtensions
/// <param name="sourceSymbolMap"></param>
/// <param name="method"></param>
/// <param name="arguments"></param>
/// <param name="pointsToAnalysisResult">If the method needs to do PointsToAnalysis, the PointsToAnalysis result will be produced by the passed value factory.</param>
/// <param name="valueContentAnalysisResult">If the method needs to do ValueContentAnalysis, the ValueContentAnalysis result will be produced by the passed value factory.</param>
/// <param name="pointsToFactory">If the method needs to do PointsToAnalysis, the PointsToAnalysis result will be produced by the passed value factory.</param>
/// <param name="valueContentFactory">If the method needs to do ValueContentAnalysis, the ValueContentAnalysis result will be produced by the passed value factory.</param>
/// <param name="allTaintedTargets"></param>
/// <returns></returns>
public static bool IsSourceMethod(
this TaintedDataSymbolMap<SourceInfo> sourceSymbolMap,
IMethodSymbol method,
ImmutableArray<IArgumentOperation> arguments,
Lazy<PointsToAnalysisResult?> pointsToAnalysisResult,
Lazy<ValueContentAnalysisResult?> valueContentAnalysisResult,
Lazy<PointsToAnalysisResult?> pointsToFactory,
Lazy<(PointsToAnalysisResult? p, ValueContentAnalysisResult? v)> valueContentFactory,
[NotNullWhen(returnValue: true)] out PooledHashSet<string>? allTaintedTargets)
{
allTaintedTargets = null;
PointsToAnalysisResult pointsToAnalysisResult = null;
ValueContentAnalysisResult valueContentAnalysisResult = null;
foreach (SourceInfo sourceInfo in sourceSymbolMap.GetInfosForType(method.ContainingType))
{
foreach ((MethodMatcher methodMatcher, ImmutableHashSet<string> taintedTargets) in sourceInfo.TaintedMethods)
Expand All @@ -54,16 +56,22 @@ internal static class TaintedDataSymbolMapExtensions

foreach ((MethodMatcher methodMatcher, ImmutableHashSet<(PointsToCheck pointsToCheck, string)> pointsToTaintedTargets) in sourceInfo.TaintedMethodsNeedsPointsToAnalysis)
{
if (methodMatcher(method.Name, arguments))
if (pointsToTaintedTargets.Any() && methodMatcher(method.Name, arguments))
{
pointsToAnalysisResult ??= pointsToFactory.Value;
if (pointsToAnalysisResult == null)
{
break;
}

IEnumerable<(PointsToCheck, string target)> positivePointsToTaintedTargets = pointsToTaintedTargets.Where(s =>
s.pointsToCheck(
arguments.Select(o =>
// TODO(dotpaul): Remove the below suppression.
#pragma warning disable CS8602 // Dereference of a possibly null reference.
pointsToAnalysisResult.Value[o.Kind, o.Syntax]).ToImmutableArray()));
pointsToAnalysisResult[o.Kind, o.Syntax]).ToImmutableArray()));
#pragma warning restore CS8602 // Dereference of a possibly null reference.
if (positivePointsToTaintedTargets != null)
if (positivePointsToTaintedTargets.Any())
{
if (allTaintedTargets == null)
{
Expand All @@ -77,17 +85,23 @@ internal static class TaintedDataSymbolMapExtensions

foreach ((MethodMatcher methodMatcher, ImmutableHashSet<(ValueContentCheck valueContentCheck, string)> valueContentTaintedTargets) in sourceInfo.TaintedMethodsNeedsValueContentAnalysis)
{
if (methodMatcher(method.Name, arguments))
if (valueContentTaintedTargets.Any() && methodMatcher(method.Name, arguments))
{
pointsToAnalysisResult ??= valueContentFactory.Value.p;
valueContentAnalysisResult ??= valueContentFactory.Value.v;
if (pointsToAnalysisResult == null || valueContentAnalysisResult == null)
{
break;
}

IEnumerable<(ValueContentCheck, string target)> positiveValueContentTaintedTargets = valueContentTaintedTargets.Where(s =>
s.valueContentCheck(
arguments.Select(o =>
// TODO(dotpaul): Remove the below suppression.
#pragma warning disable CS8602 // Dereference of a possibly null reference.
pointsToAnalysisResult.Value[o.Kind, o.Syntax]).ToImmutableArray(),
arguments.Select(o => valueContentAnalysisResult.Value[o.Kind, o.Syntax]).ToImmutableArray()));
arguments.Select(o => pointsToAnalysisResult[o.Kind, o.Syntax]).ToImmutableArray(),
arguments.Select(o => valueContentAnalysisResult[o.Kind, o.Syntax]).ToImmutableArray()));
#pragma warning restore CS8602 // Dereference of a possibly null reference.
if (positiveValueContentTaintedTargets != null)
if (positiveValueContentTaintedTargets.Any())
{
if (allTaintedTargets == null)
{
Expand Down

0 comments on commit 0339d37

Please sign in to comment.