From 82d639411d9b236eb3cbeed8d4e87854b94e3e9a Mon Sep 17 00:00:00 2001 From: Aliaksandr Valialkin Date: Tue, 25 Jun 2024 01:34:46 +0200 Subject: [PATCH] lib/httpserver: revert 9b7e532172eea7ea516bd0323fa73db5d473876d Reason for revert: this commit doesn't resolve real security issues, while it complicates the resulting code in subtle ways (aka security circus). Comparison of two strings (passwords, auth keys) takes a few nanoseconds. This comparison is performed in non-trivial http handler, which takes thousands of nanoseconds, and the request handler timing is non-deterministic because of Go runtime, Go GC and other concurrently executed goroutines. The request handler timing is even more non-deterministic when the application is executed in shared environments such as Kubernetes, where many other applications may run on the same host and use shared resources of this host (CPU, RAM bandwidth, network bandwidth). Additionally, it is expected that the passwords and auth keys are passed via TLS-encrypted connections. Establishing TLS connections takes additional non-trivial time (millions of nanoseconds), which depends on many factors such as network latency, network congestion, etc. This makes impossible to conduct timing attack on passwords and auth keys in VictoriaMetrics components. Updates https://github.com/VictoriaMetrics/VictoriaMetrics/pull/6423/files Updates https://github.com/VictoriaMetrics/VictoriaMetrics/issues/6392 --- docs/CHANGELOG.md | 1 - lib/httpserver/httpserver.go | 18 ++---------------- 2 files changed, 2 insertions(+), 17 deletions(-) diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index c7563c067..09009614f 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -37,7 +37,6 @@ Released at 2024-06-24 **Update note 2: `*.passwordFile` and similar flags are no longer trimming trailing whitespaces at the end of content. Make sure to update the templating of password files or HTTP endpoints to not include trailing whitespaces before the upgrade. See [this](https://github.com/VictoriaMetrics/VictoriaMetrics/pull/6503) PR for the details.** -* FEATURE: all VictoriaMetrics components: use constant-time comparison for comparing HTTP basic auth credentials and auth keys. This should prevent timing attacks when comparing these credentials. See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/6392) for details. Thanks to @wasim-nihal for [the pull request](https://github.com/VictoriaMetrics/VictoriaMetrics/pull/6423). * FEATURE: [alerts-vmagent](https://github.com/VictoriaMetrics/VictoriaMetrics/blob/master/deployment/docker/alerts-vmagent.yml): add new alerting rules `StreamAggrFlushTimeout` and `StreamAggrDedupFlushTimeout` to notify about issues during stream aggregation. * FEATURE: [dashboards/vmagent](https://grafana.com/grafana/dashboards/12683): add row `Streaming aggregation` with panels related to [streaming aggregation](https://docs.victoriametrics.com/stream-aggregation/) process. * FEATURE: [vmauth](https://docs.victoriametrics.com/vmauth/): add `idleConnTimeout` flag set to 50s by default. It should reduce the probability of `broken pipe` or `connection reset by peer` errors in vmauth logs. diff --git a/lib/httpserver/httpserver.go b/lib/httpserver/httpserver.go index 40573003a..4c9c768e2 100644 --- a/lib/httpserver/httpserver.go +++ b/lib/httpserver/httpserver.go @@ -2,7 +2,6 @@ package httpserver import ( "context" - "crypto/subtle" "crypto/tls" _ "embed" "errors" @@ -443,7 +442,7 @@ func CheckAuthFlag(w http.ResponseWriter, r *http.Request, flagValue string, fla if flagValue == "" { return CheckBasicAuth(w, r) } - if !constantTimeEqual(r.FormValue("authKey"), flagValue) { + if r.FormValue("authKey") != flagValue { authKeyRequestErrors.Inc() http.Error(w, fmt.Sprintf("The provided authKey doesn't match -%s", flagName), http.StatusUnauthorized) return false @@ -460,7 +459,7 @@ func CheckBasicAuth(w http.ResponseWriter, r *http.Request) bool { } username, password, ok := r.BasicAuth() if ok { - if constantTimeEqual(username, *httpAuthUsername) && constantTimeEqual(password, httpAuthPassword.Get()) { + if username == *httpAuthUsername && password == httpAuthPassword.Get() { return true } authBasicRequestErrors.Inc() @@ -713,16 +712,3 @@ func LogError(req *http.Request, errStr string) { remoteAddr := GetQuotedRemoteAddr(req) logger.Errorf("uri: %s, remote address: %q: %s", uri, remoteAddr, errStr) } - -// constantTimeEqual compares two strings in constant-time. -// -// It returns true if they are equal, else it returns false. -func constantTimeEqual(s1, s2 string) bool { - a := []byte(s1) - b := []byte(s2) - // check length explicitly because ConstantTimeCompare doesn't spend time on comparing length - if subtle.ConstantTimeEq(int32(len(a)), int32(len(b))) == 0 { - return false - } - return subtle.ConstantTimeCompare(a, b) == 1 -}