diff --git a/tracer/src/Datadog.Trace/Iast/IastModule.cs b/tracer/src/Datadog.Trace/Iast/IastModule.cs index 80b326d57874..ce7ced610bd4 100644 --- a/tracer/src/Datadog.Trace/Iast/IastModule.cs +++ b/tracer/src/Datadog.Trace/Iast/IastModule.cs @@ -60,6 +60,7 @@ internal static partial class IastModule private static readonly Func Always = (x) => true; private static readonly DbRecordManager DbRecords = new DbRecordManager(IastSettings); private static bool _showTimeoutExceptionError = true; + private static SourceType[] _dbSources = [SourceType.SqlRowValue]; internal static void LogTimeoutError(RegexMatchTimeoutException err) { @@ -74,29 +75,6 @@ internal static void LogTimeoutError(RegexMatchTimeoutException err) } } - internal static bool NoDbSource(TaintedObject tainted) - { - // Reject any vuln with all ranges coming from a db source - try - { - foreach (var range in tainted.Ranges) - { - if (range.Source?.Origin != SourceType.SqlRowValue) - { - return true; - } - } - - return false; - } - catch (Exception err) - { - Log.Error(err, "Error while checking for non Database tainted origins."); - } - - return true; - } - internal static string? OnUnvalidatedRedirect(string? evidence) { if (Iast.Instance.Settings.Enabled && evidence != null && OnUnvalidatedRedirect(evidence, IntegrationId.UnvalidatedRedirect).VulnerabilityAdded) @@ -176,7 +154,7 @@ internal static IastModuleResponse OnTrustBoundaryViolation(string name) } OnExecutedSinkTelemetry(IastInstrumentedSinks.TrustBoundaryViolation); - return GetScope(name, IntegrationId.TrustBoundaryViolation, VulnerabilityTypeName.TrustBoundaryViolation, OperationNameTrustBoundaryViolation, NoDbSource); + return GetScope(name, IntegrationId.TrustBoundaryViolation, VulnerabilityTypeName.TrustBoundaryViolation, OperationNameTrustBoundaryViolation, taintValidator: Always, safeSources: _dbSources); } catch (Exception ex) { @@ -195,7 +173,7 @@ internal static IastModuleResponse OnLdapInjection(string evidence) } OnExecutedSinkTelemetry(IastInstrumentedSinks.LdapInjection); - return GetScope(evidence, IntegrationId.Ldap, VulnerabilityTypeName.LdapInjection, OperationNameLdapInjection, NoDbSource); + return GetScope(evidence, IntegrationId.Ldap, VulnerabilityTypeName.LdapInjection, OperationNameLdapInjection, taintValidator: Always, safeSources: _dbSources); } catch (Exception ex) { @@ -214,7 +192,7 @@ internal static IastModuleResponse OnSSRF(string evidence) try { OnExecutedSinkTelemetry(IastInstrumentedSinks.Ssrf); - return GetScope(evidence, IntegrationId.Ssrf, VulnerabilityTypeName.Ssrf, OperationNameSsrf, NoDbSource, exclusionSecureMarks: SecureMarks.Ssrf); + return GetScope(evidence, IntegrationId.Ssrf, VulnerabilityTypeName.Ssrf, OperationNameSsrf, taintValidator: Always, safeSources: _dbSources, exclusionSecureMarks: SecureMarks.Ssrf); } catch (Exception ex) { @@ -252,7 +230,7 @@ public static IastModuleResponse OnPathTraversal(string evidence) try { OnExecutedSinkTelemetry(IastInstrumentedSinks.PathTraversal); - return GetScope(evidence, IntegrationId.PathTraversal, VulnerabilityTypeName.PathTraversal, OperationNamePathTraversal, NoDbSource); + return GetScope(evidence, IntegrationId.PathTraversal, VulnerabilityTypeName.PathTraversal, OperationNamePathTraversal, taintValidator: Always, safeSources: _dbSources); } catch (Exception ex) { @@ -310,7 +288,7 @@ public static IastModuleResponse OnCommandInjection(string file, string argument { OnExecutedSinkTelemetry(IastInstrumentedSinks.CommandInjection); var evidence = BuildCommandInjectionEvidence(file, argumentLine, argumentList); - return string.IsNullOrEmpty(evidence) ? IastModuleResponse.Empty : GetScope(evidence, integrationId, VulnerabilityTypeName.CommandInjection, OperationNameCommandInjection, NoDbSource); + return string.IsNullOrEmpty(evidence) ? IastModuleResponse.Empty : GetScope(evidence, integrationId, VulnerabilityTypeName.CommandInjection, OperationNameCommandInjection, taintValidator: Always, safeSources: _dbSources); } catch (Exception ex) { @@ -324,7 +302,7 @@ public static IastModuleResponse OnReflectionInjection(string param, Integration try { OnExecutedSinkTelemetry(IastInstrumentedSinks.ReflectionInjection); - return GetScope(param, integrationId, VulnerabilityTypeName.ReflectionInjection, OperationNameReflectionInjection, NoDbSource); + return GetScope(param, integrationId, VulnerabilityTypeName.ReflectionInjection, OperationNameReflectionInjection, taintValidator: Always, safeSources: _dbSources); } catch (Exception ex) { @@ -616,7 +594,17 @@ public static bool AddRequestVulnerabilitiesAllowed() return isRequest && traceContext?.IastRequestContext?.AddVulnerabilitiesAllowed() == true; } - private static IastModuleResponse GetScope(string evidenceValue, IntegrationId integrationId, string vulnerabilityType, string operationName, Func? taintValidator = null, bool addLocation = true, int? hash = null, StackTrace? externalStack = null, SecureMarks exclusionSecureMarks = SecureMarks.None) + private static IastModuleResponse GetScope( + string evidenceValue, + IntegrationId integrationId, + string vulnerabilityType, + string operationName, + Func? taintValidator = null, + bool addLocation = true, + int? hash = null, + StackTrace? externalStack = null, + SecureMarks exclusionSecureMarks = SecureMarks.None, + SourceType[]? safeSources = null) { var tracer = Tracer.Instance; if (!IastSettings.Enabled || !tracer.Settings.IsIntegrationEnabled(integrationId)) @@ -653,10 +641,17 @@ private static IastModuleResponse GetScope(string evidenceValue, IntegrationId i } } - // Contains at least one range that is not safe (when analyzing a vulnerability that can have secure marks) - if (exclusionSecureMarks != SecureMarks.None && !Ranges.ContainsUnsafeRange(tainted?.Ranges)) + var ranges = tainted?.Ranges; + if (ranges is not null) { - return IastModuleResponse.Empty; + var unsafeRanges = Ranges.GetUnsafeRanges(ranges, exclusionSecureMarks, safeSources); + if (unsafeRanges is null || unsafeRanges.Length == 0) + { + return IastModuleResponse.Empty; + } + + // Contains at least one range that is not safe (when analyzing a vulnerability that can have secure marks) + ranges = unsafeRanges; } var location = addLocation ? GetLocation(externalStack, currentSpan) : null; @@ -665,10 +660,9 @@ private static IastModuleResponse GetScope(string evidenceValue, IntegrationId i return IastModuleResponse.Empty; } - var unsafeRanges = Ranges.UnsafeRanges(tainted?.Ranges, exclusionSecureMarks); var vulnerability = (hash is null) ? - new Vulnerability(vulnerabilityType, location, new Evidence(evidenceValue, unsafeRanges), integrationId) : - new Vulnerability(vulnerabilityType, (int)hash, location, new Evidence(evidenceValue, unsafeRanges), integrationId); + new Vulnerability(vulnerabilityType, location, new Evidence(evidenceValue, ranges), integrationId) : + new Vulnerability(vulnerabilityType, (int)hash, location, new Evidence(evidenceValue, ranges), integrationId); if (!IastSettings.DeduplicationEnabled || HashBasedDeduplication.Instance.Add(vulnerability)) { @@ -801,7 +795,7 @@ internal static void OnHeaderInjection(IntegrationId integrationId, string heade var evidence = StringAspects.Concat(headerName, HeaderInjectionEvidenceSeparator, headerValue); var hash = ("HEADER_INJECTION:" + headerName).GetStaticHashCode(); - GetScope(evidence, integrationId, VulnerabilityTypeName.HeaderInjection, OperationNameHeaderInjection, NoDbSource, false, hash); + GetScope(evidence, integrationId, VulnerabilityTypeName.HeaderInjection, OperationNameHeaderInjection, taintValidator: Always, safeSources: _dbSources, addLocation: false, hash: hash); } internal static IastModuleResponse OnXpathInjection(string xpath) @@ -814,7 +808,7 @@ internal static IastModuleResponse OnXpathInjection(string xpath) try { OnExecutedSinkTelemetry(IastInstrumentedSinks.XPathInjection); - return GetScope(xpath, IntegrationId.XpathInjection, VulnerabilityTypeName.XPathInjection, OperationNameXPathInjection, NoDbSource); + return GetScope(xpath, IntegrationId.XpathInjection, VulnerabilityTypeName.XPathInjection, OperationNameXPathInjection, taintValidator: Always, safeSources: _dbSources); } catch (Exception ex) { @@ -844,8 +838,8 @@ internal static void OnEmailHtmlInjection(object? message) return; } - // We use the same secure marks as XSS - GetScope(messageDuck.Body, IntegrationId.EmailHtmlInjection, VulnerabilityTypeName.EmailHtmlInjection, OperationNameEmailHtmlInjection, taintValidator: NoDbSource, exclusionSecureMarks: SecureMarks.Xss); + // We use the same secure marks as XSS, but excluding db sources + GetScope(messageDuck.Body, IntegrationId.EmailHtmlInjection, VulnerabilityTypeName.EmailHtmlInjection, OperationNameEmailHtmlInjection, taintValidator: Always, safeSources: _dbSources, exclusionSecureMarks: SecureMarks.Xss); } internal static void RegisterDbRecord(object instance) diff --git a/tracer/src/Datadog.Trace/Iast/Range.cs b/tracer/src/Datadog.Trace/Iast/Range.cs index c4804f3a42fd..09e13a66f7ae 100644 --- a/tracer/src/Datadog.Trace/Iast/Range.cs +++ b/tracer/src/Datadog.Trace/Iast/Range.cs @@ -8,6 +8,7 @@ using System; using System.Collections.Generic; using System.Diagnostics.CodeAnalysis; +using System.Linq; namespace Datadog.Trace.Iast; @@ -61,6 +62,16 @@ public bool IsMarked(SecureMarks marks) return (SecureMarks & marks) != NotMarked; } + public bool IsSafeSource(SourceType[]? safeSources) + { + if (safeSources is null || Source is null) + { + return false; + } + + return safeSources.Contains(Source.Origin); + } + internal bool IsBefore(Range? range) { if (range == null) diff --git a/tracer/src/Datadog.Trace/Iast/Ranges.cs b/tracer/src/Datadog.Trace/Iast/Ranges.cs index 6c5ea0fcf4fc..f94d52612874 100644 --- a/tracer/src/Datadog.Trace/Iast/Ranges.cs +++ b/tracer/src/Datadog.Trace/Iast/Ranges.cs @@ -7,6 +7,7 @@ using System; using System.Collections.Generic; +using Datadog.Trace.Vendors.Newtonsoft.Json.Utilities; namespace Datadog.Trace.Iast; @@ -206,6 +207,45 @@ internal static Range[] CopyWithMark(Range[] ranges, SecureMarks secureMarks) return newRanges.ToArray(); } + internal static Range[]? GetUnsafeRanges(Range[] ranges, SecureMarks safeMarks, SourceType[]? safeSources) + { + if (safeMarks == SecureMarks.None && safeSources is null) + { + return ranges; + } + + bool existsSecureRanges = false; + for (int x = 0; x < ranges.Length; x++) + { + var range = ranges[x]; + if (range.IsMarked(safeMarks) || range.IsSafeSource(safeSources)) + { + existsSecureRanges = true; + break; + } + } + + if (!existsSecureRanges) + { + // This is made in order to avoid unnecesary allocations (most common situation) + return ranges; + } + + List insecureRanges = new List(ranges.Length); + for (int x = 0; x < ranges.Length; x++) + { + var range = ranges[x]; + if (range.IsMarked(safeMarks) || range.IsSafeSource(safeSources)) + { + continue; + } + + insecureRanges.Add(range); + } + + return insecureRanges.ToArray(); + } + internal static bool ContainsUnsafeRange(IEnumerable? ranges) { if (ranges is null) diff --git a/tracer/test/snapshots/Iast.DatabaseSourceInjection.AspNetCore5.Mixed.verified.txt b/tracer/test/snapshots/Iast.DatabaseSourceInjection.AspNetCore5.Mixed.verified.txt index 62ec85e552d0..9dda42744f05 100644 --- a/tracer/test/snapshots/Iast.DatabaseSourceInjection.AspNetCore5.Mixed.verified.txt +++ b/tracer/test/snapshots/Iast.DatabaseSourceInjection.AspNetCore5.Mixed.verified.txt @@ -42,11 +42,7 @@ "source": 0 }, { - "value": "", - "source": 1 - }, - { - "value": ":443/api/v1/test/123/?param1=pone¶m2=ptwo#fragment1=fone&fragment2=ftwo" + "value": ":443/api/v1/test/123/?param1=pone¶m2=ptwo#fragment1=fone&fragment2=ftwo" } ] } @@ -57,11 +53,6 @@ "origin": "http.request.parameter", "name": "host", "value": "localhost" - }, - { - "origin": "sql.row.value", - "name": "Details", - "value": "" } ] }