From 65c228394fb1a18c824eb472be07b848dd595745 Mon Sep 17 00:00:00 2001 From: Nick Meves Date: Sun, 19 Jul 2020 22:24:18 -0700 Subject: [PATCH] Address gosec findings Mostly handling unhandled errors appropriately. If logging to STDERR fails, we panic. Added #nosec comments to findings we are OK with. --- CHANGELOG.md | 1 + http.go | 10 ++++-- main.go | 8 +++-- oauthproxy.go | 54 ++++++++++++++++++++++------ pkg/authentication/basic/htpasswd.go | 24 ++++++++----- pkg/encryption/utils.go | 16 +++++++-- pkg/logger/logger.go | 30 ++++++++++++---- pkg/middleware/stored_session.go | 7 ++-- pkg/upstream/http.go | 2 ++ pkg/upstream/proxy.go | 5 ++- pkg/util/util.go | 2 +- pkg/validation/logging.go | 7 ++-- pkg/validation/options.go | 6 +++- providers/logingov.go | 4 ++- validator.go | 8 ++++- watcher.go | 12 +++++-- 16 files changed, 155 insertions(+), 41 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5b72f595..23c6e9a2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ ## Changes since v6.0.0 +- [#690](https://github.com/oauth2-proxy/oauth2-proxy/pull/690) Address GoSec security findings & remediate (@NickMeves) - [#689](https://github.com/oauth2-proxy/oauth2-proxy/pull/689) Fix finicky logging_handler_test from time drift (@NickMeves) - [#699](https://github.com/oauth2-proxy/oauth2-proxy/pull/699) Align persistence ginkgo tests with conventions (@NickMeves) - [#696](https://github.com/oauth2-proxy/oauth2-proxy/pull/696) Preserve query when building redirect diff --git a/http.go b/http.go index 7e1215f7..024c617b 100644 --- a/http.go +++ b/http.go @@ -124,7 +124,13 @@ func (ln tcpKeepAliveListener) Accept() (c net.Conn, err error) { if err != nil { return } - tc.SetKeepAlive(true) - tc.SetKeepAlivePeriod(3 * time.Minute) + err = tc.SetKeepAlive(true) + if err != nil { + return nil, err + } + err = tc.SetKeepAlivePeriod(3 * time.Minute) + if err != nil { + return nil, err + } return tc, nil } diff --git a/main.go b/main.go index 24d48072..a75a303b 100644 --- a/main.go +++ b/main.go @@ -25,7 +25,11 @@ func main() { config := flagSet.String("config", "", "path to config file") showVersion := flagSet.Bool("version", false, "print version string") - flagSet.Parse(os.Args[1:]) + err := flagSet.Parse(os.Args[1:]) + if err != nil { + logger.Printf("ERROR: Failed to parse flags: %v", err) + os.Exit(1) + } if *showVersion { fmt.Printf("oauth2-proxy %s (built with %s)\n", VERSION, runtime.Version()) @@ -33,7 +37,7 @@ func main() { } legacyOpts := options.NewLegacyOptions() - err := options.Load(*config, flagSet, legacyOpts) + err = options.Load(*config, flagSet, legacyOpts) if err != nil { logger.Printf("ERROR: Failed to load config: %v", err) os.Exit(1) diff --git a/oauthproxy.go b/oauthproxy.go index 64df8eb1..c5e513a7 100644 --- a/oauthproxy.go +++ b/oauthproxy.go @@ -364,7 +364,11 @@ func (p *OAuthProxy) SaveSession(rw http.ResponseWriter, req *http.Request, s *s // RobotsTxt disallows scraping pages from the OAuthProxy func (p *OAuthProxy) RobotsTxt(rw http.ResponseWriter) { rw.WriteHeader(http.StatusOK) - fmt.Fprintf(rw, "User-agent: *\nDisallow: /") + _, err := fmt.Fprintf(rw, "User-agent: *\nDisallow: /") + if err != nil { + logger.Printf("Error writing robots.txt: %s", err.Error()) + p.ErrorPage(rw, 500, "Internal Error", err.Error()) + } } // ErrorPage writes an error response @@ -379,13 +383,22 @@ func (p *OAuthProxy) ErrorPage(rw http.ResponseWriter, code int, title string, m Message: message, ProxyPrefix: p.ProxyPrefix, } - p.templates.ExecuteTemplate(rw, "error.html", t) + err := p.templates.ExecuteTemplate(rw, "error.html", t) + if err != nil { + logger.Printf("Error rendering error.html template: %s", err.Error()) + http.Error(rw, "Internal Server Error", http.StatusInternalServerError) + } } // SignInPage writes the sing in template to the response func (p *OAuthProxy) SignInPage(rw http.ResponseWriter, req *http.Request, code int) { prepareNoCache(rw) - p.ClearSessionCookie(rw, req) + err := p.ClearSessionCookie(rw, req) + if err != nil { + logger.Printf("Error clearing session cookie: %s", err.Error()) + p.ErrorPage(rw, 500, "Internal Error", err.Error()) + return + } rw.WriteHeader(code) redirectURL, err := p.GetRedirect(req) @@ -419,11 +432,15 @@ func (p *OAuthProxy) SignInPage(rw http.ResponseWriter, req *http.Request, code if p.providerNameOverride != "" { t.ProviderName = p.providerNameOverride } - p.templates.ExecuteTemplate(rw, "sign_in.html", t) + err = p.templates.ExecuteTemplate(rw, "sign_in.html", t) + if err != nil { + logger.Printf("Error rendering sign_in.html template: %s", err.Error()) + p.ErrorPage(rw, 500, "Internal Error", err.Error()) + } } // ManualSignIn handles basic auth logins to the proxy -func (p *OAuthProxy) ManualSignIn(rw http.ResponseWriter, req *http.Request) (string, bool) { +func (p *OAuthProxy) ManualSignIn(req *http.Request) (string, bool) { if req.Method != "POST" || p.basicAuthValidator == nil { return "", false } @@ -632,10 +649,15 @@ func (p *OAuthProxy) SignIn(rw http.ResponseWriter, req *http.Request) { return } - user, ok := p.ManualSignIn(rw, req) + user, ok := p.ManualSignIn(req) if ok { session := &sessionsapi.SessionState{User: user} - p.SaveSession(rw, req, session) + err = p.SaveSession(rw, req, session) + if err != nil { + logger.Printf("Error saving session: %s", err.Error()) + p.ErrorPage(rw, 500, "Internal Error", err.Error()) + return + } http.Redirect(rw, req, redirect, http.StatusFound) } else { if p.SkipProviderButton { @@ -663,7 +685,11 @@ func (p *OAuthProxy) UserInfo(rw http.ResponseWriter, req *http.Request) { } rw.Header().Set("Content-Type", "application/json") rw.WriteHeader(http.StatusOK) - json.NewEncoder(rw).Encode(userInfo) + err = json.NewEncoder(rw).Encode(userInfo) + if err != nil { + logger.Printf("Error encoding user info: %s", err.Error()) + p.ErrorPage(rw, 500, "Internal Error", err.Error()) + } } // SignOut sends a response to clear the authentication cookie @@ -674,7 +700,12 @@ func (p *OAuthProxy) SignOut(rw http.ResponseWriter, req *http.Request) { p.ErrorPage(rw, 500, "Internal Error", err.Error()) return } - p.ClearSessionCookie(rw, req) + err = p.ClearSessionCookie(rw, req) + if err != nil { + logger.Printf("Error clearing session cookie: %s", err.Error()) + p.ErrorPage(rw, 500, "Internal Error", err.Error()) + return + } http.Redirect(rw, req, redirect, http.StatusFound) } @@ -837,7 +868,10 @@ func (p *OAuthProxy) getAuthenticatedSession(rw http.ResponseWriter, req *http.R if session != nil && session.Email != "" && !p.Validator(session.Email) { logger.Printf(session.Email, req, logger.AuthFailure, "Invalid authentication via session: removing session %s", session) // Invalid session, clear it - p.ClearSessionCookie(rw, req) + err := p.ClearSessionCookie(rw, req) + if err != nil { + logger.Printf("Error clearing session cookie: %s", err.Error()) + } return nil, ErrNeedsLogin } diff --git a/pkg/authentication/basic/htpasswd.go b/pkg/authentication/basic/htpasswd.go index ff87894f..f4f46813 100644 --- a/pkg/authentication/basic/htpasswd.go +++ b/pkg/authentication/basic/htpasswd.go @@ -1,7 +1,7 @@ package basic import ( - "crypto/sha1" + "crypto/sha1" // #nosec G505 "encoding/base64" "encoding/csv" "fmt" @@ -29,11 +29,16 @@ type sha1Pass string // NewHTPasswdValidator constructs an httpasswd based validator from the file // at the path given. func NewHTPasswdValidator(path string) (Validator, error) { - r, err := os.Open(path) + r, err := os.Open(path) // #nosec G304 if err != nil { return nil, fmt.Errorf("could not open htpasswd file: %v", err) } - defer r.Close() + defer func(c io.Closer) { + cerr := c.Close() + if cerr != nil { + logger.Fatalf("error closing the htpasswd file: %v", cerr) + } + }(r) return newHtpasswd(r) } @@ -83,13 +88,16 @@ func (h *htpasswdMap) Validate(user string, password string) bool { return false } - switch real := realPassword.(type) { + switch rp := realPassword.(type) { case sha1Pass: - d := sha1.New() - d.Write([]byte(password)) - return string(real) == base64.StdEncoding.EncodeToString(d.Sum(nil)) + d := sha1.New() // #nosec G401 + _, err := d.Write([]byte(password)) + if err != nil { + return false + } + return string(rp) == base64.StdEncoding.EncodeToString(d.Sum(nil)) case bcryptPass: - return bcrypt.CompareHashAndPassword([]byte(real), []byte(password)) == nil + return bcrypt.CompareHashAndPassword([]byte(rp), []byte(password)) == nil default: return false } diff --git a/pkg/encryption/utils.go b/pkg/encryption/utils.go index 26be6b24..e3126045 100644 --- a/pkg/encryption/utils.go +++ b/pkg/encryption/utils.go @@ -2,15 +2,19 @@ package encryption import ( "crypto/hmac" - "crypto/sha1" + "crypto/rand" + "crypto/sha1" // #nosec G505 "crypto/sha256" "encoding/base64" "fmt" "hash" + "io" "net/http" "strconv" "strings" "time" + + "github.com/oauth2-proxy/oauth2-proxy/pkg/logger" ) // SecretBytes attempts to base64 decode the secret, if that fails it treats the secret as binary @@ -76,7 +80,15 @@ func SignedValue(seed string, key string, value []byte, now time.Time) string { func cookieSignature(signer func() hash.Hash, args ...string) string { h := hmac.New(signer, []byte(args[0])) for _, arg := range args[1:] { - h.Write([]byte(arg)) + _, err := h.Write([]byte(arg)) + // If signing fails, fail closed and return something that won't validate + if err != nil { + garbage := make([]byte, 16) + if _, err := io.ReadFull(rand.Reader, garbage); err != nil { + logger.Fatal("HMAC & RNG functions both failing. Shutting down for security") + } + return base64.URLEncoding.EncodeToString(garbage) + } } var b []byte b = h.Sum(b) diff --git a/pkg/logger/logger.go b/pkg/logger/logger.go index 9bfc2b3a..e0ce61dd 100644 --- a/pkg/logger/logger.go +++ b/pkg/logger/logger.go @@ -132,13 +132,19 @@ func (l *Logger) Output(calldepth int, message string) { l.mu.Lock() defer l.mu.Unlock() - l.stdLogTemplate.Execute(l.writer, stdLogMessageData{ + err := l.stdLogTemplate.Execute(l.writer, stdLogMessageData{ Timestamp: FormatTimestamp(now), File: file, Message: message, }) + if err != nil { + panic(err) + } - l.writer.Write([]byte("\n")) + _, err = l.writer.Write([]byte("\n")) + if err != nil { + panic(err) + } } // PrintAuthf writes auth info to the logger. Requires an http.Request to @@ -160,7 +166,7 @@ func (l *Logger) PrintAuthf(username string, req *http.Request, status AuthStatu l.mu.Lock() defer l.mu.Unlock() - l.authTemplate.Execute(l.writer, authLogMessageData{ + err := l.authTemplate.Execute(l.writer, authLogMessageData{ Client: client, Host: req.Host, Protocol: req.Proto, @@ -171,8 +177,14 @@ func (l *Logger) PrintAuthf(username string, req *http.Request, status AuthStatu Status: string(status), Message: fmt.Sprintf(format, a...), }) + if err != nil { + panic(err) + } - l.writer.Write([]byte("\n")) + _, err = l.writer.Write([]byte("\n")) + if err != nil { + panic(err) + } } // PrintReq writes request details to the Logger using the http.Request, @@ -208,7 +220,7 @@ func (l *Logger) PrintReq(username, upstream string, req *http.Request, url url. l.mu.Lock() defer l.mu.Unlock() - l.reqTemplate.Execute(l.writer, reqLogMessageData{ + err := l.reqTemplate.Execute(l.writer, reqLogMessageData{ Client: client, Host: req.Host, Protocol: req.Proto, @@ -222,8 +234,14 @@ func (l *Logger) PrintReq(username, upstream string, req *http.Request, url url. UserAgent: fmt.Sprintf("%q", req.UserAgent()), Username: username, }) + if err != nil { + panic(err) + } - l.writer.Write([]byte("\n")) + _, err = l.writer.Write([]byte("\n")) + if err != nil { + panic(err) + } } // GetFileLineString will find the caller file and line number diff --git a/pkg/middleware/stored_session.go b/pkg/middleware/stored_session.go index dd4d1405..d299fdf8 100644 --- a/pkg/middleware/stored_session.go +++ b/pkg/middleware/stored_session.go @@ -55,7 +55,7 @@ type storedSessionLoader struct { } // loadSession attempts to load a session as identified by the request cookies. -// If no session is found, the request will be passed to the nex handler. +// If no session is found, the request will be passed to the next handler. // If a session was loader by a previous handler, it will not be replaced. func (s *storedSessionLoader) loadSession(next http.Handler) http.Handler { return http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) { @@ -73,7 +73,10 @@ func (s *storedSessionLoader) loadSession(next http.Handler) http.Handler { // In the case when there was an error loading the session, // we should clear the session logger.Printf("Error loading cookied session: %v, removing session", err) - s.store.Clear(rw, req) + err = s.store.Clear(rw, req) + if err != nil { + logger.Printf("Error removing session: %v", err) + } } // Add the session to the scope if it was found diff --git a/pkg/upstream/http.go b/pkg/upstream/http.go index 5011b775..e18d0c77 100644 --- a/pkg/upstream/http.go +++ b/pkg/upstream/http.go @@ -103,6 +103,7 @@ func newReverseProxy(target *url.URL, upstream options.Upstream, errorHandler Pr proxy.FlushInterval = 1 * time.Second } + /* #nosec G402 */ if upstream.InsecureSkipTLSVerify { proxy.Transport = &http.Transport{ TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, @@ -156,6 +157,7 @@ func newWebSocketReverseProxy(u *url.URL, skipTLSVerify bool) http.Handler { wsURL := &url.URL{Scheme: wsScheme, Host: u.Host} wsProxy := wsutil.NewSingleHostReverseProxy(wsURL) + /* #nosec G402 */ if skipTLSVerify { wsProxy.TLSClientConfig = &tls.Config{InsecureSkipVerify: true} } diff --git a/pkg/upstream/proxy.go b/pkg/upstream/proxy.go index 6c7c581b..403b9b91 100644 --- a/pkg/upstream/proxy.go +++ b/pkg/upstream/proxy.go @@ -85,6 +85,9 @@ func NewProxyErrorHandler(errorTemplate *template.Template, proxyPrefix string) Message: "Error proxying to upstream server", ProxyPrefix: proxyPrefix, } - errorTemplate.Execute(rw, data) + err := errorTemplate.Execute(rw, data) + if err != nil { + http.Error(rw, "Internal Server Error", http.StatusInternalServerError) + } } } diff --git a/pkg/util/util.go b/pkg/util/util.go index e0a3fd3b..0f68ca70 100644 --- a/pkg/util/util.go +++ b/pkg/util/util.go @@ -12,7 +12,7 @@ func GetCertPool(paths []string) (*x509.CertPool, error) { } pool := x509.NewCertPool() for _, path := range paths { - data, err := ioutil.ReadFile(path) + data, err := ioutil.ReadFile(path) // #nosec G304 if err != nil { return nil, fmt.Errorf("certificate authority file (%s) could not be read - %s", path, err) } diff --git a/pkg/validation/logging.go b/pkg/validation/logging.go index 2c4754aa..83b965ce 100644 --- a/pkg/validation/logging.go +++ b/pkg/validation/logging.go @@ -13,13 +13,16 @@ func configureLogger(o options.Logging, msgs []string) []string { // Setup the log file if len(o.File.Filename) > 0 { // Validate that the file/dir can be written - file, err := os.OpenFile(o.File.Filename, os.O_WRONLY|os.O_CREATE, 0666) + file, err := os.OpenFile(o.File.Filename, os.O_WRONLY|os.O_CREATE, 0600) if err != nil { if os.IsPermission(err) { return append(msgs, "unable to write to log file: "+o.File.Filename) } } - file.Close() + err = file.Close() + if err != nil { + return append(msgs, "error closing the log file: "+o.File.Filename) + } logger.Printf("Redirecting logging to file: %s", o.File.Filename) diff --git a/pkg/validation/options.go b/pkg/validation/options.go index 21601d5a..741c81df 100644 --- a/pkg/validation/options.go +++ b/pkg/validation/options.go @@ -30,6 +30,7 @@ func Validate(o *options.Options) error { msgs = append(msgs, validateSessionCookieMinimal(o)...) if o.SSLInsecureSkipVerify { + /* #nosec G402 */ insecureTransport := &http.Transport{ TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, } @@ -217,7 +218,10 @@ func Validate(o *options.Options) error { } if len(o.TrustedIPs) > 0 && o.ReverseProxy { - fmt.Fprintln(os.Stderr, "WARNING: trusting of IPs with --reverse-proxy poses risks if a header spoofing attack is possible.") + _, err := fmt.Fprintln(os.Stderr, "WARNING: trusting of IPs with --reverse-proxy poses risks if a header spoofing attack is possible.") + if err != nil { + panic(err) + } } for i, ipStr := range o.TrustedIPs { diff --git a/providers/logingov.go b/providers/logingov.go index def6043b..b1a62c58 100644 --- a/providers/logingov.go +++ b/providers/logingov.go @@ -115,10 +115,12 @@ func checkNonce(idToken string, p *LoginGovProvider) (err error) { return nil, myerr } body, myerr := ioutil.ReadAll(resp.Body) - resp.Body.Close() if myerr != nil { return nil, myerr } + if myerr = resp.Body.Close(); myerr != nil { + return nil, myerr + } var pubkeys jose.JSONWebKeySet myerr = json.Unmarshal(body, &pubkeys) diff --git a/validator.go b/validator.go index e287ef50..e45543af 100644 --- a/validator.go +++ b/validator.go @@ -3,6 +3,7 @@ package main import ( "encoding/csv" "fmt" + "io" "os" "strings" "sync/atomic" @@ -47,7 +48,12 @@ func (um *UserMap) LoadAuthenticatedEmailsFile() { if err != nil { logger.Fatalf("failed opening authenticated-emails-file=%q, %s", um.usersFile, err) } - defer r.Close() + defer func(c io.Closer) { + cerr := c.Close() + if cerr != nil { + logger.Fatalf("Error closing authenticated emails file: %s", cerr) + } + }(r) csvReader := csv.NewReader(r) csvReader.Comma = ',' csvReader.Comment = '#' diff --git a/watcher.go b/watcher.go index 7c916235..d5e02a02 100644 --- a/watcher.go +++ b/watcher.go @@ -41,7 +41,12 @@ func WatchForUpdates(filename string, done <-chan bool, action func()) { logger.Fatal("failed to create watcher for ", filename, ": ", err) } go func() { - defer watcher.Close() + defer func(w *fsnotify.Watcher) { + cerr := w.Close() + if cerr != nil { + logger.Fatalf("error closing watcher: %s", err) + } + }(watcher) for { select { case <-done: @@ -55,7 +60,10 @@ func WatchForUpdates(filename string, done <-chan bool, action func()) { // can't be opened. if event.Op&(fsnotify.Remove|fsnotify.Rename|fsnotify.Chmod) != 0 { logger.Printf("watching interrupted on event: %s", event) - watcher.Remove(filename) + err = watcher.Remove(filename) + if err != nil { + logger.Printf("error removing watcher on %s: %s", filename, err) + } WaitForReplacement(filename, event.Op, watcher) } logger.Printf("reloading after event: %s", event)