Skip to content

Commit

Permalink
[IAST] Safeguard Insert Before / After aspects with try/catch (#5839)
Browse files Browse the repository at this point in the history
## Summary of changes
Covered all `InsertBefore` / `InsertAfter` aspects with try catch
clauses to ensure no crash will bubble up to client, following new
analyzer rules.

## Reason for change
SSI will make the tracer enabled for a lot more of services when
available. We must ensure we do not break any of them, and if so, that
we provide a fast answer.

## Implementation details
Apply analyzer suggestions adding a try / catch clause in all
`InsertBefore` / `InsertAfter` aspects

## 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. -->
  • Loading branch information
daniel-romano-DD authored Aug 5, 2024
1 parent 9a4bfb5 commit 51fa7a5
Show file tree
Hide file tree
Showing 39 changed files with 482 additions and 139 deletions.
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// <copyright file="BeforeAfterAspectAnalyzer.cs" company="Datadog">
// <copyright file="BeforeAfterAspectAnalyzer.cs" company="Datadog">
// 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>
Expand Down Expand Up @@ -29,7 +29,7 @@ public class BeforeAfterAspectAnalyzer : DiagnosticAnalyzer
/// <summary>
/// The severity of the diagnostic
/// </summary>
public const DiagnosticSeverity Severity = DiagnosticSeverity.Info;
public const DiagnosticSeverity Severity = DiagnosticSeverity.Error;

#pragma warning disable RS2008 // Enable analyzer release tracking for the analyzer project
private static readonly DiagnosticDescriptor MissingTryCatchRule = new(
Expand Down Expand Up @@ -135,7 +135,7 @@ private void AnalyseMethod(SyntaxNodeAnalysisContext context)
isRethrowing = false;

// check that it's catching _everything_
hasFilter = catchClause.Filter is not null;
hasFilter = catchClause.Filter is not null && catchClause.Filter.FilterExpression is not null && catchClause.Filter.FilterExpression.ToString() != "ex is not BlockException";
if (hasFilter)
{
// Skipping because we shouldn't be letting anything through
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ private async Task<Document> AddTryCatch(Document document, MethodDeclarationSyn
// create the trystatementsyntax with the internals of the method declaration
var catchDeclaration = SyntaxFactory.CatchDeclaration(SyntaxFactory.IdentifierName("Exception"), SyntaxFactory.Identifier("ex"));
var logExpression = SyntaxFactory.ExpressionStatement(
SyntaxFactory.ParseExpression($$"""Log.Error(ex, $"Error invoking {nameof({{typeName}})}.{nameof({{methodName}})}")"""));
SyntaxFactory.ParseExpression($$"""IastModule.Log.Error(ex, $"Error invoking {nameof({{typeName}})}.{nameof({{methodName}})}")"""));
var returnStatement = paramName is not null
? SyntaxFactory.ReturnStatement(SyntaxFactory.IdentifierName(paramName))
: SyntaxFactory.ReturnStatement();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,14 @@ public class HttpResponseAspect
[AspectMethodInsertBefore("Microsoft.AspNetCore.Http.HttpResponse::Redirect(System.String,System.Boolean)", 1)]
public static string? Redirect(string? url)
{
return IastModule.OnUnvalidatedRedirect(url);
try
{
return IastModule.OnUnvalidatedRedirect(url);
}
catch (global::System.Exception ex)
{
IastModule.Log.Error(ex, $"Error invoking {nameof(HttpResponseAspect)}.{nameof(Redirect)}");
return url;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,14 @@ public class ControllerBaseAspect
[AspectMethodInsertBefore("Microsoft.AspNetCore.Mvc.ControllerBase::LocalRedirectPermanentPreserveMethod(System.String)")]
public static string? Redirect(string? url)
{
return IastModule.OnUnvalidatedRedirect(url);
try
{
return IastModule.OnUnvalidatedRedirect(url);
}
catch (global::System.Exception ex)
{
IastModule.Log.Error(ex, $"Error invoking {nameof(ControllerBaseAspect)}.{nameof(Redirect)}");
return url;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@
// This product includes software developed at Datadog (https://www.datadoghq.com/). Copyright 2017 Datadog, Inc.
// </copyright>

using System;
using System.Data.Common;
using Datadog.Trace.AppSec;
using Datadog.Trace.AppSec.Rasp;
using Datadog.Trace.Configuration;
using Datadog.Trace.Iast.Dataflow;

Expand All @@ -29,12 +29,20 @@ public class EntityCommandAspect
[AspectMethodInsertBefore("System.Data.Entity.Core.EntityClient.EntityCommand::ExecuteReaderAsync(System.Data.CommandBehavior)", 1)]
public static object ReviewSqlCommand(object command)
{
if (command is DbCommand dbCommand)
try
{
var commandText = dbCommand.CommandText;
VulnerabilitiesModule.OnSqlQuery(commandText, IntegrationId.SqlClient);
}
if (command is DbCommand dbCommand)
{
var commandText = dbCommand.CommandText;
VulnerabilitiesModule.OnSqlQuery(commandText, IntegrationId.SqlClient);
}

return command;
return command;
}
catch (Exception ex) when (ex is not BlockException)
{
IastModule.Log.Error(ex, $"Error invoking {nameof(EntityCommandAspect)}.{nameof(ReviewSqlCommand)}");
return command;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@
// This product includes software developed at Datadog (https://www.datadoghq.com/). Copyright 2017 Datadog, Inc.
// </copyright>

using System;
using Datadog.Trace.AppSec;
using Datadog.Trace.AppSec.Rasp;
using Datadog.Trace.Configuration;
using Datadog.Trace.Iast.Dataflow;

Expand Down Expand Up @@ -32,8 +32,16 @@ public class EntityFrameworkCoreAspect
[AspectMethodInsertBefore("Microsoft.EntityFrameworkCore.RelationalQueryableExtensions::FromSqlRaw(Microsoft.EntityFrameworkCore.DbSet`1<!!0>,System.String,System.Object[])", 1)]
public static object ReviewSqlString(string sqlAsString)
{
VulnerabilitiesModule.OnSqlQuery(sqlAsString, IntegrationId.SqlClient);
return sqlAsString;
try
{
VulnerabilitiesModule.OnSqlQuery(sqlAsString, IntegrationId.SqlClient);
return sqlAsString;
}
catch (Exception ex) when (ex is not BlockException)
{
IastModule.Log.Error(ex, $"Error invoking {nameof(EntityFrameworkCoreAspect)}.{nameof(ReviewSqlString)}");
return sqlAsString;
}
}
}
#endif
Expand Down
12 changes: 10 additions & 2 deletions tracer/src/Datadog.Trace/Iast/Aspects/MongoDB/BsonAspect.cs
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,15 @@ public class BsonAspect
[AspectMethodInsertBefore("MongoDB.Bson.IO.JsonReader::.ctor(System.String)")]
public static object AnalyzeJsonString(string json)
{
IastModule.OnNoSqlMongoDbQuery(json, IntegrationId.MongoDb);
return json;
try
{
IastModule.OnNoSqlMongoDbQuery(json, IntegrationId.MongoDb);
return json;
}
catch (global::System.Exception ex)
{
IastModule.Log.Error(ex, $"Error invoking {nameof(BsonAspect)}.{nameof(AnalyzeJsonString)}");
return json;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -31,13 +31,13 @@ public class MongoDatabaseAspect
[AspectMethodInsertBefore("MongoDB.Driver.IMongoCollectionExtensions::FindAsync(MongoDB.Driver.IMongoCollection`1<!!0>,MongoDB.Driver.FilterDefinition`1<!!0>,MongoDB.Driver.FindOptions`2<!!0,!!0>,System.Threading.CancellationToken)", 2)]
public static object? AnalyzeCommand(object? command)
{
if (command == null || !Iast.Instance.Settings.Enabled)
{
return command;
}

try
{
if (command == null || !Iast.Instance.Settings.Enabled)
{
return command;
}

var commandType = command.GetType().Name;
switch (commandType)
{
Expand All @@ -50,12 +50,13 @@ public class MongoDatabaseAspect
MongoDbHelper.AnalyzeJsonCommand(command);
break;
}

return command;
}
catch (Exception ex)
catch (global::System.Exception ex)
{
Log.Warning(ex, "Failed to analyze the command");
IastModule.Log.Warning(ex, $"Error invoking {nameof(MongoDatabaseAspect)}.{nameof(AnalyzeCommand)}");
return command;
}

return command;
}
}
15 changes: 11 additions & 4 deletions tracer/src/Datadog.Trace/Iast/Aspects/NHibernate/ISessionAspect.cs
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,10 @@

#nullable enable

using System;
using Datadog.Trace.AppSec;
using Datadog.Trace.AppSec.Rasp;
using Datadog.Trace.Configuration;
using Datadog.Trace.Iast.Dataflow;
using static Datadog.Trace.Configuration.ConfigurationKeys;

namespace Datadog.Trace.Iast.Aspects.NHibernate;

Expand All @@ -28,7 +27,15 @@ public class ISessionAspect
[AspectMethodInsertBefore("NHibernate.ISession::CreateSQLQuery(System.String)", 0)]
public static object AnalyzeQuery(string query)
{
VulnerabilitiesModule.OnSqlQuery(query, IntegrationId.NHibernate);
return query;
try
{
VulnerabilitiesModule.OnSqlQuery(query, IntegrationId.NHibernate);
return query;
}
catch (Exception ex) when (ex is not BlockException)
{
IastModule.Log.Error(ex, $"Error invoking {nameof(ISessionAspect)}.{nameof(AnalyzeQuery)}");
return query;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@
// This product includes software developed at Datadog (https://www.datadoghq.com/). Copyright 2017 Datadog, Inc.
// </copyright>

using System;
using System.Data.Common;
using Datadog.Trace.AppSec;
using Datadog.Trace.AppSec.Rasp;
using Datadog.Trace.Configuration;
using Datadog.Trace.Iast.Dataflow;

Expand All @@ -28,12 +28,20 @@ public class DbCommandAspect
[AspectMethodInsertBefore("System.Data.Common.DbCommand::ExecuteNonQueryAsync()")]
public static object ReviewExecuteNonQuery(object command)
{
if (command is DbCommand entityCommand && command.GetType().Name == "EntityCommand")
try
{
var commandText = entityCommand.CommandText;
VulnerabilitiesModule.OnSqlQuery(commandText, IntegrationId.SqlClient);
}
if (command is DbCommand entityCommand && command.GetType().Name == "EntityCommand")
{
var commandText = entityCommand.CommandText;
VulnerabilitiesModule.OnSqlQuery(commandText, IntegrationId.SqlClient);
}

return command;
return command;
}
catch (Exception ex) when (ex is not BlockException)
{
IastModule.Log.Error(ex, $"Error invoking {nameof(DbCommandAspect)}.{nameof(ReviewExecuteNonQuery)}");
return command;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,19 @@ public partial class DirectoryEntryAspect
[AspectMethodInsertBefore("System.DirectoryServices.DirectoryEntry::.ctor(System.String,System.String,System.String,System.DirectoryServices.AuthenticationTypes)", 3)]
public static object Init(string path)
{
if (!string.IsNullOrEmpty(path) && path.ToLower().StartsWith("ldap"))
try
{
IastModule.OnLdapInjection(path);
}
if (!string.IsNullOrEmpty(path) && path.ToLower().StartsWith("ldap"))
{
IastModule.OnLdapInjection(path);
}

return path;
return path;
}
catch (global::System.Exception ex)
{
IastModule.Log.Error(ex, $"Error invoking {nameof(DirectoryEntryAspect)}.{nameof(Init)}");
return path;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,15 @@ public partial class DirectorySearcherAspect
[AspectMethodInsertBefore("System.DirectoryServices.DirectorySearcher::.ctor(System.DirectoryServices.DirectoryEntry,System.String,System.String[],System.DirectoryServices.SearchScope)", 2)]
public static object Init(string path)
{
IastModule.OnLdapInjection(path);
return path;
try
{
IastModule.OnLdapInjection(path);
return path;
}
catch (global::System.Exception ex)
{
IastModule.Log.Error(ex, $"Error invoking {nameof(DirectorySearcherAspect)}.{nameof(Init)}");
return path;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,15 @@ public partial class PrincipalContextAspect
[AspectMethodInsertBefore("System.DirectoryServices.AccountManagement.PrincipalContext::.ctor(System.DirectoryServices.AccountManagement.ContextType,System.String,System.String,System.DirectoryServices.AccountManagement.ContextOptions,System.String,System.String)", 3)]
public static object Init(string path)
{
IastModule.OnLdapInjection(path);
return path;
try
{
IastModule.OnLdapInjection(path);
return path;
}
catch (global::System.Exception ex)
{
IastModule.Log.Error(ex, $"Error invoking {nameof(PrincipalContextAspect)}.{nameof(Init)}");
return path;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,19 @@ public partial class SearchRequestAspect
[AspectMethodInsertBefore("System.DirectoryServices.Protocols.SearchRequest::set_Filter(System.Object)")]
public static object Init(object path)
{
if (path is string pathString)
try
{
IastModule.OnLdapInjection(pathString);
}
if (path is string pathString)
{
IastModule.OnLdapInjection(pathString);
}

return path;
return path;
}
catch (global::System.Exception ex)
{
IastModule.Log.Error(ex, $"Error invoking {nameof(SearchRequestAspect)}.{nameof(Init)}");
return path;
}
}
}
14 changes: 11 additions & 3 deletions tracer/src/Datadog.Trace/Iast/Aspects/System.IO/DirectoryAspect.cs
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@

#nullable enable

using System;
using Datadog.Trace.AppSec;
using Datadog.Trace.AppSec.Rasp;
using Datadog.Trace.Iast.Dataflow;

namespace Datadog.Trace.Iast.Aspects;
Expand Down Expand Up @@ -75,7 +75,15 @@ public class DirectoryAspect
[AspectMethodInsertBefore("System.IO.Directory::SetCurrentDirectory(System.String)")]
public static string ReviewPath(string path)
{
VulnerabilitiesModule.OnPathTraversal(path);
return path;
try
{
VulnerabilitiesModule.OnPathTraversal(path);
return path;
}
catch (Exception ex) when (ex is not BlockException)
{
IastModule.Log.Error(ex, $"Error invoking {nameof(DirectoryAspect)}.{nameof(ReviewPath)}");
return path;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@

#nullable enable

using System;
using Datadog.Trace.AppSec;
using Datadog.Trace.AppSec.Rasp;
using Datadog.Trace.Iast.Dataflow;

namespace Datadog.Trace.Iast.Aspects;
Expand Down Expand Up @@ -60,7 +60,15 @@ public class DirectoryInfoAspect
#endif
public static string ReviewPath(string path)
{
VulnerabilitiesModule.OnPathTraversal(path);
return path;
try
{
VulnerabilitiesModule.OnPathTraversal(path);
return path;
}
catch (Exception ex) when (ex is not BlockException)
{
IastModule.Log.Error(ex, $"Error invoking {nameof(DirectoryInfoAspect)}.{nameof(ReviewPath)}");
return path;
}
}
}
Loading

0 comments on commit 51fa7a5

Please sign in to comment.