Implementation of the basic classes that will be used in IAST and the insecure hashing vulnerability detection#3225
Merged
Merged
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
robertpi
marked this pull request as draft
September 19, 2022 09:31
Member
|
Converted this to a draft, as it has failing tests that need to be addressed. |
NachoEchevarria
force-pushed
the
nacho/insecureHashing
branch
from
October 3, 2022 10:13
5aa2ad9 to
572426c
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
NachoEchevarria
force-pushed
the
nacho/insecureHashing
branch
from
October 3, 2022 15:24
987640b to
9bf508e
Compare
This comment has been minimized.
This comment has been minimized.
NachoEchevarria
force-pushed
the
nacho/insecureHashing
branch
from
October 3, 2022 16:57
12ccdf1 to
4d19941
Compare
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.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
NachoEchevarria
force-pushed
the
nacho/insecureHashing
branch
from
October 4, 2022 13:47
fd46fa9 to
82c9a3e
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
NachoEchevarria
marked this pull request as ready for review
October 5, 2022 09:24
NachoEchevarria
force-pushed
the
nacho/insecureHashing
branch
from
October 5, 2022 09:26
1ca29db to
1abf9e3
Compare
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.
This comment has been minimized.
Co-authored-by: Lucas Pimentel-Ordyna <[email protected]>
Co-authored-by: Lucas Pimentel-Ordyna <[email protected]>
…/Program.cs Co-authored-by: Andrew Lock <[email protected]>
Co-authored-by: Andrew Lock <[email protected]>
…/Samples.WeakHashing.csproj Co-authored-by: Andrew Lock <[email protected]>
Co-authored-by: Lucas Pimentel-Ordyna <[email protected]>
NachoEchevarria
force-pushed
the
nacho/insecureHashing
branch
from
October 13, 2022 10:07
a47cb2d to
710cb4c
Compare
Member
Benchmarks Report 🐌Benchmarks for #3225 compared to master:
The following thresholds were used for comparing the benchmark speeds:
Allocation changes below 0.5% are ignored. Benchmark detailsBenchmarks.Trace.AgentWriterBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.AppSecBodyBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.AspNetCoreBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.DbCommandBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.ElasticsearchBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.GraphQLBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.HttpClientBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.ILoggerBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.Log4netBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.NLogBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.RedisBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.SerilogBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.SpanBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.TraceAnnotationsBenchmark - Same speed ✔️ Same allocations ✔️Raw results
|
Member
Code Coverage Report 📊
View the full report for further details: Datadog.Trace Breakdown
|
| master | #3225 | Change | |
|---|---|---|---|
| Lines % | 73% |
72% |
-1% |
| Branches % | 69% |
69% |
-1% |
| Complexity | 17944 |
18113 |
169 ⛔ |
The following classes have significant coverage changes.
| File | Line coverage change | Branch coverage change | Complexity change |
|---|---|---|---|
| Datadog.Trace.Agent.TraceSamplers.AnalyticsEventsSampler | -100% ⛔ |
-100% ⛔ |
0 ✔️ |
| Datadog.Trace.Agent.TraceSamplers.ErrorSampler | -100% ⛔ |
-100% ⛔ |
0 ✔️ |
| Datadog.Trace.Agent.TraceSamplers.PrioritySampler | -100% ⛔ |
0% ✔️ |
0 ✔️ |
| Datadog.Trace.Sampling.GlobalSamplingRule | -100% ⛔ |
0% ✔️ |
0 ✔️ |
| Datadog.Trace.Agent.TraceSamplers.RareSampler | -89% ⛔ |
-96% ⛔ |
0 ✔️ |
| Datadog.Trace.Util.SamplingHelpers | -50% ⛔ |
-50% ⛔ |
0 ✔️ |
| Datadog.Trace.Telemetry.Transports.JsonTelemetryTransport | -33% ⛔ |
-42% ⛔ |
0 ✔️ |
| Datadog.Trace.Logging.DirectSubmission.DirectLogSubmissionManager | -33% ⛔ |
-33% ⛔ |
0 ✔️ |
| Datadog.Trace.Logging.DirectSubmission.Sink.NullDatadogSink | -25% ⛔ |
0% ✔️ |
0 ✔️ |
| Datadog.Trace.Processors.NormalizerTraceProcessor | -22% ⛔ |
-19% ⛔ |
0 ✔️ |
| ...And 8 more |
The following classes were added in #3225:
| File | Line coverage | Branch coverage | Complexity |
|---|---|---|---|
| Datadog.Trace.ClrProfiler.AutoInstrumentation.HashAlgorithm.HashAlgorithmIntegration | 100% |
100% |
2 |
| Datadog.Trace.ClrProfiler.AutoInstrumentation.HashAlgorithm.HashAlgorithmIntegrationBis | 100% |
100% |
2 |
| Datadog.Trace.ClrProfiler.AutoInstrumentation.HashAlgorithm.HashAlgorithmIntegrationCommon | 50% |
58% |
13 |
| Datadog.Trace.ClrProfiler.AutoInstrumentation.HashAlgorithm.HashAlgorithmIntegrationTer | 100% |
100% |
2 |
| Datadog.Trace.Iast.Evidence | 100% |
100% |
1 |
| ...And 11 more |
View the full reports for further details:
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary of changes
Implementation of insecure hashing and some of the basic classes that will be used in IAST, taking as a model the Java implementation.
Reason for change
The IAST implementation starts with the insecure hashing implementation.
Implementation details
Implementation of insecure hashing and some of the basic classes that will be used in IAST, taking as a model the Java implementation.
Work included:
Hashing instrumentation for netcore.
Weak hashing detection and span creation.
Basic IAST infrastructure classes and configuration.
Integration tests for weak hashing calls.
Work NOT included:
Implement the request related variables:
https://datadoghq.atlassian.net/wiki/spaces/APS/pages/2604597520/ASM+IAST+Configuration+Properties
Implement DD_IAST_MAX_CONCURRENT_REQUESTS
Implement DD_IAST_MAX_CONTEXT_OPERATIONS
Implement DD_IAST_REQUEST_SAMPLING
Avoid sending repeated vulnerabilities within a request
Net framework support is not implemented in this PR: it would require to instrumentate mscorlib
Notes:
Right now, with this PR, we send a span everytime a weak hashing method is called. In the future, if we are in a request, we will send all the vulnerabilities after the request finishes and we will eliminate the repeated ones.
In the IastJson label of the span, a Json is sent containing the vulnerability data. The filename and line number of the vulnerability should be sent if possible. Otherwise, the full methodName is sent.
We were using in the tracer the class MD5 for uuid generation. This algorithm is considered insecure and would raise a span. The callsite instrumentation will avoid us sending spans generated from our own code. Anyway, the MD5 class has been replaced by a secure one.
Right now, we always set the IastEnabled tag to "1" in the span, but in the future, it might be zero to indicate that a request has not been analyzed
This is the actual tag field of the span:
Tags: {
_dd.iast.enabled: 1,
_dd.iast.json:
{
"Vulnerabilities": [
{
"Type": "WEAK_HASH",
"Location": {
"Path": "c:\myfile.cs",
"Line": 50
},
"Evidence": {
"Value": "System.Security.Cryptography.HMACMD5"
}
}
]
}
}
Test coverage
Other details