Escape user input used in output #2815
Conversation
Signed-off-by: Juraci Paixão Kröhling <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #2815 +/- ##
=======================================
Coverage 91.67% 91.67%
=======================================
Files 293 293
Lines 15635 15635
=======================================
Hits 14333 14333
Misses 891 891
Partials 411 411
Continue to review full report at Codecov.
|
| @@ -441,7 +442,7 @@ func (jr *jReceiver) HandleThriftHTTPBatch(w http.ResponseWriter, r *http.Reques | |||
|
|
|||
| batch, hErr := jr.decodeThriftHTTPBody(r) | |||
| if hErr != nil { | |||
| http.Error(w, hErr.msg, hErr.statusCode) | |||
| http.Error(w, html.EscapeString(hErr.msg), hErr.statusCode) | |||
There was a problem hiding this comment.
nit: (Morally) shouldn't this be also done below on the consumeTraces error too? I don't think it poses a security risk now but it's better to be safe than sorry
There was a problem hiding this comment.
We are doing this here because this error message might include user-provided input, namely, the content-type header which is returned embedded into the errors from lines 411 and 417 from decodeThriftHTTPBody. The one you mentioned doesn't seem to embed any user-provided input.
There was a problem hiding this comment.
The one you mentioned doesn't seem to embed any user-provided input.
Sure, not saying it does, just that it's easier/safer to escape things everywhere rather than doing it on a per-case basis. Just being nitpicky, feel free to dismiss this :)
There was a problem hiding this comment.
There was a warning about this specific instance in an automated security checking tool, so I would like to fast-track it. But in general, I'd agree that everything that goes back to clients should be properly escaped. Perhaps we could have a broader discussion/guideline for this, as I suspect other components might be affected too.
|
The check failures don't seem to be related to this change. |
Description:
Escape the error message sent to clients, containing user-provided input.
Theoretically, this could allow an attacker to perform a reflected cross-site scripting attack. In my view, this would be hardly exploitable, as the Jaeger Receiver has Jaeger Client as the only clients, and browsers would have to be tricked into making requests directly to it with the bad payload. On the way out, the browser would interpret the Jaeger Receiver as a single domain, as each protocol is on its own port. No session data, or any other type of senstive information, would be available for the attacker to extract.
In any case, this here fixes the theoretical case, especially to prevent this from being used in a composite attack in a scenario that I can't think of...