From fad6af11e56f286a343f593bd57dd9fd8dd5a82f Mon Sep 17 00:00:00 2001 From: Nicola Murino Date: Tue, 28 Feb 2023 18:01:09 +0100 Subject: [PATCH] don't expose error messages from pre-actions and post connect hooks always return a generic error instead to avoid leaking internal info Signed-off-by: Nicola Murino --- go.mod | 4 ++-- go.sum | 12 ++++++------ internal/common/common.go | 11 ++++++----- internal/common/connection.go | 20 ++++++++++++-------- internal/ftpd/handler.go | 4 ++-- internal/ftpd/server.go | 2 +- internal/httpd/oidc.go | 2 +- internal/httpd/oidc_test.go | 2 +- internal/httpd/server.go | 2 +- 9 files changed, 32 insertions(+), 27 deletions(-) diff --git a/go.mod b/go.mod index 3c711ac7..20278b38 100644 --- a/go.mod +++ b/go.mod @@ -35,7 +35,7 @@ require ( github.com/hashicorp/go-hclog v1.4.0 github.com/hashicorp/go-plugin v1.4.8 github.com/hashicorp/go-retryablehttp v0.7.2 - github.com/jackc/pgx/v5 v5.3.0 + github.com/jackc/pgx/v5 v5.3.1 github.com/jlaffaye/ftp v0.0.0-20201112195030-9aae4d151126 github.com/klauspost/compress v1.16.0 github.com/lestrrat-go/jwx/v2 v2.0.8 @@ -157,7 +157,7 @@ require ( golang.org/x/tools v0.6.0 // indirect golang.org/x/xerrors v0.0.0-20220907171357-04be3eba64a2 // indirect google.golang.org/appengine v1.6.7 // indirect - google.golang.org/genproto v0.0.0-20230223222841-637eb2293923 // indirect + google.golang.org/genproto v0.0.0-20230227214838-9b19f0bdc514 // indirect google.golang.org/grpc v1.53.0 // indirect google.golang.org/protobuf v1.28.1 // indirect gopkg.in/ini.v1 v1.67.0 // indirect diff --git a/go.sum b/go.sum index cce951b3..1aa05394 100644 --- a/go.sum +++ b/go.sum @@ -215,8 +215,8 @@ cloud.google.com/go/iot v1.3.0/go.mod h1:r7RGh2B61+B8oz0AGE+J72AhA0G7tdXItODWsaA cloud.google.com/go/iot v1.4.0/go.mod h1:dIDxPOn0UvNDUMD8Ger7FIaTuvMkj+aGk94RPP0iV+g= cloud.google.com/go/kms v1.5.0/go.mod h1:QJS2YY0eJGBg3mnDfuaCyLauWwBJiHRboYxJ++1xJNg= cloud.google.com/go/kms v1.6.0/go.mod h1:Jjy850yySiasBUDi6KFUwUv2n1+o7QZFyuUJg6OgjA0= -cloud.google.com/go/kms v1.7.0 h1:8FCf8C7qfOuSr6YzOQ4RGjJvswSRFeOpur3nHOlJbio= cloud.google.com/go/kms v1.7.0/go.mod h1:k2UdVoNIHLJi/Rnng6dN0vlq7lS3jHSDiZasft+gmYE= +cloud.google.com/go/kms v1.8.0 h1:VrJLOsMRzW7IqTTYn+OYupqF3iKSE060Nrn+PECrYjg= cloud.google.com/go/language v1.4.0/go.mod h1:F9dRpNFQmJbkaop6g0JhSBXCNlO90e1KWx5iDdxbWic= cloud.google.com/go/language v1.6.0/go.mod h1:6dJ8t3B+lUYfStgls25GusK04NLh3eDLQnWM3mdEbhI= cloud.google.com/go/language v1.7.0/go.mod h1:DJ6dYN/W+SQOjF8e1hLQXMF21AkH2w9wiPzPCJa2MIE= @@ -224,8 +224,8 @@ cloud.google.com/go/language v1.8.0/go.mod h1:qYPVHf7SPoNNiCL2Dr0FfEFNil1qi3pQEy cloud.google.com/go/lifesciences v0.5.0/go.mod h1:3oIKy8ycWGPUyZDR/8RNnTOYevhaMLqh5vLUXs9zvT8= cloud.google.com/go/lifesciences v0.6.0/go.mod h1:ddj6tSX/7BOnhxCSd3ZcETvtNr8NZ6t/iPhY2Tyfu08= cloud.google.com/go/longrunning v0.1.1/go.mod h1:UUFxuDWkv22EuY93jjmDMFT5GPQKeFVJBIF6QlTqdsE= -cloud.google.com/go/longrunning v0.3.0 h1:NjljC+FYPV3uh5/OwWT6pVU+doBqMg2x/rZlE+CamDs= cloud.google.com/go/longrunning v0.3.0/go.mod h1:qth9Y41RRSUE69rDcOn6DdK3HfQfsUI0YSmW3iIlLJc= +cloud.google.com/go/longrunning v0.4.1 h1:v+yFJOfKC3yZdY6ZUI933pIYdhyhV8S3NpWrXWmg7jM= cloud.google.com/go/managedidentities v1.3.0/go.mod h1:UzlW3cBOiPrzucO5qWkNkh0w33KFtBJU281hacNvsdE= cloud.google.com/go/managedidentities v1.4.0/go.mod h1:NWSBYbEMgqmbZsLIyKvxrYbtqOsxY1ZrGM+9RgDqInM= cloud.google.com/go/mediatranslation v0.5.0/go.mod h1:jGPUhGTybqsPQn91pNXw0xVHfuJ3leR1wj37oU3y1f4= @@ -1352,8 +1352,8 @@ github.com/jackc/pgx/v4 v4.12.1-0.20210724153913-640aa07df17c/go.mod h1:1QD0+tgS github.com/jackc/pgx/v4 v4.16.0/go.mod h1:N0A9sFdWzkw/Jy1lwoiB64F2+ugFZi987zRxcPez/wI= github.com/jackc/pgx/v4 v4.16.1/go.mod h1:SIhx0D5hoADaiXZVyv+3gSm3LCIIINTVO0PficsvWGQ= github.com/jackc/pgx/v4 v4.17.2/go.mod h1:lcxIZN44yMIrWI78a5CpucdD14hX0SBDbNRvjDBItsw= -github.com/jackc/pgx/v5 v5.3.0 h1:/NQi8KHMpKWHInxXesC8yD4DhkXPrVhmnwYkjp9AmBA= -github.com/jackc/pgx/v5 v5.3.0/go.mod h1:t3JDKnCBlYIc0ewLF0Q7B8MXmoIaBOZj/ic7iHozM/8= +github.com/jackc/pgx/v5 v5.3.1 h1:Fcr8QJ1ZeLi5zsPZqQeUZhNhxfkkKBOgJuYkJHoBOtU= +github.com/jackc/pgx/v5 v5.3.1/go.mod h1:t3JDKnCBlYIc0ewLF0Q7B8MXmoIaBOZj/ic7iHozM/8= github.com/jackc/puddle v0.0.0-20190413234325-e4ced69a3a2b/go.mod h1:m4B5Dj62Y0fbyuIc15OsIqK0+JU8nkqQjsgx7dvjSWk= github.com/jackc/puddle v0.0.0-20190608224051-11cab39313c9/go.mod h1:m4B5Dj62Y0fbyuIc15OsIqK0+JU8nkqQjsgx7dvjSWk= github.com/jackc/puddle v1.1.3/go.mod h1:m4B5Dj62Y0fbyuIc15OsIqK0+JU8nkqQjsgx7dvjSWk= @@ -2719,8 +2719,8 @@ google.golang.org/genproto v0.0.0-20221109142239-94d6d90a7d66/go.mod h1:rZS5c/ZV google.golang.org/genproto v0.0.0-20221118155620-16455021b5e6/go.mod h1:rZS5c/ZVYMaOGBfO68GWtjOw/eLaZM1X6iVtgjZ+EWg= google.golang.org/genproto v0.0.0-20221201164419-0e50fba7f41c/go.mod h1:rZS5c/ZVYMaOGBfO68GWtjOw/eLaZM1X6iVtgjZ+EWg= google.golang.org/genproto v0.0.0-20221201204527-e3fa12d562f3/go.mod h1:rZS5c/ZVYMaOGBfO68GWtjOw/eLaZM1X6iVtgjZ+EWg= -google.golang.org/genproto v0.0.0-20230223222841-637eb2293923 h1:znp6mq/drrY+6khTAlJUDNFFcDGV2ENLYKpMq8SyCds= -google.golang.org/genproto v0.0.0-20230223222841-637eb2293923/go.mod h1:3Dl5ZL0q0isWJt+FVcfpQyirqemEuLAK/iFvg1UP1Hw= +google.golang.org/genproto v0.0.0-20230227214838-9b19f0bdc514 h1:rtNKfB++wz5mtDY2t5C8TXlU5y52ojSu7tZo0z7u8eQ= +google.golang.org/genproto v0.0.0-20230227214838-9b19f0bdc514/go.mod h1:TvhZT5f700eVlTNwND1xoEZQeWTB2RY/65kplwl/bFA= google.golang.org/grpc v0.0.0-20160317175043-d3ddb4469d5a/go.mod h1:yo6s7OP7yaDglbqo1J04qKzAhqBH6lvTonzMVmEdcZw= google.golang.org/grpc v1.17.0/go.mod h1:6QZJwpn2B+Zp71q/5VxRsJ6NXXVCE5NRUHRo+f3cWCs= google.golang.org/grpc v1.19.0/go.mod h1:mqu4LbDTu4XGKhr4mRzUsmM4RtVoemTSY81AxZiDr8c= diff --git a/internal/common/common.go b/internal/common/common.go index fafce03b..1ceb58a2 100644 --- a/internal/common/common.go +++ b/internal/common/common.go @@ -737,7 +737,7 @@ func (c *Configuration) ExecutePostConnectHook(ipAddr, protocol string) error { if err != nil { logger.Warn(protocol, "", "Login from ip %q denied, invalid post connect hook %q: %v", ipAddr, c.PostConnectHook, err) - return err + return getPermissionDeniedError(protocol) } q := url.Query() q.Add("ip", ipAddr) @@ -747,19 +747,19 @@ func (c *Configuration) ExecutePostConnectHook(ipAddr, protocol string) error { resp, err := httpclient.RetryableGet(url.String()) if err != nil { logger.Warn(protocol, "", "Login from ip %q denied, error executing post connect hook: %v", ipAddr, err) - return err + return getPermissionDeniedError(protocol) } defer resp.Body.Close() if resp.StatusCode != http.StatusOK { logger.Warn(protocol, "", "Login from ip %q denied, post connect hook response code: %v", ipAddr, resp.StatusCode) - return errUnexpectedHTTResponse + return getPermissionDeniedError(protocol) } return nil } if !filepath.IsAbs(c.PostConnectHook) { err := fmt.Errorf("invalid post connect hook %q", c.PostConnectHook) logger.Warn(protocol, "", "Login from ip %q denied: %v", ipAddr, err) - return err + return getPermissionDeniedError(protocol) } timeout, env, args := command.GetConfig(c.PostConnectHook, command.HookPostConnect) ctx, cancel := context.WithTimeout(context.Background(), timeout) @@ -772,8 +772,9 @@ func (c *Configuration) ExecutePostConnectHook(ipAddr, protocol string) error { err := cmd.Run() if err != nil { logger.Warn(protocol, "", "Login from ip %q denied, connect hook error: %v", ipAddr, err) + return getPermissionDeniedError(protocol) } - return err + return nil } // SSHConnection defines an ssh connection. diff --git a/internal/common/connection.go b/internal/common/connection.go index 4615b681..da9ad4f6 100644 --- a/internal/common/connection.go +++ b/internal/common/connection.go @@ -1566,14 +1566,7 @@ func (c *BaseConnection) GetErrorForDeniedFile(policy int) error { // GetPermissionDeniedError returns an appropriate permission denied error for the connection protocol func (c *BaseConnection) GetPermissionDeniedError() error { - switch c.protocol { - case ProtocolSFTP: - return sftp.ErrSSHFxPermissionDenied - case ProtocolWebDAV, ProtocolFTP, ProtocolHTTP, ProtocolOIDC, ProtocolHTTPShare, ProtocolDataRetention: - return os.ErrPermission - default: - return ErrPermissionDenied - } + return getPermissionDeniedError(c.protocol) } // GetNotExistError returns an appropriate not exist error for the connection protocol @@ -1723,6 +1716,17 @@ func (c *BaseConnection) GetFsAndResolvedPath(virtualPath string) (vfs.Fs, strin return fs, fsPath, nil } +func getPermissionDeniedError(protocol string) error { + switch protocol { + case ProtocolSFTP: + return sftp.ErrSSHFxPermissionDenied + case ProtocolWebDAV, ProtocolFTP, ProtocolHTTP, ProtocolOIDC, ProtocolHTTPShare, ProtocolDataRetention: + return os.ErrPermission + default: + return ErrPermissionDenied + } +} + func keepConnectionAlive(c *BaseConnection, done chan bool, interval time.Duration) { ticker := time.NewTicker(interval) defer func() { diff --git a/internal/ftpd/handler.go b/internal/ftpd/handler.go index dfb2dfd2..2a947464 100644 --- a/internal/ftpd/handler.go +++ b/internal/ftpd/handler.go @@ -406,7 +406,7 @@ func (c *Connection) handleFTPUploadToNewFile(fs vfs.Fs, flags int, resolvedPath } if _, err := common.ExecutePreAction(c.BaseConnection, common.OperationPreUpload, resolvedPath, requestPath, 0, 0); err != nil { c.Log(logger.LevelDebug, "upload for file %q denied by pre action: %v", requestPath, err) - return nil, fmt.Errorf("%w, denied by pre-upload action", ftpserver.ErrFileNameNotAllowed) + return nil, ftpserver.ErrFileNameNotAllowed } file, w, cancelFn, err := fs.Create(filePath, flags) if err != nil { @@ -451,7 +451,7 @@ func (c *Connection) handleFTPUploadToExistingFile(fs vfs.Fs, flags int, resolve } if _, err := common.ExecutePreAction(c.BaseConnection, common.OperationPreUpload, resolvedPath, requestPath, fileSize, flags); err != nil { c.Log(logger.LevelDebug, "upload for file %q denied by pre action: %v", requestPath, err) - return nil, fmt.Errorf("%w, denied by pre-upload action", ftpserver.ErrFileNameNotAllowed) + return nil, ftpserver.ErrFileNameNotAllowed } if common.Config.IsAtomicUploadEnabled() && fs.IsAtomicUploadSupported() { diff --git a/internal/ftpd/server.go b/internal/ftpd/server.go index c8880d52..460d4dc3 100644 --- a/internal/ftpd/server.go +++ b/internal/ftpd/server.go @@ -173,7 +173,7 @@ func (s *Server) ClientConnected(cc ftpserver.ClientContext) (string, error) { return fmt.Sprintf("Access denied: %v", err.Error()), err } if err := common.Config.ExecutePostConnectHook(ipAddr, common.ProtocolFTP); err != nil { - return "Access denied by post connect hook", err + return "Access denied", err } connID := fmt.Sprintf("%v_%v", s.ID, cc.ID()) user := dataprovider.User{} diff --git a/internal/httpd/oidc.go b/internal/httpd/oidc.go index acfa0f42..35a0d8ec 100644 --- a/internal/httpd/oidc.go +++ b/internal/httpd/oidc.go @@ -431,7 +431,7 @@ func (t *oidcToken) getUser(r *http.Request) error { } if err := common.Config.ExecutePostConnectHook(ipAddr, common.ProtocolOIDC); err != nil { updateLoginMetrics(&user, dataprovider.LoginMethodIDP, ipAddr, err) - return fmt.Errorf("access denied by post connect hook: %w", err) + return fmt.Errorf("access denied: %w", err) } if err := user.CheckLoginConditions(); err != nil { updateLoginMetrics(&user, dataprovider.LoginMethodIDP, ipAddr, err) diff --git a/internal/httpd/oidc_test.go b/internal/httpd/oidc_test.go index a43db595..cc74f575 100644 --- a/internal/httpd/oidc_test.go +++ b/internal/httpd/oidc_test.go @@ -908,7 +908,7 @@ func TestOIDCToken(t *testing.T) { err = token.getUser(req) if assert.Error(t, err) { - assert.Contains(t, err.Error(), "access denied by post connect hook") + assert.Contains(t, err.Error(), "access denied") } common.Config.PostConnectHook = "" diff --git a/internal/httpd/server.go b/internal/httpd/server.go index 4624c58f..e26a04d6 100644 --- a/internal/httpd/server.go +++ b/internal/httpd/server.go @@ -246,7 +246,7 @@ func (s *httpdServer) handleWebClientLoginPost(w http.ResponseWriter, r *http.Re } if err := common.Config.ExecutePostConnectHook(ipAddr, protocol); err != nil { - s.renderClientLoginPage(w, fmt.Sprintf("access denied by post connect hook: %v", err), ipAddr) + s.renderClientLoginPage(w, fmt.Sprintf("access denied: %v", err), ipAddr) return }