From 32b2d51eb3315d97417468e3de4e8b582abd9a07 Mon Sep 17 00:00:00 2001 From: Christian Simon Date: Wed, 7 Jul 2021 14:58:44 +0100 Subject: [PATCH] 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 --- CHANGELOG.md | 2 ++ Makefile | 2 +- docs/guides/limitations.md | 2 +- pkg/tenant/resolver.go | 32 +++++++++++++++++++++++++--- pkg/tenant/resolver_test.go | 42 +++++++++++++++++++++++++++++++++++++ 5 files changed, 75 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 10fbcb96de..3949a50ff8 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/Makefile b/Makefile index 779b5ae476..41930da805 100644 --- a/Makefile +++ b/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,\ diff --git a/docs/guides/limitations.md b/docs/guides/limitations.md index cd2caefabc..cd72e061a6 100644 --- a/docs/guides/limitations.md +++ b/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 (`(`) diff --git a/pkg/tenant/resolver.go b/pkg/tenant/resolver.go index e5fbea2529..72517b082f 100644 --- a/pkg/tenant/resolver.go +++ b/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 diff --git a/pkg/tenant/resolver_test.go b/pkg/tenant/resolver_test.go index 69559263b4..4d2da24160 100644 --- a/pkg/tenant/resolver_test.go +++ b/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)) }