[ASM] Deduplication of vulnerabilities by NachoEchevarria · Pull Request #3371 · DataDog/dd-trace-dotnet

NachoEchevarria · 2022-10-19T13:54:54Z

Summary of changes

Implementation of an option, DD_IAST_DEDUPLICATION_ENABLED, enabled by default, that avoids reporting a vulnerability if it has already been reported during the execution of an App.

Reason for change

It is a requirement for all the IAST implementations across teams.

Implementation details

Test coverage

Other details

andrewlock

LGTM, with minor comments 👍

andrewlock · 2022-10-20T10:26:59Z

+        bool newVulnerability;
+        lock (vulnerabilityHashes)
+        {
+            newVulnerability = vulnerabilityHashes.Add(hashCode);


Just to point out, even if you don't want the extra memory of a queue, you could use a ConcurrentDictionary<int, bool> instead of a HashSet<int> to avoid all the extra locks etc.

andrewlock · 2022-10-20T10:30:25Z

+    [Trait("RunOnWindows", "True")]
+    [InlineData(false)]
+    [InlineData(true)]
+    public async Task SubmitsTraces(bool deduplicationEnabled)


nit: I would personally prefer to re-use the existing weak cipher etc tests for this, rather than adding an extra sample. Every extra sample we add slows down the build stage, which slows down every other stage in CI. Not required, but would be preferable IMO 🙂

Yes, I thought about it. The problem is that for deduplication, some .net versions are better than others when providing the file and line in the stack, and some provide the method name instead the file (as expected, the newest are better). This means that the behaviour of the deduplication in the already existing tests varies slightly depending on the framework because the information that it receives is slightly different. Reusing the already existing tests would require to cover a few different cases that would increase the complexity, so, finally, I decided to go for a new, very simple, test app.

andrewlock · 2022-10-20T10:31:28Z

    [Trait("RunOnWindows", "True")]
    public async Task SubmitsTraces()
    {
+        SetEnvironmentVariable("DD_IAST_DEDUPLICATION_ENABLED", "false");


As noted above, I think it would be preferable to make these theory :)

andrewlock · 2022-10-20T10:33:33Z

+    public class HashBasedDeduplicationTests
+    {
+        [Fact]
+        public void GivenTheSameVulnerabilityInstance_WhenAddedToDeduplication_OnlyOneIsStored()


This is not a correct usage of CollectionDefinition 🙂 But you can just delete it anyway, as this is the only type that uses it - tests within a single class are always run in serial. You only need to disable parallelization when you have multiple test classes using the same shared objects. Which you avoid wherever possible 😄

Couldn't we switch to a singleton with an internal .ctor to ease testing

+1 on this 🙂

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

…ithub.com/DataDog/dd-trace-dotnet into nacho/HashBasedVulnerabilityDeduplication

github-actions · 2022-10-21T13:38:31Z

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.

62 occurrences of :

+      _dd.origin: iast,

andrewlock · 2022-10-21T14:44:52Z

Benchmarks Report 🐌

Benchmarks for #3371 compared to master:

All benchmarks have the same speed
All benchmarks have the same allocations

The following thresholds were used for comparing the benchmark speeds:

Mann–Whitney U test with statistical test for significance of 5%
Only results indicating a difference greater than 10% and 0.3 ns are considered.

Allocation changes below 0.5% are ignored.

Benchmark details

Benchmarks.Trace.AgentWriterBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`WriteAndFlushEnrichedTraces`	net472	811μs	434ns	1.68μs	0.403	3.22 KB
master	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	558μs	338ns	1.27μs	0	2.63 KB
#3371	`WriteAndFlushEnrichedTraces`	net472	811μs	461ns	1.79μs	0.403	3.22 KB
#3371	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	555μs	372ns	1.44μs	0	2.63 KB

Benchmarks.Trace.AppSecBodyBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`AllCycleSimpleBody`	net472	196ns	0.0447ns	0.161ns	0.0701	0	441 B
master	`AllCycleSimpleBody`	netcoreapp3.1	254ns	0.17ns	0.659ns	0.00601	0	440 B
master	`AllCycleMoreComplexBody`	net472	197ns	0.0723ns	0.28ns	0.0663	0	417 B
master	`AllCycleMoreComplexBody`	netcoreapp3.1	255ns	0.27ns	1.05ns	0.00555	0	416 B
master	`BodyExtractorSimpleBody`	net472	276ns	0.173ns	0.646ns	0.0574	0	361 B
master	`BodyExtractorSimpleBody`	netcoreapp3.1	258ns	0.175ns	0.679ns	0.00377	0	272 B
master	`BodyExtractorMoreComplexBody`	net472	16.1μs	7.21ns	27ns	1.2	0.0161	7.62 KB
master	`BodyExtractorMoreComplexBody`	netcoreapp3.1	13.3μs	5.12ns	18.5ns	0.0923	0	6.75 KB
#3371	`AllCycleSimpleBody`	net472	199ns	0.104ns	0.402ns	0.0701	0	441 B
#3371	`AllCycleSimpleBody`	netcoreapp3.1	254ns	0.216ns	0.837ns	0.006	0	440 B
#3371	`AllCycleMoreComplexBody`	net472	199ns	0.0715ns	0.268ns	0.0663	0	417 B
#3371	`AllCycleMoreComplexBody`	netcoreapp3.1	251ns	0.123ns	0.475ns	0.00579	0	416 B
#3371	`BodyExtractorSimpleBody`	net472	269ns	0.09ns	0.337ns	0.0573	0	361 B
#3371	`BodyExtractorSimpleBody`	netcoreapp3.1	240ns	0.152ns	0.57ns	0.00372	0	272 B
#3371	`BodyExtractorMoreComplexBody`	net472	15.7μs	12.3ns	46.2ns	1.21	0.0156	7.62 KB
#3371	`BodyExtractorMoreComplexBody`	netcoreapp3.1	13μs	3.88ns	14.5ns	0.0909	0	6.75 KB

Benchmarks.Trace.AspNetCoreBenchmark - Unknown 🤷 Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`SendRequest`	net472	0ns	0ns	0ns	0	0 b
master	`SendRequest`	netcoreapp3.1	178μs	276ns	1.07μs	0.267	20.66 KB
#3371	`SendRequest`	net472	0ns	0ns	0ns	0	0 b
#3371	`SendRequest`	netcoreapp3.1	178μs	169ns	653ns	0.267	20.66 KB

Benchmarks.Trace.DbCommandBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`ExecuteNonQuery`	net472	1.81μs	1.2ns	4.64ns	0.157	0.000909	995 B
master	`ExecuteNonQuery`	netcoreapp3.1	1.42μs	0.463ns	1.67ns	0.0135	0	984 B
#3371	`ExecuteNonQuery`	net472	1.79μs	0.4ns	1.55ns	0.158	0.00089	995 B
#3371	`ExecuteNonQuery`	netcoreapp3.1	1.43μs	0.451ns	1.69ns	0.0136	0	984 B

Benchmarks.Trace.ElasticsearchBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`CallElasticsearch`	net472	2.57μs	0.855ns	3.2ns	0.191	1.2 KB
master	`CallElasticsearch`	netcoreapp3.1	1.54μs	0.628ns	2.27ns	0.0154	1.14 KB
master	`CallElasticsearchAsync`	net472	2.65μs	1.34ns	5.01ns	0.212	1.34 KB
master	`CallElasticsearchAsync`	netcoreapp3.1	1.63μs	0.513ns	1.92ns	0.0171	1.26 KB
#3371	`CallElasticsearch`	net472	2.48μs	1.87ns	7.26ns	0.19	1.2 KB
#3371	`CallElasticsearch`	netcoreapp3.1	1.57μs	0.509ns	1.84ns	0.0149	1.14 KB
#3371	`CallElasticsearchAsync`	net472	2.52μs	3ns	11.6ns	0.213	1.34 KB
#3371	`CallElasticsearchAsync`	netcoreapp3.1	1.59μs	0.397ns	1.48ns	0.0167	1.26 KB

Benchmarks.Trace.GraphQLBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`ExecuteAsync`	net472	2.64μs	1.84ns	7.14ns	0.233	1.47 KB
master	`ExecuteAsync`	netcoreapp3.1	1.72μs	0.709ns	2.46ns	0.0188	1.39 KB
#3371	`ExecuteAsync`	net472	2.69μs	1.68ns	6.06ns	0.232	1.47 KB
#3371	`ExecuteAsync`	netcoreapp3.1	1.77μs	1.18ns	4.4ns	0.0186	1.39 KB

Benchmarks.Trace.HttpClientBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`SendAsync`	net472	5.95μs	2.16ns	8.09ns	0.445	2.82 KB
master	`SendAsync`	netcoreapp3.1	3.71μs	1.02ns	3.68ns	0.0353	2.65 KB
#3371	`SendAsync`	net472	5.88μs	1.27ns	4.74ns	0.447	2.82 KB
#3371	`SendAsync`	netcoreapp3.1	3.83μs	1.64ns	6.36ns	0.0368	2.65 KB

Benchmarks.Trace.ILoggerBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`EnrichedLog`	net472	3.19μs	4.13ns	15.4ns	0.297	1.88 KB
master	`EnrichedLog`	netcoreapp3.1	2.55μs	2.59ns	9.68ns	0.0256	1.91 KB
#3371	`EnrichedLog`	net472	3.2μs	3.38ns	13.1ns	0.297	1.88 KB
#3371	`EnrichedLog`	netcoreapp3.1	2.65μs	1.73ns	6.71ns	0.0264	1.91 KB

Benchmarks.Trace.Log4netBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`EnrichedLog`	net472	150μs	66.3ns	257ns	0.748	0.224	4.72 KB
master	`EnrichedLog`	netcoreapp3.1	118μs	68.3ns	237ns	0.0591	0	4.55 KB
#3371	`EnrichedLog`	net472	151μs	177ns	687ns	0.683	0.228	4.72 KB
#3371	`EnrichedLog`	netcoreapp3.1	121μs	177ns	685ns	0.0603	0	4.55 KB

Benchmarks.Trace.NLogBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`EnrichedLog`	net472	5.84μs	2.37ns	9.18ns	0.579	0.00293	3.65 KB
master	`EnrichedLog`	netcoreapp3.1	4.4μs	1.26ns	4.55ns	0.0551	0	3.98 KB
#3371	`EnrichedLog`	net472	5.79μs	3.22ns	12.5ns	0.579	0.00289	3.65 KB
#3371	`EnrichedLog`	netcoreapp3.1	4.36μs	1.61ns	6.23ns	0.055	0	3.98 KB

Benchmarks.Trace.RedisBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`SendReceive`	net472	2.22μs	4.44ns	17.2ns	0.225	1.42 KB
master	`SendReceive`	netcoreapp3.1	1.84μs	0.609ns	2.28ns	0.0184	1.37 KB
#3371	`SendReceive`	net472	2.34μs	2.63ns	10.2ns	0.225	1.42 KB
#3371	`SendReceive`	netcoreapp3.1	1.79μs	0.656ns	2.37ns	0.0187	1.37 KB

Benchmarks.Trace.SerilogBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`EnrichedLog`	net472	5.07μs	1.53ns	5.92ns	0.363	2.3 KB
master	`EnrichedLog`	netcoreapp3.1	4.19μs	1.54ns	5.76ns	0.0254	1.86 KB
#3371	`EnrichedLog`	net472	4.95μs	2.84ns	10.2ns	0.364	2.3 KB
#3371	`EnrichedLog`	netcoreapp3.1	4.35μs	1.43ns	5.14ns	0.0239	1.86 KB

Benchmarks.Trace.SpanBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`StartFinishSpan`	net472	1.23μs	0.308ns	1.15ns	0.136	859 B
master	`StartFinishSpan`	netcoreapp3.1	936ns	0.354ns	1.33ns	0.0112	808 B
master	`StartFinishScope`	net472	1.49μs	0.35ns	1.26ns	0.149	939 B
master	`StartFinishScope`	netcoreapp3.1	1.05μs	0.479ns	1.85ns	0.0125	928 B
#3371	`StartFinishSpan`	net472	1.21μs	0.383ns	1.33ns	0.136	859 B
#3371	`StartFinishSpan`	netcoreapp3.1	989ns	0.356ns	1.38ns	0.0112	808 B
#3371	`StartFinishScope`	net472	1.46μs	0.626ns	2.34ns	0.149	939 B
#3371	`StartFinishScope`	netcoreapp3.1	1.04μs	0.6ns	2.25ns	0.0125	928 B

Benchmarks.Trace.TraceAnnotationsBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`RunOnMethodBegin`	net472	1.57μs	0.352ns	1.32ns	0.149	939 B
master	`RunOnMethodBegin`	netcoreapp3.1	1.16μs	0.389ns	1.45ns	0.0124	928 B
#3371	`RunOnMethodBegin`	net472	1.44μs	0.525ns	2.03ns	0.149	939 B
#3371	`RunOnMethodBegin`	netcoreapp3.1	1.16μs	0.42ns	1.63ns	0.0122	928 B

andrewlock · 2022-10-21T15:28:35Z

Code Coverage Report 📊

✔️ Merging #3371 into master will not change line coverage
✔️ Merging #3371 into master will not change branch coverage
⛔ Merging #3371 into master will will increase complexity by 25

	master	#3371	Change
Lines	`19616` / `27081`	`19591` / `27108`
Lines %	`72%`	`72%`	`0%` ✔️
Branches	`11602` / `16764`	`11607` / `16782`
Branches %	`69%`	`69%`	`0%` ✔️
Complexity	`18146`	`18171`	`25` ⛔

View the full report for further details:

Datadog.Trace Breakdown ✔️

	master	#3371	Change
Lines %	`72%`	`72%`	`0%` ✔️
Branches %	`69%`	`69%`	`0%` ✔️
Complexity	`18146`	`18171`	`25` ⛔

The following classes have significant coverage changes.

File	Line coverage change	Branch coverage change	Complexity change
Datadog.Trace.Ci.GitInfo	`-17%` ⛔	`-11%` ⛔	`0` ✔️

The following classes were added in #3371:

File	Line coverage	Branch coverage	Complexity
Datadog.Trace.Iast.HashBasedDeduplication	`82%`	`75%`	`6`
Datadog.Trace.Iast.IastUtils	`100%`	`83%`	`6`

View the full reports for further details:

dromanol · 2022-10-24T09:13:49Z

+
+    public override int GetHashCode()
+    {
+        return IastUtils.GetHashCode(new object?[] { Path, Line });


Maybe having a two params version to skip the new object[]?

dromanol · 2022-10-24T09:14:14Z

+
+    public override int GetHashCode()
+    {
+        return IastUtils.GetHashCode(new object[] { Origin, Name, Value });


Same here with three 🙈

dromanol

LGTM

NachoEchevarria changed the title ~~Nacho/hash based vulnerability deduplication~~ Deduplication of vulnerabilities Oct 19, 2022

NachoEchevarria added 29 commits October 19, 2022 16:13

Instrumentation of basic hashing types

dd6eadb

Test case. Exclude by black list approach.

9dcd3fb

Instrumentate async computehash call

46af117

Rename variables.

d62af1d

Rename interation folder

0a663fb

Add some basic IAST classes

00a3724

Add IAST configuration

720851e

Use new IAST category

14426d0

Add a IAST module.

94d438a

Move files.

b4899e1

Serialize vulnerability JSON

1ba038f

Move files

8c1df4d

Move weak hasing sample

106337e

Include filtered filename and line in the span.

802a98f

Improve InsecureHashingAlgorithms configuration

df29085

Add nullable enable to classes

cc1b7ad

Avoid insecure hashing spans form DD code.

ed20e71

Add new _dd.iast.enabled tag

0f23bd7

add integration tests

3e96731

Improve stack trace detection. Add some integration tests snapshots.

efe1ff2

Merge the net50/net60 differentiation in the integration tests

5231e77

Use IAST java classes to build the Json and get the right format

68bbabd

Move integration test file

7131356

Add category to hash algorithm instrumentation

3027ef7

Rename hashing tags to iastTags

90ca0cb

Fix hash tests

92ae9c1

remove not needed file

72fe562

remove not needed files

370c1b4

lowcase property names

d3e02a1

Avoid race conditions for HashBasedDeduplicationTests

65e1fa8

andrewlock approved these changes Oct 20, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

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

c1d677b

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

This comment has been minimized.

Sign in to view

NachoEchevarria added 4 commits October 20, 2022 15:27

Make HashBasedDeduplication as singleton

9094a18

Merge branch 'nacho/HashBasedVulnerabilityDeduplication' of https://g…

b2f1493

…ithub.com/DataDog/dd-trace-dotnet into nacho/HashBasedVulnerabilityDeduplication

Simplify HashBasedDeduplication

84e1a24

fix compilation error.

b05ef9e

This comment has been minimized.

Sign in to view

Add origin tag to new vulnerability spans

2ff6e6a

This comment has been minimized.

Sign in to view

NachoEchevarria changed the title ~~Deduplication of vulnerabilities~~ [ASM] Deduplication of vulnerabilities Oct 21, 2022

NachoEchevarria added the area:asm label Oct 21, 2022

anna-git reviewed Oct 21, 2022

View reviewed changes

Comment thread tracer/test/Datadog.Trace.Security.IntegrationTests/IAST/Deduplicaation/DeduplicationTests.cs

Spell correction.

3cd62bf

NachoEchevarria requested a review from anna-git October 21, 2022 15:53

dromanol reviewed Oct 24, 2022

View reviewed changes

dromanol approved these changes Oct 24, 2022

View reviewed changes

NachoEchevarria merged commit 30aecfe into master Oct 24, 2022

NachoEchevarria deleted the nacho/HashBasedVulnerabilityDeduplication branch October 24, 2022 10:03

github-actions Bot added this to the vNext milestone Oct 24, 2022

NachoEchevarria mentioned this pull request Oct 27, 2022

[ASM] IAST request management #3384

Merged

Uh oh!

Conversation

NachoEchevarria commented Oct 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of changes

Reason for change

Implementation details

Test coverage

Other details

Uh oh!

andrewlock left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

andrewlock Oct 20, 2022

Choose a reason for hiding this comment

Uh oh!

andrewlock Oct 20, 2022

Choose a reason for hiding this comment

Uh oh!

NachoEchevarria Oct 20, 2022

Choose a reason for hiding this comment

Uh oh!

andrewlock Oct 20, 2022

Choose a reason for hiding this comment

Uh oh!

andrewlock Oct 20, 2022

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Uh oh!

github-actions Bot commented Oct 21, 2022

Snapshots difference summary

Uh oh!

andrewlock commented Oct 21, 2022

Benchmarks Report 🐌

Benchmark details

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Uh oh!

andrewlock commented Oct 21, 2022

Code Coverage Report 📊

Datadog.Trace Breakdown ✔️

Uh oh!

dromanol Oct 24, 2022

Choose a reason for hiding this comment

Uh oh!

dromanol Oct 24, 2022

Choose a reason for hiding this comment

Uh oh!

dromanol left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

NachoEchevarria commented Oct 19, 2022 •

edited

Loading