Merge pull request from GHSA-xrmp-4542-q746

Advisory fix 1

Author: dropwhile

CHANGELOG.md

@@ -2,8 +2,16 @@ Changelog
 =========
 
 ## HEAD
+
+## v1.1.5 2019-07-23
+*   Security fixes / SSRF
+    *   Fix: Ensure non-GET/HEAD request does not send outbound request (#35)
+    *   Fix: Validate redirect urls the same as initial urls (#35)
 *   Split out exception for missing content types (#32)
 *   Prometheus compatible metrics endpoint added (#34)
+*   Disabled credential/userinfo (`user:pass@` style) type urls by default.
+    Added cli flag (`--allow-credential-urls`) to retain prior behavior (which
+    allows them).
 
 ## v1.1.4 2019-02-26 
 

cmd/go-camo/main.go

@@ -70,6 +70,7 @@ func main() {
 		DisableKeepAlivesFE bool          `long:"no-fk" description:"Disable frontend http keep-alive support"`
 		DisableKeepAlivesBE bool          `long:"no-bk" description:"Disable backend http keep-alive support"`
 		AllowContentVideo   bool          `long:"allow-content-video" description:"Additionally allow 'video/*' content"`
+		AllowCredetialURLs  bool          `long:"allow-credential-urls" description:"Allow urls to contain user/pass credentials"`
 		Verbose             bool          `short:"v" long:"verbose" description:"Show verbose (debug) log level output"`
 		ServerName          string        `long:"server-name" default:"go-camo" description:"Value to use for the HTTP server field"`
 		ExposeServerVersion bool          `long:"expose-server-version" description:"Include the server version in the HTTP server response header"`
@@ -142,6 +143,7 @@ func main() {
 
 	// other options
 	config.EnableXFwdFor = opts.EnableXFwdFor
+	config.AllowCredetialURLs = opts.AllowCredetialURLs
 
 	// additional content types to allow
 	config.AllowContentVideo = opts.AllowContentVideo
@@ -205,24 +207,25 @@ func main() {
 		CamoHandler: proxy,
 	}
 
-	if opts.Metrics {
-		mlog.Printf("Enabling metrics at /metrics")
-		http.Handle("/metrics", promhttp.Handler())
-	}
-
 	if opts.Stats {
 		ps := &stats.ProxyStats{}
 		proxy.SetMetricsCollector(ps)
 		mlog.Printf("Enabling stats at /status")
 		dumbrouter.StatsHandler = stats.Handler(ps)
 	}
 
-	// Wrap the dumb router in instrumentation.
-	instrumentedRouter := promhttp.InstrumentHandlerDuration(responseDuration,
-		promhttp.InstrumentHandlerResponseSize(responseSize, dumbrouter),
-	)
+	var router http.Handler = dumbrouter
+
+	if opts.Metrics {
+		mlog.Printf("Enabling metrics at /metrics")
+		http.Handle("/metrics", promhttp.Handler())
+		// Wrap the dumb router in instrumentation.
+		router = promhttp.InstrumentHandlerDuration(responseDuration,
+			promhttp.InstrumentHandlerResponseSize(responseSize, dumbrouter),
+		)
+	}
 
-	http.Handle("/", instrumentedRouter)
+	http.Handle("/", router)
 
 	if opts.BindAddress != "" {
 		mlog.Printf("Starting server on: %s", opts.BindAddress)

pkg/camo/proxy.go

@@ -8,6 +8,7 @@ package camo
 
 import (
 	"errors"
+	"fmt"
 	"io"
 	"net"
 	"net/http"
@@ -45,6 +46,8 @@ type Config struct {
 	EnableXFwdFor bool
 	// additional content types to allow
 	AllowContentVideo bool
+	// allow URLs to contain user/pass credentials
+	AllowCredetialURLs bool
 	// no ip filtering (test mode)
 	noIPFiltering bool
 }
@@ -112,40 +115,12 @@ func (p *Proxy) ServeHTTP(w http.ResponseWriter, req *http.Request) {
 		return
 	}
 
-	uHostname := strings.ToLower(u.Hostname())
-	if uHostname == "" || localhostRegex.MatchString(uHostname) {
-		http.Error(w, "Bad url host", http.StatusNotFound)
+	err = p.checkURL(u)
+	if err != nil {
+		http.Error(w, err.Error(), http.StatusNotFound)
 		return
 	}
 
-	// filtering
-	if !p.config.noIPFiltering {
-		// if allowList is set, require match
-		for _, rgx := range p.allowList {
-			if rgx.MatchString(uHostname) {
-				http.Error(w, "Allowlist host failure", http.StatusNotFound)
-				return
-			}
-		}
-
-		// filter out rejected networks
-		if ip := net.ParseIP(uHostname); ip != nil {
-			if isRejectedIP(ip) {
-				http.Error(w, "Denylist host failure", http.StatusNotFound)
-				return
-			}
-		} else {
-			if ips, err := net.LookupIP(uHostname); err == nil {
-				for _, ip := range ips {
-					if isRejectedIP(ip) {
-						http.Error(w, "Denylist host failure", http.StatusNotFound)
-						return
-					}
-				}
-			}
-		}
-	}
-
 	nreq, err := http.NewRequest(req.Method, sURL, nil)
 	if err != nil {
 		mlog.Debugm("could not create NewRequest", mlog.Map{"err": err})
@@ -203,6 +178,10 @@ func (p *Proxy) ServeHTTP(w http.ResponseWriter, req *http.Request) {
 			http.Error(w, "Error Fetching Resource", http.StatusGatewayTimeout)
 		} else if strings.Contains(errString, "use of closed") {
 			http.Error(w, "Error Fetching Resource", http.StatusBadGateway)
+		} else if strings.Contains(errString, "BadRedirect:") {
+			// Got a bad redirect
+			mlog.Debugm("response from upstream", mlog.Map{"err": err})
+			http.Error(w, "Error Fetching Resource", http.StatusNotFound)
 		} else {
 			// some other error. call it a not found (camo compliant)
 			http.Error(w, "Error Fetching Resource", http.StatusNotFound)
@@ -296,6 +275,46 @@ func (p *Proxy) ServeHTTP(w http.ResponseWriter, req *http.Request) {
 	mlog.Debugm("response to client", mlog.Map{"resp": w})
 }
 
+func (p *Proxy) checkURL(reqURL *url.URL) error {
+	// reject localhost urls
+	uHostname := strings.ToLower(reqURL.Hostname())
+	if uHostname == "" || localhostRegex.MatchString(uHostname) {
+		return errors.New("Bad url host")
+	}
+
+	// if not allowed, reject credentialed/userinfo urls
+	if !p.config.AllowCredetialURLs && reqURL.User != nil {
+		return errors.New("Userinfo URL rejected")
+	}
+
+	// ip/whitelist/blacklist filtering
+	if !p.config.noIPFiltering {
+		// if allowList is set, require match
+		for _, rgx := range p.allowList {
+			if rgx.MatchString(uHostname) {
+				return errors.New("Allowlist host failure")
+			}
+		}
+
+		// filter out rejected networks
+		if ip := net.ParseIP(uHostname); ip != nil {
+			if isRejectedIP(ip) {
+				return errors.New("Denylist host failure")
+			}
+		} else {
+			if ips, err := net.LookupIP(uHostname); err == nil {
+				for _, ip := range ips {
+					if isRejectedIP(ip) {
+						return errors.New("Denylist host failure")
+					}
+				}
+			}
+		}
+	}
+
+	return nil
+}
+
 // copy headers from src into dst
 // empty filter map will result in no filtering being done
 func (p *Proxy) copyHeaders(dst, src *http.Header, filter *map[string]bool) {
@@ -353,12 +372,6 @@ func New(pc Config) (*Proxy, error) {
 		// timeout
 		Timeout: pc.RequestTimeout,
 	}
-	client.CheckRedirect = func(req *http.Request, via []*http.Request) error {
-		if len(via) >= pc.MaxRedirects {
-			return errors.New("Too many redirects")
-		}
-		return nil
-	}
 
 	var allow []*regexp.Regexp
 	var c *regexp.Regexp
@@ -387,11 +400,28 @@ func New(pc Config) (*Proxy, error) {
 		acceptTypesRe = append(acceptTypesRe, c)
 	}
 
-	return &Proxy{
+	p := &Proxy{
 		client:            client,
 		config:            &pc,
 		allowList:         allow,
 		acceptTypesString: strings.Join(acceptTypes, ", "),
-		acceptTypesRe:     acceptTypesRe}, nil
+		acceptTypesRe:     acceptTypesRe,
+	}
+
+	client.CheckRedirect = func(req *http.Request, via []*http.Request) error {
+		if len(via) >= pc.MaxRedirects {
+			mlog.Debug("Got bad redirect: Too many redirects")
+			return errors.New("BadRedirect: Too many redirects")
+		}
+		err := p.checkURL(req.URL)
+		if err != nil {
+			mlog.Debugm("Got bad redirect", mlog.Map{"url": req})
+			return fmt.Errorf("BadRedirect: %s", err)
+		}
+
+		return nil
+	}
+
+	return p, nil
 
 }

pkg/camo/proxy_test.go

@@ -5,25 +5,29 @@
 package camo
 
 import (
+	"flag"
 	"fmt"
 	"net/http"
 	"net/http/httptest"
+	"os"
 	"testing"
 	"time"
 
 	"github.com/cactus/go-camo/pkg/camo/encoding"
 	"github.com/cactus/go-camo/pkg/router"
+	"github.com/cactus/mlog"
 
 	"github.com/stretchr/testify/assert"
 )
 
 var camoConfig = Config{
-	HMACKey:           []byte("0x24FEEDFACEDEADBEEFCAFE"),
-	MaxSize:           5120 * 1024,
-	RequestTimeout:    time.Duration(10) * time.Second,
-	MaxRedirects:      3,
-	ServerName:        "go-camo",
-	AllowContentVideo: false,
+	HMACKey:            []byte("0x24FEEDFACEDEADBEEFCAFE"),
+	MaxSize:            5120 * 1024,
+	RequestTimeout:     time.Duration(10) * time.Second,
+	MaxRedirects:       3,
+	ServerName:         "go-camo",
+	AllowContentVideo:  false,
+	AllowCredetialURLs: false,
 }
 
 func makeReq(testURL string) (*http.Request, error) {
@@ -196,6 +200,37 @@ func TestVideoContentTypeAllowed(t *testing.T) {
 	assert.Nil(t, err)
 }
 
+func TestCredetialURLsAllowed(t *testing.T) {
+	t.Parallel()
+
+	camoConfigWithCredentials := Config{
+		HMACKey:            []byte("0x24FEEDFACEDEADBEEFCAFE"),
+		MaxSize:            180 * 1024,
+		RequestTimeout:     time.Duration(10) * time.Second,
+		MaxRedirects:       3,
+		ServerName:         "go-camo",
+		AllowCredetialURLs: true,
+	}
+
+	testURL := "http://user:[email protected]/images/srpr/logo11w.png"
+	_, err := makeTestReq(testURL, 200, camoConfigWithCredentials)
+	assert.Nil(t, err)
+}
+
+func Test404OnVideo(t *testing.T) {
+	t.Parallel()
+	testURL := "http://mirrors.standaloneinstaller.com/video-sample/small.mp4"
+	_, err := makeTestReq(testURL, 400, camoConfig)
+	assert.Nil(t, err)
+}
+
+func Test404OnCredentialURL(t *testing.T) {
+	t.Parallel()
+	testURL := "http://user:[email protected]/images/srpr/logo11w.png"
+	_, err := makeTestReq(testURL, 404, camoConfig)
+	assert.Nil(t, err)
+}
+
 func Test404InfiniRedirect(t *testing.T) {
 	t.Parallel()
 	testURL := "http://httpbin.org/redirect/4"
@@ -286,6 +321,33 @@ func Test404OnLocalhostWithPort(t *testing.T) {
 	}
 }
 
+func Test404OnRedirectWithLocalhostTarget(t *testing.T) {
+	t.Parallel()
+	testURL := "http://httpbin.org/redirect-to?url=http://localhost/some.png"
+	record, err := makeTestReq(testURL, 404, camoConfig)
+	if assert.Nil(t, err) {
+		assert.Equal(t, "Error Fetching Resource\n", record.Body.String(), "Expected 404 response body but got '%s' instead", record.Body.String())
+	}
+}
+
+func Test404OnRedirectWithLoopbackIP(t *testing.T) {
+	t.Parallel()
+	testURL := "http://httpbin.org/redirect-to?url=http://127.0.0.100/some.png"
+	record, err := makeTestReq(testURL, 404, camoConfig)
+	if assert.Nil(t, err) {
+		assert.Equal(t, "Error Fetching Resource\n", record.Body.String(), "Expected 404 response body but got '%s' instead", record.Body.String())
+	}
+}
+
+func Test404OnRedirectWithLoopbackIPwCreds(t *testing.T) {
+	t.Parallel()
+	testURL := "http://httpbin.org/redirect-to?url=http://user:[email protected]/some.png"
+	record, err := makeTestReq(testURL, 404, camoConfig)
+	if assert.Nil(t, err) {
+		assert.Equal(t, "Error Fetching Resource\n", record.Body.String(), "Expected 404 response body but got '%s' instead", record.Body.String())
+	}
+}
+
 // Test will fail if dns relay implements dns rebind prevention
 func Test404OnLoopback(t *testing.T) {
 	t.Skip("Skipping test. CI environments generally enable something similar to unbound's private-address functionality, making this test fail.")
@@ -363,3 +425,18 @@ func TestTimeout(t *testing.T) {
 
 	close(cc)
 }
+
+func TestMain(m *testing.M) {
+	flag.Parse()
+
+	debug := os.Getenv("DEBUG")
+	// now configure a standard logger
+	mlog.SetFlags(mlog.Lstd)
+
+	if debug != "" {
+		mlog.SetFlags(mlog.Flags() | mlog.Ldebug)
+		mlog.Debug("debug logging enabled")
+	}
+
+	os.Exit(m.Run())
+}

pkg/router/httpdate.go

@@ -23,7 +23,7 @@ type iHTTPDate struct {
 func (h *iHTTPDate) String() string {
 	stamp := h.dateValue.Load()
 	if stamp == nil {
-		mlog.Print("got a nil datesamp. Trying to recover...")
+		mlog.Print("Got a nil datestamp. Trying to recover...")
 		h.Update()
 		return time.Now().UTC().Format(timeFormat)
 	}

pkg/router/router.go

@@ -40,11 +40,6 @@ func (dr *DumbRouter) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 
 	if r.Method != "HEAD" && r.Method != "GET" {
 		http.Error(w, "Method Not Allowed", 405)
-	}
-
-	components := strings.Split(r.URL.Path, "/")
-	if len(components) == 3 {
-		dr.CamoHandler.ServeHTTP(w, r)
 		return
 	}
 
@@ -58,5 +53,11 @@ func (dr *DumbRouter) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
+	components := strings.Split(r.URL.Path, "/")
+	if len(components) == 3 {
+		dr.CamoHandler.ServeHTTP(w, r)
+		return
+	}
+
 	http.Error(w, "404 Not Found", 404)
 }