fix(server:files): major fixes in file storage including security one…

… - path traversal

actor-server/actor-bots/src/main/scala/im/actor/server/bot/services/StickersBotService.scala

@@ -6,7 +6,7 @@ import akka.util.ByteString
 import im.actor.bots.BotMessages._
 import im.actor.concurrent.FutureResultCats
 import im.actor.server.bot.{ ApiToBotConversions, BotServiceBase }
-import im.actor.server.file.{ FileStorageAdapter, FileStorageExtension, FileUtils }
+import im.actor.server.file.{ UnsafeFileName, FileStorageAdapter, FileStorageExtension, FileUtils }
 import im.actor.server.sticker.{ Sticker, StickerImage }
 import im.actor.server.stickers.{ StickerErrors, StickersExtension }
 
@@ -142,7 +142,7 @@ private[bot] final class StickersBotService(_system: ActorSystem) extends BotSer
   private def uploadSticker(name: String, bytes: Array[Byte], w: Int, h: Int): Future[Option[StickerImage]] =
     Try(for {
       (path, size) ← FileUtils.writeBytes(ByteString(bytes))
-      fileLocation ← fsAdapter.uploadFileF(name, path.toFile)
+      fileLocation ← fsAdapter.uploadFileF(UnsafeFileName(name), path.toFile)
     } yield Some(StickerImage(fileLocation, w, h, size))).toOption getOrElse Future.successful(None)
 
 }

actor-server/actor-core/src/main/scala/im/actor/server/file/ImageUtils.scala

