From cc4473faf3c52b937b21874a07f78ded7ea0664b Mon Sep 17 00:00:00 2001 From: Artur Wierzbicki Date: Wed, 15 Jun 2022 12:32:29 +0400 Subject: [PATCH] Storage: validation and sanitization stubs (#50523) * add `IsPathValidationError` util to fs api * refactor storage.Upload method * remove unused struct * extract `RootUpload` constant * move file validation outside of the service * Make UploadErrorToStatusCode exported * validation/sanitization * refactor pathValidationError check * refactor, rename sanitize to transform * add a todo * refactor * transform -> sanitize * lint fix * #50608: fix jpg/jpeg Co-authored-by: Tania B Co-authored-by: Ryan McKinley --- pkg/services/store/entity_events.go | 1 + pkg/services/store/http.go | 15 ++--- pkg/services/store/sanitize.go | 28 +++++++++ pkg/services/store/service.go | 59 ++++++++----------- pkg/services/store/service_test.go | 9 +-- pkg/services/store/validate.go | 90 +++++++++++++++++++++++++++++ 6 files changed, 153 insertions(+), 49 deletions(-) create mode 100644 pkg/services/store/sanitize.go create mode 100644 pkg/services/store/validate.go diff --git a/pkg/services/store/entity_events.go b/pkg/services/store/entity_events.go index e8101ca9e23..1698b8ffc1f 100644 --- a/pkg/services/store/entity_events.go +++ b/pkg/services/store/entity_events.go @@ -25,6 +25,7 @@ type EntityType string const ( EntityTypeDashboard EntityType = "dashboard" EntityTypeFolder EntityType = "folder" + EntityTypeImage EntityType = "image" ) // CreateDatabaseEntityId creates entityId for entities stored in the existing SQL tables diff --git a/pkg/services/store/http.go b/pkg/services/store/http.go index 9d1e93869aa..413d5aa6fed 100644 --- a/pkg/services/store/http.go +++ b/pkg/services/store/http.go @@ -34,18 +34,12 @@ func ProvideHTTPService(store StorageService) HTTPStorageService { func UploadErrorToStatusCode(err error) int { switch { case errors.Is(err, ErrUploadFeatureDisabled): - return 400 - - case errors.Is(err, ErrUnsupportedFolder): - return 400 - - case errors.Is(err, ErrFileTooBig): - return 400 + return 404 - case errors.Is(err, ErrInvalidPath): + case errors.Is(err, ErrUnsupportedStorage): return 400 - case errors.Is(err, ErrInvalidFileType): + case errors.Is(err, ErrValidationFailed): return 400 case errors.Is(err, ErrFileAlreadyExists): @@ -102,9 +96,10 @@ func (s *httpStorage) Upload(c *models.ReqContext) response.Response { mimeType := http.DetectContentType(data) - err = s.store.Upload(c.Req.Context(), c.SignedInUser, UploadRequest{ + err = s.store.Upload(c.Req.Context(), c.SignedInUser, &UploadRequest{ Contents: data, MimeType: mimeType, + EntityType: EntityTypeImage, Path: path, OverwriteExistingFile: true, }) diff --git a/pkg/services/store/sanitize.go b/pkg/services/store/sanitize.go new file mode 100644 index 00000000000..6634b77b7f5 --- /dev/null +++ b/pkg/services/store/sanitize.go @@ -0,0 +1,28 @@ +package store + +import ( + "context" + "path/filepath" + + "github.com/grafana/grafana/pkg/infra/filestorage" + "github.com/grafana/grafana/pkg/models" +) + +func (s *standardStorageService) sanitizeUploadRequest(ctx context.Context, user *models.SignedInUser, req *UploadRequest, storagePath string) (*filestorage.UpsertFileCommand, error) { + if req.EntityType == EntityTypeImage { + ext := filepath.Ext(req.Path) + //nolint: staticcheck + if ext == ".svg" { + // TODO: sanitize svg + } + } + + return &filestorage.UpsertFileCommand{ + Path: storagePath, + Contents: req.Contents, + MimeType: req.MimeType, + CacheControl: req.CacheControl, + ContentDisposition: req.ContentDisposition, + Properties: req.Properties, + }, nil +} diff --git a/pkg/services/store/service.go b/pkg/services/store/service.go index 38495dcab2c..8be2bddb2a8 100644 --- a/pkg/services/store/service.go +++ b/pkg/services/store/service.go @@ -20,11 +20,9 @@ import ( var grafanaStorageLogger = log.New("grafanaStorageLogger") var ErrUploadFeatureDisabled = errors.New("upload feature is disabled") -var ErrUnsupportedFolder = errors.New("unsupported folder for uploads") -var ErrFileTooBig = errors.New("file is too big") -var ErrInvalidPath = errors.New("path is invalid") +var ErrUnsupportedStorage = errors.New("storage does not support upload operation") var ErrUploadInternalError = errors.New("upload internal error") -var ErrInvalidFileType = errors.New("invalid file type") +var ErrValidationFailed = errors.New("request validation failed") var ErrFileAlreadyExists = errors.New("file exists") const RootPublicStatic = "public-static" @@ -41,9 +39,14 @@ type StorageService interface { // Read raw file contents out of the store Read(ctx context.Context, user *models.SignedInUser, path string) (*filestorage.File, error) - Upload(ctx context.Context, user *models.SignedInUser, req UploadRequest) error + Upload(ctx context.Context, user *models.SignedInUser, req *UploadRequest) error Delete(ctx context.Context, user *models.SignedInUser, path string) error + + validateUploadRequest(ctx context.Context, user *models.SignedInUser, req *UploadRequest, storagePath string) validationResult + + // sanitizeUploadRequest sanitizes the upload request and converts it into a command accepted by the FileStorage API + sanitizeUploadRequest(ctx context.Context, user *models.SignedInUser, req *UploadRequest, storagePath string) (*filestorage.UpsertFileCommand, error) } type standardStorageService struct { @@ -117,48 +120,43 @@ func (s *standardStorageService) Read(ctx context.Context, user *models.SignedIn return s.tree.GetFile(ctx, getOrgId(user), path) } -func isFileTypeValid(filetype string) bool { - if (filetype == "image/jpeg") || (filetype == "image/jpg") || (filetype == "image/gif") || (filetype == "image/png") || (filetype == "image/webp") { - return true - } - return false -} - type UploadRequest struct { Contents []byte - MimeType string + MimeType string // TODO: remove MimeType from the struct once we can infer it from file contents Path string CacheControl string ContentDisposition string Properties map[string]string + EntityType EntityType OverwriteExistingFile bool } -func (s *standardStorageService) Upload(ctx context.Context, user *models.SignedInUser, req UploadRequest) error { +func (s *standardStorageService) Upload(ctx context.Context, user *models.SignedInUser, req *UploadRequest) error { upload, _ := s.tree.getRoot(getOrgId(user), RootUpload) if upload == nil { return ErrUploadFeatureDisabled } if !strings.HasPrefix(req.Path, RootUpload+"/") { - return ErrUnsupportedFolder - } - - validFileType := isFileTypeValid(req.MimeType) - if !validFileType { - return ErrInvalidFileType + return ErrUnsupportedStorage } - grafanaStorageLogger.Info("uploading a file", "filetype", req.MimeType, "path", req.Path) - storagePath := strings.TrimPrefix(req.Path, RootUpload) + validationResult := s.validateUploadRequest(ctx, user, req, storagePath) + if !validationResult.ok { + grafanaStorageLogger.Warn("file upload validation failed", "filetype", req.MimeType, "path", req.Path, "reason", validationResult.reason) + return ErrValidationFailed + } - if err := filestorage.ValidatePath(storagePath); err != nil { - grafanaStorageLogger.Info("uploading file failed due to invalid path", "filetype", req.MimeType, "path", req.Path, "err", err) - return ErrInvalidPath + upsertCommand, err := s.sanitizeUploadRequest(ctx, user, req, storagePath) + if err != nil { + grafanaStorageLogger.Error("failed while sanitizing the upload request", "filetype", req.MimeType, "path", req.Path, "error", err) + return ErrUploadInternalError } + grafanaStorageLogger.Info("uploading a file", "filetype", req.MimeType, "path", req.Path) + if !req.OverwriteExistingFile { file, err := upload.Get(ctx, storagePath) if err != nil { @@ -171,16 +169,7 @@ func (s *standardStorageService) Upload(ctx context.Context, user *models.Signed } } - err := upload.Upsert(ctx, &filestorage.UpsertFileCommand{ - Path: storagePath, - Contents: req.Contents, - MimeType: req.MimeType, - CacheControl: req.CacheControl, - ContentDisposition: req.ContentDisposition, - Properties: req.Properties, - }) - - if err != nil { + if err := upload.Upsert(ctx, upsertCommand); err != nil { grafanaStorageLogger.Error("failed while uploading the file", "err", err, "path", req.Path) return ErrUploadInternalError } diff --git a/pkg/services/store/service_test.go b/pkg/services/store/service_test.go index 696dbada6c7..073f135cb3b 100644 --- a/pkg/services/store/service_test.go +++ b/pkg/services/store/service_test.go @@ -61,10 +61,11 @@ func TestUpload(t *testing.T) { cfg := &setting.Cfg{AppURL: "http://localhost:3000/", DataPath: path} s := ProvideService(sqlstore.InitTestDB(t), features, cfg) request := UploadRequest{ - Contents: make([]byte, 0), - Path: "upload/myFile.jpg", - MimeType: "image/jpeg", + EntityType: EntityTypeImage, + Contents: make([]byte, 0), + Path: "upload/myFile.jpg", + MimeType: "image/jpg", } - err = s.Upload(context.Background(), dummyUser, request) + err = s.Upload(context.Background(), dummyUser, &request) require.NoError(t, err) } diff --git a/pkg/services/store/validate.go b/pkg/services/store/validate.go new file mode 100644 index 00000000000..b067e0216fd --- /dev/null +++ b/pkg/services/store/validate.go @@ -0,0 +1,90 @@ +package store + +import ( + "context" + "encoding/json" + "path/filepath" + + "github.com/grafana/grafana/pkg/infra/filestorage" + "github.com/grafana/grafana/pkg/models" +) + +var ( + allowedImageExtensions = map[string]bool{ + ".jpg": true, + ".jpeg": true, + ".gif": true, + ".png": true, + ".webp": true, + } + imageExtensionsToMatchingMimeTypes = map[string]map[string]bool{ + ".jpg": {"image/jpg": true, "image/jpeg": true}, + ".jpeg": {"image/jpg": true, "image/jpeg": true}, + ".gif": {"image/gif": true}, + ".png": {"image/png": true}, + ".webp": {"image/webp": true}, + } +) + +type validationResult struct { + ok bool + reason string +} + +func success() validationResult { + return validationResult{ + ok: true, + } +} + +func fail(reason string) validationResult { + return validationResult{ + ok: false, + reason: reason, + } +} + +func (s *standardStorageService) detectMimeType(ctx context.Context, user *models.SignedInUser, uploadRequest *UploadRequest) string { + // TODO: implement a spoofing-proof MimeType detection based on the contents + return uploadRequest.MimeType +} + +func (s *standardStorageService) validateImage(ctx context.Context, user *models.SignedInUser, uploadRequest *UploadRequest) validationResult { + ext := filepath.Ext(uploadRequest.Path) + if !allowedImageExtensions[ext] { + return fail("unsupported extension") + } + + mimeType := s.detectMimeType(ctx, user, uploadRequest) + if !imageExtensionsToMatchingMimeTypes[ext][mimeType] { + return fail("mismatched extension and file contents") + } + + return success() +} + +func (s *standardStorageService) validateUploadRequest(ctx context.Context, user *models.SignedInUser, req *UploadRequest, storagePath string) validationResult { + // TODO: validateSize + // TODO: validateProperties + + if err := filestorage.ValidatePath(storagePath); err != nil { + return fail("path validation failed: " + err.Error()) + } + + switch req.EntityType { + case EntityTypeFolder: + fallthrough + case EntityTypeDashboard: + // TODO: add proper validation + var something interface{} + if err := json.Unmarshal(req.Contents, &something); err != nil { + return fail(err.Error()) + } + + return success() + case EntityTypeImage: + return s.validateImage(ctx, user, req) + default: + return fail("unknown entity") + } +}