Refactoring and hardening of security coordinator (#6143)

## Summary of changes

Security coordinator should NEVER be able to be instantiated if http
context is null

## Reason for change

Refering to several errors at customers with NullReferenceException or
waf additive contexts disposed

## Implementation details

## Test coverage

## Other details
<!-- Fixes #{issue} -->

<!-- ⚠️ Note: where possible, please obtain 2 approvals prior to
merging. Unless CODEOWNERS specifies otherwise, for external teams it is
typically best to have one review from a team member, and one review
from apm-dotnet. Trivial changes do not require 2 reviews. -->

---------

Co-authored-by: Andrew Lock <[email protected]>

tracer/missing-nullability-files.csv

@@ -60,7 +60,6 @@ src/Datadog.Trace/Agent/TracesTransportType.cs
 src/Datadog.Trace/AppSec/AddressesConstants.cs
 src/Datadog.Trace/AppSec/AppSecRateLimiter.cs
 src/Datadog.Trace/AppSec/BlockingAction.cs
-src/Datadog.Trace/AppSec/CoreHttpContextStore.cs
 src/Datadog.Trace/AppSec/EventTrackingSdk.cs
 src/Datadog.Trace/AppSec/IDatadogSecurity.cs
 src/Datadog.Trace/AppSec/IEvent.cs

tracer/src/Datadog.Trace/AppSec/AttackerFingerprint/AttackerFingerprintHelper.cs

@@ -24,10 +24,15 @@ public static void AddSpanTags(Span span, IResult result)
  return;
  }
 
- var securityCoordinator = new SecurityCoordinator(Security.Instance, span);
+ var securityCoordinator = SecurityCoordinator.TryGet(Security.Instance, span);
+
+ if (securityCoordinator is null)
+ {
+ return;
+ }
 
  // We need a context
- if (!securityCoordinator.HasContext() || securityCoordinator.IsAdditiveContextDisposed())
+ if (securityCoordinator.Value.IsAdditiveContextDisposed())
  {
  return;
  }

tracer/src/Datadog.Trace/AppSec/ControllerContextExtensions.Framework.cs

@@ -9,6 +9,7 @@
 using System.Collections.Generic;
 using System.Linq;
 using System.Web;
+using Datadog.Trace.AppSec.Coordinator;
 using Datadog.Trace.AspNet;
 using Datadog.Trace.ClrProfiler.AutoInstrumentation.AspNet;
 using Datadog.Trace.Iast;
