From 59299b9cbd6dc2c0fa91ba392c2b9d7c54056b3b Mon Sep 17 00:00:00 2001 From: Andy Zivkovic Date: Mon, 17 Jul 2023 09:30:55 +0200 Subject: [PATCH] Fix NuGetAudit severity level mapping (#5313) --- .../RestoreCommand/Utility/AuditUtility.cs | 35 +- .../Utility/AuditUtilityTests.cs | 371 +++++++++++++++++- 2 files changed, 374 insertions(+), 32 deletions(-) diff --git a/src/NuGet.Core/NuGet.Commands/RestoreCommand/Utility/AuditUtility.cs b/src/NuGet.Core/NuGet.Commands/RestoreCommand/Utility/AuditUtility.cs index 62cb01299a8..0882b875506 100644 --- a/src/NuGet.Core/NuGet.Commands/RestoreCommand/Utility/AuditUtility.cs +++ b/src/NuGet.Core/NuGet.Commands/RestoreCommand/Utility/AuditUtility.cs @@ -72,6 +72,11 @@ public async Task CheckPackageVulnerabilitiesAsync(CancellationToken cancellatio stopwatch.Stop(); DownloadDurationSeconds = stopwatch.Elapsed.TotalSeconds; + if (allVulnerabilityData?.Exceptions is not null) + { + ReplayErrors(allVulnerabilityData.Exceptions); + } + if (allVulnerabilityData is null || !AnyVulnerabilityDataFound(allVulnerabilityData.KnownVulnerabilities)) { if (_auditEnabled == EnabledValue.ExplicitOptIn) @@ -84,12 +89,7 @@ public async Task CheckPackageVulnerabilitiesAsync(CancellationToken cancellatio return; } - if (allVulnerabilityData.Exceptions != null) - { - ReplayErrors(allVulnerabilityData.Exceptions); - } - - if (allVulnerabilityData.KnownVulnerabilities != null) + if (allVulnerabilityData.KnownVulnerabilities is not null) { CheckPackageVulnerabilities(allVulnerabilityData.KnownVulnerabilities); } @@ -119,7 +119,8 @@ private void ReplayErrors(AggregateException exceptions) foreach (Exception exception in exceptions.InnerExceptions) { var messageText = string.Format(Strings.Error_VulnerabilityDataFetch, exception.Message); - RestoreLogMessage logMessage = RestoreLogMessage.CreateError(NuGetLogCode.NU1900, messageText); + RestoreLogMessage logMessage = RestoreLogMessage.CreateWarning(NuGetLogCode.NU1900, messageText); + logMessage.ProjectPath = _projectFullPath; _logger.Log(logMessage); } } @@ -237,13 +238,13 @@ private static (string severityLabel, NuGetLogCode code) GetSeverityLabelAndCode { switch (severity) { - case 1: + case 0: return (Strings.Vulnerability_Severity_1, NuGetLogCode.NU1901); - case 2: + case 1: return (Strings.Vulnerability_Severity_2, NuGetLogCode.NU1902); - case 3: + case 2: return (Strings.Vulnerability_Severity_3, NuGetLogCode.NU1903); - case 4: + case 3: return (Strings.Vulnerability_Severity_4, NuGetLogCode.NU1904); default: return (Strings.Vulnerability_Severity_unknown, NuGetLogCode.NU1900); @@ -368,31 +369,31 @@ private int ParseAuditLevel() if (auditLevel == null) { - return 1; + return 0; } if (string.Equals("low", auditLevel, StringComparison.OrdinalIgnoreCase)) { - return 1; + return 0; } if (string.Equals("moderate", auditLevel, StringComparison.OrdinalIgnoreCase)) { - return 2; + return 1; } if (string.Equals("high", auditLevel, StringComparison.OrdinalIgnoreCase)) { - return 3; + return 2; } if (string.Equals("critical", auditLevel, StringComparison.OrdinalIgnoreCase)) { - return 4; + return 3; } string messageText = string.Format(Strings.Error_InvalidNuGetAuditLevelValue, auditLevel, "low, moderate, high, critical"); RestoreLogMessage message = RestoreLogMessage.CreateError(NuGetLogCode.NU1014, messageText); message.ProjectPath = _projectFullPath; _logger.Log(message); - return 1; + return 0; } internal enum NuGetAuditMode { Unknown, Direct, All } diff --git a/test/NuGet.Core.Tests/NuGet.Commands.Test/RestoreCommandTests/Utility/AuditUtilityTests.cs b/test/NuGet.Core.Tests/NuGet.Commands.Test/RestoreCommandTests/Utility/AuditUtilityTests.cs index b93efbf0a13..e72bd9a3c62 100644 --- a/test/NuGet.Core.Tests/NuGet.Commands.Test/RestoreCommandTests/Utility/AuditUtilityTests.cs +++ b/test/NuGet.Core.Tests/NuGet.Commands.Test/RestoreCommandTests/Utility/AuditUtilityTests.cs @@ -6,6 +6,7 @@ using System; using System.Collections.Generic; using System.Linq; +using System.Net.Http; using System.Threading; using System.Threading.Tasks; using FluentAssertions; @@ -26,6 +27,220 @@ namespace NuGet.Commands.Test.RestoreCommandTests.Utility; public class AuditUtilityTests { + private static Uri CveUrl = new Uri("https://cve.test/1"); + private static VersionRange UpToV2 = new VersionRange(maxVersion: new NuGetVersion(2, 0, 0), includeMaxVersion: false); + + [Fact] + public async Task Check_VulnerabilityProviderWithExceptions_WarningsReplayedToLogger() + { + // Arrange + var context = new AuditTestContext(); + var exception1 = new AggregateException(new HttpRequestException("404")); + context.WithVulnerabilityProvider().WithException(exception1); + var exception2 = new AggregateException(new HttpRequestException("401")); + context.WithVulnerabilityProvider().WithException(exception2); + + context.WithRestoreTarget(); + + // Act + _ = await context.CheckPackageVulnerabilitiesAsync(CancellationToken.None); + + // Assert + context.Log.LogMessages.Count.Should().Be(2); + context.Log.LogMessages.All(m => m.Code == NuGetLogCode.NU1900).Should().BeTrue(); + context.Log.LogMessages.All(m => m.ProjectPath == context.ProjectFullPath).Should().BeTrue(); + context.Log.LogMessages.Where(m => m.Message.Contains("404")).Should().ContainSingle(); + context.Log.LogMessages.Where(m => m.Message.Contains("401")).Should().ContainSingle(); + context.Log.LogMessages.All(m => m.Level == LogLevel.Warning).Should().BeTrue(); + } + + [Theory] + [InlineData("default", false)] + [InlineData("true", true)] + public async Task Check_WithNoVulnerabilitySources_NU1905Warning(string enable, bool expectWarning) + { + // Arrange + var context = new AuditTestContext(); + context.Enabled = enable; + + context.WithRestoreTarget(); + + // Act + var auditUtility = await context.CheckPackageVulnerabilitiesAsync(CancellationToken.None); + + // Assert + if (expectWarning) + { + context.Log.LogMessages.Count.Should().Be(1); + var message = (RestoreLogMessage)context.Log.LogMessages.Single(); + message.Code.Should().Be(NuGetLogCode.NU1905); + message.ProjectPath.Should().Be(context.ProjectFullPath); + message.Level.Should().Be(LogLevel.Warning); + } + else + { + context.Log.LogMessages.Count.Should().Be(0); + } + + // for perf, when we don't have data to check, we shouldn't waste time checking + auditUtility.DownloadDurationSeconds.Should().NotBeNull(); + auditUtility.CheckPackagesDurationSeconds.Should().BeNull(); + auditUtility.GenerateOutputDurationSeconds.Should().BeNull(); + } + + [Fact] + public async Task Check_ProjectWithoutVulnerablePackages_NoWarnings() + { + // Arrange + var context = new AuditTestContext(); + + var packageVulnerabilities = context.WithVulnerabilityProvider().WithPackageVulnerability("SomePackage"); + packageVulnerabilities.Add(new PackageVulnerabilityInfo(CveUrl, 1, UpToV2)); + + context.WithRestoreTarget(); + + // Act + var auditUtil = await context.CheckPackageVulnerabilitiesAsync(CancellationToken.None); + + // Assert + context.Log.LogMessages.Count.Should().Be(0); + + // time to output should be zero since there are no messages, for perf. + auditUtil.DownloadDurationSeconds.Should().NotBeNull(); + auditUtil.CheckPackagesDurationSeconds.Should().NotBeNull(); + auditUtil.GenerateOutputDurationSeconds.Should().BeNull(); + } + + [Theory] + [InlineData(0, NuGetLogCode.NU1901)] + [InlineData(1, NuGetLogCode.NU1902)] + [InlineData(2, NuGetLogCode.NU1903)] + [InlineData(3, NuGetLogCode.NU1904)] + [InlineData(4, NuGetLogCode.NU1900)] + public async Task Check_ProjectReferencingPackageWithVulnerability_WarningLogged(int severity, NuGetLogCode expectedCode) + { + // Arrange + var context = new AuditTestContext(); + context.Mode = "all"; + + var vulnerabilityProvider = context.WithVulnerabilityProvider(); + var knownVulnerabilities = vulnerabilityProvider.WithPackageVulnerability("pkga"); + knownVulnerabilities.Add( + new PackageVulnerabilityInfo( + CveUrl, + severity, + UpToV2)); + knownVulnerabilities = vulnerabilityProvider.WithPackageVulnerability("pkgb"); + knownVulnerabilities.Add( + new PackageVulnerabilityInfo( + CveUrl, + severity, + UpToV2)); + + context.WithRestoreTarget() + .DependsOn("pkga", "1.0.0"); + + context.PackagesDependencyProvider.Package("pkga", "1.0.0").DependsOn("pkgb", "1.0.0"); + context.PackagesDependencyProvider.Package("pkgb", "1.0.0"); + + // Act + var auditUtility = await context.CheckPackageVulnerabilitiesAsync(CancellationToken.None); + + // Assert + context.Log.LogMessages.Count.Should().Be(2); + + context.Log.LogMessages.Where(m => m.Message.Contains("pkga")).Should().NotBeNullOrEmpty(); + RestoreLogMessage message = (RestoreLogMessage)context.Log.LogMessages.Where(m => m.Message.Contains("pkga")).Single(); + ValidateRestoreLogMessage(message, "pkga", expectedCode, context); + + context.Log.LogMessages.Where(m => m.Message.Contains("pkgb")).Should().NotBeNullOrEmpty(); + message = (RestoreLogMessage)context.Log.LogMessages.Where(m => m.Message.Contains("pkgb")).Single(); + ValidateRestoreLogMessage(message, "pkgb", expectedCode, context); + + auditUtility.DownloadDurationSeconds.Should().NotBeNull(); + auditUtility.CheckPackagesDurationSeconds.Should().NotBeNull(); + auditUtility.GenerateOutputDurationSeconds.Should().NotBeNull(); + + auditUtility.DirectPackagesWithAdvisory.Should().NotBeNull(); + auditUtility.DirectPackagesWithAdvisory!.Should().BeEquivalentTo(new[] { "pkga" }); + + auditUtility.TransitivePackagesWithAdvisory.Should().NotBeNull(); + auditUtility.TransitivePackagesWithAdvisory!.Should().BeEquivalentTo(new[] { "pkgb" }); + + int expectedCount = severity == 0 ? 1 : 0; + auditUtility.Sev0DirectMatches.Should().Be(expectedCount); + auditUtility.Sev0TransitiveMatches.Should().Be(expectedCount); + + expectedCount = severity == 1 ? 1 : 0; + auditUtility.Sev1DirectMatches.Should().Be(expectedCount); + auditUtility.Sev1TransitiveMatches.Should().Be(expectedCount); + + expectedCount = severity == 2 ? 1 : 0; + auditUtility.Sev2DirectMatches.Should().Be(expectedCount); + auditUtility.Sev2TransitiveMatches.Should().Be(expectedCount); + + expectedCount = severity == 3 ? 1 : 0; + auditUtility.Sev3DirectMatches.Should().Be(expectedCount); + auditUtility.Sev3TransitiveMatches.Should().Be(expectedCount); + + expectedCount = severity < 0 || severity > 3 ? 1 : 0; + auditUtility.InvalidSevDirectMatches.Should().Be(expectedCount); + auditUtility.InvalidSevTransitiveMatches.Should().Be(expectedCount); + + static void ValidateRestoreLogMessage(RestoreLogMessage message, string packageId, NuGetLogCode expectedCode, AuditTestContext context) + { + message.Message.Should().Contain("1.0.0", "Message doesn't contain package version"); + message.Message.Should().Contain(CveUrl.OriginalString, "Message doesn't contain CVE URL"); + message.Code.Should().Be(expectedCode); + message.ProjectPath.Should().Be(context.ProjectFullPath); + message.LibraryId.Should().Be(packageId); + message.TargetGraphs.Should().BeEquivalentTo(new[] { "net6.0" }); + + } + } + + [Fact] + public async Task Check_TwoVulnerabilityProviders_MergesKnownVulnerabilities() + { + // Arrange + var context = new AuditTestContext(); + context.Mode = "all"; + + PackageVulnerabilityInfo commonKnownVulnerability = new PackageVulnerabilityInfo(CveUrl, severity: 1, UpToV2); + Uri cve2Url = new("https://cve.test/2"); + Uri cve3Url = new("https://cve.test/3"); + + // provider 1 knows about vulnerabilities 1 and 2 + var vulnerabilityProvider = context.WithVulnerabilityProvider(); + var knownVulnerabilities = vulnerabilityProvider.WithPackageVulnerability("pkga"); + knownVulnerabilities.Add(commonKnownVulnerability); + knownVulnerabilities.Add(new PackageVulnerabilityInfo(cve2Url, severity: 1, UpToV2)); + + // provider 2 knows about vulnerabilities 1 and 3 + vulnerabilityProvider = context.WithVulnerabilityProvider(); + knownVulnerabilities = vulnerabilityProvider.WithPackageVulnerability("pkga"); + knownVulnerabilities.Add(commonKnownVulnerability); + knownVulnerabilities.Add(new PackageVulnerabilityInfo(cve3Url, severity: 1, UpToV2)); + + context.WithRestoreTarget().DependsOn("pkga", "1.0.0"); + context.PackagesDependencyProvider.Package("pkga", "1.0.0"); + + // Act + var auditUtility = await context.CheckPackageVulnerabilitiesAsync(CancellationToken.None); + + // Assert + // the common cve both vulnerability providers know about should be deduplicated + context.Log.LogMessages.Count.Should().Be(3); + + List messages = new(3); + messages.AddRange(context.Log.LogMessages.Cast()); + + messages.All(m => m.LibraryId == "pkga").Should().BeTrue(); + messages.Any(m => m.Message.Contains(CveUrl.OriginalString)).Should().BeTrue(); + messages.Any(m => m.Message.Contains(cve2Url.OriginalString)).Should().BeTrue(); + messages.Any(m => m.Message.Contains(cve3Url.OriginalString)).Should().BeTrue(); + } + /// /// Diamond dependency pkga has a known vulnerability on the lower version, but none on the higher version. /// Therefore, no warnings or vulnerable packages should be detected. @@ -48,7 +263,9 @@ public async Task Check_RejectedTransitivePackageInGraphHasKnownVulnerability_No .DependsOn("pkgb", "1.0.0") .DependsOn("pkgc", "1.0.0"); - var pkgaVulnerabilities = context.WithPackageVulnerability("pkga"); + var pkgaVulnerabilities = context + .WithVulnerabilityProvider() + .WithPackageVulnerability("pkga"); pkgaVulnerabilities.Add( new PackageVulnerabilityInfo( new Uri("https://cve.test/cve1"), @@ -86,7 +303,9 @@ public async Task Check_TransitivePackageHasKnownVulnerability_WarningInAllMode( context.WithRestoreTarget() .DependsOn("pkgb", "1.0.0"); - var pkgaVulnerabilities = context.WithPackageVulnerability(vulnerablePackage); + var pkgaVulnerabilities = context + .WithVulnerabilityProvider() + .WithPackageVulnerability(vulnerablePackage); pkgaVulnerabilities.Add( new PackageVulnerabilityInfo( new Uri("https://cve.test/cve1"), @@ -115,6 +334,82 @@ public async Task Check_TransitivePackageHasKnownVulnerability_WarningInAllMode( auditUtility.TransitivePackagesWithAdvisory.Should().BeEquivalentTo(new[] { vulnerablePackage }); } + [Fact] + public async Task Check_MultiTargetingProjectFile_WarningsHaveExpectedProperties() + { + // Arrange + + DependencyProvider packageDependencyProvider = new(); + packageDependencyProvider.Package("pkga", "1.0.0"); + packageDependencyProvider.Package("pkgb", "1.0.0"); + + Task[] createGraphTasks = + { + CreateGraphAsync(packageDependencyProvider, "pkga", FrameworkConstants.CommonFrameworks.Net60), + CreateGraphAsync(packageDependencyProvider, "pkgb", FrameworkConstants.CommonFrameworks.Net50) + }; + + List vulnerabilityProviderContexts = new(1) + { + new VulnerabilityProviderTestContext() + }; + vulnerabilityProviderContexts[0].WithPackageVulnerability("pkga").Add(new PackageVulnerabilityInfo(CveUrl, 0, UpToV2)); + vulnerabilityProviderContexts[0].WithPackageVulnerability("pkgb").Add(new PackageVulnerabilityInfo(CveUrl, 0, UpToV2)); + + var vulnerabilityProviders = AuditTestContext.CreateVulnerabilityInformationProviders(vulnerabilityProviderContexts); + + RestoreTargetGraph[] graphs = + { + await createGraphTasks[0], + await createGraphTasks[1] + }; + + RestoreAuditProperties restoreAuditProperties = new() + { + EnableAudit = "default", + }; + + var log = new TestLogger(); + + // Act + var audit = new AuditUtility( + AuditUtility.ParseEnableValue(restoreAuditProperties.EnableAudit), + restoreAuditProperties, + "/path/proj.csproj", + graphs, + vulnerabilityProviders, + log); + await audit.CheckPackageVulnerabilitiesAsync(CancellationToken.None); + + // Assert + log.LogMessages.Count.Should().Be(2); + + RestoreLogMessage message = log.LogMessages.Cast().Single(m => m.LibraryId == "pkga"); + message.TargetGraphs.Should().BeEquivalentTo(new[] { "net6.0" }); + + message = log.LogMessages.Cast().Single(m => m.LibraryId == "pkgb"); + message.TargetGraphs.Should().BeEquivalentTo(new[] { "net5.0" }); + + static async Task CreateGraphAsync(DependencyProvider packageProvider, string dependencyId, NuGetFramework targetFramework) + { + DependencyProvider projectProvider = new(); + projectProvider.Package("proj", "1.0.0", LibraryType.Project).DependsOn(dependencyId, "1.0.0"); + + var walkContext = new TestRemoteWalkContext(); + walkContext.LocalLibraryProviders.Add(packageProvider); + walkContext.ProjectLibraryProviders.Add(projectProvider); + var walker = new RemoteDependencyWalker(walkContext); + + LibraryRange restoreTarget = new("proj", new VersionRange(NuGetVersion.Parse("1.0.0")), LibraryDependencyTarget.Project); + + var walkResult = await walker.WalkAsync(restoreTarget, targetFramework, "", RuntimeGraph.Empty, true); + + var graph = RestoreTargetGraph.Create(new[] { walkResult }, walkContext, NullLogger.Instance, targetFramework); + + return graph; + } + } + private class AuditTestContext { public string ProjectFullPath { get; set; } = RuntimeEnvironmentHelper.IsWindows ? @"n:\proj\proj.csproj" : "/src/proj/proj.csproj"; @@ -129,8 +424,11 @@ private class AuditTestContext private LibraryRange? _walkTarget; - private Dictionary> _knownVulnerabilities = new(); + private List? _vulnerabilityProviders; + /// + /// Set up the project that is being restored (not just a project reference) + /// public DependencyProvider.TestPackage WithRestoreTarget(string projectName = "proj", string version = "1.0.0") { if (_walkTarget != null) @@ -146,13 +444,16 @@ public DependencyProvider.TestPackage WithRestoreTarget(string projectName = "pr return testProject; } - public List WithPackageVulnerability(string packageId) + public VulnerabilityProviderTestContext WithVulnerabilityProvider() { - List packageVulnerabilities = new(); - - _knownVulnerabilities.Add(packageId, packageVulnerabilities); + if (_vulnerabilityProviders is null) + { + _vulnerabilityProviders = new(); + } - return packageVulnerabilities; + VulnerabilityProviderTestContext provider = new(); + _vulnerabilityProviders.Add(provider); + return provider; } public async Task CheckPackageVulnerabilitiesAsync(CancellationToken cancellationToken) @@ -179,9 +480,9 @@ public async Task CheckPackageVulnerabilitiesAsync(CancellationTok var graphs = await CreateGraphsAsync(); - var vulnProviders = CreateVulnerabilityInformationProviders(); + var vulnProviders = CreateVulnerabilityInformationProviders(_vulnerabilityProviders); - var audit = new AuditUtility(AuditUtility.EnabledValue.ImplicitOptIn, restoreAuditProperties, ProjectFullPath, graphs, vulnProviders, Log); + var audit = new AuditUtility(enabled, restoreAuditProperties, ProjectFullPath, graphs, vulnProviders, Log); await audit.CheckPackageVulnerabilitiesAsync(CancellationToken.None); return audit; @@ -203,19 +504,59 @@ async Task CreateGraphsAsync() return graphs; } + } + + public static List CreateVulnerabilityInformationProviders(List? providers) + { + List result = new(); - List CreateVulnerabilityInformationProviders() + if (providers is null) { - List>> allKnownVulnerabilities = new() { _knownVulnerabilities }; - GetVulnerabilityInfoResult getVulnerabilityInfoResult = new(allKnownVulnerabilities, exceptions: null); + return result; + } + foreach (var provider in providers) + { + List>>? knownVulnerabilities = + provider.KnownVulnerabilities is not null ? new() { provider.KnownVulnerabilities } : null; + GetVulnerabilityInfoResult getVulnerabilityInfoResult = new(knownVulnerabilities, provider.Exceptions); var vulnProvider = new Mock(); vulnProvider.Setup(p => p.GetVulnerabilityInformationAsync(It.IsAny())) .Returns(Task.FromResult(getVulnerabilityInfoResult)); - var vulnProviders = new List(1) { vulnProvider.Object }; + result.Add(vulnProvider.Object); + } + + return result; + } + } + + private class VulnerabilityProviderTestContext + { + public Dictionary>? KnownVulnerabilities { get; private set; } + public AggregateException? Exceptions { get; private set; } + + public List WithPackageVulnerability(string packageId) + { + List packageVulnerabilities = new(); + + if (KnownVulnerabilities is null) + { + KnownVulnerabilities = new(); + } + + KnownVulnerabilities.Add(packageId, packageVulnerabilities); - return vulnProviders; + return packageVulnerabilities; + } + + internal void WithException(AggregateException exceptions) + { + if (Exceptions is not null) + { + throw new InvalidOperationException("Vulnerability provider exceptions cannot be set more than once"); } + + Exceptions = exceptions; } } }