CVE-2021-3851: URL Redirection to Untrusted Site ('Open Redirect') (CWE-601)
value: firefly-iii is vulnerable to URL Redirection to Untrusted Site
CWE description: A web application accepts a user-controlled input that specifies a link to an external site, and uses that link in a Redirect. This simplifies phishing attacks.
CVSS v3 base score: 5.4
Published: 2021-10-19T13:15Z
Repository: https://github.com/firefly-iii/firefly-iii
Language: PHP
Files changed: app/Exceptions/Handler.php, app/Http/Middleware/StartFireflySession.php, app/Support/Http/Controllers/UserNavigation.php
Commit date: 2021-10-02 13:28:56 +0200
Commit message: Fix unsecure redirect code.

Diff: diff --git a/app/Exceptions/Handler.php b/app/Exceptions/Handler.php
index f6fdbd62442..ba68632bd0d 100644
--- a/app/Exceptions/Handler.php
+++ b/app/Exceptions/Handler.php
@@ -27,12 +27,15 @@
 namespace FireflyIII\Exceptions;
 
 use ErrorException;
 use FireflyIII\Jobs\MailError;
 use Illuminate\Auth\AuthenticationException;
+use Illuminate\Contracts\Foundation\Application;
 use Illuminate\Foundation\Exceptions\Handler as ExceptionHandler;
+use Illuminate\Http\RedirectResponse;
 use Illuminate\Http\Request;
+use Illuminate\Routing\Redirector;
 use Illuminate\Session\TokenMismatchException;
 use Illuminate\Support\Arr;
 use Illuminate\Validation\ValidationException as LaravelValidationException;
 use Laravel\Passport\Exceptions\OAuthServerException as LaravelOAuthException;
 use League\OAuth2\Server\Exception\OAuthServerException;
@@ -185,6 +188,60 @@ private function shouldntReportLocal(Throwable $e): bool
                 return $e instanceof $type;
             }
             )
         );
     }
+
+    /**
+     * Convert a validation exception into a response.
+     *
+     * @param Request                    $request
+     * @param LaravelValidationException $exception
+     *
+     * @return Application|RedirectResponse|Redirector
+     */
+    protected function invalid($request, LaravelValidationException $exception): Application|RedirectResponse|Redirector
+    {
+        // protect against open redirect when submitting invalid forms.
+        $previous = $this->getPreviousUrl();
+        $redirect = $this->getRedirectUrl($exception);
+
+        return redirect($redirect ?? $previous)
+            ->withInput(Arr::except($request->input(), $this->dontFlash))
+            ->withErrors($exception->errors(), $request->input('_error_bag', $exception->errorBag));
+    }
+
+    /**
+     * Only return the previousUrl() if it is a valid URL. Return default redirect otherwise.
+     *
+     * @return string
+     */
+    private function getPreviousUrl(): string
+    {
+        $safe         = route('index');
+        $previous     = url()->previous();
+        $previousHost = parse_url($previous, PHP_URL_HOST);
+        $safeHost     = parse_url($safe, PHP_URL_HOST);
+
+        return null !== $previousHost && $previousHost === $safeHost ? $previous : $safe;
+    }
+
+    /**
+     * Only return the redirectTo property from the exception if it is a valid URL. Return NULL otherwise.
+     *
+     * @param LaravelValidationException $exception
+     *
+     * @return string|null
+     */
+    private function getRedirectUrl(LaravelValidationException $exception): ?string
+    {
+        if (null === $exception->redirectTo) {
+            return null;
+        }
+        $safe         = route('index');
+        $previous     = $exception->redirectTo;
+        $previousHost = parse_url($previous, PHP_URL_HOST);
+        $safeHost     = parse_url($safe, PHP_URL_HOST);
+
+        return null !== $previousHost && $previousHost === $safeHost ? $previous : $safe;
+    }
 }
