[ASM] IAST Header injection vulnerability detection. #4981
Conversation
| return ( | ||
| taintedValue.Ranges.Length == 1 && | ||
| taintedValue.Ranges[0].Source is not null && | ||
| taintedValue.Ranges[0].Source!.OriginByte == (byte)SourceTypeName.RequestHeaderValue && |
There was a problem hiding this comment.
I think OriginByte is not needed (it is not serialized) and makes code reading more difficult. It should be a SourceType directly
There was a problem hiding this comment.
Do you mean comparing the string value instead of the byte? This would be more expensive in terms of performance than just comparing a single byte. The byte value should always be present even if we don't serialize it.
There was a problem hiding this comment.
I mean using the enum instead of a raw byte
tracer/src/Datadog.Trace/Iast/SensitiveData/HeaderInjectionTokenizer.cs
Outdated
Show resolved
Hide resolved
tracer/test/Datadog.Trace.Security.IntegrationTests/IAST/AspNetCore5IastTests.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Daniel Romano <108014683+daniel-romano-DD@users.noreply.github.com>
| AnalyzeHeaderInjectionVulnerability(responseHeaders, integrationId); | ||
| } | ||
|
|
||
| #if NETFRAMEWORK |
There was a problem hiding this comment.
I agree, I suggest extracting this as two different methods, and then extracting the body of the method to a shared helper function
| continue; | ||
| } | ||
|
|
||
| string headerValue = responseHeaders[headerKey]; |
There was a problem hiding this comment.
In .NET Core, the return value is StringValues. By casting to a string, you will throw an exception if there are actually multiple header values.
A better option is to handle the possibility of there being more than one value set, and handle it appropriately. Luckily, I wrote a blog post about this 3 days ago 😂 https://andrewlock.net/a-brief-look-at-stringvalues/
Also, just FYI, if the same header is set multiple times in .NET FX NameValueCollection will return them here using string.Join(",", values). That may be ok for you here, but I don't know if you have anything in the spec about how to handle this situation (i.e. the header is set more than once)?
There was a problem hiding this comment.
I have updated the code to handle multiple values in one header for both netcore and net framework. In Net framework, I have used GetValues to get all the values and avoid the join. I have also added integration tests that cover this case. Thank you!
There was a problem hiding this comment.
In Net framework, I have used GetValues to get all the values and avoid the join.
So there's definitely a question of whether we should do that tbh... By using GetValues() we're always allocating an array for every accessed header. I wonder if it's actually a problem or not if we use GetValue() and join the values with a ','? Happy with whatever you decide, just wanted to flag it 🙂
There was a problem hiding this comment.
The problem with the "," separated values is that we loose the tainting since the join is done in libraries that we don't instrument, so we would not be able to know if the value is tainted.
| if (separatorStart > 0) | ||
| { | ||
| var separatorEnd = separatorStart + IastModule.HeaderInjectionEvidenceSeparator.Length; | ||
| var valuePart = evidence.Substring(separatorEnd); |
There was a problem hiding this comment.
nit: you may able to avoid the substring here in .NET Core? I haven't checked if Regex works on spans, but might be worth a look
There was a problem hiding this comment.
Actually, the overload public bool IsMatch(string input, int startat) can be used also in .Net frameowrk. Thanks!
tracer/src/Datadog.Trace/Iast/SensitiveData/HeaderInjectionTokenizer.cs
Outdated
Show resolved
Hide resolved
| } | ||
| catch (Exception ex) | ||
| { | ||
| Log.Error(ex, $"Error in {nameof(VulnerabilityBatch)}.{nameof(ToJson)}"); |
There was a problem hiding this comment.
Just wondering when this might throw? We need to be careful using Log.Error if it could fail in "normal" execution, because it can flood the logs (and also flood our telemetry)
There was a problem hiding this comment.
There should be no exceptions in normal execution inside this code. If we have an exception here, we should take a look because there will probably be a bug or unexpected situation.
| "value": ": " | ||
| }, | ||
| { | ||
| "value": ":bearer secret", |
There was a problem hiding this comment.
I assumed this should be redacted if it's sensitive, no? 🤔
There was a problem hiding this comment.
In this test, redaction is disabled. The enabled version is in Iast.HeaderInjection.AspNetCore2.Vuln.SensitiveValue.RedactionEnabled.verified.txt
| }, | ||
| { | ||
| "redacted": true, | ||
| "pattern": "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789abcdefghijklmnopqrs", |
There was a problem hiding this comment.
Where is this value coming from, I can't seem to find it? 🤔
There was a problem hiding this comment.
This is a redaction pattern that is replacing the real value
…enizer.cs Co-authored-by: Andrew Lock <andrew.lock@datadoghq.com>
…-trace-dotnet into nacho/headerInjection
| if (_keyPattern.IsMatch(evidence.Substring(0, separatorStart)) || | ||
| _valuePattern.IsMatch(evidence, separatorEnd)) | ||
| { | ||
| return [new Range(separatorEnd, evidence.Length - separatorEnd)]; | ||
| } |
There was a problem hiding this comment.
I think you need to add a try-catch here in case the regexes timeout?
There was a problem hiding this comment.
The exceptions here would be catch in VulnerabilityBatch.ToJson(), that calls this method. Still, thinking about it, it's worth to add a catch for the RegexMatchTimeoutException here since I don't think that this kind of exception should launch an error but a warning. If any other exception happens, an error should be launched. Done! Thanks!
|
Thank very much you for your feedback! |
Summary of changes
This PR contains the implementation of the IAST header injection vulnerability detection.
The header injection vulnerability is launched when a header added to the response contains values that are tainted (coming from untrusted sources).
The full description of the vulnerability can be found here: https://datadoghq.atlassian.net/wiki/spaces/APS/pages/3295019091/Header+Injection
Reason for change
It's in the IAST roadmap
Implementation details
Test coverage
Other details