From d9db155394f25b9dcfa3fc876794f6d73ba657a2 Mon Sep 17 00:00:00 2001 From: Artur Wierzbicki Date: Mon, 25 Jul 2022 11:30:20 +0400 Subject: [PATCH] Storage: Mime type detection (#52512) * Storage: implement mime type detection * lint --- pkg/services/store/go-is-svg/svg.go | 62 +++++++++++++++++++++ pkg/services/store/http.go | 1 - pkg/services/store/sanitize.go | 10 +++- pkg/services/store/service.go | 17 +++--- pkg/services/store/service_test.go | 77 ++++++++++++++++++++++---- pkg/services/store/testdata/image.jpg | Bin 0 -> 134 bytes pkg/services/store/testdata/image.svg | 1 + pkg/services/store/testdata/page.html | 9 +++ pkg/services/store/validate.go | 21 +++++-- 9 files changed, 169 insertions(+), 29 deletions(-) create mode 100644 pkg/services/store/go-is-svg/svg.go create mode 100644 pkg/services/store/testdata/image.jpg create mode 100644 pkg/services/store/testdata/image.svg create mode 100644 pkg/services/store/testdata/page.html diff --git a/pkg/services/store/go-is-svg/svg.go b/pkg/services/store/go-is-svg/svg.go new file mode 100644 index 00000000000..8dd980486aa --- /dev/null +++ b/pkg/services/store/go-is-svg/svg.go @@ -0,0 +1,62 @@ +// The MIT License +// +//Copyright (c) 2016 Tomas Aparicio +// +//Permission is hereby granted, free of charge, to any person +//obtaining a copy of this software and associated documentation +//files (the "Software"), to deal in the Software without +//restriction, including without limitation the rights to use, +//copy, modify, merge, publish, distribute, sublicense, and/or sell +//copies of the Software, and to permit persons to whom the +//Software is furnished to do so, subject to the following +//conditions: +// +//The above copyright notice and this permission notice shall be +//included in all copies or substantial portions of the Software. +// +//THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, +//EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES +//OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND +//NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT +//HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, +//WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING +//FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR +//OTHER DEALINGS IN THE SOFTWARE. + +package issvg + +import ( + "regexp" + "unicode/utf8" +) + +var ( + // nolint:gosimple + htmlCommentRegex = regexp.MustCompile("(?i)") + svgRegex = regexp.MustCompile(`(?i)^\s*(?:<\?xml[^>]*>\s*)?(?:]*>\s*)?]*>[^*]*<\/svg>\s*$`) +) + +// isBinary checks if the given buffer is a binary file. +func isBinary(buf []byte) bool { + if len(buf) < 24 { + return false + } + for i := 0; i < 24; i++ { + charCode, _ := utf8.DecodeRuneInString(string(buf[i])) + if charCode == 65533 || charCode <= 8 { + return true + } + } + return false +} + +// Is returns true if the given buffer is a valid SVG image. +func Is(buf []byte) bool { + return !isBinary(buf) && svgRegex.Match(htmlCommentRegex.ReplaceAll(buf, []byte{})) +} + +// IsSVG returns true if the given buffer is a valid SVG image. +// Alias to: Is() +func IsSVG(buf []byte) bool { + return Is(buf) +} diff --git a/pkg/services/store/http.go b/pkg/services/store/http.go index 49ea3199481..f8e27fd112a 100644 --- a/pkg/services/store/http.go +++ b/pkg/services/store/http.go @@ -129,7 +129,6 @@ func (s *httpStorage) Upload(c *models.ReqContext) response.Response { err = s.store.Upload(c.Req.Context(), c.SignedInUser, &UploadRequest{ Contents: data, - MimeType: mimeType, EntityType: entityType, Path: path, OverwriteExistingFile: overwriteExistingFile, diff --git a/pkg/services/store/sanitize.go b/pkg/services/store/sanitize.go index df19e9ceaee..8efaaf8660e 100644 --- a/pkg/services/store/sanitize.go +++ b/pkg/services/store/sanitize.go @@ -2,6 +2,7 @@ package store import ( "context" + "mime" "path/filepath" "github.com/grafana/grafana/pkg/infra/filestorage" @@ -41,10 +42,17 @@ func (s *standardStorageService) sanitizeUploadRequest(ctx context.Context, user return nil, err } + // we have already validated that the file contents match the extension in `./validate.go` + mimeType := mime.TypeByExtension(filepath.Ext(req.Path)) + if mimeType == "" { + grafanaStorageLogger.Info("failed to find mime type", "path", req.Path) + mimeType = "application/octet-stream" + } + return &filestorage.UpsertFileCommand{ Path: storagePath, Contents: contents, - MimeType: req.MimeType, + MimeType: mimeType, CacheControl: req.CacheControl, ContentDisposition: req.ContentDisposition, Properties: req.Properties, diff --git a/pkg/services/store/service.go b/pkg/services/store/service.go index dbbf505d1b8..b4da1f2decc 100644 --- a/pkg/services/store/service.go +++ b/pkg/services/store/service.go @@ -194,7 +194,9 @@ func ProvideService(sql *sqlstore.SQLStore, features featuremgmt.FeatureToggles, } }) - return newStandardStorageService(sql, globalRoots, initializeOrgStorages, authService) + return newStandardStorageService(sql, globalRoots, initializeOrgStorages, authService, storageServiceConfig{ + allowUnsanitizedSvgUpload: false, + }) } func createSystemBrandingPathFilter() filestorage.PathFilter { @@ -205,7 +207,7 @@ func createSystemBrandingPathFilter() filestorage.PathFilter { nil) } -func newStandardStorageService(sql *sqlstore.SQLStore, globalRoots []storageRuntime, initializeOrgStorages func(orgId int64) []storageRuntime, authService storageAuthService) *standardStorageService { +func newStandardStorageService(sql *sqlstore.SQLStore, globalRoots []storageRuntime, initializeOrgStorages func(orgId int64) []storageRuntime, authService storageAuthService, cfg storageServiceConfig) *standardStorageService { rootsByOrgId := make(map[int64][]storageRuntime) rootsByOrgId[ac.GlobalOrgID] = globalRoots @@ -218,9 +220,7 @@ func newStandardStorageService(sql *sqlstore.SQLStore, globalRoots []storageRunt sql: sql, tree: res, authService: authService, - cfg: storageServiceConfig{ - allowUnsanitizedSvgUpload: false, - }, + cfg: cfg, } } @@ -252,7 +252,6 @@ func (s *standardStorageService) Read(ctx context.Context, user *models.SignedIn type UploadRequest struct { Contents []byte - MimeType string // TODO: remove MimeType from the struct once we can infer it from file contents Path string CacheControl string ContentDisposition string @@ -279,17 +278,17 @@ func (s *standardStorageService) Upload(ctx context.Context, user *models.Signed 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) + grafanaStorageLogger.Warn("file upload validation failed", "path", req.Path, "reason", validationResult.reason) return ErrValidationFailed } 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) + grafanaStorageLogger.Error("failed while sanitizing the upload request", "path", req.Path, "error", err) return ErrUploadInternalError } - grafanaStorageLogger.Info("uploading a file", "filetype", req.MimeType, "path", req.Path) + grafanaStorageLogger.Info("uploading a file", "path", req.Path) if !req.OverwriteExistingFile { file, err := root.Store().Get(ctx, storagePath) diff --git a/pkg/services/store/service_test.go b/pkg/services/store/service_test.go index 916ae84e430..0c99165973f 100644 --- a/pkg/services/store/service_test.go +++ b/pkg/services/store/service_test.go @@ -3,6 +3,7 @@ package store import ( "bytes" "context" + "io/ioutil" "path/filepath" "testing" @@ -16,6 +17,9 @@ import ( ) var ( + htmlBytes, _ = ioutil.ReadFile("testdata/page.html") + jpgBytes, _ = ioutil.ReadFile("testdata/image.jpg") + svgBytes, _ = ioutil.ReadFile("testdata/image.svg") dummyUser = &models.SignedInUser{OrgId: 1} allowAllAuthService = newStaticStorageAuthService(func(ctx context.Context, user *models.SignedInUser, storageName string) map[string]filestorage.PathFilter { return map[string]filestorage.PathFilter{ @@ -53,7 +57,7 @@ func TestListFiles(t *testing.T) { store := newStandardStorageService(sqlstore.InitTestDB(t), roots, func(orgId int64) []storageRuntime { return make([]storageRuntime, 0) - }, allowAllAuthService) + }, allowAllAuthService, storageServiceConfig{}) frame, err := store.List(context.Background(), dummyUser, "public/testdata") require.NoError(t, err) @@ -73,7 +77,7 @@ func TestListFilesWithoutPermissions(t *testing.T) { store := newStandardStorageService(sqlstore.InitTestDB(t), roots, func(orgId int64) []storageRuntime { return make([]storageRuntime, 0) - }, denyAllAuthService) + }, denyAllAuthService, storageServiceConfig{}) frame, err := store.List(context.Background(), dummyUser, "public/testdata") require.NoError(t, err) rowLen, err := frame.RowLen() @@ -98,7 +102,7 @@ func setupUploadStore(t *testing.T, authService storageAuthService) (StorageServ } store := newStandardStorageService(sqlstore.InitTestDB(t), []storageRuntime{sqlStorage}, func(orgId int64) []storageRuntime { return make([]storageRuntime, 0) - }, authService) + }, authService, storageServiceConfig{allowUnsanitizedSvgUpload: true}) return store, mockStorage, storageName } @@ -106,14 +110,18 @@ func setupUploadStore(t *testing.T, authService storageAuthService) (StorageServ func TestShouldUploadWhenNoFileAlreadyExists(t *testing.T) { service, mockStorage, storageName := setupUploadStore(t, nil) - mockStorage.On("Get", mock.Anything, "/myFile.jpg").Return(nil, nil) - mockStorage.On("Upsert", mock.Anything, mock.Anything).Return(nil) + fileName := "/myFile.jpg" + mockStorage.On("Get", mock.Anything, fileName).Return(nil, nil) + mockStorage.On("Upsert", mock.Anything, &filestorage.UpsertFileCommand{ + Path: fileName, + MimeType: "image/jpeg", + Contents: jpgBytes, + }).Return(nil) err := service.Upload(context.Background(), dummyUser, &UploadRequest{ EntityType: EntityTypeImage, - Contents: make([]byte, 0), - Path: storageName + "/myFile.jpg", - MimeType: "image/jpg", + Contents: jpgBytes, + Path: storageName + fileName, }) require.NoError(t, err) } @@ -123,9 +131,8 @@ func TestShouldFailUploadWithoutAccess(t *testing.T) { err := service.Upload(context.Background(), dummyUser, &UploadRequest{ EntityType: EntityTypeImage, - Contents: make([]byte, 0), + Contents: jpgBytes, Path: storageName + "/myFile.jpg", - MimeType: "image/jpg", }) require.ErrorIs(t, err, ErrAccessDenied) } @@ -137,9 +144,8 @@ func TestShouldFailUploadWhenFileAlreadyExists(t *testing.T) { err := service.Upload(context.Background(), dummyUser, &UploadRequest{ EntityType: EntityTypeImage, - Contents: make([]byte, 0), + Contents: jpgBytes, Path: storageName + "/myFile.jpg", - MimeType: "image/jpg", }) require.ErrorIs(t, err, ErrFileAlreadyExists) } @@ -173,3 +179,50 @@ func TestShouldDelegateFolderDeletion(t *testing.T) { }) require.NoError(t, err) } + +func TestShouldUploadSvg(t *testing.T) { + service, mockStorage, storageName := setupUploadStore(t, nil) + + fileName := "/myFile.svg" + mockStorage.On("Get", mock.Anything, fileName).Return(nil, nil) + mockStorage.On("Upsert", mock.Anything, &filestorage.UpsertFileCommand{ + Path: fileName, + MimeType: "image/svg+xml", + Contents: svgBytes, + }).Return(nil) + + err := service.Upload(context.Background(), dummyUser, &UploadRequest{ + EntityType: EntityTypeImage, + Contents: svgBytes, + Path: storageName + fileName, + }) + require.NoError(t, err) +} + +func TestShouldNotUploadHtmlDisguisedAsSvg(t *testing.T) { + service, mockStorage, storageName := setupUploadStore(t, nil) + + fileName := "/myFile.svg" + mockStorage.On("Get", mock.Anything, fileName).Return(nil, nil) + + err := service.Upload(context.Background(), dummyUser, &UploadRequest{ + EntityType: EntityTypeImage, + Contents: htmlBytes, + Path: storageName + fileName, + }) + require.ErrorIs(t, err, ErrValidationFailed) +} + +func TestShouldNotUploadJpgDisguisedAsSvg(t *testing.T) { + service, mockStorage, storageName := setupUploadStore(t, nil) + + fileName := "/myFile.svg" + mockStorage.On("Get", mock.Anything, fileName).Return(nil, nil) + + err := service.Upload(context.Background(), dummyUser, &UploadRequest{ + EntityType: EntityTypeImage, + Contents: jpgBytes, + Path: storageName + fileName, + }) + require.ErrorIs(t, err, ErrValidationFailed) +} diff --git a/pkg/services/store/testdata/image.jpg b/pkg/services/store/testdata/image.jpg new file mode 100644 index 0000000000000000000000000000000000000000..6878c904f713ce1d34fc1d712382be106c76ca79 GIT binary patch literal 134 zcmex= diff --git a/pkg/services/store/testdata/page.html b/pkg/services/store/testdata/page.html new file mode 100644 index 00000000000..f8fe81af5ba --- /dev/null +++ b/pkg/services/store/testdata/page.html @@ -0,0 +1,9 @@ + + + +

My First Heading

+ +

My first paragraph.

+ + + diff --git a/pkg/services/store/validate.go b/pkg/services/store/validate.go index cc0c2f011dc..2cea23394cb 100644 --- a/pkg/services/store/validate.go +++ b/pkg/services/store/validate.go @@ -3,10 +3,14 @@ package store import ( "context" "encoding/json" + "fmt" + "net/http" "path/filepath" + "strings" "github.com/grafana/grafana/pkg/infra/filestorage" "github.com/grafana/grafana/pkg/models" + issvg "github.com/grafana/grafana/pkg/services/store/go-is-svg" ) var ( @@ -24,7 +28,7 @@ var ( ".gif": {"image/gif": true}, ".png": {"image/png": true}, ".webp": {"image/webp": true}, - ".svg": {"text/xml; charset=utf-8": true, "text/plain; charset=utf-8": true, "image/svg+xml": true}, + ".svg": {"image/svg+xml": true}, } ) @@ -47,19 +51,24 @@ func fail(reason string) validationResult { } 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 + if strings.HasSuffix(uploadRequest.Path, ".svg") { + if issvg.IsSVG(uploadRequest.Contents) { + return "image/svg+xml" + } + } + + return http.DetectContentType(uploadRequest.Contents) } 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") + return fail(fmt.Sprintf("unsupported extension: %s", ext)) } mimeType := s.detectMimeType(ctx, user, uploadRequest) if !imageExtensionsToMatchingMimeTypes[ext][mimeType] { - return fail("mismatched extension and file contents") + return fail(fmt.Sprintf("extension '%s' does not match the detected MimeType: %s", ext, mimeType)) } return success() @@ -70,7 +79,7 @@ func (s *standardStorageService) validateUploadRequest(ctx context.Context, user // TODO: validateProperties if err := filestorage.ValidatePath(storagePath); err != nil { - return fail("path validation failed. error:" + err.Error() + ". path: " + storagePath) + return fail(fmt.Sprintf("path validation failed. error: %s. path: %s", err.Error(), storagePath)) } switch req.EntityType {