mediaapi/writers: Add validation and error handling to getPathFromMediaMetadata

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)

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 {
+// getPathFromMediaMetadata validates and constructs the on-disk path to the media
-	return path.Join(
+// 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]),
+		subDir,
-		string(m.MediaID[3:]),
+		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

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")),