@@ -118,8 +118,8 @@ object ImageUtils {
               _ ← Future.fromTry(Try(smallAimg.toImage.forWriter(smallDesc.writer).write(smallFile)))
               _ ← Future.fromTry(Try(largeAimg.toImage.forWriter(largeDesc.writer).write(largeFile)))
 
-              smallFileLocation ← fsAdapter.uploadFileF(smallDesc.name, smallFile)
-              largeFileLocation ← fsAdapter.uploadFileF(largeDesc.name, largeFile)
+              smallFileLocation ← fsAdapter.uploadFileF(UnsafeFileName(smallDesc.name), smallFile)
+              largeFileLocation ← fsAdapter.uploadFileF(UnsafeFileName(largeDesc.name), largeFile)
             } yield {
               // TODO: #perf calculate file sizes efficiently
 

actor-server/actor-enrich/src/main/scala/im/actor/server/enrich/RichMessageWorker.scala

@@ -11,7 +11,7 @@ import com.sksamuel.scrimage.nio.JpegWriter
 import im.actor.api.rpc.files.ApiFastThumb
 import im.actor.api.rpc.messaging._
 import im.actor.server.db.DbExtension
-import im.actor.server.file.{ FileStorageAdapter, FileStorageExtension, FileUtils, ImageUtils }
+import im.actor.server.file._
 import im.actor.server.pubsub.{ PeerMessage, PubSubExtension }
 import im.actor.util.log.AnyRefLogSource
 import org.joda.time.DateTime
@@ -94,7 +94,7 @@ final class RichMessageWorker(config: RichMessageConfig)(implicit materializer:
       db.run {
         for {
           (file, fileSize) ← DBIO.from(FileUtils.writeBytes(imageBytes))
-          location ← fsAdapter.uploadFile(fullName, file.toFile)
+          location ← fsAdapter.uploadFile(UnsafeFileName(fullName), file.toFile)
           thumb ← DBIO.from(ImageUtils.scaleTo(image, 90))
           thumbBytes = thumb.toImage.forWriter(JpegWriter()).bytes
 

actor-server/actor-fs-adapters/src/main/scala/im/actor/server/file/FileStorageAdapter.scala

@@ -13,17 +13,21 @@ object FileStorageAdapter {
 
 trait FileStorageAdapter extends UploadActions with DownloadActions with UploadKeyParsing
 
+final case class UnsafeFileName(fileName: String) {
+  lazy val safe: String = new File(fileName).toPath.normalize().getFileName.toString
+}
+
 private[file] trait UploadActions {
 
   def getFileUploadPartUrl(fileId: Long, partNumber: Int): Future[(UploadKey, String)]
 
   def getFileUploadUrl(fileId: Long): Future[(UploadKey, String)]
 
-  def completeFileUpload(fileId: Long, fieSize: Long, fileName: String, partNames: Seq[String]): Future[Unit]
+  def completeFileUpload(fileId: Long, fieSize: Long, fileName: UnsafeFileName, partNames: Seq[String]): Future[Unit]
 
-  def uploadFile(name: String, file: File): DBIO[FileLocation]
+  def uploadFile(name: UnsafeFileName, file: File): DBIO[FileLocation]
 
-  def uploadFileF(name: String, file: File): Future[FileLocation]
+  def uploadFileF(name: UnsafeFileName, file: File): Future[FileLocation]
 }
 
 private[file] trait UploadKeyParsing {

actor-server/actor-fs-adapters/src/main/scala/im/actor/server/file/local/LocalFileStorageAdapter.scala

@@ -74,7 +74,7 @@ final class LocalFileStorageAdapter(_system: ActorSystem)
 
   val baseUri = Uri(httpConfig.baseUri)
 
-  override def uploadFile(name: String, file: io.File): DBIO[FileLocation] = {
+  override def uploadFile(name: UnsafeFileName, file: io.File): DBIO[FileLocation] = {
     val scalaFile = file.toScala
     val rng = ThreadLocalRandom.current()
     val id = ACLFiles.randomLong(rng)
@@ -83,13 +83,13 @@ final class LocalFileStorageAdapter(_system: ActorSystem)
     val size = scalaFile.size
     for {
       _ ← persist.FileRepo.create(id, size, accessSalt, LocalUploadKey.fileKey(id).key)
-      _ ← DBIO.from(createFile(id, name, scalaFile))
-      _ ← persist.FileRepo.setUploaded(id, name)
+      _ ← DBIO.from(createFile(id, name.safe, scalaFile))
+      _ ← persist.FileRepo.setUploaded(id, name.safe)
     } yield FileLocation(id, ACLFiles.fileAccessHash(id, accessSalt))
 
   }
 
-  override def uploadFileF(name: String, file: io.File): Future[FileLocation] = db.run(uploadFile(name, file))
+  override def uploadFileF(name: UnsafeFileName, file: io.File): Future[FileLocation] = db.run(uploadFile(name, file))
 
   /**
    * Generates upload uri similar to:
@@ -105,12 +105,13 @@ final class LocalFileStorageAdapter(_system: ActorSystem)
     Future.successful(LocalUploadKey.fileKey(fileId) → signRequest(HttpMethods.PUT, query, ACLFiles.secretKey()).toString)
   }
 
-  override def completeFileUpload(fileId: Long, fileSize: Long, fileName: String, partNames: Seq[String]): Future[Unit] = {
+  override def completeFileUpload(fileId: Long, fileSize: Long, fileName: UnsafeFileName, partNames: Seq[String]): Future[Unit] = {
     val fileDir = fileDirectory(fileId)
     for {
       isComplete ← haveAllParts(fileDir, partNames, fileSize)
-      result ← concatFiles(fileDir, partNames, fileName, fileSize)
+      result ← concatFiles(fileDir, partNames, fileName.safe, fileSize)
       _ ← if (isComplete) deleteUploadedParts(fileDir, partNames) else Future.successful(())
+      _ ← db.run(persist.FileRepo.setUploaded(fileId, fileName.safe))
     } yield ()
   }
 

actor-server/actor-fs-adapters/src/main/scala/im/actor/server/file/s3/S3StorageAdapter.scala

@@ -20,7 +20,7 @@ import scala.concurrent.duration._
 import scala.concurrent.forkjoin.ThreadLocalRandom
 import scala.concurrent.{ ExecutionContext, Future }
 
-class S3StorageAdapter(_system: ActorSystem) extends FileStorageAdapter {
+final class S3StorageAdapter(_system: ActorSystem) extends FileStorageAdapter {
 
   private implicit val system: ActorSystem = _system
   private implicit val ec: ExecutionContext = system.dispatcher
@@ -33,10 +33,10 @@ class S3StorageAdapter(_system: ActorSystem) extends FileStorageAdapter {
   val s3Client = new AmazonS3ScalaClient(awsCredentials)
   val transferManager = new TransferManager(awsCredentials)
 
-  override def uploadFile(name: String, file: File): DBIO[FileLocation] =
+  override def uploadFile(name: UnsafeFileName, file: File): DBIO[FileLocation] =
     uploadFile(bucketName, name, file)
 
-  override def uploadFileF(name: String, file: File): Future[FileLocation] =
+  override def uploadFileF(name: UnsafeFileName, file: File): Future[FileLocation] =
     db.run(uploadFile(name, file))
 
   override def downloadFile(id: Long): DBIO[Option[File]] = {
@@ -73,17 +73,17 @@ class S3StorageAdapter(_system: ActorSystem) extends FileStorageAdapter {
     } yield file
   }
 
-  private def uploadFile(bucketName: String, name: String, file: File): DBIO[FileLocation] = {
+  private def uploadFile(bucketName: String, name: UnsafeFileName, file: File): DBIO[FileLocation] = {
     val rnd = ThreadLocalRandom.current()
     val id = rnd.nextLong()
     val accessSalt = ACLFiles.nextAccessSalt(rnd)
     val sizeF = FileUtils.getFileLength(file)
 
     for {
       size ← DBIO.from(sizeF)
-      _ ← persist.FileRepo.create(id, size, accessSalt, s3Key(id, name))
-      _ ← DBIO.from(s3Upload(bucketName, id, name, file))
-      _ ← persist.FileRepo.setUploaded(id, name)
+      _ ← persist.FileRepo.create(id, size, accessSalt, s3Key(id, name.safe))
+      _ ← DBIO.from(s3Upload(bucketName, id, name.safe, file))
+      _ ← persist.FileRepo.setUploaded(id, name.safe)
     } yield FileLocation(id, ACLFiles.fileAccessHash(id, accessSalt))
   }
 
@@ -93,7 +93,7 @@ class S3StorageAdapter(_system: ActorSystem) extends FileStorageAdapter {
 
   override def getFileUploadPartUrl(fileId: Long, partNumber: Int): Future[(UploadKey, String)] = {
     val fileKey = uploadKey(fileId)
-    val partKey = S3UploadKey(s"upload_part_${fileKey.key}_${partNumber}")
+    val partKey = S3UploadKey(s"upload_part_${fileKey.key}_$partNumber")
     val request = new GeneratePresignedUrlRequest(bucketName, partKey.key)
     val expiration = new java.util.Date
     expiration.setTime(expiration.getTime + 1.day.toMillis)
@@ -115,28 +115,29 @@ class S3StorageAdapter(_system: ActorSystem) extends FileStorageAdapter {
     for (url ← s3Client.generatePresignedUrlRequest(presignedRequest)) yield fileKey → url.toString
   }
 
-  override def completeFileUpload(fileId: Long, fileSize: Long, fileName: String, partNames: Seq[String]): Future[Unit] = {
+  override def completeFileUpload(fileId: Long, fileSize: Long, fileName: UnsafeFileName, partNames: Seq[String]): Future[Unit] = {
     for {
       tempDir ← createTempDir()
       fk = uploadKey(fileId).key
       _ ← FutureTransfer.listenFor {
-        transferManager.downloadDirectory(bucketName, s"upload_part_${fk}", tempDir)
+        transferManager.downloadDirectory(bucketName, s"upload_part_$fk", tempDir)
       } map (_.waitForCompletion())
       concatFile ← concatFiles(tempDir, partNames)
       _ ← FutureTransfer.listenFor {
-        transferManager.upload(bucketName, s3Key(fileId, fileName), concatFile)
+        transferManager.upload(bucketName, s3Key(fileId, fileName.safe), concatFile)
       } map (_.waitForCompletion())
+      _ ← db.run(persist.FileRepo.setUploaded(fileId, fileName.safe))
       _ ← deleteDir(tempDir)
     } yield ()
   }
 
-  private def uploadKey(fileId: Long): S3UploadKey = S3UploadKey(s"upload_${fileId}")
+  private def uploadKey(fileId: Long): S3UploadKey = S3UploadKey(s"upload_$fileId")
 
   private def s3Key(id: Long, name: String): String =
     if (name.isEmpty) {
-      s"file_${id}"
+      s"file_$id"
     } else {
-      s"file_${id}/${name}"
+      s"file_$id/$name"
     }
 
   override def parseKey(bytes: Array[Byte]): UploadKey = S3UploadKey.parseFrom(bytes)

actor-server/actor-rpc-api/src/main/scala/im/actor/server/api/rpc/service/files/FilesServiceImpl.scala

@@ -2,20 +2,16 @@ package im.actor.server.api.rpc.service.files
 
 import akka.actor._
 import cats.data.Xor
-import cats.std.all._
-import cats.syntax.traverse._
 import im.actor.api.rpc.FileHelpers.Errors
 import im.actor.api.rpc.files._
 import im.actor.api.rpc._
 import im.actor.concurrent.FutureExt
 import im.actor.server.acl.ACLUtils
 import im.actor.server.db.DbExtension
 import im.actor.server.file._
-import im.actor.server.persist
-import im.actor.server.persist.FileRepo
+import im.actor.server.persist.{ FilePartRepo, FileRepo }
 import slick.driver.PostgresDriver.api._
 
-import scala.concurrent.duration._
 import scala.concurrent.{ ExecutionContext, Future }
 
 class FilesServiceImpl(implicit actorSystem: ActorSystem) extends FilesService {
@@ -30,7 +26,7 @@ class FilesServiceImpl(implicit actorSystem: ActorSystem) extends FilesService {
   override def jhandleGetFileUrl(location: ApiFileLocation, clientData: ClientData): Future[HandlerResult[ResponseGetFileUrl]] =
     authorized(clientData) { client ⇒
       (for {
-        file ← fromFutureOption(Errors.LocationInvalid)(db.run(persist.FileRepo.find(location.fileId)))
+        file ← fromFutureOption(Errors.LocationInvalid)(db.run(FileRepo.find(location.fileId)))
         url ← fromFutureOption(Errors.LocationInvalid)(fsAdapter.getFileDownloadUrl(file, location.accessHash))
       } yield ResponseGetFileUrl(url, FileStorageAdapter.UrlExpirationTimeout.toSeconds.toInt)).value map (_.toScalaz)
     }
@@ -61,27 +57,26 @@ class FilesServiceImpl(implicit actorSystem: ActorSystem) extends FilesService {
       (for {
         uploadKeyUrl ← fromFuture(fsAdapter.getFileUploadUrl(id))
         (uploadKey, url) = uploadKeyUrl
-        _ ← fromFuture(db.run(persist.FileRepo.create(id, expectedSize.toLong, accessSalt = ACLUtils.nextAccessSalt(), uploadKey.key)))
+        _ ← fromFuture(db.run(FileRepo.create(id, expectedSize.toLong, accessSalt = ACLUtils.nextAccessSalt(), uploadKey.key)))
       } yield ResponseGetFileUploadUrl(url, uploadKey.toByteArray)).value map (_.toScalaz)
     }
 
   override def jhandleGetFileUploadPartUrl(partNumber: Int, partSize: Int, keyBytes: Array[Byte], clientData: ClientData): Future[HandlerResult[ResponseGetFileUploadPartUrl]] =
     authorized(clientData) { client ⇒
       (for {
-        file ← fromFutureOption(Errors.FileNotFound)(db.run(persist.FileRepo.findByKey(fsAdapter.parseKey(keyBytes).key)))
+        file ← fromFutureOption(Errors.FileNotFound)(db.run(FileRepo.findByKey(fsAdapter.parseKey(keyBytes).key)))
         partKeyUrl ← fromFuture(fsAdapter.getFileUploadPartUrl(file.id, partNumber))
         (partKey, url) = partKeyUrl
-        _ ← fromFuture(db.run(persist.FilePartRepo.createOrUpdate(file.id, partNumber, partSize, partKey.key)))
+        _ ← fromFuture(db.run(FilePartRepo.createOrUpdate(file.id, partNumber, partSize, partKey.key)))
       } yield ResponseGetFileUploadPartUrl(url)).value map (_.toScalaz)
     }
 
   override def jhandleCommitFileUpload(keyBytes: Array[Byte], fileName: String, clientData: ClientData): Future[HandlerResult[ResponseCommitFileUpload]] =
     authorized(clientData) { client ⇒
       (for {
-        file ← fromFutureOption(Errors.FileNotFound)(db.run(persist.FileRepo.findByKey(fsAdapter.parseKey(keyBytes).key)))
-        partNames ← fromFuture(db.run(persist.FilePartRepo.findByFileId(file.id) map (_.map(_.uploadKey))))
-        _ ← fromFuture(fsAdapter.completeFileUpload(file.id, file.size, fileName, partNames))
-        _ ← fromFuture(db.run(persist.FileRepo.setUploaded(file.id, fileName)))
+        file ← fromFutureOption(Errors.FileNotFound)(db.run(FileRepo.findByKey(fsAdapter.parseKey(keyBytes).key)))
+        partNames ← fromFuture(db.run(FilePartRepo.findByFileId(file.id) map (_.map(_.uploadKey))))
+        _ ← fromFuture(fsAdapter.completeFileUpload(file.id, file.size, UnsafeFileName(fileName), partNames))
       } yield ResponseCommitFileUpload(ApiFileLocation(file.id, ACLUtils.fileAccessHash(file.id, file.accessSalt)))).value map (_.toScalaz)
     }
 }

actor-server/actor-tests/src/test/scala/im/actor/server/api/http/HttpApiFrontendSpec.scala

@@ -18,7 +18,7 @@ import im.actor.server.api.http.json.JsonFormatters._
 import im.actor.server.api.http.json.{ AvatarUrls, _ }
 import im.actor.server.api.rpc.service.groups.{ GroupInviteConfig, GroupsServiceImpl }
 import im.actor.server.api.rpc.service.messaging
-import im.actor.server.file.{ FileStorageExtension, ImageUtils }
+import im.actor.server.file.{ UnsafeFileName, FileStorageExtension, ImageUtils }
 import im.actor.server.webhooks.WebhooksExtension
 import im.actor.server.webhooks.http.routes.OutgoingHooksErrors
 import play.api.libs.json._
@@ -330,7 +330,7 @@ final class HttpApiFrontendSpec
 
     def groupInvitesAvatars1() = {
       val avatarFile = Paths.get(getClass.getResource("/valid-avatar.jpg").toURI).toFile
-      val fileLocation = whenReady(db.run(fsAdapter.uploadFile("avatar", avatarFile)))(identity)
+      val fileLocation = whenReady(db.run(fsAdapter.uploadFile(UnsafeFileName("avatar"), avatarFile)))(identity)
 
       whenReady(db.run(ImageUtils.scaleAvatar(fileLocation.fileId, ThreadLocalRandom.current()))) { result ⇒
         result should matchPattern { case Right(_) ⇒ }
@@ -371,7 +371,7 @@ final class HttpApiFrontendSpec
 
     def groupInvitesAvatars2() = {
       val avatarFile = Paths.get(getClass.getResource("/valid-avatar.jpg").toURI).toFile
-      val fileLocation = whenReady(db.run(fsAdapter.uploadFile("avatar", avatarFile)))(identity)
+      val fileLocation = whenReady(db.run(fsAdapter.uploadFile(UnsafeFileName("avatar"), avatarFile)))(identity)
       whenReady(db.run(ImageUtils.scaleAvatar(fileLocation.fileId, ThreadLocalRandom.current()))) { result ⇒
         result should matchPattern { case Right(_) ⇒ }
         val avatar =

actor-server/actor-tests/src/test/scala/im/actor/server/api/rpc/service/FilesServiceSpec.scala

@@ -10,21 +10,21 @@ import im.actor.server.api.rpc.service.files.FilesServiceImpl
 import im.actor.server.{ BaseAppSuite, ImplicitAuthService, ImplicitSessionRegion }
 import org.apache.commons.io.IOUtils
 
-class FilesServiceSpec
+final class FilesServiceSpec
   extends BaseAppSuite
   with ImplicitSessionRegion
   with ImplicitAuthService {
   behavior of "FilesService"
 
-  it should "Generate upload url" in e1
+  it should "Generate upload url" in generateUploadUrl
 
-  it should "Generate valid upload part urls" in e2
+  it should "Generate valid upload part urls" in generateUploadPartUrls
 
-  it should "Complete upload" in e3
+  it should "Complete upload" in completeUpload
 
-  it should "Generate valid download urls" in e4
+  it should "Generate valid download urls" in generateValidDownloadUrls
 
-  it should "Generate valid upload part urls when same request comes twice" in e5
+  it should "Generate valid upload part urls when same request comes twice" in validUploadPartUrlsDuplRequest
 
   lazy val service = new FilesServiceImpl
   HttpApi(system)
@@ -40,7 +40,7 @@ class FilesServiceSpec
 
   var expectedContents: Option[String] = None
 
-  def e1() = {
+  def generateUploadUrl() = {
     val size = 20
 
     whenReady(service.handleGetFileUploadUrl(size)) { resp ⇒
@@ -53,7 +53,7 @@ class FilesServiceSpec
     }
   }
 
-  def e2() = {
+  def generateUploadPartUrls() = {
     val part1Size = 1024 * 32 // big part
     val part2Size = 5 // small part
 
@@ -81,19 +81,19 @@ class FilesServiceSpec
         connection.setRequestMethod("PUT")
         connection.addRequestProperty("Content-Type", "application/octet-stream")
         val out = new OutputStreamWriter(connection.getOutputStream)
-        val partContents = ("." * size)
+        val partContents = "." * size
         out.write(partContents)
         out.close()
-        val responseCode = connection.getResponseCode()
+        val responseCode = connection.getResponseCode
         responseCode should ===(200)
         partContents
     }
 
     this.expectedContents = Some(parts.foldLeft("") { (acc, p) ⇒ acc + p })
   }
 
-  def e3() = {
-    whenReady(service.handleCommitFileUpload(uploadKey, "The.File")) { resp ⇒
+  def completeUpload() = {
+    whenReady(service.handleCommitFileUpload(uploadKey, "/etc/passwd/The.Filë%00 – 'Fear and Loathing in Las Vegas'")) { resp ⇒
       resp should matchPattern {
         case Ok(ResponseCommitFileUpload(_)) ⇒
       }
@@ -102,7 +102,7 @@ class FilesServiceSpec
     }
   }
 
-  def e4() = {
+  def generateValidDownloadUrls() = {
     val urlStr = whenReady(service.handleGetFileUrl(fileLocation.get)) { resp ⇒
       resp should matchPattern {
         case Ok(ResponseGetFileUrl(_, _)) ⇒
@@ -111,7 +111,10 @@ class FilesServiceSpec
       resp.toOption.get.url
     }
 
-    urlStr should include("The.File?")
+    urlStr should include("The.Fil%C3%AB%00%20%E2%80%93%20'Fear%20and%20Loathing%20in%20Las%20Vegas'?")
+    urlStr shouldNot include("//The")
+    urlStr shouldNot include("etc")
+    urlStr shouldNot include("passwd")
 
     val url = new URL(urlStr)
     val connection = url.openConnection().asInstanceOf[HttpURLConnection]
@@ -122,7 +125,7 @@ class FilesServiceSpec
     IOUtils.toString(connection.getInputStream) should ===(expectedContents.get)
   }
 
-  def e5() = {
+  def validUploadPartUrlsDuplRequest() = {
     val partSize = 1024 * 32
     whenReady(service.handleGetFileUploadPartUrl(1, partSize, uploadKey)) { resp ⇒
       resp should matchPattern {