Restrict path segments in TenantIDs (#20)

* Restrict path segments in TenantIDs

This prevents paths generated from TenantIDs to become vulnerable to
path traversal attacks. (CVE-2021-36157)

Signed-off-by: Christian Simon <simon@swine.de>

* Update CHANGELOG.md

Signed-off-by: Marco Pracucci <marco@pracucci.com>

Co-authored-by: Marco Pracucci <marco@pracucci.com>

CHANGELOG.md

@@ -5,8 +5,11 @@
 * [CHANGE] Removed `configdb` support from Ruler and Alertmanager backend storages. #15
 * [CHANGE] Changed `-ruler.storage.type` default value from `configdb` to `local`. #15
 * [CHANGE] Changed `-alertmanager.storage.type` default value from `configdb` to `local`. #15
+* [CHANGE] Prevent path traversal attack from users able to control the HTTP header `X-Scope-OrgID`. (CVE-2021-36157) #20
+  * Users only have control of the HTTP header when Mimir is not frontend by an auth proxy validating the tenant IDs
 * [BUGFIX] Upgrade Prometheus. TSDB now waits for pending readers before truncating Head block, fixing the `chunk not found` error and preventing wrong query results. #16
 
+
 ## master / unreleased
 * [FEATURE] Ruler: Add new `-ruler.query-stats-enabled` which when enabled will report the `cortex_ruler_query_seconds_total` as a per-user metric that tracks the sum of the wall time of executing queries in the ruler in seconds. #4317
 * [FEATURE] Query Frontend: Add `cortex_query_fetched_series_total` and `cortex_query_fetched_chunks_bytes_total` per-user counters to expose the number of series and bytes fetched as part of queries. These metrics can be enabled with the `-frontend.query-stats-enabled` flag (or its respective YAML config option `query_stats_enabled`). #4343

Makefile

@@ -159,7 +159,7 @@ lint:
 	# Configured via .golangci.yml.
 	golangci-lint run
 
-	# Ensure no blacklisted package is imported.
+	# Ensure no blocklisted package is imported.
 	GOFLAGS="-tags=requires_docker" faillint -paths "github.com/bmizerany/assert=github.com/stretchr/testify/assert,\
 		golang.org/x/net/context=context,\
 		sync/atomic=go.uber.org/atomic,\

docs/guides/limitations.md

@@ -24,7 +24,7 @@ The following character sets are generally **safe for use in the tenant ID**:
   - Exclamation point (`!`)
   - Hyphen (`-`)
   - Underscore (`_`)
-  - Period (`.`)
+  - Single Period (`.`), but the tenant IDs `.` and `..` is considered invalid
   - Asterisk (`*`)
   - Single quote (`'`)
   - Open parenthesis (`(`)

pkg/tenant/resolver.go

@@ -2,6 +2,7 @@ package tenant
 
 import (
 	"context"
+	"errors"
 	"net/http"
 	"strings"
 
@@ -59,14 +60,36 @@ func NewSingleResolver() *SingleResolver {
 type SingleResolver struct {
 }
 
+// containsUnsafePathSegments will return true if the string is a directory
+// reference like `.` and `..` or if any path separator character like `/` and
+// `\` can be found.
+func containsUnsafePathSegments(id string) bool {
+	// handle the relative reference to current and parent path.
+	if id == "." || id == ".." {
+		return true
+	}
+
+	return strings.ContainsAny(id, "\\/")
+}
+
+var errInvalidTenantID = errors.New("invalid tenant ID")
+
 func (t *SingleResolver) TenantID(ctx context.Context) (string, error) {
 	//lint:ignore faillint wrapper around upstream method
-	return user.ExtractOrgID(ctx)
+	id, err := user.ExtractOrgID(ctx)
+	if err != nil {
+		return "", err
+	}
+
+	if containsUnsafePathSegments(id) {
+		return "", errInvalidTenantID
+	}
+
+	return id, nil
 }
 
 func (t *SingleResolver) TenantIDs(ctx context.Context) ([]string, error) {
-	//lint:ignore faillint wrapper around upstream method
-	orgID, err := user.ExtractOrgID(ctx)
+	orgID, err := t.TenantID(ctx)
 	if err != nil {
 		return nil, err
 	}
@@ -109,6 +132,9 @@ func (t *MultiResolver) TenantIDs(ctx context.Context) ([]string, error) {
 		if err := ValidTenantID(orgID); err != nil {
 			return nil, err
 		}
+		if containsUnsafePathSegments(orgID) {
+			return nil, errInvalidTenantID
+		}
 	}
 
 	return NormalizeTenantIDs(orgIDs), nil

pkg/tenant/resolver_test.go

@@ -64,6 +64,18 @@ var commonResolverTestCases = []resolverTestCase{
 		tenantID:    "tenant-a",
 		tenantIDs:   []string{"tenant-a"},
 	},
+	{
+		name:         "parent-dir",
+		headerValue:  strptr(".."),
+		errTenantID:  errInvalidTenantID,
+		errTenantIDs: errInvalidTenantID,
+	},
+	{
+		name:         "current-dir",
+		headerValue:  strptr("."),
+		errTenantID:  errInvalidTenantID,
+		errTenantIDs: errInvalidTenantID,
+	},
 }
 
 func TestSingleResolver(t *testing.T) {
@@ -75,6 +87,18 @@ func TestSingleResolver(t *testing.T) {
 			tenantID:    "tenant-a|tenant-b",
 			tenantIDs:   []string{"tenant-a|tenant-b"},
 		},
+		{
+			name:         "containing-forward-slash",
+			headerValue:  strptr("forward/slash"),
+			errTenantID:  errInvalidTenantID,
+			errTenantIDs: errInvalidTenantID,
+		},
+		{
+			name:         "containing-backward-slash",
+			headerValue:  strptr(`backward\slash`),
+			errTenantID:  errInvalidTenantID,
+			errTenantIDs: errInvalidTenantID,
+		},
 	}...) {
 		t.Run(tc.name, tc.test(r))
 	}
@@ -101,6 +125,24 @@ func TestMultiResolver(t *testing.T) {
 			errTenantID: user.ErrTooManyOrgIDs,
 			tenantIDs:   []string{"tenant-a", "tenant-b"},
 		},
+		{
+			name:         "multi-tenant-with-relative-path",
+			headerValue:  strptr("tenant-a|tenant-b|.."),
+			errTenantID:  errInvalidTenantID,
+			errTenantIDs: errInvalidTenantID,
+		},
+		{
+			name:         "containing-forward-slash",
+			headerValue:  strptr("forward/slash"),
+			errTenantID:  &errTenantIDUnsupportedCharacter{pos: 7, tenantID: "forward/slash"},
+			errTenantIDs: &errTenantIDUnsupportedCharacter{pos: 7, tenantID: "forward/slash"},
+		},
+		{
+			name:         "containing-backward-slash",
+			headerValue:  strptr(`backward\slash`),
+			errTenantID:  &errTenantIDUnsupportedCharacter{pos: 8, tenantID: "backward\\slash"},
+			errTenantIDs: &errTenantIDUnsupportedCharacter{pos: 8, tenantID: "backward\\slash"},
+		},
 	}...) {
 		t.Run(tc.name, tc.test(r))
 	}