Fix SSRF security issue #3755 in ping target, email server and LDAP server APIs

src/common/models/models_test.go

@@ -1,21 +0,0 @@
-// Copyright (c) 2017 VMware, Inc. All Rights Reserved.
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-//    http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-package models
-
-import (
-	"testing"
-)
-
-func TestMain(m *testing.M) {
-}

src/common/models/replication_job.go

@@ -120,14 +120,15 @@ func (r *RepTarget) Valid(v *validation.Validation) {
 		v.SetError("name", "max length is 64")
 	}
 
-	if len(r.URL) == 0 {
+	url, err := utils.ParseEndpoint(r.URL)
-		v.SetError("endpoint", "can not be empty")
+	if err != nil {
-	}
+		v.SetError("endpoint", err.Error())
-
+	} else {
-	r.URL = utils.FormatEndpoint(r.URL)
+		// Prevent SSRF security issue #3755
-
+		r.URL = url.Scheme + "://" + url.Host + url.Path
-	if len(r.URL) > 64 {
+		if len(r.URL) > 64 {
-		v.SetError("endpoint", "max length is 64")
+			v.SetError("endpoint", "max length is 64")
+		}
 	}
 
 	// password is encoded using base64, the length of this field

src/common/models/target_test.go

@@ -0,0 +1,131 @@
+// Copyright (c) 2017 VMware, Inc. All Rights Reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package models
+
+import (
+	"testing"
+
+	"github.com/astaxie/beego/validation"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestValidOfTarget(t *testing.T) {
+	cases := []struct {
+		target   RepTarget
+		err      bool
+		expected RepTarget
+	}{
+		// name is null
+		{
+			RepTarget{
+				Name: "",
+			},
+			true,
+			RepTarget{}},
+
+		// url is null
+		{
+			RepTarget{
+				Name: "endpoint01",
+				URL:  "",
+			},
+			true,
+			RepTarget{},
+		},
+
+		// invalid url
+		{
+			RepTarget{
+				Name: "endpoint01",
+				URL:  "ftp://example.com",
+			},
+			true,
+			RepTarget{},
+		},
+
+		// invalid url
+		{
+			RepTarget{
+				Name: "endpoint01",
+				URL:  "ftp://example.com",
+			},
+			true,
+			RepTarget{},
+		},
+
+		// valid url
+		{
+			RepTarget{
+				Name: "endpoint01",
+				URL:  "example.com",
+			},
+			false,
+			RepTarget{
+				Name: "endpoint01",
+				URL:  "http://example.com",
+			},
+		},
+
+		// valid url
+		{
+			RepTarget{
+				Name: "endpoint01",
+				URL:  "http://example.com",
+			},
+			false,
+			RepTarget{
+				Name: "endpoint01",
+				URL:  "http://example.com",
+			},
+		},
+
+		// valid url
+		{
+			RepTarget{
+				Name: "endpoint01",
+				URL:  "https://example.com",
+			},
+			false,
+			RepTarget{
+				Name: "endpoint01",
+				URL:  "https://example.com",
+			},
+		},
+
+		// valid url
+		{
+			RepTarget{
+				Name: "endpoint01",
+				URL:  "http://example.com/redirect?key=value",
+			},
+			false,
+			RepTarget{
+				Name: "endpoint01",
+				URL:  "http://example.com/redirect",
+			}},
+	}
+
+	for _, c := range cases {
+		v := &validation.Validation{}
+		c.target.Valid(v)
+		if c.err {
+			require.True(t, v.HasErrors())
+			continue
+		}
+		require.False(t, v.HasErrors())
+		assert.Equal(t, c.expected, c.target)
+	}
+}

src/common/utils/registry/registry.go

@@ -129,7 +129,7 @@ func (r *Registry) Catalog() ([]string, error) {
 
 // Ping ...
 func (r *Registry) Ping() error {
-	req, err := http.NewRequest("GET", buildPingURL(r.Endpoint.String()), nil)
+	req, err := http.NewRequest(http.MethodHead, buildPingURL(r.Endpoint.String()), nil)
 	if err != nil {
 		return err
 	}

src/common/utils/registry/registry_test.go

@@ -28,7 +28,7 @@ import (
 func TestPing(t *testing.T) {
 	server := test.NewServer(
 		&test.RequestHandlerMapping{
-			Method:  "GET",
+			Method:  http.MethodHead,
 			Pattern: "/v2/",
 			Handler: test.Handler(nil),
 		})

src/common/utils/utils.go

@@ -29,27 +29,24 @@ import (
 	"github.com/vmware/harbor/src/common/utils/log"
 )
 
-// FormatEndpoint formats endpoint
+// ParseEndpoint parses endpoint to a URL
-func FormatEndpoint(endpoint string) string {
+func ParseEndpoint(endpoint string) (*url.URL, error) {
-	endpoint = strings.TrimSpace(endpoint)
+	endpoint = strings.Trim(endpoint, " ")
 	endpoint = strings.TrimRight(endpoint, "/")
-	if !strings.HasPrefix(endpoint, "http://") &&
+	if len(endpoint) == 0 {
-		!strings.HasPrefix(endpoint, "https://") {
+		return nil, fmt.Errorf("empty URL")
+	}
+	i := strings.Index(endpoint, "://")
+	if i >= 0 {
+		scheme := endpoint[:i]
+		if scheme != "http" && scheme != "https" {
+			return nil, fmt.Errorf("invalid scheme: %s", scheme)
+		}
+	} else {
 		endpoint = "http://" + endpoint
 	}
 
-	return endpoint
+	return url.ParseRequestURI(endpoint)
-}
-
-// ParseEndpoint parses endpoint to a URL
-func ParseEndpoint(endpoint string) (*url.URL, error) {
-	endpoint = FormatEndpoint(endpoint)
-
-	u, err := url.Parse(endpoint)
-	if err != nil {
-		return nil, err
-	}
-	return u, nil
 }
 
 // ParseRepository splits a repository into two parts: project and rest

src/common/utils/utils_test.go

@@ -23,37 +23,30 @@ import (
 	"time"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestParseEndpoint(t *testing.T) {
-	endpoint := "example.com"
+	cases := []struct {
-	u, err := ParseEndpoint(endpoint)
+		input    string
-	if err != nil {
+		err      bool
-		t.Fatalf("failed to parse endpoint %s: %v", endpoint, err)
+		expected string
+	}{
+		{" example.com/ ", false, "http://example.com"},
+		{"ftp://example.com", true, ""},
+		{"http://example.com", false, "http://example.com"},
+		{"https://example.com", false, "https://example.com"},
+		{"http://example!@#!?//#", true, ""},
 	}
 
-	if u.String() != "http://example.com" {
+	for _, c := range cases {
-		t.Errorf("unexpected endpoint: %s != %s", endpoint, "http://example.com")
+		u, err := ParseEndpoint(c.input)
-	}
+		if c.err {
-
+			require.NotNil(t, err)
-	endpoint = "https://example.com"
+			continue
-	u, err = ParseEndpoint(endpoint)
+		}
-	if err != nil {
+		require.Nil(t, err)
-		t.Fatalf("failed to parse endpoint %s: %v", endpoint, err)
+		assert.Equal(t, c.expected, u.String())
-	}
-
-	if u.String() != "https://example.com" {
-		t.Errorf("unexpected endpoint: %s != %s", endpoint, "https://example.com")
-	}
-
-	endpoint = "  example.com/ "
-	u, err = ParseEndpoint(endpoint)
-	if err != nil {
-		t.Fatalf("failed to parse endpoint %s: %v", endpoint, err)
-	}
-
-	if u.String() != "http://example.com" {
-		t.Errorf("unexpected endpoint: %s != %s", endpoint, "http://example.com")
 	}
 }
 

src/ui/api/email.go

@@ -106,7 +106,9 @@ func (e *EmailAPI) Ping() {
 	addr := net.JoinHostPort(host, strconv.Itoa(port))
 	if err := email.Ping(addr, identity, username,
 		password, pingEmailTimeout, ssl, insecure); err != nil {
-		log.Debugf("ping %s failed: %v", addr, err)
+		log.Errorf("failed to ping email server: %v", err)
-		e.CustomAbort(http.StatusBadRequest, err.Error())
+		// do not return any detail information of the error, or may cause SSRF security issue #3755
+		e.RenderError(http.StatusBadRequest, "failed to ping email server")
+		return
 	}
 }

src/ui/api/email_test.go

@@ -16,7 +16,6 @@ package api
 
 import (
 	"fmt"
-	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -36,7 +35,7 @@ func TestPingEmail(t *testing.T) {
 
 	assert.Equal(401, code, "the status code of ping email server with non-admin user should be 401")
 
-	//case 2: bad request
+	//case 2: empty email host
 	settings := `{
 		"email_host":     ""
 	}`
@@ -47,7 +46,7 @@ func TestPingEmail(t *testing.T) {
 		return
 	}
 
-	assert.Equal(400, code, "the status code of ping email server should be 400")
+	assert.Equal(400, code)
 
 	//case 3: secure connection with admin role
 	settings = `{
@@ -58,18 +57,13 @@ func TestPingEmail(t *testing.T) {
 		"email_ssl":      true
 	}`
 
-	code, body, err := apiTest.PingEmail(*admin, []byte(settings))
+	code, _, err = apiTest.PingEmail(*admin, []byte(settings))
 	if err != nil {
 		t.Errorf("failed to test ping email server: %v", err)
 		return
 	}
 
-	assert.Equal(400, code, "the status code of ping email server should be 400")
+	assert.Equal(400, code)
-
-	if !strings.Contains(body, "535") {
-		t.Errorf("unexpected error: %s does not contains 535", body)
-		return
-	}
 
 	//case 4: ping email server whose settings are read from config
 	code, _, err = apiTest.PingEmail(*admin, nil)
@@ -78,5 +72,5 @@ func TestPingEmail(t *testing.T) {
 		return
 	}
 
-	assert.Equal(400, code, "the status code of ping email server should be 400")
+	assert.Equal(400, code)
 }

src/ui/api/ldap.go

@@ -29,7 +29,7 @@ type LdapAPI struct {
 }
 
 const (
-	pingErrorMessage       = "LDAP connection test failed!"
+	pingErrorMessage       = "LDAP connection test failed"
 	loadSystemErrorMessage = "Can't load system configuration!"
 	canNotOpenLdapSession  = "Can't open LDAP session!"
 	searchLdapFailMessage  = "LDAP search failed!"
@@ -72,6 +72,7 @@ func (l *LdapAPI) Ping() {
 
 	if err != nil {
 		log.Errorf("ldap connect fail, error: %v", err)
+		// do not return any detail information of the error, or may cause SSRF security issue #3755
 		l.RenderError(http.StatusBadRequest, pingErrorMessage)
 		return
 	}

src/ui/api/target.go

@@ -16,15 +16,12 @@ package api
 
 import (
 	"fmt"
-	"net"
 	"net/http"
-	"net/url"
 	"strconv"
 
 	"github.com/vmware/harbor/src/common/dao"
 	"github.com/vmware/harbor/src/common/models"
 	"github.com/vmware/harbor/src/common/utils"
-	registry_error "github.com/vmware/harbor/src/common/utils/error"
 	"github.com/vmware/harbor/src/common/utils/log"
 	"github.com/vmware/harbor/src/common/utils/registry"
 	"github.com/vmware/harbor/src/common/utils/registry/auth"
@@ -60,27 +57,15 @@ func (t *TargetAPI) Prepare() {
 
 func (t *TargetAPI) ping(endpoint, username, password string, insecure bool) {
 	registry, err := newRegistryClient(endpoint, insecure, username, password)
-	if err != nil {
+	if err == nil {
-		// timeout, dns resolve error, connection refused, etc.
+		err = registry.Ping()
-		if urlErr, ok := err.(*url.Error); ok {
-			if netErr, ok := urlErr.Err.(net.Error); ok {
-				t.CustomAbort(http.StatusBadRequest, netErr.Error())
-			}
-
-			t.CustomAbort(http.StatusBadRequest, urlErr.Error())
-		}
-
-		log.Errorf("failed to create registry client: %#v", err)
-		t.CustomAbort(http.StatusInternalServerError, http.StatusText(http.StatusInternalServerError))
 	}
 
-	if err = registry.Ping(); err != nil {
+	if err != nil {
-		if regErr, ok := err.(*registry_error.HTTPError); ok {
+		log.Errorf("failed to ping target: %v", err)
-			t.CustomAbort(regErr.StatusCode, regErr.Detail)
+		// do not return any detail information of the error, or may cause SSRF security issue #3755
-		}
+		t.RenderError(http.StatusBadRequest, "failed to ping target")
-
+		return
-		log.Errorf("failed to ping registry %s: %v", registry.Endpoint.String(), err)
-		t.CustomAbort(http.StatusInternalServerError, http.StatusText(http.StatusInternalServerError))
 	}
 }
 
@@ -117,7 +102,14 @@ func (t *TargetAPI) Ping() {
 	}
 
 	if req.Endpoint != nil {
-		target.URL = *req.Endpoint
+		url, err := utils.ParseEndpoint(*req.Endpoint)
+		if err != nil {
+			t.HandleBadRequest(err.Error())
+			return
+		}
+
+		// Prevent SSRF security issue #3755
+		target.URL = url.Scheme + "://" + url.Host + url.Path
 	}
 	if req.Username != nil {
 		target.Username = *req.Username
@@ -129,11 +121,6 @@ func (t *TargetAPI) Ping() {
 		target.Insecure = *req.Insecure
 	}
 
-	if len(target.URL) == 0 {
-		t.HandleBadRequest("empty endpoint")
-		return
-	}
-
 	t.ping(target.URL, target.Username, target.Password, target.Insecure)
 }