LGTM.com - false positive Golang varargs confused re item in args vs len of varargs

[philpennock]

Description of the false positive

The complaint is This operation, which is used in an allocation, involves a potentially large value and might overflow.; the Go function declaration uses args ...interface{} for varargs, and the code is:

t := make([]interface{}, 3, len(args)+3)

The potentially large object in the complaint paths is not being passed into this function as separate parameters, so even if it were passed in, the contribution to len(args) for that one object would be 1, not varying upon the size of the object.

In fact, I'm actually passing in len(largeObject) as a parameter, and this length value is triggering, so that might be a second false positive, that len(X) does not de-taint the largeness of X.

https://lgtm.com/projects/g/PennockTech/ocsprenewer/snapshot/f3884a03c6ffbf224bd431611a0131480d205a2c/files/renew/log.go?sort=name&dir=ASC&mode=heatmap#xf1b87a2499b61794:1

[smowton]

Interesting problem, and not unique to varargs. For example:

func f(request *http.Request) []byte {

  f, _ := ioutil.ReadAll(request.Body)
  array := make([]int, 1) 
  array[0] = len(f)
  alloc := make([]byte, len(array)+3)
  return alloc

}

len(f) really is tainted, we write it to array, that taints the whole array, then we can't differentiate "tainted array" from "array with one or more tainted cells" when we evaluate len(array).

[smowton]

Candidate solution: github/codeql-go#374

it's difficult to avoid this large-array vs. array-containing-large-numbers distinction with our current dataflow framework, but we can exclude len calls whose operands don't receive a large array. That PR breaks the query into two pieces, one that identifies potentially suspect len calls without actually introducing the problematic len rule, then another that selectively allows those identified calls to propagate taint.

I'll have to seek input from more experienced people as to whether that's going way too far to hack around not being able to distinguish taint types!

[smowton]

That PR has been merged, so this should be fixed as of the next release