codeql icon indicating copy to clipboard operation
codeql copied to clipboard

Go: `template/text.Template` execution methods: support reading arbitrary content

Open smowton opened this issue 1 year ago • 1 comments
trafficstars

NB. This requires adding a hook to shared dataflow; this will need adding (as an empty stub) to other languages.

smowton avatar Oct 08 '24 16:10 smowton

:warning: The head of this PR and the base branch were compared for differences in the framework coverage reports. The generated reports are available in the artifacts of this workflow run. The differences will be picked up by the nightly job after the PR gets merged.

Click to show differences in coverage

go

Generated file changes for go

  • Changes to framework-coverage-go.rst:
-    `Standard library <https://pkg.go.dev/std>`_,"````, ``archive/*``, ``bufio``, ``bytes``, ``cmp``, ``compress/*``, ``container/*``, ``context``, ``crypto``, ``crypto/*``, ``database/*``, ``debug/*``, ``embed``, ``encoding``, ``encoding/*``, ``errors``, ``expvar``, ``flag``, ``fmt``, ``go/*``, ``hash``, ``hash/*``, ``html``, ``html/*``, ``image``, ``image/*``, ``index/*``, ``io``, ``io/*``, ``log``, ``log/*``, ``maps``, ``math``, ``math/*``, ``mime``, ``mime/*``, ``net``, ``net/*``, ``os``, ``os/*``, ``path``, ``path/*``, ``plugin``, ``reflect``, ``reflect/*``, ``regexp``, ``regexp/*``, ``slices``, ``sort``, ``strconv``, ``strings``, ``sync``, ``sync/*``, ``syscall``, ``syscall/*``, ``testing``, ``testing/*``, ``text/*``, ``time``, ``time/*``, ``unicode``, ``unicode/*``, ``unsafe``",52,605,104
+    `Standard library <https://pkg.go.dev/std>`_,"````, ``archive/*``, ``bufio``, ``bytes``, ``cmp``, ``compress/*``, ``container/*``, ``context``, ``crypto``, ``crypto/*``, ``database/*``, ``debug/*``, ``embed``, ``encoding``, ``encoding/*``, ``errors``, ``expvar``, ``flag``, ``fmt``, ``go/*``, ``hash``, ``hash/*``, ``html``, ``html/*``, ``image``, ``image/*``, ``index/*``, ``io``, ``io/*``, ``log``, ``log/*``, ``maps``, ``math``, ``math/*``, ``mime``, ``mime/*``, ``net``, ``net/*``, ``os``, ``os/*``, ``path``, ``path/*``, ``plugin``, ``reflect``, ``reflect/*``, ``regexp``, ``regexp/*``, ``slices``, ``sort``, ``strconv``, ``strings``, ``sync``, ``sync/*``, ``syscall``, ``syscall/*``, ``testing``, ``testing/*``, ``text/*``, ``time``, ``time/*``, ``unicode``, ``unicode/*``, ``unsafe``",52,603,104
-    Totals,,459,943,1532
+    Totals,,459,941,1532
  • Changes to framework-coverage-go.csv:
- text/template,,,6,,,,,,,,,,,,,,,,,,,,,,,6,
+ text/template,,,4,,,,,,,,,,,,,,,,,,,,,,,4,

github-actions[bot] avatar Oct 17 '24 14:10 github-actions[bot]

@smowton What is the status of this? Is it waiting for review, or are you still working on it?

owen-mc avatar Dec 17 '24 13:12 owen-mc

@owen-mc thanks to my currently blurry work situation I found time to fix it up. Currently running QA; please do review.

smowton avatar Dec 17 '24 15:12 smowton

QA results: new TPs from around 20 repos. Found and fixed an FP based on defining an interface that could be text/template.Template or could be html/template.Template (the latter is sanitizing, and in practice text/template is not used).

Significant slowdowns in two, which so far as I can see are simply caused by significantly more flow being possible when text-template instantiation is able to pull taint down from fields to top level. I propose to go ahead on grounds that the vast majority of repositories suffer no consequences, and that is real flow (though we could iterate to try to find heuristics to prevent excessively-expensive exploration if necessary).

smowton avatar Dec 19 '24 14:12 smowton

I rebased this on latest main and reran QA: found one large performance outlier, yaklang/yaklang. Tested whether the dataflow performance improvement I discovered before Christmas works there too; started a DCA experiment to check and reported more fully at https://github.com/github/codeql/pull/18355

smowton avatar Jan 15 '25 18:01 smowton

Performance issue with yaklang resolved by https://github.com/github/codeql/pull/18515

smowton avatar Jan 20 '25 12:01 smowton