Go: template/text.Template execution methods: support reading arbitrary content
#17701
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| or | ||
| defaultImplicitTaintReadGlobal(node, c) |
There was a problem hiding this comment.
This also needs to be done in the corresponding file in tainttracking2.
| any(ImplicitFieldReadNode ifrn).shouldImplicitlyReadAllFields(node) and | ||
| getElementTypeIncludingFields*(node.getType()) = containerType |
There was a problem hiding this comment.
I think it would be slightly easier to understand if you replace this with or defaultImplicitTaintReadGlobal(node, c). (In theory you could also remove these lines and the overall functionality would be the same, but I think best to not do that.)
There was a problem hiding this comment.
Do you think this new test folder is best in go/ql/test/library-tests/semmle/go/frameworks? All the other folders in there are for tests of particular libraries. What about in go/ql/test/library-tests/semmle/go/dataflow?
go/ql/test/library-tests/semmle/go/frameworks/serialization/texttemplate.go
Show resolved
Hide resolved
smowton
force-pushed
the
smowton/feature/read-fields-before-executetemplate
branch
from
c0cbb4a to
63ff0cb
Compare
Click to show differences in coveragegoGenerated file changes for go
- `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``",34,604,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``",34,602,104
- Totals,,308,928,1532
+ Totals,,308,926,1532
- text/template,,,6,,,,,,,,,,,,,,,,,,,,,,6,
+ text/template,,,4,,,,,,,,,,,,,,,,,,,,,,4, |
smowton
force-pushed
the
smowton/feature/read-fields-before-executetemplate
branch
from
fe808f7 to
d822d14
Compare
|
@smowton What is the status of this? Is it waiting for review, or are you still working on it? |
|
@owen-mc thanks to my currently blurry work situation I found time to fix it up. Currently running QA; please do review. |
Otherwise it's too easy to define a common interface to both text/template, which doesn't sanitize, and html/template, which does.
|
QA results: new TPs from around 20 repos. Found and fixed an FP based on defining an interface that could be 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). |
Observations: * revFlowThrough can be much larger than the other reverse-flow predicates, presumably when there are many different innerReturnAps. * It is only ever used in conjunction with flowThroughIntoCall, which can therefore be pushed in, and several of its parameters can thereby be dropped in exchange for exposing `arg`. * `revFlowThroughArg` can then be trivially inlined. Result: on repository `go-gitea/gitea` with PR github#17701 producing a wider selection of access paths than are seen on `main`, `revFlowThrough` drops in size from ~120m tuples to ~4m, and the runtime of the reverse-flow computation for dataflow stage 4 goes from dominating the forward-flow cost to relatively insignificant. Overall runtime falls from 3 minutes to 2 with substantial ram available, and presumably falls much more under GHA-style memory pressure.
Observations: * revFlowThrough can be much larger than the other reverse-flow predicates, presumably when there are many different innerReturnAps. * It is only ever used in conjunction with flowThroughIntoCall, which can therefore be pushed in, and several of its parameters can thereby be dropped in exchange for exposing `arg`. * `revFlowThroughArg` can then be trivially inlined. Result: on repository `go-gitea/gitea` with PR github#17701 producing a wider selection of access paths than are seen on `main`, `revFlowThrough` drops in size from ~120m tuples to ~4m, and the runtime of the reverse-flow computation for dataflow stage 4 goes from dominating the forward-flow cost to relatively insignificant. Overall runtime falls from 3 minutes to 2 with substantial ram available, and presumably falls much more under GHA-style memory pressure.
Add a mechanism for specifying dataflow nodes that ought read taint through any access path, and use it to make
template/textfunctions read taint out of fields.Note this PR currently has #18355 applied because it appears to significantly improve performance for some outlier repositories. This shouldn't be merged until that PR has been dealt with.