RUMM-2377 Add checks on intake request headers #1005
Conversation
| "Datadog/$sdkVersion " + | ||
| sanitizeHeaderValue(System.getProperty(SYSTEM_UA)) | ||
| .ifBlank { | ||
| "Datadog/${sanitizeHeaderValue(sdkVersion)} " + |
There was a problem hiding this comment.
not even sure if the sdkVersion should be sanitized at all
There was a problem hiding this comment.
The sdkVersion will possibly be updatable from crossplatform SDKs, meaning it will be using public APIs. So in theory, it could have an invalid character yes
Codecov Report
@@ Coverage Diff @@
## develop #1005 +/- ##
===========================================
+ Coverage 83.19% 83.22% +0.03%
===========================================
Files 270 270
Lines 9209 9212 +3
Branches 1463 1468 +5
===========================================
+ Hits 7661 7666 +5
+ Misses 1137 1134 -3
- Partials 411 412 +1
|
| } | ||
|
|
||
| private fun isValidHeaderValueChar(c: Char): Boolean { | ||
| return c == '\t' || c in '\u0020'..'\u007F' |
There was a problem hiding this comment.
A couple of questions:
- isn't 7F a control char? shouldn't be excluded?
- are you sure that
\tdoesn't trigger the vuln?
There was a problem hiding this comment.
\t indeed is a valid character (all ascii whitespace chars are allowed : , \n and \t. But you're right 0x7f should be excluded.
| System.getProperty(SYSTEM_UA).let { | ||
| if (it.isNullOrBlank()) { | ||
| "Datadog/$sdkVersion " + | ||
| sanitizeHeaderValue(System.getProperty(SYSTEM_UA)) |
There was a problem hiding this comment.
Curiosity: Why don't you trust SYSTEM_UA property but you trust your androidInfoProvider, which relays on Build class, which in turns take information from system properties?
There was a problem hiding this comment.
Mostly because the syntax for User Agent can become very sketchy and more likely to have weird characters.
Also the Build.xxx will be already fetched from the app zygote, whereas the UA is retrieved at Runtime, making it possible for the host app to override it.
There was a problem hiding this comment.
Got your point. It is a good trade-off.
xgouchet
force-pushed
the
xgouchet/RUMM-2377/sanitize_upload_headers
branch
from
4c85864
to
ecb6433
Compare
| System.getProperty(SYSTEM_UA).let { | ||
| if (it.isNullOrBlank()) { | ||
| "Datadog/$sdkVersion " + | ||
| sanitizeHeaderValue(System.getProperty(SYSTEM_UA)) |
There was a problem hiding this comment.
Got your point. It is a good trade-off.
What does this PR do?
Sanitize our intake request headers to be complient with the HTTP specification
Motivation
Before OkHttp version
4.9.2(cf okhttp#6551), if a header value contains illegal characters (any non ASCII or Control character), the header would be printed as is in the logcat. This could potentially leak secrets (especially our customer's client token).This is not a critical issue since:
/pub[0-9a-f]+/, so they never have any invalid charactersAdditional Notes
We are still sanitizing every header that might contain value that we do not control.
Review checklist (to be filled by reviewers)