Go: Promote go/uncontrolled-allocation-size from experimental#15843
Go: Promote go/uncontrolled-allocation-size from experimental#15843atorralba merged 4 commits intogithub:mainfrom
go/uncontrolled-allocation-size from experimental#15843Conversation
|
QHelp previews: go/ql/src/Security/CWE-770/UncontrolledAllocationSize.qhelpSlice memory allocation with excessive size valueUsing untrusted input to allocate slices with the built-in RecommendationImplement a maximum allowed value for size allocations with the built-in ExampleIn the following example snippet, the If the external user provides an excessively large value, the application allocates a slice of size package main
import (
"encoding/json"
"fmt"
"net/http"
"strconv"
)
func OutOfMemoryBad(w http.ResponseWriter, r *http.Request) {
query := r.URL.Query()
queryStr := query.Get("n")
collectionSize, err := strconv.Atoi(queryStr)
if err != nil {
http.Error(w, err.Error(), http.StatusBadRequest)
return
}
result := make([]string, collectionSize)
for i := 0; i < collectionSize; i++ {
result[i] = fmt.Sprintf("Item %d", i+1)
}
w.Header().Set("Content-Type", "application/json")
json.NewEncoder(w).Encode(result)
}One way to prevent this vulnerability is by implementing a maximum allowed value for the user-controlled input, as seen in the following example: package main
import (
"encoding/json"
"fmt"
"net/http"
"strconv"
)
func OutOfMemoryGood(w http.ResponseWriter, r *http.Request) {
query := r.URL.Query()
MaxValue := 6
queryStr := query.Get("n")
collectionSize, err := strconv.Atoi(queryStr)
if err != nil {
http.Error(w, err.Error(), http.StatusBadRequest)
return
}
if collectionSize < 0 || collectionSize > MaxValue {
http.Error(w, "Bad request", http.StatusBadRequest)
return
}
result := make([]string, collectionSize)
for i := 0; i < collectionSize; i++ {
result[i] = fmt.Sprintf("Item %d", i+1)
}
w.Header().Set("Content-Type", "application/json")
json.NewEncoder(w).Encode(result)
}References
|
go/uncontrolled-allocation-size from experimental
owen-mc
left a comment
There was a problem hiding this comment.
Looks great. One observation, which doesn't require any code change unless you can think of a way of sharing code with other queries nicely.
|
|
||
| predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) { | ||
| exists(Function f, DataFlow::CallNode cn | cn = f.getACall() | | ||
| f.hasQualifiedName("strconv", ["Atoi", "ParseInt", "ParseUint", "ParseFloat"]) and |
There was a problem hiding this comment.
I note that we have the Strconv::IntegerParser class, which covers 3 out of 4 of these. I assume ParseFloat is in this list because the float could later be cast to an integer?
I also note that there is an identical isAdditionalFlowStep for DivideByZero.ql. I don't think we should try to share the code, though, unless you can think of something very nice.
There was a problem hiding this comment.
How harmful would it be to make these models-as-data summaries (i.e. globally applicable)? I'd expect that queries not interested in numeric types already discard them in some way (even if implicitly by having incompatible sources/sinks), so the impact shouldn't be too high.
But if we did this I think it should be handled in a different PR to better analyze the impact.
There was a problem hiding this comment.
Yes, I think that would be a good idea. If you control the input to one of those functions then you control the output.
subatoi
left a comment
There was a problem hiding this comment.
Looks great, thanks! Only one tiny thing that's probably not necessary to change 👍
Co-authored-by: Ben Ahmady <32935794+subatoi@users.noreply.github.com>
Promotes the query
go/uncontrolled-allocation-size(previouslygo/denial-of-service) from experimental. Sinks and barriers have been reused fromAllocationSizeOverflowdue to their similarities.This adds coverage for CVE-2023-37279 and CVE-2023-2253.