From 469d36d979b1377a3595947c0b8e87f5977f4f7b Mon Sep 17 00:00:00 2001 From: Nicola Murino Date: Sat, 16 May 2020 15:15:32 +0200 Subject: [PATCH] certificate auth: fix source address checking inside crypto/ssh So we can avoid to check source address ourself https://github.com/drakkan/crypto/commit/81aafe6d26b96f13d1f994cb62690fcba59e8529 Signed-off-by: Nicola Murino --- go.mod | 8 ++++---- go.sum | 9 +++++---- sftpd/internal_test.go | 5 ++++- sftpd/server.go | 21 ++++++++++++--------- sftpd/sftpd_test.go | 2 +- utils/utils.go | 31 ------------------------------- 6 files changed, 26 insertions(+), 50 deletions(-) diff --git a/go.mod b/go.mod index 326826e3..cf9d5f5f 100644 --- a/go.mod +++ b/go.mod @@ -34,9 +34,9 @@ require ( github.com/spf13/viper v1.6.3 github.com/stretchr/testify v1.5.1 go.etcd.io/bbolt v1.3.4 - golang.org/x/crypto v0.0.0-20200323165209-0ec3e9974c59 - golang.org/x/sys v0.0.0-20200331124033-c3d80250170d - golang.org/x/tools v0.0.0-20200403170748-4480df5f1627 // indirect + golang.org/x/crypto v0.0.0-20200510223506-06a226fb4e37 + golang.org/x/sys v0.0.0-20200515095857-1151b9dac4a9 + golang.org/x/tools v0.0.0-20200515220128-d3bf790afa53 // indirect google.golang.org/api v0.20.0 google.golang.org/genproto v0.0.0-20200403120447-c50568487044 // indirect gopkg.in/ini.v1 v1.55.0 // indirect @@ -45,5 +45,5 @@ require ( replace ( github.com/pkg/sftp => github.com/drakkan/sftp v0.0.0-20200319122022-2fc68482d27f - golang.org/x/crypto => github.com/drakkan/crypto v0.0.0-20200409210311-95730af1ff98 + golang.org/x/crypto => github.com/drakkan/crypto v0.0.0-20200516130408-81aafe6d26b9 ) diff --git a/go.sum b/go.sum index 7a6c2cbd..2a5997cd 100644 --- a/go.sum +++ b/go.sum @@ -63,8 +63,8 @@ github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/dgrijalva/jwt-go v3.2.0+incompatible/go.mod h1:E3ru+11k8xSBh+hMPgOLZmtrrCbhqsmaPHjLKYnJCaQ= github.com/dgryski/go-sip13 v0.0.0-20181026042036-e10d5fee7954/go.mod h1:vAd38F8PWV+bWy6jNmig1y/TA+kYO4g3RSRF0IAv0no= -github.com/drakkan/crypto v0.0.0-20200409210311-95730af1ff98 h1:5yTRWoqE1GhPfctw9E0wh1d3+xHajMyhQAQuxuNqwQ8= -github.com/drakkan/crypto v0.0.0-20200409210311-95730af1ff98/go.mod h1:v3bhWOXGYda7H5d2s5t9XA6th3fxW3s0MQxU1R96G/w= +github.com/drakkan/crypto v0.0.0-20200516130408-81aafe6d26b9 h1:H+Pr8rDH54EXtBEA5ejYdO9yhFC2xKlCRidiyseuIAg= +github.com/drakkan/crypto v0.0.0-20200516130408-81aafe6d26b9/go.mod h1:v3bhWOXGYda7H5d2s5t9XA6th3fxW3s0MQxU1R96G/w= github.com/drakkan/sftp v0.0.0-20200319122022-2fc68482d27f h1:hD7R/tb8VF5Lwj0J3PnPZwN1twzpwWS2QJQYkliJ/Jw= github.com/drakkan/sftp v0.0.0-20200319122022-2fc68482d27f/go.mod h1:PIrgHN0+qgDmYTNiwryjoEqmXo9tv8aMwQ//Yg1xwIs= github.com/eikenb/pipeat v0.0.0-20200430215831-470df5986b6d h1:8RvCRWer7TB2n+DKhW4uW15hRiqPmabSnSyYhju/Nuw= @@ -367,8 +367,9 @@ golang.org/x/sys v0.0.0-20200202164722-d101bd2416d5/go.mod h1:h1NjWce9XRLGQEsW7w golang.org/x/sys v0.0.0-20200212091648-12a6c2dcc1e4/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20200223170610-d5e6a3e2c0ae/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20200323222414-85ca7c5b95cd/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= -golang.org/x/sys v0.0.0-20200331124033-c3d80250170d h1:nc5K6ox/4lTFbMVSL9WRR81ixkcwXThoiF6yf+R9scA= golang.org/x/sys v0.0.0-20200331124033-c3d80250170d/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= +golang.org/x/sys v0.0.0-20200515095857-1151b9dac4a9 h1:YTzHMGlqJu67/uEo1lBv0n3wBXhXNeUbB1XfN2vmTm0= +golang.org/x/sys v0.0.0-20200515095857-1151b9dac4a9/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/text v0.0.0-20170915032832-14c0d48ead0c/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= golang.org/x/text v0.3.1-0.20180807135948-17ff2d5776d2/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= @@ -411,7 +412,7 @@ golang.org/x/tools v0.0.0-20200207183749-b753a1ba74fa/go.mod h1:TB2adYChydJhpapK golang.org/x/tools v0.0.0-20200212150539-ea181f53ac56/go.mod h1:TB2adYChydJhpapKDTa4BR/hXlZSLoq2Wpct/0txZ28= golang.org/x/tools v0.0.0-20200224181240-023911ca70b2/go.mod h1:TB2adYChydJhpapKDTa4BR/hXlZSLoq2Wpct/0txZ28= golang.org/x/tools v0.0.0-20200331025713-a30bf2db82d4/go.mod h1:Sl4aGygMT6LrqrWclx+PTx3U+LnKx/seiNR+3G19Ar8= -golang.org/x/tools v0.0.0-20200403170748-4480df5f1627/go.mod h1:EkVYQZoAsY45+roYkvgYkIh4xh/qjgUK9TdY2XT94GE= +golang.org/x/tools v0.0.0-20200515220128-d3bf790afa53/go.mod h1:EkVYQZoAsY45+roYkvgYkIh4xh/qjgUK9TdY2XT94GE= golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= golang.org/x/xerrors v0.0.0-20191011141410-1b5146add898/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= diff --git a/sftpd/internal_test.go b/sftpd/internal_test.go index 1ac5f2e7..76246c27 100644 --- a/sftpd/internal_test.go +++ b/sftpd/internal_test.go @@ -1739,6 +1739,9 @@ func TestProxyProtocolVersion(t *testing.T) { func TestLoadHostKeys(t *testing.T) { c := Configuration{} c.Keys = []Key{ + { + PrivateKey: ".", + }, { PrivateKey: "missing file", }, @@ -1761,7 +1764,7 @@ func TestLoadHostKeys(t *testing.T) { func TestCertCheckerInitErrors(t *testing.T) { c := Configuration{} - c.TrustedUserCAKeys = append(c.TrustedUserCAKeys, "missing file") + c.TrustedUserCAKeys = []string{".", "missing file"} err := c.initializeCertChecker("") assert.Error(t, err) testfile := filepath.Join(os.TempDir(), "invalidkey") diff --git a/sftpd/server.go b/sftpd/server.go index e2b94d88..10bf8a71 100644 --- a/sftpd/server.go +++ b/sftpd/server.go @@ -167,7 +167,7 @@ func (c Configuration) Initialize(configDir string) error { PublicKeyCallback: func(conn ssh.ConnMetadata, pubKey ssh.PublicKey) (*ssh.Permissions, error) { sp, err := c.validatePublicKeyCredentials(conn, pubKey) if err == ssh.ErrPartialSuccess { - return nil, err + return sp, err } if err != nil { return nil, &authenticationError{err: fmt.Sprintf("could not validate public key credentials: %v", err)} @@ -530,6 +530,11 @@ func (c *Configuration) checkAndLoadHostKeys(configDir string, serverConfig *ssh } for _, k := range c.Keys { privateFile := k.PrivateKey + if !utils.IsFileInputValid(privateFile) { + logger.Warn(logSender, "", "unable to load invalid host key: %#v", privateFile) + logger.WarnToConsole("unable to load invalid host key: %#v", privateFile) + continue + } if !filepath.IsAbs(privateFile) { privateFile = filepath.Join(configDir, privateFile) } @@ -553,6 +558,11 @@ func (c *Configuration) checkAndLoadHostKeys(configDir string, serverConfig *ssh func (c *Configuration) initializeCertChecker(configDir string) error { for _, keyPath := range c.TrustedUserCAKeys { + if !utils.IsFileInputValid(keyPath) { + logger.Warn(logSender, "", "unable to load invalid trusted user CA key: %#v", keyPath) + logger.WarnToConsole("unable to load invalid trusted user CA key: %#v", keyPath) + continue + } if !filepath.IsAbs(keyPath) { keyPath = filepath.Join(configDir, keyPath) } @@ -611,19 +621,12 @@ func (c Configuration) validatePublicKeyCredentials(conn ssh.ConnMetadata, pubKe updateLoginMetrics(conn, method, err) return nil, err } - // we need to check source address ourself since crypto/ssh will skip this check if we return partial success - if cert.Permissions.CriticalOptions != nil && cert.Permissions.CriticalOptions[sourceAddressCriticalOption] != "" { - if err := utils.CheckSourceAddress(conn.RemoteAddr(), cert.Permissions.CriticalOptions[sourceAddressCriticalOption]); err != nil { - updateLoginMetrics(conn, method, err) - return nil, err - } - } certPerm = &cert.Permissions } if user, keyID, err = dataprovider.CheckUserAndPubKey(dataProvider, conn.User(), pubKey.Marshal()); err == nil { if user.IsPartialAuth(method) { logger.Debug(logSender, connectionID, "user %#v authenticated with partial success", conn.User()) - return nil, ssh.ErrPartialSuccess + return certPerm, ssh.ErrPartialSuccess } sshPerm, err = loginUser(user, method, keyID, conn) if err == nil && certPerm != nil { diff --git a/sftpd/sftpd_test.go b/sftpd/sftpd_test.go index 09b5f080..f97866b8 100644 --- a/sftpd/sftpd_test.go +++ b/sftpd/sftpd_test.go @@ -833,7 +833,7 @@ func TestLoginUserCert(t *testing.T) { err = os.RemoveAll(user.GetHomeDir()) assert.NoError(t, err) - // now login with a username not in the set of valid principals for given certificate + // now login with a username not in the set of valid principals for the given certificate u.Username += "1" user, _, err = httpd.AddUser(u, http.StatusOK) assert.NoError(t, err) diff --git a/utils/utils.go b/utils/utils.go index 1580ebb9..bd8bee7b 100644 --- a/utils/utils.go +++ b/utils/utils.go @@ -317,34 +317,3 @@ func CleanDirInput(dirInput string) string { } return filepath.Clean(dirInput) } - -// CheckSourceAddress check the source address against the one defined inside an SSH user certificate -func CheckSourceAddress(addr net.Addr, sourceAddrs string) error { - if addr == nil { - return errors.New("ssh: no address known for client, but source-address match required") - } - - tcpAddr, ok := addr.(*net.TCPAddr) - if !ok { - return fmt.Errorf("ssh: remote address %v is not an TCP address when checking source-address match", addr) - } - - for _, sourceAddr := range strings.Split(sourceAddrs, ",") { - if allowedIP := net.ParseIP(sourceAddr); allowedIP != nil { - if allowedIP.Equal(tcpAddr.IP) { - return nil - } - } else { - _, ipNet, err := net.ParseCIDR(sourceAddr) - if err != nil { - return fmt.Errorf("ssh: error parsing source-address restriction %q: %v", sourceAddr, err) - } - - if ipNet.Contains(tcpAddr.IP) { - return nil - } - } - } - - return fmt.Errorf("ssh: remote address %v is not allowed because of source-address restriction", addr) -}