Address gosec findings

Mostly handling unhandled errors appropriately.
If logging to STDERR fails, we panic. Added #nosec
comments to findings we are OK with.

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
 	}

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)

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

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

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}
 	}

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)
+		}
 	}
 }

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)
 		}

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)
 

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 {