From 854639b2d2c2945a9d8caed191ec537f1927da33 Mon Sep 17 00:00:00 2001
From: Christian Simon <simon@swine.de>
Date: Thu, 8 Aug 2024 12:46:14 +0100
Subject: [PATCH] adhocprofile: Validate IDs and santize names properly (#3470)

We currently allow any character to be user specified at upload. This
limits that character set. It also makes sure only IDs are listend and
retrieve with get, when they are fulfilling the character limit.

This validation will ensure that the generated identifiers are withing
the allowed set use for object store.
---
 pkg/adhocprofiles/adhocprofiles.go      | 43 ++++++++++++++++++++++++-
 pkg/adhocprofiles/adhocprofiles_test.go | 37 ++++++++++++++++++---
 2 files changed, 74 insertions(+), 6 deletions(-)

diff --git a/pkg/adhocprofiles/adhocprofiles.go b/pkg/adhocprofiles/adhocprofiles.go
index d947940f36..a9d49263c5 100644
--- a/pkg/adhocprofiles/adhocprofiles.go
+++ b/pkg/adhocprofiles/adhocprofiles.go
@@ -6,6 +6,7 @@ import (
 	"crypto/rand"
 	"encoding/base64"
 	"encoding/json"
+	"fmt"
 	"io"
 	"slices"
 	"strings"
@@ -41,6 +42,33 @@ type AdHocProfile struct {
 	UploadedAt time.Time `json:"uploadedAt"`
 }
 
+func validRunes(r rune) bool {
+	if r >= 'a' && r <= 'z' || r >= 'A' && r <= 'Z' || r >= '0' && r <= '9' || r == '.' || r == '-' || r == '_' {
+		return true
+	}
+	return false
+}
+
+// check if the id is valid
+func validID(id string) bool {
+	for _, r := range id {
+		if !validRunes(r) {
+			return false
+		}
+	}
+	return true
+}
+
+// replaces invalid runes in the id with underscores
+func replaceInvalidRunes(id string) string {
+	return strings.Map(func(r rune) rune {
+		if validRunes(r) {
+			return r
+		}
+		return '_'
+	}, id)
+}
+
 func NewAdHocProfiles(bucket objstore.Bucket, logger log.Logger, limits frontend.Limits) *AdHocProfiles {
 	a := &AdHocProfiles{
 		logger: logger,
@@ -68,6 +96,9 @@ func (a *AdHocProfiles) Upload(ctx context.Context, c *connect.Request[v1.AdHocP
 		UploadedAt: time.Now().UTC(),
 	}
 
+	// replace runes outside of [a-zA-Z0-9_-.] with underscores
+	adHocProfile.Name = replaceInvalidRunes(adHocProfile.Name)
+
 	// TODO: Add per-tenant upload limits (number of files, total size, etc.)
 
 	maxNodes, err := validation.ValidateMaxNodes(a.limits, []string{tenantID}, c.Msg.GetMaxNodes())
@@ -118,7 +149,12 @@ func (a *AdHocProfiles) Get(ctx context.Context, c *connect.Request[v1.AdHocProf
 
 	bucket := a.getBucket(tenantID)
 
-	reader, err := bucket.Get(ctx, c.Msg.GetId())
+	id := c.Msg.GetId()
+	if !validID(id) {
+		return nil, connect.NewError(connect.CodeInvalidArgument, fmt.Errorf("id '%s' is invalid: can only contain [a-zA-Z0-9_-.]", id))
+	}
+
+	reader, err := bucket.Get(ctx, id)
 	if err != nil {
 		return nil, errors.Wrapf(err, "failed to get profile")
 	}
@@ -167,6 +203,11 @@ func (a *AdHocProfiles) List(ctx context.Context, c *connect.Request[v1.AdHocPro
 
 	profiles := make([]*v1.AdHocProfilesProfileMetadata, 0)
 	err = bucket.Iter(ctx, "", func(s string) error {
+		// do not list elements with invalid ids
+		if !validID(s) {
+			return nil
+		}
+
 		separatorIndex := strings.IndexRune(s, '-')
 		id, err := ulid.Parse(s[0:separatorIndex])
 		if err != nil {
diff --git a/pkg/adhocprofiles/adhocprofiles_test.go b/pkg/adhocprofiles/adhocprofiles_test.go
index d8f47a8559..3180331d9b 100644
--- a/pkg/adhocprofiles/adhocprofiles_test.go
+++ b/pkg/adhocprofiles/adhocprofiles_test.go
@@ -6,6 +6,7 @@ import (
 	"encoding/base64"
 	"encoding/json"
 	"os"
+	"strings"
 	"testing"
 
 	"connectrpc.com/connect"
@@ -125,9 +126,10 @@ func TestAdHocProfiles_Upload(t *testing.T) {
 		c   *connect.Request[v1.AdHocProfilesUploadRequest]
 	}
 	tests := []struct {
-		name    string
-		args    args
-		wantErr bool
+		name           string
+		args           args
+		wantErr        bool
+		expectedSuffix string
 	}{
 		{
 			name: "reject requests with missing tenant id",
@@ -153,11 +155,24 @@ func TestAdHocProfiles_Upload(t *testing.T) {
 			args: args{
 				ctx: tenant.InjectTenantID(context.Background(), "tenant"),
 				c: connect.NewRequest(&v1.AdHocProfilesUploadRequest{
-					Name:    "test",
+					Name:    "test.cpu.pb.gz",
 					Profile: encodedProfile,
 				}),
 			},
-			wantErr: false,
+			wantErr:        false,
+			expectedSuffix: "-test.cpu.pb.gz",
+		},
+		{
+			name: "should limit profile names to particular character set",
+			args: args{
+				ctx: tenant.InjectTenantID(context.Background(), "tenant"),
+				c: connect.NewRequest(&v1.AdHocProfilesUploadRequest{
+					Name:    "test/../../../etc/passwd",
+					Profile: encodedProfile,
+				}),
+			},
+			wantErr:        false,
+			expectedSuffix: "-test_.._.._.._etc_passwd",
 		},
 	}
 	for _, tt := range tests {
@@ -172,6 +187,18 @@ func TestAdHocProfiles_Upload(t *testing.T) {
 				t.Errorf("Upload() error = %v, wantErr %v", err, tt.wantErr)
 				return
 			}
+
+			if tt.expectedSuffix != "" {
+				found := false
+				err := bucket.Iter(tt.args.ctx, "tenant/adhoc", func(name string) error {
+					if strings.HasSuffix(name, tt.expectedSuffix) {
+						found = true
+					}
+					return nil
+				})
+				require.NoError(t, err)
+				require.True(t, found)
+			}
 		})
 	}
 }