github / codeql

CodeQL: the libraries and queries that power security researchers around the world, as well as code scanning in GitHub Advanced Security
https://codeql.github.com
MIT License
7.66k stars 1.53k forks source link

Go: False positive for go/index-out-of-bounds #9300

Open cannist opened 2 years ago

cannist commented 2 years ago

Observed on LGTM:

https://lgtm.com/projects/g/caddyserver/caddy/snapshot/d8e8a8ca215a4ed7b519656ecbe3c081cd1a840a/files/modules/caddyhttp/rewrite/caddyfile.go?sort=name&dir=ASC&mode=heatmap#x5ef0d8f84bad448a:1

The read operation that is referred to by the alert is guarded by a later length check.

owen-mc commented 2 years ago

Thanks for reporting this. It does seem like a false positive. At some point I will have a look and see if it's worth making the query more complicated to deal with this kind of situation, which I imagine doesn't come up that much.

owen-mc commented 2 years ago

I have looked into this further. It is due to a known shortcoming of our modelling of switch statements. The fix is rather involved and we may not get around to it for a while.

owen-mc commented 1 year ago

For future reference after lgtm shuts down, the code looks like this:

                if len(args) < 2 {
                        return nil, h.ArgErr()
                }
                switch args[0] {
                case "strip_prefix":
                        if len(args) > 2 {
                                return nil, h.ArgErr()
                        }
                        rewr.StripPathPrefix = args[1]
                        if !strings.HasPrefix(rewr.StripPathPrefix, "/") {
                                rewr.StripPathPrefix = "/" + rewr.StripPathPrefix
                        }
                case "strip_suffix":
                        if len(args) > 2 {
                                return nil, h.ArgErr()
                        }
                        rewr.StripPathSuffix = args[1]
                case "replace":
                        var find, replace, lim string
                        switch len(args) {
                        case 4:
                                lim = args[3]
                                fallthrough
                        case 3:
                                find = args[1]
                                replace = args[2]
                        default:
                                return nil, h.ArgErr()
                        }

The alert message is "Off-by-one index comparison against length may lead to out-of-bounds read". The alert location is the first liine I have quoted and the read links to replace = args[2].