Go: Promote go/uncontrolled-allocation-size from experimental

[atorralba]

Promotes the query go/uncontrolled-allocation-size (previously go/denial-of-service) from experimental. Sinks and barriers have been reused from AllocationSizeOverflow due to their similarities.

This adds coverage for CVE-2023-37279 and CVE-2023-2253.

[github-actions]

QHelp previews:

go/ql/src/Security/CWE-770/UncontrolledAllocationSize.qhelp

Slice memory allocation with excessive size value

Using untrusted input to allocate slices with the built-in make function could lead to excessive memory allocation and potentially cause the program to crash due to running out of memory. This vulnerability could be exploited to perform a denial-of-service attack by consuming all available server resources.

Recommendation

Implement a maximum allowed value for size allocations with the built-in make function to prevent excessively large allocations.

Example

In the following example snippet, the n parameter is user-controlled.

If the external user provides an excessively large value, the application allocates a slice of size n without further verification, potentially exhausting all the available memory.

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

• OWASP: Denial of Service Cheat Sheet
• Common Weakness Enumeration: CWE-770.

[owen-mc]

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.

[owen-mc]

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.

[atorralba]

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.

[owen-mc]

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]

Looks great, thanks! Only one tiny thing that's probably not necessary to change 👍