[IAST] Vulnerability and Evidence truncation #5302

daniel-romano-DD · 2024-03-11T22:01:14Z

Summary of changes

Implementation of this RFC

Reason for change

Under certain circumstances, evidences could be too long, incurring in an excessive resource consumption. Also, too big vulnerability batches (more than 25KB) could make the agent misbehave.

Implementation details

Added controls to limit final vulnerability batch size and max evidence values length.

Test coverage

Updated redaction test suite and implemented ad-hoc unit tests

datadog-ddstaging · 2024-03-11T22:16:05Z

Datadog Report

Branch report: dani/iast/evidence_redaction_truncation
Commit report: d098e98
Test service: dd-trace-dotnet

✅ 0 Failed, 328959 Passed, 1576 Skipped, 35m 5.97s Wall Time
❄️ 1 New Flaky

New Flaky Tests (1)

SubmitsDsmMetrics - Datadog.Trace.ClrProfiler.IntegrationTests.AWS.DataStreamsMonitoringAwsSqsTests - Last Failure

Expand for error

 Results do not match.
 Differences:
 Received: DataStreamsMonitoringAwsSqsTests.SubmitsDsmMetrics.NetCore.received.txt
 Verified: DataStreamsMonitoringAwsSqsTests.SubmitsDsmMetrics.NetCore.verified.txt
 Received Content:
 {
   Env: integration_tests,
   Service: Samples.AWS.SQS,
   TracerVersion: <snip>,
   Lang: dotnet,
 ...

andrewlock · 2024-03-11T22:30:12Z

Execution-Time Benchmarks Report ⏱️

Execution-time results for samples comparing the following branches/commits:

Execution-time benchmarks measure the whole time it takes to execute a program. And are intended to measure the one-off costs. Cases where the execution time results for the PR are worse than latest master results are shown in red. The following thresholds were used for comparing the execution times:

Welch test with statistical test for significance of 5%
Only results indicating a difference greater than 5% and 5 ms are considered.

Note that these results are based on a single point-in-time result for each branch. For full results, see the dashboard.

Graphs show the p99 interval based on the mean and StdDev of the test run, as well as the mean value of the run (shown as a diamond below the graph).

gantt
    title Execution time (ms) FakeDbCommand (.NET Framework 4.6.2) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (5302) - mean (74ms)  : 65, 83
     .   : milestone, 74,
    master - mean (74ms)  : 65, 83
     .   : milestone, 74,

    section CallTarget+Inlining+NGEN
    This PR (5302) - mean (993ms)  : 971, 1014
     .   : milestone, 993,
    master - mean (985ms)  : 961, 1008
     .   : milestone, 985,

gantt
    title Execution time (ms) FakeDbCommand (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (5302) - mean (110ms)  : 107, 114
     .   : milestone, 110,
    master - mean (118ms)  : 96, 140
     .   : milestone, 118,

    section CallTarget+Inlining+NGEN
    This PR (5302) - mean (717ms)  : 690, 745
     .   : milestone, 717,
    master - mean (713ms)  : 691, 735
     .   : milestone, 713,

gantt
    title Execution time (ms) FakeDbCommand (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (5302) - mean (95ms)  : 92, 99
     .   : milestone, 95,
    master - mean (95ms)  : 91, 98
     .   : milestone, 95,

    section CallTarget+Inlining+NGEN
    This PR (5302) - mean (669ms)  : 643, 695
     .   : milestone, 669,
    master - mean (671ms)  : 647, 694
     .   : milestone, 671,

gantt
    title Execution time (ms) HttpMessageHandler (.NET Framework 4.6.2) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (5302) - mean (188ms)  : 185, 191
     .   : milestone, 188,
    master - mean (188ms)  : 182, 193
     .   : milestone, 188,

    section CallTarget+Inlining+NGEN
    This PR (5302) - mean (1,067ms)  : 1048, 1087
     .   : milestone, 1067,
    master - mean (1,063ms)  : 1038, 1089
     .   : milestone, 1063,

gantt
    title Execution time (ms) HttpMessageHandler (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (5302) - mean (271ms)  : 267, 276
     .   : milestone, 271,
    master - mean (269ms)  : 262, 276
     .   : milestone, 269,

    section CallTarget+Inlining+NGEN
    This PR (5302) - mean (867ms)  : 847, 887
     .   : milestone, 867,
    master - mean (861ms)  : 839, 884
     .   : milestone, 861,

gantt
    title Execution time (ms) HttpMessageHandler (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (5302) - mean (259ms)  : 254, 264
     .   : milestone, 259,
    master - mean (258ms)  : 250, 265
     .   : milestone, 258,

    section CallTarget+Inlining+NGEN
    This PR (5302) - mean (856ms)  : 829, 883
     .   : milestone, 856,
    master - mean (855ms)  : 834, 876
     .   : milestone, 855,

github-actions · 2024-03-12T07:20:30Z

Snapshots difference summary

The following differences have been observed in committed snapshots. It is meant to help the reviewer.
The diff is simplistic, so please check some files anyway while we improve it.

1 occurrences of :

-      "hash": -177455026,
+      "hash": -430498668,

andrewlock · 2024-03-12T08:33:33Z

tracer/src/Datadog.Trace/Configuration/ConfigurationKeys.Iast.cs

+            /// Configuration key for IAST evidence max lenght in chars.
+            /// Default value is 250
+            /// </summary>
+            public const string TruncationMaxValueLength = "IAST_TRUNCATION_MAX_VALUE_LENGTH";


This doesn't look correct - it should start with a DD_, DD_INTERNAL, _DD_ or something key 😅 Also, should be standardised across languages - have we done that?

Don't forget to add to config_norm.json (and in dd-go)

You are right. Fixing it. Go PR here

andrewlock · 2024-03-12T08:35:27Z

tracer/src/Datadog.Trace/Iast/EvidenceConverter.cs

@@ -78,7 +78,7 @@ public override void WriteJson(JsonWriter writer, Evidence? evidence, JsonSerial
        if (evidenceValue.Ranges == null || evidenceValue.Ranges.Length == 0)
        {
            writer.WritePropertyName("value");
-            writer.WriteValue(evidenceValue.Value);
+            writer.WriteTruncableValue(evidenceValue.Value);


Should be WriteTruncatableValue I guess? 🤔 Or maybe WriteTruncatedValue() (though that suggests it's already truncated...WriteAndTruncateValue()? 😅 🤷‍♂️

I took the same name Java is using 🙈

Java shouldn't make up names 😅

tracer/test/Datadog.Trace.Security.Unit.Tests/IAST/Tainted/VulnerabilityBatchTests.cs

andrewlock

Nearly there 😄

andrewlock · 2024-03-12T09:25:34Z

tracer/src/Datadog.Trace/Iast/SensitiveData/EvidenceRedactor.cs

    {
        _timeout = timeout;
        _logger = logger;

+        TruncationUtils.Init(truncationMaxValueLength);


FWIW, this is just asking for testing issues 🙁 Shared static state is evil 😛 I would strongly suggest passing the truncation value into the EvidenceConverter constructor, and making the truncation method a pure method instead of having a shared static

e-n-0

LGTM! Thanks

commit 832de4b Author: Flavien Darche <11708575+e-n-0@users.noreply.github.com> Date: Tue Mar 12 20:24:21 2024 +0000 [ASM][IAST] Configure maximum IAST Ranges (#5292) * Add configuration key * Use a RangeList in some case to not exceed the max number * Revert some code + implem correct merge * Fix + Add unit and integration tests * Usual macos fix for snapshot * Fix snapshots hashs * Update snapshots (remove other tests as they can't apply different env var values in same run) * Apply comment * Re-integrate integration tests with multiple processes (new fixture) * Add test case for setting MaxRangeCount to zero commit 83f6ab1 Author: Tony Redondo <tony.redondo@datadoghq.com> Date: Tue Mar 12 21:20:39 2024 +0100 [CI Visibility] - Enable snapshot testing of current testing framework implementations (#5226) commit 233695a Author: Daniel Romano <108014683+daniel-romano-DD@users.noreply.github.com> Date: Tue Mar 12 17:06:06 2024 +0100 [IAST] Vulnerability and Evidence truncation (#5302) * Initial implementation * Updated test bundle * Fix test compilation error * Fix snapshot (from rebase) * Fix typo in config value. Updated tests * Fix typo * Refactor converters initialization commit ea31cf5 Author: Anna <anna.yafi@datadoghq.com> Date: Tue Mar 12 16:39:09 2024 +0100 Deactivate benchmark for legacy encoder (#5299) commit d0d713a Author: NachoEchevarria <53266532+NachoEchevarria@users.noreply.github.com> Date: Tue Mar 12 09:25:27 2024 +0100 Set big regex timeouts for tests (#5297) commit d5388d6 Author: Lucas Pimentel <lucas.pimentel@datadoghq.com> Date: Mon Mar 11 15:20:58 2024 -0400 [Tracing] Support configuring `DD_TRACE_ENABLED` remotely (#5181) * add support for remote TraceEnabled setting * fix unrelated typo * add ApmTracingEnabled capability 19 * add missing RCM capability 18 * add mapping * add unit test * add comments to unit test * rename property to match RCM constant * include config in integration tests * fix test json * rewrite tests to use raw values instead of strings commit 2b95f46 Author: Flavien Darche <11708575+e-n-0@users.noreply.github.com> Date: Mon Mar 11 17:47:55 2024 +0100 [ASM][IAST] Support manual JSON deserialisation (Newtonsoft.Json) (#5238) * Add Newtonsoft.Json (non -working yet) * Refactor the tainting proces + add tests * Add the JToken Parse aspect + test * Rename Aspects class + Duck orignal method call * Add integration test * Fix nullability * Fix compilation issue for netfx * Change JSON formatting in ParseTests * Fix a test json format * Refactor NewtonsoftJsonAspects to static constructor commit 0d511f9 Author: Igor Kravchenko <21974069+kr-igor@users.noreply.github.com> Date: Mon Mar 11 09:35:23 2024 -0500 [DSM] - Fixes for IbmMq instrumentation (#5271) * Use byte properties instead of strings * Fixed nullability files * Added some debug info * Fixed lint issues * Added a bit more logs * Using slow byte->sbyte conversion * Added noop headers adapter * Fixed nullability files * Added more logs * Cleaned up debug logs * Removed symlink * Update tracer/src/Datadog.Trace/ClrProfiler/AutoInstrumentation/IbmMq/IbmMqHeadersAdapter.cs Removed debug code Co-authored-by: Andrew Lock <andrew.lock@datadoghq.com> * Update tracer/src/Datadog.Trace/ClrProfiler/AutoInstrumentation/IbmMq/IbmMqHeadersAdapter.cs Using Unsafe.As instead of BlockCopy Co-authored-by: Andrew Lock <andrew.lock@datadoghq.com> * Update tracer/src/Datadog.Trace/ClrProfiler/AutoInstrumentation/IbmMq/IbmMqHeadersAdapter.cs Use Unsafe.As instead of BlockCopy Co-authored-by: Andrew Lock <andrew.lock@datadoghq.com> * Addressed some of the comments * Removed context propagation options --------- Co-authored-by: Andrew Lock <andrew.lock@datadoghq.com> commit 5684a72 Author: Zach Montoya <zach.montoya@datadoghq.com> Date: Fri Mar 8 20:56:30 2024 -0800 [Tracing] Update instrumentation point for DD_TRACE_DELAY_WCF_INSTRUMENTATION_ENABLED=true (#5206) Updates the instrumentation point for `DD_TRACE_DELAY_WCF_INSTRUMENTATION_ENABLED=true` so that now a server span is created immediately before IDispatchMessageInspector implementations are run, so application code can access the root span from inside a IDispatchMessageInspector.AfterReceiveRequest callback. This PR also does some cleanup to remove unused Wcf files and it makes the entire Wcf instrumentation use nullable reference types. commit ca1bb6e Author: Andrew Lock <andrew.lock@datadoghq.com> Date: Fri Mar 8 17:43:57 2024 +0000 Fix errors identified from telemetry (#5279) * Try to avoid MongoDb exception We're seeing exceptions like this: ``` System.FieldAccessException at REDACTED at Datadog.Trace.ClrProfiler.AutoInstrumentation.MongoDb.MongoDbIntegration.CreateScope[TConnection](Object wireProtocol, TConnection connection) at REDACTED at MongoDB.Driver.Core.WireProtocol.CommandWireProtocol`1.ExecuteAsync(IConnection connection, CancellationToken cancellationToken) ``` and the only explanation I can think of is a duck-chaining issue, so stopped doing duck chaining and being explicit instead * Add local functions to try to isolate problems * Fix ArgumentNullException in AWS SQS integration

daniel-romano-DD added area:asm area:asm-iast labels Mar 11, 2024

daniel-romano-DD requested review from a team as code owners March 11, 2024 22:01

daniel-romano-DD added 4 commits March 12, 2024 07:59

Initial implementation

18370ef

Updated test bundle

3473cee

Fix test compilation error

b562367

Fix snapshot (from rebase)

02f1b0f

daniel-romano-DD force-pushed the dani/iast/evidence_redaction_truncation branch from d21efc6 to 02f1b0f Compare March 12, 2024 07:19

andrewlock reviewed Mar 12, 2024

View reviewed changes

daniel-romano-DD added 2 commits March 12, 2024 10:20

Fix typo in config value. Updated tests

082e05d

Fix typo

2842e87

andrewlock reviewed Mar 12, 2024

View reviewed changes

e-n-0 approved these changes Mar 12, 2024

View reviewed changes

Refactor converters initialization

d098e98

andrewlock approved these changes Mar 12, 2024

View reviewed changes

daniel-romano-DD merged commit 233695a into master Mar 12, 2024
53 of 56 checks passed

daniel-romano-DD deleted the dani/iast/evidence_redaction_truncation branch March 12, 2024 16:06

github-actions bot added this to the vNext milestone Mar 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[IAST] Vulnerability and Evidence truncation #5302

[IAST] Vulnerability and Evidence truncation #5302

daniel-romano-DD commented Mar 11, 2024 •

edited

Loading

datadog-ddstaging bot commented Mar 11, 2024 •

edited

Loading

andrewlock commented Mar 11, 2024 •

edited

Loading

github-actions bot commented Mar 12, 2024

andrewlock Mar 12, 2024

daniel-romano-DD Mar 12, 2024 •

edited

Loading

andrewlock Mar 12, 2024

daniel-romano-DD Mar 12, 2024

andrewlock Mar 12, 2024

andrewlock left a comment

andrewlock Mar 12, 2024

e-n-0 left a comment

[IAST] Vulnerability and Evidence truncation #5302

[IAST] Vulnerability and Evidence truncation #5302

Conversation

daniel-romano-DD commented Mar 11, 2024 • edited Loading

Summary of changes

Reason for change

Implementation details

Test coverage

datadog-ddstaging bot commented Mar 11, 2024 • edited Loading

Datadog Report

New Flaky Tests (1)

andrewlock commented Mar 11, 2024 • edited Loading

Execution-Time Benchmarks Report ⏱️

github-actions bot commented Mar 12, 2024

Snapshots difference summary

andrewlock Mar 12, 2024

Choose a reason for hiding this comment

daniel-romano-DD Mar 12, 2024 • edited Loading

Choose a reason for hiding this comment

andrewlock Mar 12, 2024

Choose a reason for hiding this comment

daniel-romano-DD Mar 12, 2024

Choose a reason for hiding this comment

andrewlock Mar 12, 2024

Choose a reason for hiding this comment

andrewlock left a comment

Choose a reason for hiding this comment

andrewlock Mar 12, 2024

Choose a reason for hiding this comment

e-n-0 left a comment

Choose a reason for hiding this comment

daniel-romano-DD commented Mar 11, 2024 •

edited

Loading

datadog-ddstaging bot commented Mar 11, 2024 •

edited

Loading

andrewlock commented Mar 11, 2024 •

edited

Loading

daniel-romano-DD Mar 12, 2024 •

edited

Loading