zpages: G203: The used method does not auto-escape HTML

[pellared]

Description

Issue detected by gosec

golangci-lint ./zpages
templates.go:70:10: G203: The used method does not auto-escape HTML. This can potentially lead to 'Cross-site Scripting' vulnerabilities, in case the attacker controls the input. (gosec)
                return template.HTML(fmt.Sprintf(`trace_id: <b style="color:%s">%s</b> span_id: %s parent_span_id: %s`, col, r.SpanContext.TraceID(), r.SpanContext.SpanID(), r.ParentSpanContext.SpanID()))
                       ^
templates.go:72:9: G203: The used method does not auto-escape HTML. This can potentially lead to 'Cross-site Scripting' vulnerabilities, in case the attacker controls the input. (gosec)
        return template.HTML(fmt.Sprintf(`trace_id: <b style="color:%s">%s</b> span_id: %s`, col, r.SpanContext.TraceID(), r.SpanContext.SpanID()))
               ^

Expected behavior

Refactor the code to get rid of this issue. The spanRowFormatter function can use a HTML template to generate auto-escaped HTML.

[aliakseip]

I am starting to work on this issue.

[AkhigbeEromo]

Hello @pellared can i be assigned to this issue

[pellared]

@AkhigbeEromo Done.

[AkhigbeEromo]

Thank you, i just submitted a PR request

[dmathieu]

I've given this a look, and I wonder if it's worth fixing. The template content doesn't seem it can be controlled by an attacker. The dynamic attributes are TraceID, SpanID and Parent.SpanID, which must all be 32 or 16 bytes (making it harder to perform an attack on such a small size), and their content is actually controlled by us in SpanContext.

zPages are also probably not something folks would use in production, but rather to look into their development/test traces. Production data is usually accessed through a real observability solution. Not using zpages in production makes the attack area non-existent.

Based on this, I'm thinking we could update the golint comment to mention this shouldn't actually be a problem, and close this issue.

For added safety, we could call template.EscapeHTMLString.

[AkhigbeEromo]

Thank you very much for review @dmathieu i tried using template.EscapeHTMLString, but was still getting the same error.

Can i close the PR I submitted as this might no longer be an issue again @pellared

[pellared]

This is not only about possible attacks, but also about possible unproperly rendered content.

We should use template.EscapeHTMLString and ignore false-positive lint errors.

[dmathieu]

This is not only about possible attacks, but also about possible unproperly rendered content.

We shouldn't escape the full content, as we output HTML tags on purpose. Only the input should be escaped, which is span and trace IDs.

Those are byte arrays encoded and decoded by encoding/hex. It can contain only hexadecimal values. We ensure that in the SDK. https://github.com/open-telemetry/opentelemetry-go/blob/9e34895a3e917727167073a9719a16d862cddf9e/trace/trace.go#L128-L137

But even if someone were to use their own SDK, remove that check, and somehow have a manually crafted Trace/Span ID, it could only contain hexadecimal values, none of which are escaped by HTML.

Which is why, as mentioned in my previous comment, using EscapeHTMLString on those hexadecimal values is not necessary.