@@ -78,7 +79,7 @@ internal static void MonitorBodyAndPathParams(this IControllerContext controller
 
  if (security.Enabled)
  {
- var securityTransport = new Coordinator.SecurityCoordinator(security, scope.Span!);
+ var securityTransport = SecurityCoordinator.Get(security, scope.Span!, context);
  if (!securityTransport.IsBlocked)
  {
  var inputData = new Dictionary<string, object>();

tracer/src/Datadog.Trace/AppSec/Coordinator/HttpTransportBase.cs

@@ -4,6 +4,7 @@
 // </copyright>
 
 #nullable enable
+using System;
 using System.Collections.Generic;
 using Datadog.Trace.AppSec.Waf;
 using Datadog.Trace.Headers;

tracer/src/Datadog.Trace/AppSec/Coordinator/SecurityCoordinator.Core.cs

@@ -20,15 +20,31 @@ namespace Datadog.Trace.AppSec.Coordinator;
 
 internal readonly partial struct SecurityCoordinator
 {
- internal SecurityCoordinator(Security security, Span span, HttpTransport? transport = null)
+ private SecurityCoordinator(Security security, Span span, HttpTransport transport)
  {
  _security = security;
  _localRootSpan = TryGetRoot(span);
- _httpTransport = transport ?? new HttpTransport(CoreHttpContextStore.Instance.Get());
+ _httpTransport = transport;
  }
 
  private static bool CanAccessHeaders => true;
 
+ internal static SecurityCoordinator? TryGet(Security security, Span span)
+ {
+ var context = CoreHttpContextStore.Instance.Get();
+ if (context is null)
+ {
+ Log.Warning("Can't instantiate SecurityCoordinator.Core as no transport has been provided and CoreHttpContextStore.Instance.Get() returned null, make sure HttpContext is available");
+ return null;
+ }
+
+ return new SecurityCoordinator(security, span, new(context));
+ }
+
+ internal static SecurityCoordinator Get(Security security, Span span, HttpContext context) => new(security, span, new HttpTransport(context));
+
+ internal static SecurityCoordinator Get(Security security, Span span, HttpTransport transport) => new(security, span, transport);
+
  public static Dictionary<string, object> ExtractHeadersFromRequest(IHeaderDictionary headers)
  {
  var headersDic = new Dictionary<string, object>(headers.Keys.Count);
@@ -162,7 +178,7 @@ internal override bool IsBlocked
  {
  if (Context.Items.TryGetValue(BlockingAction.BlockDefaultActionName, out var value))
  {
- return value is bool boolValue && boolValue;
+ return value is true;
  }
 
  return false;

tracer/src/Datadog.Trace/AppSec/Coordinator/SecurityCoordinator.Framework.cs

@@ -45,15 +45,32 @@ static SecurityCoordinator()
  }
  }
 
- internal SecurityCoordinator(Security security, Span span, HttpTransport? transport = null)
+ private SecurityCoordinator(Security security, Span span, HttpTransport transport)
  {
  _security = security;
  _localRootSpan = TryGetRoot(span);
- _httpTransport = transport ?? new HttpTransport(HttpContext.Current);
+ _httpTransport = transport;
  }
 
  private bool CanAccessHeaders => UsingIntegratedPipeline is true or null;
 
+ internal static SecurityCoordinator? TryGet(Security security, Span span)
+ {
+ if (HttpContext.Current is not { } current)
+ {
+ Log.Warning("Can't instantiate SecurityCoordinator.Framework as no transport has been provided and HttpContext.Current null, make sure HttpContext is available");
+ return null;
+ }
+
+ var transport = new HttpTransport(current);
+
+ return new SecurityCoordinator(security, span, transport);
+ }
+
+ internal static SecurityCoordinator Get(Security security, Span span, HttpContext context) => new(security, span, new HttpTransport(context));
+
+ internal static SecurityCoordinator Get(Security security, Span span, HttpTransport transport) => new(security, span, transport);
+
  private static Action<IResult, HttpStatusCode, string, string>? CreateThrowHttpResponseExceptionDynMeth()
  {
  try

tracer/src/Datadog.Trace/AppSec/Coordinator/SecurityCoordinator.cs

@@ -67,23 +67,10 @@ private static void LogMatchesIfDebugEnabled(IReadOnlyCollection<object>? result
  return RunWaf(args, lastTime);
  }
 
- public bool HasContext()
- {
- return _httpTransport.Context is not null;
- }
-
- public bool IsAdditiveContextDisposed()
- {
- return _httpTransport.IsAdditiveContextDisposed();
- }
+ public bool IsAdditiveContextDisposed() => _httpTransport.IsAdditiveContextDisposed();
 
  public IResult? RunWaf(Dictionary<string, object> args, bool lastWafCall = false, bool runWithEphemeral = false, bool isRasp = false)
  {
- if (!HasContext())
- {
- return null;
- }
-
  LogAddressIfDebugEnabled(args);
  IResult? result = null;
  try
@@ -99,6 +86,11 @@ public bool IsAdditiveContextDisposed()
  _httpTransport.SetAdditiveContext(additiveContext);
  }
  }
+ else if (_httpTransport.IsAdditiveContextDisposed())
+ {
+ Log.Warning("Waf could not run as waf additive context is disposed");
+ return null;
+ }
 
  _security.ApiSecurity.ShouldAnalyzeSchema(lastWafCall, _localRootSpan, args, _httpTransport.StatusCode.ToString(), _httpTransport.RouteData);
 

tracer/src/Datadog.Trace/AppSec/Coordinator/SecurityCoordinatorHelpers.Core.cs

@@ -25,7 +25,7 @@ internal static void CheckAndBlock(this Security security, HttpContext context,
  var transport = new SecurityCoordinator.HttpTransport(context);
  if (!transport.IsBlocked)
  {
- var securityCoordinator = new SecurityCoordinator(security, span, transport);
+ var securityCoordinator = SecurityCoordinator.Get(security, span, context);
  var result = securityCoordinator.Scan();
  securityCoordinator.BlockAndReport(result);
  }
@@ -41,7 +41,8 @@ internal static void CheckReturnedHeaders(this Security security, Span span, IHe
  var transport = new SecurityCoordinator.HttpTransport(httpContext);
  if (!transport.IsBlocked)
  {
- var securityCoordinator = new SecurityCoordinator(security, span, transport);
+ var securityCoordinator = SecurityCoordinator.Get(security, span, transport);
+
  var args = new Dictionary<string, object>
  {
  {
@@ -73,7 +74,7 @@ internal static void CheckPathParams(this Security security, HttpContext context
  var transport = new SecurityCoordinator.HttpTransport(context);
  if (!transport.IsBlocked)
  {
- var securityCoordinator = new SecurityCoordinator(security, span, transport);
+ var securityCoordinator = SecurityCoordinator.Get(security, span, transport);
  var args = new Dictionary<string, object> { { AddressesConstants.RequestPathParams, pathParams } };
  var result = securityCoordinator.RunWaf(args);
  securityCoordinator.BlockAndReport(result);
@@ -88,7 +89,7 @@ internal static void CheckUser(this Security security, HttpContext context, Span
  var transport = new SecurityCoordinator.HttpTransport(context);
  if (!transport.IsBlocked)
  {
- var securityCoordinator = new SecurityCoordinator(security, span, transport);
+ var securityCoordinator = SecurityCoordinator.Get(security, span, transport);
  var args = new Dictionary<string, object> { { AddressesConstants.UserId, userId } };
  var result = securityCoordinator.RunWaf(args);
  securityCoordinator.BlockAndReport(result);
@@ -103,7 +104,7 @@ internal static void CheckPathParamsFromAction(this Security security, HttpConte
  var transport = new SecurityCoordinator.HttpTransport(context);
  if (!transport.IsBlocked)
  {
- var securityCoordinator = new SecurityCoordinator(security, span, transport);
+ var securityCoordinator = SecurityCoordinator.Get(security, span, transport);
  var pathParams = new Dictionary<string, object>(actionPathParams.Count);
  for (var i = 0; i < actionPathParams.Count; i++)
  {
@@ -131,7 +132,7 @@ internal static void CheckPathParamsFromAction(this Security security, HttpConte
  var transport = new SecurityCoordinator.HttpTransport(context);
  if (!transport.IsBlocked)
  {
- var securityCoordinator = new SecurityCoordinator(security, span, transport);
+ var securityCoordinator = SecurityCoordinator.Get(security, span, transport);
  var keysAndValues = ObjectExtractor.Extract(body);
 
  if (keysAndValues is not null)

tracer/src/Datadog.Trace/AppSec/CoreHttpContextStore.cs

@@ -2,6 +2,7 @@
 // Unless explicitly stated otherwise all files in this repository are licensed under the Apache 2 License.
 // This product includes software developed at Datadog (https://www.datadoghq.com/). Copyright 2017 Datadog, Inc.
 // </copyright>
+#nullable enable
 #if !NETFRAMEWORK
 
 using System;
@@ -10,19 +11,31 @@
 using System.Text;
 using System.Threading;
 using System.Threading.Tasks;
+using Datadog.Trace.AppSec.Coordinator;
+using Datadog.Trace.Logging;
 using Microsoft.AspNetCore.Http;
 
 namespace Datadog.Trace.AppSec
 {
  internal class CoreHttpContextStore
  {
+ private static readonly IDatadogLogger Log = DatadogLogging.GetLoggerFor<CoreHttpContextStore>();
+
  public static readonly CoreHttpContextStore Instance = new();
 
- private AsyncLocal<HttpContext> localStore = new();
+ private readonly AsyncLocal<HttpContext> _localStore = new();
+
+ public HttpContext? Get()
+ {
+ if (_localStore.Value is null)
+ {
+ Log.Debug("CoreHttpContextStore.Get called but returning null for HttpContext");
+ }
 
- public HttpContext Get() => localStore.Value;
+ return _localStore.Value;
+ }
 
- public void Set(HttpContext context) => localStore.Value = context;
+ public void Set(HttpContext context) => _localStore.Value = context;
  }
 }
 

tracer/src/Datadog.Trace/AppSec/Rasp/RaspModule.cs

@@ -107,15 +107,16 @@ private static void RecordRaspTelemetry(string address, bool isMatch, bool timeO
 
  private static void RunWafRasp(Dictionary<string, object> arguments, Span rootSpan, string address)
  {
- var securityCoordinator = new SecurityCoordinator(Security.Instance, rootSpan);
+ var securityCoordinator = SecurityCoordinator.TryGet(Security.Instance, rootSpan);
 
  // We need a context for RASP
- if (!securityCoordinator.HasContext() || securityCoordinator.IsAdditiveContextDisposed())
+ if (securityCoordinator is null)
  {
+ Log.Warning("Tried to run Rasp but security coordinator couldn't be instantiated, probably because of httpcontext missing");
  return;
  }
 
- var result = securityCoordinator.RunWaf(arguments, runWithEphemeral: true, isRasp: true);
+ var result = securityCoordinator.Value.RunWaf(arguments, runWithEphemeral: true, isRasp: true);
 
  if (result is not null)
  {
@@ -139,7 +140,7 @@ private static void RunWafRasp(Dictionary<string, object> arguments, Span rootSp
  }
  }
  }
- catch (System.Exception ex)
+ catch (Exception ex)
  {
  Log.Error(ex, "RASP: Error while sending stack.");
  }
@@ -148,7 +149,7 @@ private static void RunWafRasp(Dictionary<string, object> arguments, Span rootSp
 
  // we want to report first because if we are inside a try{} catch(Exception ex){} block, we will not report
  // the blockings, so we report first and then block
- securityCoordinator.ReportAndBlock(result);
+ securityCoordinator.Value.ReportAndBlock(result);
  }
 
  private static void AddSpanId(IResult? result)

tracer/src/Datadog.Trace/AspNet/TracingHttpModule.cs

@@ -184,7 +184,7 @@ private void OnBeginRequest(object sender, EventArgs eventArgs)
  if (security.Enabled)
  {
  SecurityCoordinator.ReportWafInitInfoOnce(security, scope.Span);
- var securityCoordinator = new SecurityCoordinator(security, scope.Span);
+ var securityCoordinator = SecurityCoordinator.Get(security, scope.Span, httpContext);
 
  // request args
  var args = securityCoordinator.GetBasicRequestArgsForWaf();
@@ -245,7 +245,7 @@ private void OnEndRequest(object sender, EventArgs eventArgs)
  var security = Security.Instance;
  if (security.Enabled)
  {
- var securityCoordinator = new SecurityCoordinator(security, rootSpan);
+ var securityCoordinator = SecurityCoordinator.Get(security, rootSpan, app.Context);
  var args = securityCoordinator.GetBasicRequestArgsForWaf();
  args.Add(AddressesConstants.RequestPathParams, securityCoordinator.GetPathParams());
 

tracer/src/Datadog.Trace/ClrProfiler/AutoInstrumentation/AspNet/ControllerActionInvoker_InvokeAction_Integration.cs

@@ -96,7 +96,7 @@ internal static CallTargetState OnMethodBegin<TTarget, TContext, TActionDescript
  var scope = SharedItems.TryPeekScope(HttpContext.Current, AspNetMvcIntegration.HttpContextKey);
  if (scope is not null)
  {
- var securityTransport = new SecurityCoordinator(security, scope.Span);
+ var securityTransport = SecurityCoordinator.Get(security, scope.Span, HttpContext.Current);
  if (!securityTransport.IsBlocked)
  {
  var extractedObject = ObjectExtractor.Extract(responseObject);

tracer/src/Datadog.Trace/ClrProfiler/AutoInstrumentation/AspNet/ReflectedHttpActionDescriptor_ExecuteAsync_Integration.cs

@@ -87,7 +87,7 @@ internal static CallTargetState OnMethodBegin<TTarget, TController>(TTarget inst
  var scope = SharedItems.TryPeekScope(HttpContext.Current, AspNetWebApi2Integration.HttpContextKey);
  if (scope is not null)
  {
- var securityTransport = new SecurityCoordinator(security, scope.Span);
+ var securityTransport = SecurityCoordinator.Get(security, scope.Span, HttpContext.Current);
  if (!securityTransport.IsBlocked)
  {
  var extractedObj = ObjectExtractor.Extract(responseObject);

tracer/src/Datadog.Trace/ClrProfiler/AutoInstrumentation/AspNetCore/BlockingMiddleware.cs

@@ -80,7 +80,7 @@ internal async Task Invoke(HttpContext context)
  {
  if (Tracer.Instance?.ActiveScope?.Span is Span span)
  {
- var securityCoordinator = new SecurityCoordinator(security, span, new SecurityCoordinator.HttpTransport(context));
+ var securityCoordinator = SecurityCoordinator.Get(security, span, new SecurityCoordinator.HttpTransport(context));
  if (_endPipeline && !context.Response.HasStarted)
  {
  context.Response.StatusCode = 404;
@@ -123,7 +123,7 @@ internal async Task Invoke(HttpContext context)
  {
  if (Tracer.Instance?.ActiveScope?.Span is Span span)
  {
- var securityCoordinator = new SecurityCoordinator(security, span, new SecurityCoordinator.HttpTransport(context));
+ var securityCoordinator = SecurityCoordinator.Get(security, span, new SecurityCoordinator.HttpTransport(context));
  if (!blockException.Reported)
  {
  securityCoordinator.TryReport(blockException.Result, endedResponse);

tracer/src/Datadog.Trace/DiagnosticListeners/AspNetCoreDiagnosticObserver.cs

@@ -423,7 +423,7 @@ private void OnHostingHttpRequestInStart(object arg)
 
  if (arg.TryDuckCast<HttpRequestInStartStruct>(out var requestStruct))
  {
- HttpContext httpContext = requestStruct.HttpContext;
+ var httpContext = requestStruct.HttpContext;
  if (shouldTrace)
  {
  // Use an empty resource name here, as we will likely replace it as part of the request

tracer/src/Datadog.Trace/PlatformHelpers/AspNetCoreHttpRequestHandler.cs

@@ -169,7 +169,7 @@ public void StopAspNetCorePipelineScope(Tracer tracer, Security security, Scope
  span.SetHeaderTags(new HeadersCollectionAdapter(httpContext.Response.Headers), tracer.Settings.HeaderTagsInternal, defaultTagPrefix: SpanContextPropagator.HttpResponseHeadersTagPrefix);
  if (security.Enabled)
  {
- var transport = new SecurityCoordinator(security, span, new SecurityCoordinator.HttpTransport(httpContext));
+ var transport = SecurityCoordinator.Get(security, span, new SecurityCoordinator.HttpTransport(httpContext));
  transport.AddResponseHeadersToSpanAndCleanup();
  }
  else

tracer/src/Datadog.Trace/SpanExtensions.Framework.cs

@@ -26,14 +26,18 @@ private static void RunBlockingCheck(Span span, string userId)
 
  if (security.Enabled)
  {
- var securityCoordinator = new SecurityCoordinator(Security.Instance, span);
+ var securityCoordinator = SecurityCoordinator.TryGet(Security.Instance, span);
+ if (securityCoordinator is null)
+ {
+ return;
+ }
 
  var wafArgs = new Dictionary<string, object>
  {
  { AddressesConstants.UserId, userId },
  };
 
- securityCoordinator.BlockAndReport(wafArgs);
+ securityCoordinator.Value.BlockAndReport(wafArgs);
  }
  }
  }