diff --git a/app/Http/Middleware/StartFireflySession.php b/app/Http/Middleware/StartFireflySession.php
index 0cd5de179e1..eaf9b8ff9e3 100644
--- a/app/Http/Middleware/StartFireflySession.php
+++ b/app/Http/Middleware/StartFireflySession.php
@@ -43,11 +43,10 @@ class StartFireflySession extends StartSession
     protected function storeCurrentUrl(Request $request, $session): void
     {
         $url          = $request->fullUrl();
         $forbiddenWords = strpos($url, 'offline') || strpos($url, 'jscript') || strpos($url, 'delete') || strpos($url, '/login') || strpos($url, '/json') || strpos($url, 'serviceworker') || strpos($url, '/attachments/view');
 
-        // also stop remembering "delete" URL's.
         if (false === $forbiddenWords
             && 'GET' === $request->method()
             && !$request->ajax()) {
             //Log::debug(sprintf('Redirect is now "%s".', $url));
             $session->setPreviousUrl($url);
diff --git a/app/Support/Http/Controllers/UserNavigation.php b/app/Support/Http/Controllers/UserNavigation.php
index 3b0eafaabbe..58192ac75db 100644
--- a/app/Support/Http/Controllers/UserNavigation.php
+++ b/app/Support/Http/Controllers/UserNavigation.php
@@ -181,11 +181,11 @@ final protected function rememberPreviousUri(string $identifier): ?string
         $default = parse_url(route('index'), PHP_URL_HOST);
 
         // get host of previous URL:
         $previous = parse_url($return, PHP_URL_HOST);
 
-        if ($default === $previous && (null === $errors || (0 === $errors->count())) && !Str::contains($return, $forbidden)) {
+        if (null !== $previous && $default === $previous && (null === $errors || (0 === $errors->count())) && !Str::contains($return, $forbidden)) {
             Log::debug(sprintf('Saving URL %s under key %s', $return, $identifier));
             session()->put($identifier, $return);
 
             return $return;
         }

Vulnerable code:         // also stop remembering "delete" URL's.
        if ($default === $previous && (null === $errors || (0 === $errors->count())) && !Str::contains($return, $forbidden)) {
Fixed code: use Illuminate\Contracts\Foundation\Application;
use Illuminate\Http\RedirectResponse;
use Illuminate\Routing\Redirector;

    /**
     * Convert a validation exception into a response.
     *
     * @param Request                    $request
     * @param LaravelValidationException $exception
     *
     * @return Application|RedirectResponse|Redirector
     */
    protected function invalid($request, LaravelValidationException $exception): Application|RedirectResponse|Redirector
    {
        // protect against open redirect when submitting invalid forms.
        $previous = $this->getPreviousUrl();
        $redirect = $this->getRedirectUrl($exception);

        return redirect($redirect ?? $previous)
            ->withInput(Arr::except($request->input(), $this->dontFlash))
            ->withErrors($exception->errors(), $request->input('_error_bag', $exception->errorBag));
    }

    /**
     * Only return the previousUrl() if it is a valid URL. Return default redirect otherwise.
     *
     * @return string
     */
    private function getPreviousUrl(): string
    {
        $safe         = route('index');
        $previous     = url()->previous();
        $previousHost = parse_url($previous, PHP_URL_HOST);
        $safeHost     = parse_url($safe, PHP_URL_HOST);

        return null !== $previousHost && $previousHost === $safeHost ? $previous : $safe;
    }

    /**
     * Only return the redirectTo property from the exception if it is a valid URL. Return NULL otherwise.
     *
     * @param LaravelValidationException $exception
     *
     * @return string|null
     */
    private function getRedirectUrl(LaravelValidationException $exception): ?string
    {
        if (null === $exception->redirectTo) {
            return null;
        }
        $safe         = route('index');
        $previous     = $exception->redirectTo;
        $previousHost = parse_url($previous, PHP_URL_HOST);
        $safeHost     = parse_url($safe, PHP_URL_HOST);

        return null !== $previousHost && $previousHost === $safeHost ? $previous : $safe;
    }
        if (null !== $previous && $default === $previous && (null === $errors || (0 === $errors->count())) && !Str::contains($return, $forbidden)) {