From a5439c43cdc39e417e186b1b6680b31ce3872932 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Thu, 11 Aug 2022 17:11:04 -0700 Subject: [PATCH 1/5] Remove ciphersuites without Lucky13 countermeasures SHA-256 variants of the CBC ciphersuites don't implement any Lucky13 countermeasures. See http://www.isg.rhul.ac.uk/tls/Lucky13.html and https://www.imperialviolet.org/2013/02/04/luckythirteen.html. --- authority/config/tls_options.go | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/authority/config/tls_options.go b/authority/config/tls_options.go index 0db202e5..0c8ef198 100644 --- a/authority/config/tls_options.go +++ b/authority/config/tls_options.go @@ -22,18 +22,19 @@ var ( } // ApprovedTLSCipherSuites smallstep approved ciphersuites. ApprovedTLSCipherSuites = CipherSuites{ - "TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA", - "TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256", + // AEADs w/ ECDHE "TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256", - "TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA", - "TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384", - "TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256", - "TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA", - "TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256", "TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256", - "TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA", + "TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384", "TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384", + "TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256", "TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256", + + // CBC w/ ECDHE + "TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA", + "TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA", + "TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA", + "TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA", } // DefaultTLSOptions represents the default TLS version as well as the cipher // suites used in the TLS certificates. @@ -146,8 +147,8 @@ var cipherSuites = map[string]uint16{ "TLS_CHACHA20_POLY1305_SHA256": tls.TLS_CHACHA20_POLY1305_SHA256, // Legacy names. - "TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305": tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305, - "TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305": tls.TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305, + "TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305": tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256, + "TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305": tls.TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256, } // TLSOptions represents the TLS options that can be specified on *tls.Config From b62f4d1000219b1327bac0b74456d0d5c05607b7 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Thu, 11 Aug 2022 17:32:57 -0700 Subject: [PATCH 2/5] Add lgtm comments on some security warnings --- acme/client.go | 2 +- authority/config/tls_options.go | 12 ++++++------ authority/linkedca.go | 2 +- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/acme/client.go b/acme/client.go index 31f4c975..aaf4c85e 100644 --- a/acme/client.go +++ b/acme/client.go @@ -56,7 +56,7 @@ func NewClient() Client { Timeout: 30 * time.Second, Transport: &http.Transport{ TLSClientConfig: &tls.Config{ - InsecureSkipVerify: true, + InsecureSkipVerify: true, // lgtm[go/disabled-certificate-check] }, }, }, diff --git a/authority/config/tls_options.go b/authority/config/tls_options.go index 0c8ef198..2d6de084 100644 --- a/authority/config/tls_options.go +++ b/authority/config/tls_options.go @@ -118,22 +118,22 @@ func (c CipherSuites) Value() []uint16 { // cipherSuites has the list of supported cipher suites. var cipherSuites = map[string]uint16{ // TLS 1.0 - 1.2 cipher suites. - "TLS_RSA_WITH_RC4_128_SHA": tls.TLS_RSA_WITH_RC4_128_SHA, + "TLS_RSA_WITH_RC4_128_SHA": tls.TLS_RSA_WITH_RC4_128_SHA, // lgtm[go/insecure-tls] "TLS_RSA_WITH_3DES_EDE_CBC_SHA": tls.TLS_RSA_WITH_3DES_EDE_CBC_SHA, "TLS_RSA_WITH_AES_128_CBC_SHA": tls.TLS_RSA_WITH_AES_128_CBC_SHA, "TLS_RSA_WITH_AES_256_CBC_SHA": tls.TLS_RSA_WITH_AES_256_CBC_SHA, - "TLS_RSA_WITH_AES_128_CBC_SHA256": tls.TLS_RSA_WITH_AES_128_CBC_SHA256, + "TLS_RSA_WITH_AES_128_CBC_SHA256": tls.TLS_RSA_WITH_AES_128_CBC_SHA256, // lgtm[go/insecure-tls] "TLS_RSA_WITH_AES_128_GCM_SHA256": tls.TLS_RSA_WITH_AES_128_GCM_SHA256, "TLS_RSA_WITH_AES_256_GCM_SHA384": tls.TLS_RSA_WITH_AES_256_GCM_SHA384, - "TLS_ECDHE_ECDSA_WITH_RC4_128_SHA": tls.TLS_ECDHE_ECDSA_WITH_RC4_128_SHA, + "TLS_ECDHE_ECDSA_WITH_RC4_128_SHA": tls.TLS_ECDHE_ECDSA_WITH_RC4_128_SHA, // lgtm[go/insecure-tls] "TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA": tls.TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA, "TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA": tls.TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA, - "TLS_ECDHE_RSA_WITH_RC4_128_SHA": tls.TLS_ECDHE_RSA_WITH_RC4_128_SHA, + "TLS_ECDHE_RSA_WITH_RC4_128_SHA": tls.TLS_ECDHE_RSA_WITH_RC4_128_SHA, // lgtm[go/insecure-tls] "TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA": tls.TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA, "TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA": tls.TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA, "TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA": tls.TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA, - "TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256": tls.TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256, - "TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256": tls.TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256, + "TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256": tls.TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256, // lgtm[go/insecure-tls] + "TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256": tls.TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256, // lgtm[go/insecure-tls] "TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256": tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, "TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256": tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, "TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384": tls.TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384, diff --git a/authority/linkedca.go b/authority/linkedca.go index 01a3630d..5829f341 100644 --- a/authority/linkedca.go +++ b/authority/linkedca.go @@ -461,7 +461,7 @@ func getRootCertificate(endpoint, fingerprint string) (*x509.Certificate, error) defer cancel() conn, err := grpc.DialContext(ctx, endpoint, grpc.WithTransportCredentials(credentials.NewTLS(&tls.Config{ - InsecureSkipVerify: true, + InsecureSkipVerify: true, // lgtm[go/disabled-certificate-check] }))) if err != nil { return nil, errors.Wrapf(err, "error connecting %s", endpoint) From 90d2785776a95aa8b0538ee263a0cbc76cc9f805 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Thu, 11 Aug 2022 17:44:31 -0700 Subject: [PATCH 3/5] Sanitize log entries in logging package --- logging/handler.go | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/logging/handler.go b/logging/handler.go index dd969f89..2669027f 100644 --- a/logging/handler.go +++ b/logging/handler.go @@ -5,6 +5,7 @@ import ( "net/http" "os" "strconv" + "strings" "time" "github.com/sirupsen/logrus" @@ -78,7 +79,7 @@ func (l *LoggerHandler) writeEntry(w ResponseLogger, r *http.Request, t time.Tim uri = r.Host } if uri == "" { - uri = r.URL.RequestURI() + uri = sanitizeLogEntry(r.URL.RequestURI()) } status := w.StatusCode() @@ -96,8 +97,8 @@ func (l *LoggerHandler) writeEntry(w ResponseLogger, r *http.Request, t time.Tim "protocol": r.Proto, "status": status, "size": w.Size(), - "referer": r.Referer(), - "user-agent": r.UserAgent(), + "referer": sanitizeLogEntry(r.Referer()), + "user-agent": sanitizeLogEntry(r.UserAgent()), } for k, v := range w.Fields() { @@ -117,3 +118,8 @@ func (l *LoggerHandler) writeEntry(w ResponseLogger, r *http.Request, t time.Tim l.logger.WithFields(fields).Error() } } + +func sanitizeLogEntry(s string) string { + escaped := strings.Replace(s, "\n", "", -1) + return strings.Replace(escaped, "\r", "", -1) +} From 759aa26a57b0e190c25e8263ee885376254a774b Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Thu, 11 Aug 2022 17:47:58 -0700 Subject: [PATCH 4/5] Fix linter warning --- logging/handler.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/logging/handler.go b/logging/handler.go index 2669027f..a8b77d60 100644 --- a/logging/handler.go +++ b/logging/handler.go @@ -120,6 +120,6 @@ func (l *LoggerHandler) writeEntry(w ResponseLogger, r *http.Request, t time.Tim } func sanitizeLogEntry(s string) string { - escaped := strings.Replace(s, "\n", "", -1) - return strings.Replace(escaped, "\r", "", -1) + escaped := strings.ReplaceAll(s, "\n", "") + return strings.ReplaceAll(escaped, "\r", "") } From 2db15e4eb5f0d945c1cf8bea3618243bc100614f Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Thu, 11 Aug 2022 18:14:36 -0700 Subject: [PATCH 5/5] Remove unnecessary log entries These log entries add CodeQL warnings and are not necessary because our default http.ResponseWriter allows adding log entries. --- api/log/log.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/api/log/log.go b/api/log/log.go index cb31410b..e5c8c45a 100644 --- a/api/log/log.go +++ b/api/log/log.go @@ -3,7 +3,6 @@ package log import ( "fmt" - "log" "net/http" "os" @@ -28,8 +27,6 @@ type StackTracedError interface { func Error(rw http.ResponseWriter, err error) { rl, ok := rw.(logging.ResponseLogger) if !ok { - log.Println(err) - return } @@ -72,8 +69,6 @@ func EnabledResponse(rw http.ResponseWriter, v interface{}) { rl.WithFields(map[string]interface{}{ "response": out, }) - } else { - log.Println(out) } } }