LGTM.com - false positive go-path-injection despite using `path.Clean`

[thediveo]

Description of the false positive

https://codeql.github.com/codeql-query-help/go/go-path-injection/ triggers despite using Go stdlib path.Clean to sanitize user input before constructing a sandboxed filesystem path using path.Join(h.staticPath, uriPath).

path.Clean ensures that the user-supplied path cannot escape its root, see https://pkg.go.dev/path#Clean. path.Join using a well-defined sandbox root path ensures that access is kept within the sandbox for serving files.

URL to the alert on the project page on LGTM.com

https://lgtm.com/projects/g/TheDiveO/lxkns/snapshot/29308eb48388242ce0069aab9760f2dcceb84b29/files/cmd/lxkns/server.go?sort=name&dir=ASC&mode=heatmap#xaab8d054ed837544:1

[smowton]

Noting that prefixing with / before calling Clean is necessary, as you do here, but I'm happy to recognise and exclude that pattern. This is probably simple enough to make a Friday fix :)

[thediveo]

Thank you!

[thediveo]

Maybe https://codeql.github.com/codeql-query-help/go/go-path-injection/ could be updated, too? While it currently shows what is bad code, it sadly doesn't show a good example. Could you please add using path.Clean("/"+path) as a good example to help programmers to quickly iron out vulnerable code?

[thediveo]

Any news on this? I had recent repo updates on master and the false positive still is very alive...

[smowton]

github/codeql-go#678 should pick up your case I believe.

[smowton]

Fix is in, should appear in the next LGTM distribution upgrade cycle (1-2 weeks from now)

[thediveo]

Unfortunately, this still doesn't look good to me after over a month and a recent fresh run. I still get this false positive reported on LGTM again and again. While I understand that things take time, this now looks to be stalled.

[smowton]

Gah, the difference is filepath.Clean vs. path.Clean. A quick experiment suggests the santiizer works for both methods under both forward-slash and backslash-using environments, so github/codeql-go#700 should fix it, plus another LGTM upgrade cycle (again, 1-2 weeks).

[thediveo]

argh, totally missed the different packages ... thanks for spotting this!