[security] Path parser inconsistency could lead to bypass several security checks in emicklei/go-restful

[JamieSlome]

Reported via huntr.dev here.

Created: May 28th 2022

Description

There is a inconsistency between how golang(and other packages) and go-restful parses url path. This incosistency could lead several security check bypass in a complex system.

Steps to reproduce

Copy and run the code below

package main

import (
    "fmt"
    "html"
    "log"
    "net/http"
)

func main() {

    http.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {
        fmt.Fprintf(w, "Hello, %q", html.EscapeString(r.URL.Path))
    })

// If a user request matches with this path then it will be checked and if it didn't match then the request will be directly forwarded to the go-restful server.
    http.HandleFunc("/users/id_will_be_here", func(w http.ResponseWriter, r *http.Request) {
        fmt.Fprintf(w, "I'm checking if the user is authorized to see this user info. If he is authorized then I will forward this request to the go-restful server which will return the user info")
    })

    log.Fatal(http.ListenAndServe(":8081", nil))

}

Now If you send a request to the http://localhost:8081/users/id_will_be_here then you will get a hit to the expected endpoint and it will check the authorization part
Now if you send a request to the http://localhost:8081/users/id_will_be_here/ (notice the extra / in last) then this server won't process this request as the path name doesn't match but the problem is that go-restful treat this path and the previous as same.
So this inconsistency could lead some issues in this scenerio as the first sever wont check the security checks but the second server(go-restful) will return user data.

Impact

Security check bypass

Occurrences

web_service.go L80

[emicklei]

Thank you for reporting this. I will investigate this further.

[emicklei]

i was able to reproduce this issue.

your example output

~/tmp/issue497
$ curl http://localhost:8081/users/id_will_be_here
I'm checking if the user is authorized to see this user info. If he is authorized then I will forward this request to the go-restful server which will return the user info

emicklei@Ernests-iMac-10 on Sat Jun 04 at 10:35 AM
~/tmp/issue497
$ curl http://localhost:8081/users/id_will_be_here/
Hello, "/users/id_will_be_here/"

go-restful example:

package main

import (
	"fmt"
	"html"
	"log"
	"net/http"

	restful "github.com/emicklei/go-restful/v3"
)

func main() {
	ws := new(restful.WebService)
	ws.Route(ws.GET("/").To(hello))
	ws.Route(ws.GET("/users/id_will_be_here").To(check))
	restful.Add(ws)
	log.Fatal(http.ListenAndServe(":8081", nil))
}

func hello(req *restful.Request, resp *restful.Response) {
	fmt.Fprintf(resp, "Hello, %q", html.EscapeString(req.Request.URL.Path))
}

func check(req *restful.Request, resp *restful.Response) {
	fmt.Fprintf(resp, "I'm checking if the user is authorized to see this user info. If he is authorized then I will forward this request to the go-restful server which will return the user info")
}

go-restful output:

~/tmp/issue497
$ curl http://localhost:8081/users/id_will_be_here/
I'm checking if the user is authorized to see this user info. If he is authorized then I will forward this request to the go-restful server which will return the user info

emicklei@Ernests-iMac-10 on Sat Jun 04 at 10:39 AM
~/tmp/issue497
$ curl http://localhost:8081/users/id_will_be_here
I'm checking if the user is authorized to see this user info. If he is authorized then I will forward this request to the go-restful server which will return the user info

[emicklei]

here, the slash is trimmed

go-restful/route.go

Line 167 in c2c010a

return strings.Split(strings.Trim(path, "/"), "/")

now the problem is that change this code will break existing behavior ; client of the package may be affected negatively.
So I am thinking about added a feature flag to disable the trimming and by default this is set to false. Only when it is explicitly set to true then the behavior is conform the standard route matching. This of course need to be documented.

[starzhanganz]

@emicklei May I know any update on this? As OTEL has used this package, it's affecting many other users using OTEL images. thanks!
OTEL Link

Report from Security scanning tools:
github.com/emicklei/go-restful/v3 module from all versions is vulnerable to Authentication Bypass by Primary Weakness. There is an inconsistency in how go-restful parses URL paths. This inconsistency could lead several security check bypass in a complex system.

