Restrict path segments in TenantIDs (#4375)

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

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

Author: simonswine

CHANGELOG.md

@@ -6,6 +6,8 @@
 * [CHANGE] Update Go version to 1.16.6. #4362
 * [CHANGE] Querier / ruler: Change `-querier.max-fetched-chunks-per-query` configuration to limit to maximum number of chunks that can be fetched in a single query. The number of chunks fetched by ingesters AND long-term storare combined should not exceed the value configured on `-querier.max-fetched-chunks-per-query`. #4260
 * [CHANGE] Memberlist: the `memberlist_kv_store_value_bytes` has been removed due to values no longer being stored in-memory as encoded bytes. #4345
+* [CHANGE] Prevent path traversal attack from users able to control the HTTP header `X-Scope-OrgID`. #4375 (CVE-2021-36157)
+  * Users only have control of the HTTP header when Cortex is not frontend by an auth proxy validating the tenant IDs
 * [ENHANCEMENT] Add timeout for waiting on compactor to become ACTIVE in the ring. #4262
 * [ENHANCEMENT] Reduce memory used by streaming queries, particularly in ruler. #4341
 * [ENHANCEMENT] Ring: allow experimental configuration of disabling of heartbeat timeouts by setting the relevant configuration value to zero. Applies to the following: #4342

Makefile

@@ -176,7 +176,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))
 	}