Note that the %q format directive escapes newlines, and therefore prevents log injection #658
Conversation
…vents log injection
ql/lib/semmle/go/StringOps.qll
Outdated
| module Formatting { | ||
| /** | ||
| * Gets a regular expression for matching simple format-string components, including flags, | ||
| * width and precision specifiers, but not including `*` specifiers or explicit argument |
There was a problem hiding this comment.
(I know this code is just being moved, so you didn't introduce this.) It seems to me that this regex does support * specifiers, so perhaps we should update the qldoc. Looking at the history it seems it was always this way, so perhaps I am missing something.
Also, would it be worth updating this to include explicit argument indices? There are only so many places they can go, so it seems fiddly but doable to me.
There was a problem hiding this comment.
I've amended this to mention that * specifiers are indeed accepted, but explicit indices are not.
We can include them, but we would need to do more work besides since both * specifiers and [n] specifiers (whether on a * or on the main formatting verb) require us to do a stateful left-to-right parse rather than our current stateless parse in order to get the arg <-> format verb alignment correct. I'd suggest pursuing this as a separate task.
| formatDirective = this.getComponent(n) and | ||
| formatDirective.charAt(0) = "%" and | ||
| formatDirective.charAt(1) != "%" and | ||
| result = this.getArgument((n / 2) + f.getFirstFormattedParameterIndex()) |
There was a problem hiding this comment.
(I know this code is just being moved, so you didn't introduce this.) It seems to me that the n / 2 assumes that the format string starts with a literal, and hence alternates literal, verb, literal, verb, ... . If the format string starts with a literal won't we get verb, literal, verb, literal, ... Actually I guess we could have two verbs in a row, and mess things up that way as well. I guess the fix would be to replace n / 2 with count(int m | 0 <= m and m < n and not this.getComponent(m).regexpMatch(getLiteralRegex()) | m).
There was a problem hiding this comment.
The two alternating cases are fine, since the verbs either have indices 0, 2, 4, 6 or 1, 3, 5, 7, and integer division rounds down, so in either case n / 2 is 0, 1, 2, 3.
The case where there are successive verbs without any interspersed literal is however broken as you say. To fix, now with somewhat increased priority since that's going to happen pretty frequently.
Rename class Co-authored-by: Owen Mansel-Chan <[email protected]>
|
I've done the easy fixes here, and suggest we make three followups with increasing difficulty and decreasing priority:
|
There was a problem hiding this comment.
I've created issues https://github.com/github/codeql-go-team/issues/284, https://github.com/github/codeql-go-team/issues/285 and https://github.com/github/codeql-go-team/issues/286 as you suggested.
No description provided.