Repository navigation
internal/graph: escape file and title strings written to dot - #1053
Medios-lgtm wants to merge 3 commits into
Conversation
Mapping/object file names and the graph title come from the profile and are attacker-controlled, but three sites wrote them into the dot output without escaping: the digraph name in start(), the legend tooltip in addLegend(), and the File/Objfile components of a node label in multilinePrintableName(). A double quote in any of them closes the quoted attribute and lets the profile inject further dot attributes (for example a URL=), which Graphviz emits as an xlink:href in the SVG that the web UI renders inline. Wrap each site in escapeForDot, matching the sibling node and edge tooltips that already escape.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1053 +/- ##
==========================================
+ Coverage 73.42% 73.45% +0.03%
==========================================
Files 46 46
Lines 7936 7938 +2
==========================================
+ Hits 5827 5831 +4
+ Misses 2086 2084 -2
Partials 23 23 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| compareGraphs(t, buf.Bytes(), "compose7.dot") | ||
| } | ||
|
|
||
| func TestComposeEscapesFileAndTitleInDot(t *testing.T) { |
There was a problem hiding this comment.
Should we instead update the existing test like in https://github.com/google/pprof/pull/1032/changes?
One thing I like about this test here though is that it doesn't depend on the file stored elsewhere and encodes the checks directly. But maybe we can update that existing test to do this for all escaping checks universally and remove the testdata *.dot file?
There was a problem hiding this comment.
Moved the checks into TestComposeWithNamesThatNeedEscaping and dropped compose7.dot. It now covers the function name, file, objfile, title and legend label, and asserts directly that each one shows up escaped and never raw, so the existing name cases are still checked without the golden file. Still fails on the three unescaped sites without the fix.
…atNeedEscaping Fold the file, objfile and title cases into the existing escaping test and have it assert on the escaped and unescaped forms of each string directly, so the compose7.dot golden is no longer needed.
aalexand
left a comment
aalexand
left a comment
There was a problem hiding this comment.
Thank you for working on this.
| for _, raw := range []string{`evil.go" peripheries=3`, `evil.so" peripheries=3`, `prof" bgcolor="red`} { | ||
| if strings.Contains(out, raw) { | ||
| t.Errorf("unescaped %q in dot output allows attribute injection:\n%s", raw, out) | ||
| for _, s := range []string{`var"src"`, `file"name.go`, `obj"file.so`, `test"title`, `label3: "foo"`} { |
There was a problem hiding this comment.
label3 does not have a character to escape I think? Or am I missing something?
There was a problem hiding this comment.
It did, but only indirectly: baseAttrsAndConfig defines that label as label3: "foo", so the quote was hiding in the helper and the test was leaning on it to cover the legend labels that compose7.dot used to check. I made it explicit by setting c.Labels in the test to label"text so the character being escaped is visible right there.
| for _, escaped := range []string{`evil.go\" peripheries=3`, `evil.so\" peripheries=3`, `prof\" bgcolor=\"red`} { | ||
| if !strings.Contains(out, escaped) { | ||
| t.Errorf("expected escaped %q in dot output:\n%s", escaped, out) | ||
| if want := escapeForDot(s); !strings.Contains(got, want) { |
There was a problem hiding this comment.
Maybe include the expected escaped strings also directly in the test, without calling this same function that the code is calling? I think that way it would be more visual / explicit.
There was a problem hiding this comment.
Makes sense. Switched it to a table of raw/escaped pairs with the expected strings written out literally (e.g. var"src" -> var\"src\"), so the test no longer calls escapeForDot itself. Still fails on the file, objfile and title sites without the fix.
Mapping/object file names and the graph title come from the profile, but three sites in
internal/graph/dotgraph.gowrote them into the generated dot without escaping: thedigraphname instart(), the legendtooltipinaddLegend(), and theFile/Objfilecomponents of a node label inmultilinePrintableName(). A"in any of them closes the quoted attribute, so a crafted profile can inject dot attributes such asURL=, which Graphviz turns into anxlink:hrefin the SVG the web UI renders inline. The sibling node and edge tooltips already run throughescapeForDot; this wraps the three missed sites the same way.Repro: a profile whose mapping file name is
lib" URL="javascript:alert(1)renders, before this change, an<a xlink:href="javascript:alert(1)">inpprof -svg/-http; after it the quote is escaped and no such link appears.TestComposeWithNamesThatNeedEscapingnow checks the escaping directly instead of againstcompose7.dot(removed); it fails on the three unescaped sites and passes with the fix.