From 102a97bb082edc831cf35d27f9e5c4f55f10ae85 Mon Sep 17 00:00:00 2001
From: Adrien Joly <531781+adrienjoly@users.noreply.github.com>
Date: Sun, 26 Dec 2021 21:17:12 +0100
Subject: [PATCH] fix(security): Make login redirects more secure (#519)

Addresses:
- [Improper Authorization vulnerability found in openwhyd](https://huntr.dev/bounties/d66f90d6-1b5f-440d-8be6-cdffc9d4587e/)
- [Open Redirect vulnerability found in openwhyd](https://huntr.dev/bounties/8852bb22-7312-4af5-b9c2-507a68e37a6a/)
- [Cross-site Scripting (XSS) - Reflected vulnerability found in openwhyd](https://huntr.dev/bounties/84a14deb-5dd8-4cc0-810f-d0987566f845/)

Tests:
* test: /login should allow redirect to /stream
* test: /login should NOT allow javascript in redirect URL
* test: /login should should NOT allow redirect to other domain
* test: /login should NOT allow script element in redirect URL
---
 app/controllers/api/login.js   |  9 ++++-
 app/models/logging.js          |  5 ++-
 app/snip.js                    |  7 ++++
 test/api/security.api.tests.js | 60 +++++++++++++++++++++++++++++++++-
 4 files changed, 76 insertions(+), 5 deletions(-)

diff --git a/app/controllers/api/login.js b/app/controllers/api/login.js
index 8b08499c9..cda8c281f 100644
--- a/app/controllers/api/login.js
+++ b/app/controllers/api/login.js
@@ -1,6 +1,8 @@
 /**
  * login controller, to authenticate users
  */
+const snip = require('../../snip.js');
+const config = require('../../models/config.js');
 var emailModel = require('../../models/email.js');
 var userModel = require('../../models/user.js');
 var notifEmails = require('../../models/notifEmails.js');
@@ -60,7 +62,12 @@ exports.handleRequest = function (request, form, response, ignorePassword) {
     userModel[form.email.indexOf('@') > -1 ? 'fetchByEmail' : 'fetchByHandle'](
       form.email,
       function (dbUser) {
-        if (!dbUser) {
+        if (
+          form.redirect &&
+          snip.getSafeOpenwhydURL(form.redirect, config.urlPrefix) === false
+        ) {
+          form.error = 'Unsafe redirect target';
+        } else if (!dbUser) {
           form.error = "Are you sure? We don't recognize your email address!";
         } else if (form.action == 'forgot') {
           notifEmails.sendPasswordReset(dbUser._id, dbUser.pwd, form.redirect);
diff --git a/app/models/logging.js b/app/models/logging.js
index 472457690..d0f9e49a0 100644
--- a/app/models/logging.js
+++ b/app/models/logging.js
@@ -261,9 +261,8 @@ http.ServerResponse.prototype.redirect = function (url) {
 };
 
 http.ServerResponse.prototype.safeRedirect = function (url) {
-  const fullURL = new URL(url, config.urlPrefix);
-  if (`${fullURL.protocol}//${fullURL.host}` !== config.urlPrefix)
-    return this.forbidden();
+  const safeURL = snip.getSafeOpenwhydURL(url, config.urlPrefix);
+  if (safeURL === false) return this.forbidden();
   this.redirect(url);
 };
 
diff --git a/app/snip.js b/app/snip.js
index 753aaad9b..0b8955f88 100644
--- a/app/snip.js
+++ b/app/snip.js
@@ -109,6 +109,13 @@ exports.sanitizeJsStringInHtml = function (str) {
   return exports.htmlEntities(exports.addSlashes(str || ''));
 };
 
+exports.getSafeOpenwhydURL = function (url, safeUrlPrefix) {
+  if (typeof url !== 'string' || url.includes('<script')) return false;
+  const fullURL = new URL(url, safeUrlPrefix);
+  if (`${fullURL.protocol}//${fullURL.host}` !== safeUrlPrefix) return false;
+  else return fullURL;
+};
+
 var timeScales = [
   { 'minute(s)': 60 },
   { 'hour(s)': 60 },
diff --git a/test/api/security.api.tests.js b/test/api/security.api.tests.js
index c4d09f3e7..4253e9203 100644
--- a/test/api/security.api.tests.js
+++ b/test/api/security.api.tests.js
@@ -12,7 +12,65 @@ const loginAs = promisify(apiClient.loginAs);
 before(cleanup);
 
 describe('security', () => {
-  describe('Open Redirect', () => {
+  describe('Open Redirect from /login', () => {
+    it('should allow redirect to /stream', async () => {
+      const target = `/stream`;
+      const { response } = await postRaw(null, `/login`, {
+        action: 'login',
+        email: ADMIN_USER.email,
+        md5: ADMIN_USER.md5,
+        redirect: target,
+      });
+      assert(
+        response.body.includes(`window.location.href="${target}"`) === true,
+        `page body should include redirect to ${target}`
+      );
+    });
+
+    it('should NOT allow redirect to other domain', async () => {
+      const target = `https://google.com`;
+      const { response } = await postRaw(null, `/login`, {
+        action: 'login',
+        email: ADMIN_USER.email,
+        md5: ADMIN_USER.md5,
+        redirect: target,
+      });
+      assert(
+        response.body.includes(`window.location.href="${target}"`) === false,
+        `page body should NOT include redirect to ${target}`
+      );
+    });
+
+    it('should NOT allow javascript in redirect URL', async () => {
+      const target = `javascript:alert()`;
+      const { response } = await postRaw(null, `/login`, {
+        action: 'login',
+        email: ADMIN_USER.email,
+        md5: ADMIN_USER.md5,
+        redirect: target,
+      });
+      assert(
+        response.body.includes(`window.location.href="${target}"`) === false,
+        `page body should NOT include redirect to ${target}`
+      );
+    });
+
+    it('should NOT allow script element in redirect URL', async () => {
+      const target = `<script>alert(document.cookie)</script>`;
+      const { response } = await postRaw(null, `/login`, {
+        action: 'login',
+        email: ADMIN_USER.email,
+        md5: ADMIN_USER.md5,
+        redirect: target,
+      });
+      assert(
+        response.body.includes(`window.location.href="${target}"`) === false,
+        `page body should NOT include redirect to ${target}`
+      );
+    });
+  });
+
+  describe('Open Redirect from /consent', () => {
     it('should allow redirect to /stream', async () => {
       const target = `/stream`;
       const { jar } = await loginAs(ADMIN_USER);