OWASP / Go-SCP

Golang Secure Coding Practices guide
https://owasp.org/www-project-go-secure-coding-practices-guide/
Creative Commons Attribution Share Alike 4.0 International
4.83k stars 369 forks source link

Gorilla Toolkit - vulnerable or not? #59

Closed aaron-junot closed 4 years ago

aaron-junot commented 4 years ago

The portion about Input Validation, and specifically sanitation of the URL request path mentions a third party package called Gorilla Toolkit, but it does not specify whether this package is also vulnerable to path traversal attacks or whether it's preferred to use because perhaps it doesn't have this vulnerability.

Can someone please advise on whether this package is also vulnerable so we can update this section to indicate that?

PauloASilva commented 4 years ago

Hi @aaron-suarez, Thanks for raising the question.

The reference to the Gorilla Toolkit MUX package was added to show an alternative to the native HTTP request multiplexer since it provides additional features.

Since packages keep changing, it is hard to say something about their security, especially considering Go SCP maintenance.

I think we can improve this providing some context about the third-party packages:

The following third-party packages are alternatives to the native HTTP request multiplexer, providing additional features. Always choose well tested and actively maintained packages.

What do you think?

Cheers, Paulo A. Silva

aaron-junot commented 4 years ago

That certainly clarifies it. And you're correct, it's very hard to make a statement about a package's security without doing a point-in-time audit and trying to find vulnerabilities and making the decision based on that. I think adding that blurb would clarify it enough. Would you like me to open a Pull Request to add it?

ilyaglow commented 4 years ago

Definitely agree with @PauloASilva improvement proposal, but just for the record – a quick look at the gorillamux's most widely used HandleFunc function shows no sign that the same path traversal like in ServeMux is possible, because of the way it handles placeholders and slashes (including it's URL-encoded representation).

It's hard to imagine a case where a user of the library can screw things up, but I do not tend to call something completely bulletproof and invulnerable.

The snippet I played with:

package main

import (
    "io/ioutil"
    "log"
    "net/http"
    "path/filepath"
    "strings"

    "github.com/gorilla/mux"
)

const root = "/tmp"

func main() {
    gmux := mux.NewRouter()
    gmux.HandleFunc("/{file}", func(w http.ResponseWriter, r *http.Request) {
        filename := filepath.Join(root, strings.Trim(mux.Vars(r)["file"], "/"))
        log.Println(filename)
        contents, err := ioutil.ReadFile(filename)
        if err != nil {
            w.WriteHeader(http.StatusNotFound)
            return
        }
        w.Write(contents)
    })

    server := &http.Server{
        Addr:    "127.0.0.1:50000",
        Handler: gmux,
    }

    log.Fatal(server.ListenAndServe())
}
Freddy-Recurly commented 4 years ago

just my $.02 but might it be clearer if the section read more as URL request vulnerabilities but starting with the path traversal attack which is better explained here https://www.acunetix.com/websitesecurity/directory-traversal/ than I can do it justice in this post. If the section then listed common mux/router implementations that do/not suffer from the vulnerability as of a given version.

Also the Note mentioning the vulnerability alludes to the CONNECT http method. I am not sure that CONNECT is the most common means by which the attack is exploited.

PauloASilva commented 4 years ago

Thank you all for your feedback.

Would you like me to open a Pull Request to add it?

@aaron-suarez definitely, I would love to accept & merge it.

Cheers, Paulo A. Silva

PauloASilva commented 4 years ago

Hi @aaron-suarez, Your contribution was released: please check v2.5.4.

Cheers, Paulo A. Silva