False positive: Go / MongoDB Find method

[yogurt-ui]

trafficstars

Description of the false positive

Code samples or links to source code

https://github.com/github/codeql/blob/dc440aaee6695deb0d9676b87e06ea984e1b4ae5/go/ql/src/Security/CWE-089/SqlInjection/

The following code has a large number of vulnerability false positives in the case of a MongoDB database. The current MongoDB parameters have defined specific data types, and there are no injection vulnerabilities.

type LogFilter struct {
    ID             []string
}
filter *LogFilter
filterM["id"] = filter.ID
cur, err := dl.Find(ctx, filterM, opts)

-->

[jketema]

Hi @yogurt-ui

Thank you for this false positive report. Resolving this issue is not a current product priority, but we acknowledge the report and will track it internally for future consideration, or if we observe repeated instances of the same problem.

[owen-mc]

@yogurt-ui I would like to make sure that we understand your issue. I assume dl in your code excerpt is a mongo.Collection. We consider it a (no)sql injection if any user-controlled data can flow to the second argument to Collection.Find. Do you disagree with this? Or do you think that in your specific situation, user-controlled data cannot flow there? If so, I would need to see more information about the alerts to understand what is going wrong.

[yogurt-ui]

@yogurt-ui I would like to make sure that we understand your issue. I assume dl in your code excerpt is a mongo.Collection. We consider it a (no)sql injection if any user-controlled data can flow to the second argument to Collection.Find. Do you disagree with this? Or do you think that in your specific situation, user-controlled data cannot flow there? If so, I would need to see more information about the alerts to understand what is going wrong. Sorry, I missed a condition，filterM is a structured map type, so it does not pose an injection risk. type LogFilter struct { ID []string } filter *LogFilter filterM := bson.M{} filterM["id"] = filter.ID cur, err := dl.Find(ctx, filterM, opts)

[owen-mc]

Are you saying that if the second argument of Collection.Find has type bson.M then there cannot be a no-sql injection attack - perhaps because it will be escaped? If so, can you point me to some documentation of that? I wasn't able to find any. On the contrary, the documentation for bson.M says "There's no special handling for this type in addition to what's done anyway for an equivalent map type.", and I found this article which says that it is vulnerable.

It might help if you could point me to a FP detected by code scanning, if there is one publicly visible.

[yogurt-ui]

The type bson.M can be a no-sql injection attack,but Currently filterM["id"] = filter.ID, "id" is fixed, filter.ID defines a specific type and will only be used as an array value, and will not parse the user input value,This is equivalent to bson.M{"id":filter.ID} or map[string]interface{}{ "id": filter.ID}

[yogurt-ui]

At the same time, the SQL injection rules of Go, when the parameter is of this type(map[string]interface{}{"id": filter.ID}) and filter.ID is a SQL query that can be controlled by the user, there is a false positive,example: filterM=map[string]interface{}{"id": filter.ID} db.Where(filterM) the code uses map[string]interface as the query condition. When map is used as a parameter to pass to the "Where" method, GORM will automatically convert it into a query using parameter binding. filter.ID will be regarded as a parameter instead of being directly spliced ​​into the SQL statement, which has prevented sgl injection.

[owen-mc]

Thanks, I think I understand the point better now.