false negative when analyze the url parameters about gin framework

[dialectically]

False negative report

Use this issue template to describe a situation where the analyzer failed to recognize that a piece of unsafe code is unsafe. For example, if the analyzer did not produce a report on the following piece of code, then that would be a false negative:

func ginhandler(c *gin.Context) {
    name := c.Query("name") // url parameter name set to source
    c.String(200, name)
    log.Println(name) //sink
}

func test123() {
    r := gin.Default()
    r.GET("/hello", ginhandler)
    r.Run()
}

(We are assuming that Source has been configured as a source and Sink has been configured as a sink.)

Describe the issue Please include a clear and concise description of what happened and why you think it is a false negative. I hava configured the levee and did some tests. It is ok with net/http.Form setted as source and log.Println as sink. However when I test the code I showed, levee can't recognize it as an unsafe case.

To Reproduce Please make it as easy as possible for us to reproduce what you observed. If possible, provide the exact configuration and code on which the analyzer failed to produce a report. If the code cannot be shared, please provide a simplified example and confirm that it also contains the false negative.

config file ` { "Sources": [ { "PackageRE": "github.com/gin-gonic/gin", "TypeRE": "Context", "FieldRE": "" } ], "Sinks": [ { "PackageRE": "^log$", "MethodRE": "Print" } ]

} `

command: levee -config=config.json example.go

Additional context Add any other context about the problem here.

The main point here is that I want to do some analysis on the url parameters' taint, especially the gin framework, thanks for your time to help me with this.

[mlevesquedion]

Hello @4huo!

Sorry for the late reply, it seems I missed the notification.

Thank you for your report! If I understand correctly, since you identified the Context type as a source, you are expecting the value returned by c.Query("name") (where c is c *gin.Context) to be tainted. Can you confirm this? Is it just the "name" key you are worried about, or anything returned by c.Query? Are you expecting other methods to also return tainted values?

Also, can you tell me what version of the analyzer you used, ideally a git commit SHA if possible? (e.g., if you built from source, or if you used git install or go get with a go.mod file). If that's not available, do you have a rough idea of when you installed the analyzer?

[dialectically]

Hello @4huo!

Sorry for the late reply, it seems I missed the notification.

Thank you for your report! If I understand correctly, since you identified the Context type as a source, you are expecting the value returned by c.Query("name") (where c is c *gin.Context) to be tainted. Can you confirm this? Is it just the "name" key you are worried about, or anything returned by c.Query? Are you expecting other methods to also return tainted values?

Also, can you tell me what version of the analyzer you used, ideally a git commit SHA if possible? (e.g., if you built from source, or if you used git install or go get with a go.mod file). If that's not available, do you have a rough idea of when you installed the analyzer?

[dialectically]

Thanks for replying. Yes, I would expect the value returned by c.Query("name") and I am worried about all returned by c.Query cause I want to get any input from url parameter which pass to some sink points. So the "name" key is just an example.

I used go get with a go.mod file to get binarey file and I installed it about ten days ago.

And Sir, I was wondering if you could reproduce this scene I showed.Is my configure file wrong or somethings wired happens? I truely want to understand how levee works and learn to do some developing.

[mlevesquedion]

Thank you for providing these additional details!

Your configuration looks fine to me. The reason you are getting a "false negative" in this case is that, given a source type, levee doesn't automatically consider that any method declared on that source type returns tainted values. Indeed, this tends to produce far too many false positives. What we do instead is we try to look for methods that return source fields. In your config since you use "FieldRE": "", which means all the fields on the Context type are considered sources. However, judging from gin/context.go, the Query method does not directly return a source field, so the analyzer does not think that it returns a source value. (This is a limitation in the implementation.)

We do not provide a straightforward way to indicate that a given function/method returns a source value. You could do it by using a function summary (this requires you to write a small wrapper around levee):

// main.go

package main

import (
    "github.com/google/go-flow-levee/pkg/levee"
    "golang.org/x/tools/go/analysis/singlechecker"
)

func main() {
    // This says that if the Context value is tainted, then the return value of the target function will be tainted.
    // (The Context should always be tainted, given that it is a source type.)
    querySummary := levee.Summary{
        IfTainted:   0b1,
        TaintedRets: []int{0},
    }
    levee.FuncSummaries["(*github.com/gin-gonic/gin.Context).Query"] = querySummary
    singlechecker.Main(levee.Analyzer)
}

// go.mod

module levee-wrapper

go 1.16

require (
    github.com/google/go-flow-levee v0.1.5
    golang.org/x/tools v0.0.0-20200416214402-fc959738d646
)

If you can afford to wait a little, I think some upcoming improvements may enable the analyzer to trace taint properly in the case that you presented, without having to mess with function summaries.

[dialectically]

Thanks very much. That makes sense and I get it. I think I should read some code of levee and try to understand its main concept.