-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Go: new query for detect DOS vulnerability #15130
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Hello Malayke 👋 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.
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! |
Are you aware of the existing query |
I carefully read the code of According to my testing, when the 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@Malayke Do you intend to address the review comments so this can be merged and your bounty submission can be completed? |
Co-authored-by: Owen Mansel-Chan <62447351+owen-mc@users.noreply.github.com>
Hi @owen-mc, I have addressed all of your comments. Sorry for the late reply. I have been very busy these days. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more minor comment.
QHelp previews: go/ql/src/experimental/CWE-770/DenialOfService.qhelpDenial Of ServiceUsing 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. RecommendationImplement 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. ExampleIn the following example snippet, the The server trusts that n has an acceptable value, however when using a maliciously large value, it allocates a slice of 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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Co-authored-by: Owen Mansel-Chan <62447351+owen-mc@users.noreply.github.com>
hi @owen-mc I'm committed your suggestion. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much for this contribution.
Thank you very much for your help as well. |
Note that this query is being promoted in this PR. |
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.