From f93d8694c31b7ddd87ffd5631f556748c463db7f Mon Sep 17 00:00:00 2001
From: Diogo Cordeiro <diogo@fc.up.pt>
Date: Sat, 27 Jun 2020 22:39:09 +0100
Subject: [PATCH] [AVATAR] Ensure this Action stays secure

---
 actions/attachment.php |  3 +--
 actions/avatar.php     | 32 +++++++++++++-------------------
 classes/File.php       | 23 +++++++++++++++++------
 3 files changed, 31 insertions(+), 27 deletions(-)

diff --git a/actions/attachment.php b/actions/attachment.php
index 6f66d72662..6e6179c4ae 100644
--- a/actions/attachment.php
+++ b/actions/attachment.php
@@ -173,7 +173,7 @@ class AttachmentAction extends ManagedAction
      * @return string etag http header
      * @throws ServerException
      */
-    public function etag(): string
+    public function etag(): ?string
     {
         if (common_config('site', 'use_x_sendfile')) {
             return null;
@@ -202,5 +202,4 @@ class AttachmentAction extends ManagedAction
             return null;
         }
     }
-
 }
diff --git a/actions/avatar.php b/actions/avatar.php
index d549604135..924b3d9086 100644
--- a/actions/avatar.php
+++ b/actions/avatar.php
@@ -15,19 +15,17 @@
 // along with GNU social.  If not, see <http://www.gnu.org/licenses/>.
 
 /**
- * Retrieve user avatar by nickname action class.
+ * Retrieve user avatar by filename action class.
  *
  * @category Action
  * @package  GNUsocial
  *
  * @license  http://www.fsf.org/licensing/licenses/agpl.html AGPLv3
  */
-if (!defined('GNUSOCIAL')) {
-    exit(1);
-}
+defined('GNUSOCIAL') || die;
 
 /**
- * Retrieve user avatar by nickname action class.
+ * Retrieve user avatar by filename action class.
  *
  * @category Action
  * @package  GNUsocial
@@ -42,14 +40,21 @@ if (!defined('GNUSOCIAL')) {
  */
 class AvatarAction extends Action
 {
-    public $filename;
+    public $filename = null;
+    public $filepath = null;
+    public $mimetype = null;
+
     protected function prepare(array $args = [])
     {
         parent::prepare($args);
-        if (empty($this->filename = $this->trimmed('file'))) {
+        $this->filename = File::tryFilename($this->trimmed('file'));
+        $this->filepath = File::path($this->filename, common_config('avatar', 'dir'), false);
+        if (!file_exists($this->filepath)) {
             // TRANS: Client error displayed trying to get a non-existing avatar.
             $this->clientError(_m('No such avatar.'), 404);
         }
+        $this->mimetype = (new ImageFile(-1, $this->filepath))->mimetype;
+
         return true;
     }
 
@@ -57,17 +62,6 @@ class AvatarAction extends Action
     {
         parent::handle();
 
-        if (is_string($srv = common_config('avatar', 'server')) && $srv != '') {
-            common_redirect(Avatar::url($this->filename), 302);
-        } else {
-            $filepath = common_config('avatar', 'dir') . $this->filename;
-            $info = @getimagesize($filepath);
-            if ($info !== false) {
-                common_send_file($filepath, $info['mime'], $this->filename, 'inline');
-            } else {
-                throw new UnsupportedMediaException(_m("Avatar is not an image."));
-            }
-        }
-        return true;
+        common_send_file($this->filepath, $this->mimetype, $this->filename, 'inline');
     }
 }
diff --git a/classes/File.php b/classes/File.php
index 4a4d4208dd..3fe603f58a 100644
--- a/classes/File.php
+++ b/classes/File.php
@@ -413,7 +413,7 @@ class File extends Managed_DataObject
 
     /**
      * Validation for as-saved base filenames
-     * @param $filename
+     * @param mixed $filename
      * @return false|int
      */
     public static function validFilename($filename)
@@ -421,7 +421,12 @@ class File extends Managed_DataObject
         return preg_match('/^[A-Za-z0-9._-]+$/', $filename);
     }
 
-    public static function tryFilename($filename)
+    /**
+     * @param mixed $filename
+     * @return string
+     * @throws InvalidFilenameException
+     */
+    public static function tryFilename($filename): string
     {
         if (!self::validFilename($filename)) {
             throw new InvalidFilenameException($filename);
@@ -431,16 +436,22 @@ class File extends Managed_DataObject
     }
 
     /**
-     * @param $filename
+     * Construct a path
+     *
+     * @param mixed $filename Will be tested by tryFilename
+     * @param string|null $dir Attachments directory by default
+     * @param bool $test_filename
      * @return string
      * @throws InvalidFilenameException
      * @throws ServerException
      */
-    public static function path($filename)
+    public static function path($filename, ?string $dir = null, bool $test_filename = true)
     {
-        self::tryFilename($filename);
+        if ($test_filename) {
+            self::tryFilename($filename);
+        }
 
-        $dir = common_config('attachments', 'dir');
+        $dir = $dir ?? common_config('attachments', 'dir');
 
         if (!in_array($dir[mb_strlen($dir)-1], ['/', '\\'])) {
             $dir .= DIRECTORY_SEPARATOR;