diff --git a/src/common/models/models_test.go b/src/common/models/models_test.go deleted file mode 100644 index 822312399..000000000 --- a/src/common/models/models_test.go +++ /dev/null @@ -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) { -} diff --git a/src/common/models/replication_job.go b/src/common/models/replication_job.go index 255fc1d9a..262ff2473 100644 --- a/src/common/models/replication_job.go +++ b/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 { - v.SetError("endpoint", "can not be empty") - } - - r.URL = utils.FormatEndpoint(r.URL) - - if len(r.URL) > 64 { - v.SetError("endpoint", "max length is 64") + url, err := utils.ParseEndpoint(r.URL) + if err != nil { + v.SetError("endpoint", err.Error()) + } else { + // Prevent SSRF security issue #3755 + r.URL = url.Scheme + "://" + url.Host + url.Path + if len(r.URL) > 64 { + v.SetError("endpoint", "max length is 64") + } } // password is encoded using base64, the length of this field diff --git a/src/common/models/target_test.go b/src/common/models/target_test.go new file mode 100644 index 000000000..f8bf2ea92 --- /dev/null +++ b/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) + } +} diff --git a/src/common/utils/registry/registry.go b/src/common/utils/registry/registry.go index b84dabb97..bbd351309 100644 --- a/src/common/utils/registry/registry.go +++ b/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 } diff --git a/src/common/utils/registry/registry_test.go b/src/common/utils/registry/registry_test.go index 4f62ba595..b4204aaa3 100644 --- a/src/common/utils/registry/registry_test.go +++ b/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), }) diff --git a/src/common/utils/utils.go b/src/common/utils/utils.go index 01be8ac45..0cc64b402 100644 --- a/src/common/utils/utils.go +++ b/src/common/utils/utils.go @@ -29,27 +29,24 @@ import ( "github.com/vmware/harbor/src/common/utils/log" ) -// FormatEndpoint formats endpoint -func FormatEndpoint(endpoint string) string { - endpoint = strings.TrimSpace(endpoint) +// ParseEndpoint parses endpoint to a URL +func ParseEndpoint(endpoint string) (*url.URL, error) { + endpoint = strings.Trim(endpoint, " ") endpoint = strings.TrimRight(endpoint, "/") - if !strings.HasPrefix(endpoint, "http://") && - !strings.HasPrefix(endpoint, "https://") { + if len(endpoint) == 0 { + 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 -} - -// 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 + return url.ParseRequestURI(endpoint) } // ParseRepository splits a repository into two parts: project and rest diff --git a/src/common/utils/utils_test.go b/src/common/utils/utils_test.go index 24539fc54..7dcabc527 100644 --- a/src/common/utils/utils_test.go +++ b/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" - u, err := ParseEndpoint(endpoint) - if err != nil { - t.Fatalf("failed to parse endpoint %s: %v", endpoint, err) + cases := []struct { + input string + err bool + 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" { - t.Errorf("unexpected endpoint: %s != %s", endpoint, "http://example.com") - } - - endpoint = "https://example.com" - u, err = ParseEndpoint(endpoint) - if err != nil { - t.Fatalf("failed to parse endpoint %s: %v", endpoint, err) - } - - 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") + for _, c := range cases { + u, err := ParseEndpoint(c.input) + if c.err { + require.NotNil(t, err) + continue + } + require.Nil(t, err) + assert.Equal(t, c.expected, u.String()) } } diff --git a/src/ui/api/email.go b/src/ui/api/email.go index 9dd3d67b8..66a666a46 100644 --- a/src/ui/api/email.go +++ b/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) - e.CustomAbort(http.StatusBadRequest, err.Error()) + log.Errorf("failed to ping email server: %v", err) + // 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 } } diff --git a/src/ui/api/email_test.go b/src/ui/api/email_test.go index bf5d26111..154fb69e2 100644 --- a/src/ui/api/email_test.go +++ b/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") - - if !strings.Contains(body, "535") { - t.Errorf("unexpected error: %s does not contains 535", body) - return - } + assert.Equal(400, code) //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) } diff --git a/src/ui/api/ldap.go b/src/ui/api/ldap.go index f3f2005a5..42ffb058c 100644 --- a/src/ui/api/ldap.go +++ b/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 } diff --git a/src/ui/api/target.go b/src/ui/api/target.go index 68c3c2689..22d68fdf0 100644 --- a/src/ui/api/target.go +++ b/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 { - // timeout, dns resolve error, connection refused, etc. - 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 == nil { + err = registry.Ping() } - if err = registry.Ping(); err != nil { - if regErr, ok := err.(*registry_error.HTTPError); ok { - t.CustomAbort(regErr.StatusCode, regErr.Detail) - } - - log.Errorf("failed to ping registry %s: %v", registry.Endpoint.String(), err) - t.CustomAbort(http.StatusInternalServerError, http.StatusText(http.StatusInternalServerError)) + if err != nil { + log.Errorf("failed to ping target: %v", err) + // 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 } } @@ -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) }