From 00e8fed3a704610f5c85a0dc53ac0537783d771f Mon Sep 17 00:00:00 2001 From: Robert Swain Date: Thu, 18 May 2017 17:39:30 +0200 Subject: [PATCH] mediaapi/writers: Add validation and error handling to getPathFromMediaMetadata --- .../dendrite/mediaapi/writers/download.go | 27 ++++++++++-- .../dendrite/mediaapi/writers/fileutils.go | 41 ++++++++++++++++--- .../dendrite/mediaapi/writers/upload.go | 10 ++++- 3 files changed, 68 insertions(+), 10 deletions(-) diff --git a/src/github.com/matrix-org/dendrite/mediaapi/writers/download.go b/src/github.com/matrix-org/dendrite/mediaapi/writers/download.go index 667849220..c9c9be441 100644 --- a/src/github.com/matrix-org/dendrite/mediaapi/writers/download.go +++ b/src/github.com/matrix-org/dendrite/mediaapi/writers/download.go @@ -187,7 +187,16 @@ func (r *downloadRequest) respondFromLocalFile(w http.ResponseWriter, absBasePat "Content-Disposition": r.MediaMetadata.ContentDisposition, }).Infof("Downloading file") - filePath := getPathFromMediaMetadata(r.MediaMetadata, absBasePath) + filePath, err := getPathFromMediaMetadata(r.MediaMetadata, absBasePath) + if err != nil { + // FIXME: Remove erroneous file from database? + r.Logger.Warnln("Failed to get file path from metadata:", err) + r.jsonErrorResponse(w, util.JSONResponse{ + Code: 404, + JSON: jsonerror.NotFound(fmt.Sprintf("File with media ID %q does not exist", r.MediaMetadata.MediaID)), + }) + return + } file, err := os.Open(filePath) if err != nil { // FIXME: Remove erroneous file from database? @@ -364,9 +373,19 @@ func (r *downloadRequest) commitFileAndMetadata(tmpDir types.Path, absBasePath t }).Infof("Storing file metadata to media repository database") // The database is the source of truth so we need to have moved the file first - err := moveFile( + finalPath, err := getPathFromMediaMetadata(r.MediaMetadata, absBasePath) + if err != nil { + r.Logger.Warnf("Failed to get file path from metadata: %q\n", err) + tmpDirErr := os.RemoveAll(string(tmpDir)) + if tmpDirErr != nil { + r.Logger.Warnf("Failed to remove tmpDir (%v): %q\n", tmpDir, tmpDirErr) + } + return updateActiveRemoteRequests + } + + err = moveFile( types.Path(path.Join(string(tmpDir), "content")), - types.Path(getPathFromMediaMetadata(r.MediaMetadata, absBasePath)), + types.Path(finalPath), ) if err != nil { tmpDirErr := os.RemoveAll(string(tmpDir)) @@ -387,7 +406,7 @@ func (r *downloadRequest) commitFileAndMetadata(tmpDir types.Path, absBasePath t // if written to disk, add to db err = db.StoreMediaMetadata(r.MediaMetadata) if err != nil { - finalDir := path.Dir(getPathFromMediaMetadata(r.MediaMetadata, absBasePath)) + finalDir := path.Dir(finalPath) finalDirErr := os.RemoveAll(finalDir) if finalDirErr != nil { r.Logger.Warnf("Failed to remove finalDir (%v): %q\n", finalDir, finalDirErr) diff --git a/src/github.com/matrix-org/dendrite/mediaapi/writers/fileutils.go b/src/github.com/matrix-org/dendrite/mediaapi/writers/fileutils.go index 7045df06a..d931707b0 100644 --- a/src/github.com/matrix-org/dendrite/mediaapi/writers/fileutils.go +++ b/src/github.com/matrix-org/dendrite/mediaapi/writers/fileutils.go @@ -20,6 +20,8 @@ import ( "io/ioutil" "os" "path" + "path/filepath" + "strings" log "github.com/Sirupsen/logrus" "github.com/matrix-org/dendrite/clientapi/jsonerror" @@ -75,13 +77,42 @@ func createTempFileWriter(absBasePath types.Path, logger *log.Entry) (*bufio.Wri return writer, tmpFile, tmpDir, nil } -func getPathFromMediaMetadata(m *types.MediaMetadata, absBasePath types.Path) string { - return path.Join( +// getPathFromMediaMetadata validates and constructs the on-disk path to the media +// based on its origin and mediaID +// If a mediaID is too short, which could happen for other homeserver implementations, +// place it into a short-id subdirectory of the origin directory +// If the mediaID is long enough, we split it in two using one part as a subdirectory +// name and the other part as the file name. This is to allow storage of more files due +// to filesystem limitations on the number of files in a directory. For example, if +// mediaID is 'qwerty', we create subdirectory called 'qwe' and place the file in 'qwe' +// and call it 'rty'. +func getPathFromMediaMetadata(m *types.MediaMetadata, absBasePath types.Path) (string, error) { + var subDir string + var fileName string + + if len(m.MediaID) > 3 { + subDir = string(m.MediaID[:3]) + fileName = string(m.MediaID[3:]) + } else { + subDir = "short-id" + fileName = string(m.MediaID) + } + + filePath, err := filepath.Abs(path.Join( string(absBasePath), string(m.Origin), - string(m.MediaID[:3]), - string(m.MediaID[3:]), - ) + subDir, + fileName, + )) + + // check if the absolute absBasePath is a prefix of the absolute filePath + // if so, no directory escape has occurred and the filePath is valid + // Note: absBasePath is already absolute + if err != nil || strings.HasPrefix(filePath, string(absBasePath)) == false { + return "", fmt.Errorf("Invalid filePath (not within absBasePath %v): %v", absBasePath, filePath) + } + + return filePath, nil } // moveFile attempts to move the file src to dst diff --git a/src/github.com/matrix-org/dendrite/mediaapi/writers/upload.go b/src/github.com/matrix-org/dendrite/mediaapi/writers/upload.go index 668c1e5f9..44439c850 100644 --- a/src/github.com/matrix-org/dendrite/mediaapi/writers/upload.go +++ b/src/github.com/matrix-org/dendrite/mediaapi/writers/upload.go @@ -210,7 +210,15 @@ func Upload(req *http.Request, cfg *config.MediaAPI, db *storage.Database) util. // TODO: generate thumbnails - finalPath := getPathFromMediaMetadata(r.MediaMetadata, cfg.AbsBasePath) + finalPath, err := getPathFromMediaMetadata(r.MediaMetadata, cfg.AbsBasePath) + if err != nil { + logger.Warnf("Failed to get file path from metadata: %q\n", err) + removeDir(tmpDir, logger) + return util.JSONResponse{ + Code: 400, + JSON: jsonerror.Unknown(fmt.Sprintf("Failed to upload")), + } + } err = moveFile( types.Path(path.Join(string(tmpDir), "content")),