From ad3208a878f23951dbf9457bd92665a7a79e7aa8 Mon Sep 17 00:00:00 2001 From: Christian Simon Date: Wed, 21 Jul 2021 15:04:45 +0200 Subject: [PATCH] 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 --- CHANGELOG.md | 5 +++++ Makefile | 2 +- docs/guides/limitations.md | 2 +- pkg/tenant/resolver.go | 32 +++++++++++++++++++++++++--- pkg/tenant/resolver_test.go | 42 +++++++++++++++++++++++++++++++++++++ 5 files changed, 78 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 568ae88685..0ca5952ea1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,11 @@ ## master / unreleased +## 1.10.0-rc.1 / 2021-07-21 + +* [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 + ## 1.10.0-rc.0 / 2021-06-28 * [CHANGE] Enable strict JSON unmarshal for `pkg/util/validation.Limits` struct. The custom `UnmarshalJSON()` will now fail if the input has unknown fields. #4298 diff --git a/Makefile b/Makefile index 0cac289efd..c3dc9aa661 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)) }