Go: new query for detect DOS vulnerability

[Malayke]

This is part of All for one, one for all query submission, I'm going to submit an issue in github/securitylab for this pull request too.

slices created with the built-in make function from user-controlled sources using a maliciously large value possibly leading to a denial of service.

[ghsecuritylab]

Hello Malayke 👋
You have submitted this pull request as a bug bounty report in the github/securitylab repository and therefore this pull request has been put into draft state to give time for the GitHub Security Lab to assess the PR. When GitHub Security Lab has finished assessing your pull request, it will be marked automatically as Ready for review. Until then, please don't change the draft state.

In the meantime, feel free to make changes to the pull request. If you'd like to maximize payout for your this and future submissions, here are a few general guidelines, that we might take into consideration when reviewing a submission.

• the submission models widely-used frameworks/libraries
• the vulnerability modeled in the submission is impactful
• the submission finds new true positive vulnerabilities
• the submission finds very few false positives
• code in the submission is easy to read and will be easy to maintain
• documentation is written clearly, highlighting the impact of the issue it finds and is written without grammatical or other errors. The code samples clearly show the vulnerability
• the submission includes tests, change note etc.

Please note that these are guidelines, not rules. Since we have a lot of different types of submissions, the guidelines might vary for each submission.

Happy hacking!

[owen-mc]

Are you aware of the existing query go/ql/src/Security/CWE-190/AllocationSizeOverflow.ql? This seems very closely related. Can you comment on whether they overlap?

[Malayke]

I carefully read the code of AllocationSizeOverflow. If I understand correctly, AllocationSizeOverflow restricts the input of the source to the Marshal function, and the query focuses on whether there will be an overflow.

According to my testing, when the make slice is very large and exceeds the maximum value, it will trigger a panic error, but the program will continue to run normally and not cause denial of service. If it does not exceed the maximum value, the program will run normally but consume a large amount of memory. In this case, the program will exhaust the memory resources, leading to denial of service.

The query I wrote specifically checks whether the size of the make slice is directly defined by an untrusted source. From the results of the two queries, it seems that AllocationSizeOverflow cannot detect the vulnerabilities of the two CVEs I mentioned.

[owen-mc]

This is looking good. I've given a few fairly minor comments, but overall this looks in good shape and I expect it will get merged without too many changes.

[owen-mc]

@Malayke Do you intend to address the review comments so this can be merged and your bounty submission can be completed?

[Malayke]

Hi @owen-mc, I have addressed all of your comments. Sorry for the late reply. I have been very busy these days.

[owen-mc]

One more minor comment.

[github-actions]

QHelp previews:

go/ql/src/experimental/CWE-770/DenialOfService.qhelp

Denial Of Service

Using untrusted input to created 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 DoS attack by consuming all available server resources.

Recommendation

Implement a maximum allowed value for creates a slice with the built-in make function to prevent excessively large allocations. For instance, you could restrict it to a reasonable upper limit.

Example

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

The server trusts that n has an acceptable value, however when using a maliciously large value, it allocates a slice of n of strings before filling the slice with data.

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:

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]

I've fixed a minor formatting problem, and merged main in and updated the test expectations. CI is still failing because of a problem with the query metadata. I've made a suggestion which I think fixes it, but I'll leave it to you to check that it's what you intended.

[Malayke]

hi @owen-mc I'm committed your suggestion.

[owen-mc]

Thank you very much for this contribution.

[Malayke]

Thank you very much for your help as well.

[owen-mc]

Note that this query is being promoted in this PR.