[codechris1]

Hello guys,

Is there any update on this?

[emicklei]

hi all, sorry for the delay. I will add this feature flag as mentioned. Will that suffice?

[emicklei]

After the change, two test fail. Need to check if the test was correct in the first place

--- FAIL: Test_matchesRouteByPathTokens (0.00s)
--- FAIL: TestMatchesPath_VarOnFront (0.00s)

[emicklei]

please check #511

[emicklei]

@JamieSlome i believe the PR will fix the reported issue

[JamieSlome]

@benharvie

[starzhanganz]

Looks like the PR is still not merged yet, probably due to the code test coverage filed?
Appreciate any update on this!

[dvasilen]

PRISMA-2022-0227. The fix or an ETA is appreciated.

[jnewfield]

Should this PR be closed since PR #511 has been merged?

[jimmyseto]

PRISMA-2022-0227. The fix or an ETA is appreciated.

hi. did the fix for this issue address PRISMA-2022-0227? a scan against v3.10.1 suggests the vulnerability is still here (?).

[brutif]

I'm not sure this is a bypass. Yes, go-restful handles trailing slashes differently than net/http, but how could a bypass realistically occur? From above go-restful example,

// called from ws.Route(ws.GET("/users/id_will_be_here").To(check))
func check(req *restful.Request, resp *restful.Response) {
	fmt.Fprintf(resp, "I'm checking if the user is authorized to see this user info. If he is authorized then I will forward this request to the go-restful server which will return the user info")
}

The presence or absence of the trailing / won't cause the authorization check to be skipped, or the wrong route to be selected. I'm trying to think of a realistic example where it would, and I can't.

[sam-trace]

Is there a CVE assigned to this bug?

[eastebry]

Hey @emicklei - sorry to resurrect an old issue, but there has been a surprising amount of noise in the Go community around this - particularly due to the fact that Prisma cloud is reporting this as a high severity issue and the decision to not upgrade this dependency by some members of the k8s community (which I personally think is quite reasonable).

I'm in agreement with @brutif above, I'm struggling to see how this bug would ever constitute a security vulnerability. Barring a very bespoke authorization system, I can't imagine a situation in which this issue could reasonably be considered a vulnerability. The code example provided in this issue feels contrived, and to me only demonstrates unusual/unexpected behavior. It takes quite a leap of logic to consider that behavior to be a security vulnerability, in my opinion.

I'm wondering, as the maintainer of this project, do you consider this to be a valid vulnerability? If you do not think it is, and are willing to indicate so publicly in this issue, I'd be happy to open a case with Palo Alto to see if we can have the prisma-2022-0227 vulnerability removed from their database.

Thank you!

[emicklei]

hi @eastebry , thank you for notifying me about this ongoing discussion. I need to spent some time on reading the status and reasons for not upgrading (btw the breaking change was reverted in a later release). I will get back to this issue later.

[eastebry]

Hey @emicklei - curious if you've had time to think about this? I feel that this is an entirely manufactured vulnerability that does not pose any legitimate risk. I'd also note that Palo Alto (Prisma) is the only vendor reporting this issue and a proper CVE was never filed, which would allow it to be disputed by third party.

I opened a support issue with Palo Alto, but was ignored. If you are in agreement and indicate so in this issue, I'd be happy to reopen my case with them.

Thank you for your time!

[emicklei]

@eastebry

IMO this issue should not have a high severity level.
And yes, I think the code example is contrived but correct and does exhibit the behaviour.

Next, the problem is that without making a breaking api change, this cannot be avoided.
I did add the possibility to disable the behaviour (TrimRightSlashEnabled) but clearly it does not prevent it from happening.

As a maintainer of the go-restful package and supporter of the Kubernetes project, I am happy to work on a solution.

[emicklei]

Looking at the Kubernetes project (https://github.com/kubernetes/kubernetes/blob/master/go.mod) I see they use github.com/emicklei/go-restful/v3 v3.11.0 which reverted a breaking change earlier. Current version is v3.12.1. I do not know why the team decided not (yet) to upgrade.