Skip to content
This repository has been archived by the owner on Apr 9, 2024. It is now read-only.

False positives for SSRF #7

Open
stevenjohnstone opened this issue Aug 19, 2021 · 2 comments
Open

False positives for SSRF #7

stevenjohnstone opened this issue Aug 19, 2021 · 2 comments
Labels
bug Something isn't working

Comments

@stevenjohnstone
Copy link

Finding gokart really useful so far: nice work!

Using 0.1.0, I see quite a lot of false positives for SSRF. For example,

package bug

import "net/http"

func doSomething(req *http.Request) (*http.Response, error) {
	return http.DefaultClient.Do(req)
}

func DoSomething() (*http.Response, error) {
	req, err := http.NewRequest("GET", "http://example.com", nil)
	if err != nil {
		return nil, err
	}
	return doSomething(req)
}
$ gokart scan
Using default analyzers config found at "~/.gokart/analyzers.yml".

Revving engines VRMMM VRMMM
3...2...1...Go!

(SSRF) Danger: possible SSRF detected

/home/stevie/gokart/bug.go:6
Vulnerable Function: [ doSomething(...) (*net/http.Response, error ]
      5:	func doSomething(req *http.Request) (*http.Response, error) {
    > 6:		return http.DefaultClient.Do(req)
      7:	}

/home/stevie/gokart/bug.go:5
Source of Untrusted Input: [ doSomething(...) (*net/http.Response, error ]
      4:	
    > 5:	func doSomething(req *http.Request) (*http.Response, error) {
      6:		return http.DefaultClient.Do(req)
------------------------------------------------------------------------------

Race Complete! Analysis took 417.122169ms and 81 Go files were scanned (including imported packages)
GoKart found 1 potentially vulnerable functions

It looks like any function which takes an *http.Request as an argument and passes it to Do will be flagged as potentially vulnerable? In the above example, the request url comes from a string literal so isn't tainted.

@praetorian-harry praetorian-harry added the bug Something isn't working label Aug 19, 2021
@praetorian-harry
Copy link
Collaborator

Hey @stevenjohnstone, thanks for the issue submission. We're glad that you're finding gokart useful!

I am able to reproduce this issue using the test file that you've included. We'll take a deeper look here to resolve.

@saullocarvalho
Copy link

I have reproduced this issue scanning the provided test file using gokart v0.5.1 and tried to check if the same result is returned after scanning the following code, which replaces the doSomething function call for its body.

package bug

import "net/http"

func DoSomething() (*http.Response, error) {
    req, err := http.NewRequest("GET", "http://example.com", nil)
    if err != nil {
        return nil, err
    }
    return http.DefaultClient.Do(req)
}

The scan does not consider the function vulnerable to SSRF as presented below.

$ gokart scan bugs/ssrf03/bug.go 
Using config found at /home/saullo/.gokart/analyzers.yml

Revving engines VRMMM VRMMM
3...2...1...Go!

Race Complete! Analysis took 398.462088ms and 82 Go files were scanned (including imported packages)
GoKart found 0 potentially vulnerable functions

As this got me curious, I took a deeper look into what gokart does and discovered the following.

While recursively taint analyzing the provided test code, the req expression is *ssa.Parameter and the req variable type is *http/net.Request, which meets a tainted condition and vulnerable is set to true in util/taint.go at line 133.

gokart/util/taint.go

Lines 119 to 142 in 3d38a9a

case *ssa.Parameter:
// Check if this function call is part of the tainted function source list
globalPkgName := (expr).Parent().Pkg.Pkg.Name()
if val, ok := VulnGlobalFuncs[globalPkgName]; ok {
for _, funcName := range val {
if (expr).Name() == funcName {
vulnerable = true
}
}
}
for pkg, types_ := range VulnTypes {
for _, type_ := range types_ {
if strings.TrimPrefix(expr.Type().String(), "*") == pkg+"."+type_ {
vulnerable = true
}
}
}
var values []*ssa.Value
values = cg.ResolveParam(expr)
if len(values) > 0 {
vulnerable = vulnerable || ta.ContainsTaintRecurse(startCall, values[0], cg, depth+1, visitedMutable) //loop B
}

The functions that have a *http/net.Request parameter are considered tainted by the default gokart configuration in ~/.gokart/analyzers.yml.

# Sources that are defined in Go documentation as a "type" go here (note: adding types will consider all functions that use that type to be tainted).
types:
"net/http":
- "Request"

I wonder if it wouldn't be possible to use a more detailed condition to taint vulnerable functions and avoid false positives.
Where does the Go documentation state that a *http/net.Request parameter is untrusted?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Development

No branches or pull requests

3 participants