From 9b7e532172eea7ea516bd0323fa73db5d473876d Mon Sep 17 00:00:00 2001 From: Nihal <38865967+wasim-nihal@users.noreply.github.com> Date: Wed, 19 Jun 2024 13:06:56 +0530 Subject: [PATCH] victoria-metrics: constant-time comparison of credentials like authkeys and basic auth credentials (#6423) Changes for constant-time comparison of credentials like authkeys and basic auth credentials. See: https://github.com/VictoriaMetrics/VictoriaMetrics/issues/6392 --------- Signed-off-by: Syed Nihal --- docs/CHANGELOG.md | 1 + lib/httpserver/httpserver.go | 18 ++++++++++++++++-- 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 3a6fd16444..1d5408e12b 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -32,6 +32,7 @@ See also [LTS releases](https://docs.victoriametrics.com/lts-releases/). **Update note 1: the `--vm-disable-progress-bar` command-line flag at `vmctl` was deprecated. Use `--disable-progress-bar` instead.** +* 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 4c9c768e25..40573003a4 100644 --- a/lib/httpserver/httpserver.go +++ b/lib/httpserver/httpserver.go @@ -2,6 +2,7 @@ package httpserver import ( "context" + "crypto/subtle" "crypto/tls" _ "embed" "errors" @@ -442,7 +443,7 @@ func CheckAuthFlag(w http.ResponseWriter, r *http.Request, flagValue string, fla if flagValue == "" { return CheckBasicAuth(w, r) } - if r.FormValue("authKey") != flagValue { + if !constantTimeEqual(r.FormValue("authKey"), flagValue) { authKeyRequestErrors.Inc() http.Error(w, fmt.Sprintf("The provided authKey doesn't match -%s", flagName), http.StatusUnauthorized) return false @@ -459,7 +460,7 @@ func CheckBasicAuth(w http.ResponseWriter, r *http.Request) bool { } username, password, ok := r.BasicAuth() if ok { - if username == *httpAuthUsername && password == httpAuthPassword.Get() { + if constantTimeEqual(username, *httpAuthUsername) && constantTimeEqual(password, httpAuthPassword.Get()) { return true } authBasicRequestErrors.Inc() @@ -712,3 +713,16 @@ 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 +}