From 42581dfe93a0eb417dddc5fd66876ef24896bff5 Mon Sep 17 00:00:00 2001 From: Clement Michaud Date: Sat, 27 Oct 2018 18:18:25 +0200 Subject: [PATCH] Fix open redirection vulnerability. In order to redirect the user after authentication, Authelia uses rd query parameter provided by the proxy. However an attacker could use phishing to make the user be redirected to a bad domain. In order to avoid the user to be redirected to a bad location, Authelia now verifies the redirection URL is under the protected domain. --- Gruntfile.js | 10 ++++- client/src/lib/SafeRedirect.ts | 10 +++++ client/src/lib/firstfactor/index.ts | 5 ++- client/src/lib/secondfactor/U2FValidator.ts | 14 +------ client/src/lib/secondfactor/index.ts | 36 +++++++++------- client/src/lib/u2f-register/u2f-register.ts | 5 ++- server/src/lib/routes/firstfactor/get.ts | 40 ++++++++++++------ server/src/lib/routes/firstfactor/post.ts | 10 +++-- .../secondfactor/u2f/sign_request/get.ts | 2 +- .../src/lib/routes/verify/get_basic_auth.ts | 2 +- .../lib/routes/verify/get_session_cookie.ts | 2 +- server/src/lib/utils/DomainExtractor.ts | 6 --- server/src/lib/utils/SafeRedirection.spec.ts | 33 +++++++++++++++ server/src/lib/utils/SafeRedirection.ts | 22 ++++++++++ server/src/views/secondfactor.pug | 2 +- shared/BelongToDomain.ts | 8 ++++ .../utils => shared}/DomainExtractor.spec.ts | 13 +++++- shared/DomainExtractor.ts | 11 +++++ shared/UserMessages.ts | 2 + test/complete-config/closed-redirection.ts | 41 +++++++++++++++++++ .../mongo-broken-connection.ts | 22 +++++----- test/environment.ts | 2 +- test/helpers/full-login.ts | 12 ++++++ test/helpers/login-and-register-totp.ts | 5 +-- test/helpers/login-as.ts | 2 - test/minimal-config/bad_password.ts | 8 +--- test/minimal-config/fail_totp.ts | 11 +---- test/minimal-config/reset_password.ts | 1 - test/minimal-config/validate_totp.ts | 16 ++------ 29 files changed, 248 insertions(+), 105 deletions(-) create mode 100644 client/src/lib/SafeRedirect.ts delete mode 100644 server/src/lib/utils/DomainExtractor.ts create mode 100644 server/src/lib/utils/SafeRedirection.spec.ts create mode 100644 server/src/lib/utils/SafeRedirection.ts create mode 100644 shared/BelongToDomain.ts rename {server/src/lib/utils => shared}/DomainExtractor.spec.ts (54%) create mode 100644 shared/DomainExtractor.ts create mode 100644 test/complete-config/closed-redirection.ts create mode 100644 test/helpers/full-login.ts diff --git a/Gruntfile.js b/Gruntfile.js index 28bdd6d8..f8b33fd1 100644 --- a/Gruntfile.js +++ b/Gruntfile.js @@ -9,6 +9,9 @@ module.exports = function (grunt) { }, "env-test-client-unit": { TS_NODE_PROJECT: "client/tsconfig.json" + }, + "env-test-shared-unit": { + TS_NODE_PROJECT: "server/tsconfig.json" } }, run: { @@ -37,6 +40,10 @@ module.exports = function (grunt) { cmd: "./node_modules/.bin/mocha", args: ['--colors', '--require', 'ts-node/register', 'server/src/**/*.spec.ts'] }, + "test-shared-unit": { + cmd: "./node_modules/.bin/mocha", + args: ['--colors', '--require', 'ts-node/register', 'shared/**/*.spec.ts'] + }, "test-client-unit": { cmd: "./node_modules/.bin/mocha", args: ['--colors', '--require', 'ts-node/register', 'client/test/**/*.test.ts'] @@ -193,8 +200,9 @@ module.exports = function (grunt) { grunt.registerTask('compile-client', ['run:lint-client', 'run:compile-client']) grunt.registerTask('test-server', ['env:env-test-server-unit', 'run:test-server-unit']) + grunt.registerTask('test-shared', ['env:env-test-shared-unit', 'run:test-shared-unit']) grunt.registerTask('test-client', ['env:env-test-client-unit', 'run:test-client-unit']) - grunt.registerTask('test-unit', ['test-server', 'test-client']); + grunt.registerTask('test-unit', ['test-server', 'test-client', 'test-shared']); grunt.registerTask('test-int', ['run:test-cucumber', 'run:test-minimal-config', 'run:test-complete-config', 'run:test-inactivity']); grunt.registerTask('copy-resources', ['copy:resources', 'copy:views', 'copy:images', 'copy:thirdparties', 'concat:css']); diff --git a/client/src/lib/SafeRedirect.ts b/client/src/lib/SafeRedirect.ts new file mode 100644 index 00000000..7e7684b8 --- /dev/null +++ b/client/src/lib/SafeRedirect.ts @@ -0,0 +1,10 @@ +import { BelongToDomain } from "../../../shared/BelongToDomain"; + +export function SafeRedirect(url: string, cb: () => void): void { + const domain = window.location.hostname.split(".").slice(-2).join("."); + if (url.startsWith("/") || BelongToDomain(url, domain)) { + window.location.href = url; + return; + } + cb(); +} \ No newline at end of file diff --git a/client/src/lib/firstfactor/index.ts b/client/src/lib/firstfactor/index.ts index a6e2e197..24affee2 100644 --- a/client/src/lib/firstfactor/index.ts +++ b/client/src/lib/firstfactor/index.ts @@ -6,6 +6,7 @@ import { QueryParametersRetriever } from "../QueryParametersRetriever"; import Constants = require("../../../../shared/constants"); import Endpoints = require("../../../../shared/api"); import UserMessages = require("../../../../shared/UserMessages"); +import { SafeRedirect } from "../SafeRedirect"; export default function (window: Window, $: JQueryStatic, firstFactorValidator: typeof FirstFactorValidator, jslogger: typeof JSLogger) { @@ -28,7 +29,9 @@ export default function (window: Window, $: JQueryStatic, } function onFirstFactorSuccess(redirectUrl: string) { - window.location.href = redirectUrl; + SafeRedirect(redirectUrl, () => { + notifier.error("Cannot redirect to an external domain."); + }); } function onFirstFactorFailure(err: Error) { diff --git a/client/src/lib/secondfactor/U2FValidator.ts b/client/src/lib/secondfactor/U2FValidator.ts index a2e3cd3f..5812922f 100644 --- a/client/src/lib/secondfactor/U2FValidator.ts +++ b/client/src/lib/secondfactor/U2FValidator.ts @@ -1,7 +1,6 @@ import U2f = require("u2f"); import U2fApi from "u2f-api"; import BluebirdPromise = require("bluebird"); -import { SignMessage } from "../../../../shared/SignMessage"; import Endpoints = require("../../../../shared/api"); import UserMessages = require("../../../../shared/UserMessages"); import { INotifier } from "../INotifier"; @@ -31,24 +30,13 @@ function finishU2fAuthentication(responseData: U2fApi.SignResponse, }); } -function startU2fAuthentication($: JQueryStatic, notifier: INotifier) - : BluebirdPromise { - +export function validate($: JQueryStatic): BluebirdPromise { return GetPromised($, Endpoints.SECOND_FACTOR_U2F_SIGN_REQUEST_GET, {}, undefined, "json") .then(function (signRequest: U2f.Request) { - notifier.info(UserMessages.PLEASE_TOUCH_TOKEN); return U2fApi.sign(signRequest, 60); }) .then(function (signResponse: U2fApi.SignResponse) { return finishU2fAuthentication(signResponse, $); }); } - -export function validate($: JQueryStatic, notifier: INotifier) { - return startU2fAuthentication($, notifier) - .catch(function (err: Error) { - notifier.error(UserMessages.U2F_TRANSACTION_FINISH_FAILED); - return BluebirdPromise.reject(err); - }); -} diff --git a/client/src/lib/secondfactor/index.ts b/client/src/lib/secondfactor/index.ts index c145db8f..279723dc 100644 --- a/client/src/lib/secondfactor/index.ts +++ b/client/src/lib/secondfactor/index.ts @@ -3,38 +3,44 @@ import U2FValidator = require("./U2FValidator"); import ClientConstants = require("./constants"); import { Notifier } from "../Notifier"; import { QueryParametersRetriever } from "../QueryParametersRetriever"; -import Endpoints = require("../../../../shared/api"); -import ServerConstants = require("../../../../shared/constants"); import UserMessages = require("../../../../shared/UserMessages"); import SharedConstants = require("../../../../shared/constants"); +import { SafeRedirect } from "../SafeRedirect"; export default function (window: Window, $: JQueryStatic) { - const notifierTotp = new Notifier(".notification-totp", $); - const notifierU2f = new Notifier(".notification-u2f", $); + const notifier = new Notifier(".notification", $); - function onAuthenticationSuccess(serverRedirectUrl: string, notifier: Notifier) { - if (QueryParametersRetriever.get(SharedConstants.REDIRECT_QUERY_PARAM)) - window.location.href = QueryParametersRetriever.get(SharedConstants.REDIRECT_QUERY_PARAM); - else if (serverRedirectUrl) - window.location.href = serverRedirectUrl; - else + function onAuthenticationSuccess(serverRedirectUrl: string) { + const queryRedirectUrl = QueryParametersRetriever.get(SharedConstants.REDIRECT_QUERY_PARAM); + if (queryRedirectUrl) { + SafeRedirect(queryRedirectUrl, () => { + notifier.error(UserMessages.CANNOT_REDIRECT_TO_EXTERNAL_DOMAIN); + }); + } else if (serverRedirectUrl) { + SafeRedirect(serverRedirectUrl, () => { + notifier.error(UserMessages.CANNOT_REDIRECT_TO_EXTERNAL_DOMAIN); + }); + } else { notifier.success(UserMessages.AUTHENTICATION_SUCCEEDED); + } } function onSecondFactorTotpSuccess(redirectUrl: string) { - onAuthenticationSuccess(redirectUrl, notifierTotp); + onAuthenticationSuccess(redirectUrl); } function onSecondFactorTotpFailure(err: Error) { - notifierTotp.error(UserMessages.AUTHENTICATION_TOTP_FAILED); + notifier.error(UserMessages.AUTHENTICATION_TOTP_FAILED); } function onU2fAuthenticationSuccess(redirectUrl: string) { - onAuthenticationSuccess(redirectUrl, notifierU2f); + onAuthenticationSuccess(redirectUrl); } function onU2fAuthenticationFailure() { - notifierU2f.error(UserMessages.AUTHENTICATION_U2F_FAILED); + // TODO(clems4ever): we should not display this error message until a device + // is registered. + // notifier.error(UserMessages.AUTHENTICATION_U2F_FAILED); } function onTOTPFormSubmitted(): boolean { @@ -47,7 +53,7 @@ export default function (window: Window, $: JQueryStatic) { $(window.document).ready(function () { $(ClientConstants.TOTP_FORM_SELECTOR).on("submit", onTOTPFormSubmitted); - U2FValidator.validate($, notifierU2f) + U2FValidator.validate($) .then(onU2fAuthenticationSuccess, onU2fAuthenticationFailure); }); } \ No newline at end of file diff --git a/client/src/lib/u2f-register/u2f-register.ts b/client/src/lib/u2f-register/u2f-register.ts index 3c09fdbb..abf40ee0 100644 --- a/client/src/lib/u2f-register/u2f-register.ts +++ b/client/src/lib/u2f-register/u2f-register.ts @@ -8,6 +8,7 @@ import Endpoints = require("../../../../shared/api"); import UserMessages = require("../../../../shared/UserMessages"); import { RedirectionMessage } from "../../../../shared/RedirectionMessage"; import { ErrorMessage } from "../../../../shared/ErrorMessage"; +import { SafeRedirect } from "../SafeRedirect"; export default function (window: Window, $: JQueryStatic) { const notifier = new Notifier(".notification", $); @@ -44,7 +45,9 @@ export default function (window: Window, $: JQueryStatic) { $(document).ready(function () { requestRegistration() .then((redirectionUrl: string) => { - document.location.href = redirectionUrl; + SafeRedirect(redirectionUrl, () => { + notifier.error(UserMessages.CANNOT_REDIRECT_TO_EXTERNAL_DOMAIN); + }); }) .catch((err) => { onRegisterFailure(err); diff --git a/server/src/lib/routes/firstfactor/get.ts b/server/src/lib/routes/firstfactor/get.ts index a4ee1b4b..dc7260fb 100644 --- a/server/src/lib/routes/firstfactor/get.ts +++ b/server/src/lib/routes/firstfactor/get.ts @@ -7,45 +7,61 @@ import { AuthenticationSessionHandler } from "../../AuthenticationSessionHandler import Constants = require("../../../../../shared/constants"); import Util = require("util"); import { ServerVariables } from "../../ServerVariables"; +import { SafeRedirector } from "../../utils/SafeRedirection"; -function getRedirectParam(req: express.Request) { +function getRedirectParam( + req: express.Request) { return req.query[Constants.REDIRECT_QUERY_PARAM] != "undefined" ? req.query[Constants.REDIRECT_QUERY_PARAM] : undefined; } -function redirectToSecondFactorPage(req: express.Request, res: express.Response) { +function redirectToSecondFactorPage( + req: express.Request, + res: express.Response) { + const redirectUrl = getRedirectParam(req); if (!redirectUrl) res.redirect(Endpoints.SECOND_FACTOR_GET); else - res.redirect(Util.format("%s?%s=%s", Endpoints.SECOND_FACTOR_GET, - Constants.REDIRECT_QUERY_PARAM, - redirectUrl)); + res.redirect( + Util.format("%s?%s=%s", + Endpoints.SECOND_FACTOR_GET, + Constants.REDIRECT_QUERY_PARAM, + redirectUrl)); } -function redirectToService(req: express.Request, res: express.Response) { +function redirectToService( + req: express.Request, + res: express.Response, + redirector: SafeRedirector) { const redirectUrl = getRedirectParam(req); - if (!redirectUrl) + if (!redirectUrl) { res.redirect(Endpoints.LOGGED_IN); - else - res.redirect(redirectUrl); + } else { + redirector.redirectOrElse(res, redirectUrl, Endpoints.LOGGED_IN); + } } -function renderFirstFactor(res: express.Response) { +function renderFirstFactor( + res: express.Response) { + res.render("firstfactor", { first_factor_post_endpoint: Endpoints.FIRST_FACTOR_POST, reset_password_request_endpoint: Endpoints.RESET_PASSWORD_REQUEST_GET }); } -export default function (vars: ServerVariables) { +export default function ( + vars: ServerVariables) { + + const redirector = new SafeRedirector(vars.config.session.domain); return function (req: express.Request, res: express.Response): BluebirdPromise { return new BluebirdPromise(function (resolve, reject) { const authSession = AuthenticationSessionHandler.get(req, vars.logger); if (authSession.first_factor) { if (authSession.second_factor) - redirectToService(req, res); + redirectToService(req, res, redirector); else redirectToSecondFactorPage(req, res); resolve(); diff --git a/server/src/lib/routes/firstfactor/post.ts b/server/src/lib/routes/firstfactor/post.ts index 20731fcd..1698fd77 100644 --- a/server/src/lib/routes/firstfactor/post.ts +++ b/server/src/lib/routes/firstfactor/post.ts @@ -9,7 +9,7 @@ import Endpoint = require("../../../../../shared/api"); import ErrorReplies = require("../../ErrorReplies"); import { AuthenticationSessionHandler } from "../../AuthenticationSessionHandler"; import Constants = require("../../../../../shared/constants"); -import { DomainExtractor } from "../../utils/DomainExtractor"; +import { DomainExtractor } from "../../../../../shared/DomainExtractor"; import UserMessages = require("../../../../../shared/UserMessages"); import { MethodCalculator } from "../../authentication/MethodCalculator"; import { ServerVariables } from "../../ServerVariables"; @@ -51,14 +51,16 @@ export default function (vars: ServerVariables) { authSession.userid = username; authSession.keep_me_logged_in = keepMeLoggedIn; authSession.first_factor = true; - const redirectUrl = req.query[Constants.REDIRECT_QUERY_PARAM] !== "undefined" + const redirectUrl: string = req.query[Constants.REDIRECT_QUERY_PARAM] !== "undefined" // Fuck, don't know why it is a string! ? req.query[Constants.REDIRECT_QUERY_PARAM] : undefined; const emails: string[] = groupsAndEmails.emails; const groups: string[] = groupsAndEmails.groups; - const redirectHost: string = DomainExtractor.fromUrl(redirectUrl); + + const domain = DomainExtractor.fromUrl(redirectUrl); + const redirectHost = (domain) ? domain : ""; const authMethod = MethodCalculator.compute( vars.config.authentication_methods, redirectHost); vars.logger.debug(req, "Authentication method for \"%s\" is \"%s\"", @@ -72,7 +74,7 @@ export default function (vars: ServerVariables) { vars.regulator.mark(username, true); if (authMethod == "single_factor") { - let newRedirectionUrl: string = redirectUrl; + let newRedirectionUrl = redirectUrl; if (!newRedirectionUrl) newRedirectionUrl = Endpoint.LOGGED_IN; res.send({ diff --git a/server/src/lib/routes/secondfactor/u2f/sign_request/get.ts b/server/src/lib/routes/secondfactor/u2f/sign_request/get.ts index f6907d8f..9e93dde0 100644 --- a/server/src/lib/routes/secondfactor/u2f/sign_request/get.ts +++ b/server/src/lib/routes/secondfactor/u2f/sign_request/get.ts @@ -24,7 +24,7 @@ export default function (vars: ServerVariables) { }) .then(function (doc: U2FRegistrationDocument): BluebirdPromise { if (!doc) - return BluebirdPromise.reject(new exceptions.AccessDeniedError("No U2F registration found")); + return BluebirdPromise.reject(new exceptions.AccessDeniedError("No U2F registration document found.")); const appId: string = u2f_common.extract_app_id(req); vars.logger.info(req, "Start authentication of app '%s'", appId); diff --git a/server/src/lib/routes/verify/get_basic_auth.ts b/server/src/lib/routes/verify/get_basic_auth.ts index e489bb88..0710d88b 100644 --- a/server/src/lib/routes/verify/get_basic_auth.ts +++ b/server/src/lib/routes/verify/get_basic_auth.ts @@ -4,7 +4,7 @@ import ObjectPath = require("object-path"); import { ServerVariables } from "../../ServerVariables"; import { AuthenticationSession } from "../../../../types/AuthenticationSession"; -import { DomainExtractor } from "../../utils/DomainExtractor"; +import { DomainExtractor } from "../../../../../shared/DomainExtractor"; import { MethodCalculator } from "../../authentication/MethodCalculator"; import AccessControl from "./access_control"; diff --git a/server/src/lib/routes/verify/get_session_cookie.ts b/server/src/lib/routes/verify/get_session_cookie.ts index 504c8707..476aa846 100644 --- a/server/src/lib/routes/verify/get_session_cookie.ts +++ b/server/src/lib/routes/verify/get_session_cookie.ts @@ -6,7 +6,7 @@ import ObjectPath = require("object-path"); import Exceptions = require("../../Exceptions"); import { Configuration } from "../../configuration/schema/Configuration"; import Constants = require("../../../../../shared/constants"); -import { DomainExtractor } from "../../utils/DomainExtractor"; +import { DomainExtractor } from "../../../../../shared/DomainExtractor"; import { ServerVariables } from "../../ServerVariables"; import { MethodCalculator } from "../../authentication/MethodCalculator"; import { IRequestLogger } from "../../logging/IRequestLogger"; diff --git a/server/src/lib/utils/DomainExtractor.ts b/server/src/lib/utils/DomainExtractor.ts deleted file mode 100644 index ea162a04..00000000 --- a/server/src/lib/utils/DomainExtractor.ts +++ /dev/null @@ -1,6 +0,0 @@ -export class DomainExtractor { - static fromUrl(url: string): string { - if (!url) return ""; - return url.match(/https?:\/\/([^\/:]+).*/)[1]; - } -} \ No newline at end of file diff --git a/server/src/lib/utils/SafeRedirection.spec.ts b/server/src/lib/utils/SafeRedirection.spec.ts new file mode 100644 index 00000000..4126949f --- /dev/null +++ b/server/src/lib/utils/SafeRedirection.spec.ts @@ -0,0 +1,33 @@ +import Assert = require("assert"); +import Sinon = require("sinon"); +import { SafeRedirector } from "./SafeRedirection"; + +describe("web_server/middlewares/SafeRedirection", () => { + describe("Url is in protected domain", () => { + before(() => { + this.redirector = new SafeRedirector("example.com"); + this.res = {redirect: Sinon.stub()}; + }); + + it("should redirect to provided url", () => { + this.redirector.redirectOrElse(this.res, + "https://mysubdomain.example.com:8080/abc", + "https://authelia.example.com"); + Assert(this.res.redirect.calledWith("https://mysubdomain.example.com:8080/abc")); + }); + + it("should redirect to default url when wrong domain", () => { + this.redirector.redirectOrElse(this.res, + "https://mysubdomain.domain.rtf:8080/abc", + "https://authelia.example.com"); + Assert(this.res.redirect.calledWith("https://authelia.example.com")); + }); + + it("should redirect to default url when not terminating by domain", () => { + this.redirector.redirectOrElse(this.res, + "https://mysubdomain.example.com.rtf:8080/abc", + "https://authelia.example.com"); + Assert(this.res.redirect.calledWith("https://authelia.example.com")); + }); + }); +}); \ No newline at end of file diff --git a/server/src/lib/utils/SafeRedirection.ts b/server/src/lib/utils/SafeRedirection.ts new file mode 100644 index 00000000..9e6a32e0 --- /dev/null +++ b/server/src/lib/utils/SafeRedirection.ts @@ -0,0 +1,22 @@ +import Express = require("express"); +import { DomainExtractor } from "../../../../shared/DomainExtractor"; +import { BelongToDomain } from "../../../../shared/BelongToDomain"; + + +export class SafeRedirector { + private domain: string; + + constructor(domain: string) { + this.domain = domain; + } + + redirectOrElse( + res: Express.Response, + url: string, + defaultUrl: string): void { + if (BelongToDomain(url, this.domain)) { + res.redirect(url); + } + res.redirect(defaultUrl); + } +} \ No newline at end of file diff --git a/server/src/views/secondfactor.pug b/server/src/views/secondfactor.pug index 74aae408..4df8ec25 100644 --- a/server/src/views/secondfactor.pug +++ b/server/src/views/secondfactor.pug @@ -8,7 +8,7 @@ block form-header block content div - div(class="notification notification-totp") + div(class="notification") h3 Hi #{username} div(class="row") div(class="u2f-token") diff --git a/shared/BelongToDomain.ts b/shared/BelongToDomain.ts new file mode 100644 index 00000000..aa2e6815 --- /dev/null +++ b/shared/BelongToDomain.ts @@ -0,0 +1,8 @@ +import { DomainExtractor } from "./DomainExtractor"; + +export function BelongToDomain(url: string, domain: string): boolean { + const urlDomain =  DomainExtractor.fromUrl(url); + if (!urlDomain) return false; + const idx = urlDomain.indexOf(domain); + return idx + domain.length == urlDomain.length; +} \ No newline at end of file diff --git a/server/src/lib/utils/DomainExtractor.spec.ts b/shared/DomainExtractor.spec.ts similarity index 54% rename from server/src/lib/utils/DomainExtractor.spec.ts rename to shared/DomainExtractor.spec.ts index c5910d88..9df525b5 100644 --- a/server/src/lib/utils/DomainExtractor.spec.ts +++ b/shared/DomainExtractor.spec.ts @@ -1,7 +1,7 @@ import { DomainExtractor } from "./DomainExtractor"; import Assert = require("assert"); -describe("utils/DomainExtractor", function () { +describe.only("shared/DomainExtractor", function () { describe("test fromUrl", function () { it("should return domain from https url", function () { const domain = DomainExtractor.fromUrl("https://www.example.com/test/abc"); @@ -17,5 +17,16 @@ describe("utils/DomainExtractor", function () { const domain = DomainExtractor.fromUrl("https://www.example.com:8080/test/abc"); Assert.equal(domain, "www.example.com"); }); + + it("should return domain when url contains redirect param", function () { + const domain0 = DomainExtractor.fromUrl("https://www.example.com:8080/test/abc?rd=https://cool.test.com"); + Assert.equal(domain0, "www.example.com"); + + const domain1 = DomainExtractor.fromUrl("https://login.example.com:8080/?rd=https://public.example.com:8080/"); + Assert.equal(domain1, "login.example.com"); + + const domain2 = DomainExtractor.fromUrl("https://single_factor.example.com:8080/secret.html"); + Assert.equal(domain2, "single_factor.example.com"); + }); }); }); \ No newline at end of file diff --git a/shared/DomainExtractor.ts b/shared/DomainExtractor.ts new file mode 100644 index 00000000..a84a77ec --- /dev/null +++ b/shared/DomainExtractor.ts @@ -0,0 +1,11 @@ +export class DomainExtractor { + static fromUrl(url: string): string { + if (!url) return; + const matches = url.match(/(https?:\/\/)?([a-zA-Z0-9_.-]+).*/); + + if (matches.length > 2) { + return matches[2]; + } + return; + } +} \ No newline at end of file diff --git a/shared/UserMessages.ts b/shared/UserMessages.ts index 40640b1d..ce83d23f 100644 --- a/shared/UserMessages.ts +++ b/shared/UserMessages.ts @@ -2,6 +2,8 @@ export const AUTHENTICATION_FAILED = "Authentication failed. Please check your credentials."; export const AUTHENTICATION_SUCCEEDED = "Authentication succeeded. You can now access your services."; +export const CANNOT_REDIRECT_TO_EXTERNAL_DOMAIN = "Cannot redirect to an external domain."; + export const AUTHENTICATION_U2F_FAILED = "Authentication failed. Have you already registered your device?"; export const AUTHENTICATION_TOTP_FAILED = "Authentication failed. Have you already registered your secret?"; diff --git a/test/complete-config/closed-redirection.ts b/test/complete-config/closed-redirection.ts new file mode 100644 index 00000000..c44721ec --- /dev/null +++ b/test/complete-config/closed-redirection.ts @@ -0,0 +1,41 @@ +import WithDriver from "../helpers/with-driver"; +import LoginAndRegisterTotp from "../helpers/login-and-register-totp"; +import SeeNotification from "../helpers/see-notification"; +import VisitPage from "../helpers/visit-page"; +import FillLoginPageWithUserAndPasswordAndClick from '../helpers/fill-login-page-and-click'; +import ValidateTotp from "../helpers/validate-totp"; +import {CANNOT_REDIRECT_TO_EXTERNAL_DOMAIN} from '../../shared/UserMessages'; + +/* + * Authelia should not be vulnerable to open redirection. Otherwise it would aid an + * attacker in conducting a phishing attack. + * + * To avoid the issue, Authelia's client scans the URL and prevent any redirection if + * the URL is pointing to an external domain. + */ +describe("Redirection should be performed only if in domain", function() { + this.timeout(10000); + WithDriver(); + + before(function() { + const that = this; + return LoginAndRegisterTotp(this.driver, "john", true) + .then((secret: string) => that.secret = secret) + }); + + function DoNotRedirect(url: string) { + it(`should see an error message instead of redirecting to ${url}`, function() { + const driver = this.driver; + const secret = this.secret; + return VisitPage(driver, `https://login.example.com:8080/?rd=${url}`) + .then(() => FillLoginPageWithUserAndPasswordAndClick(driver, 'john', 'password')) + .then(() => ValidateTotp(driver, secret)) + .then(() => SeeNotification(driver, "error", CANNOT_REDIRECT_TO_EXTERNAL_DOMAIN)) + .then(() => driver.get(`https://login.example.com:8080/logout`)); + }); + } + + DoNotRedirect("www.google.fr"); + DoNotRedirect("http://www.google.fr"); + DoNotRedirect("https://www.google.fr"); +}) \ No newline at end of file diff --git a/test/complete-config/mongo-broken-connection.ts b/test/complete-config/mongo-broken-connection.ts index e14c504b..c4635986 100644 --- a/test/complete-config/mongo-broken-connection.ts +++ b/test/complete-config/mongo-broken-connection.ts @@ -1,19 +1,17 @@ -require("chromedriver"); -import SeleniumWebdriver = require("selenium-webdriver"); import WithDriver from '../helpers/with-driver'; -import LoginAndRegisterTotp from '../helpers/login-and-register-totp'; -import LoginAs from '../helpers/login-as'; -import VisitPage from '../helpers/visit-page'; +import fullLogin from '../helpers/full-login'; +import loginAndRegisterTotp from '../helpers/login-and-register-totp'; -describe('Connection retry when mongo fails or restarts', function() { - this.timeout(20000); +describe("Connection retry when mongo fails or restarts", function() { + this.timeout(30000); WithDriver(); - it('should be able to login after mongo restarts', function() { + it("should be able to login after mongo restarts", function() { const that = this; - return that.environment.stop_service("mongo") - .then(() => that.environment.restart_service("authelia", 2000)) - .then(() => that.environment.restart_service("mongo")) - .then(() => LoginAs(that.driver, "john")); + let secret; + return loginAndRegisterTotp(that.driver, "john", true) + .then(_secret => secret = _secret) + .then(() => that.environment.restart_service("mongo", 1000)) + .then(() => fullLogin(that.driver, "https://admin.example.com:8080/secret.html", "john", secret)); }) }); diff --git a/test/environment.ts b/test/environment.ts index 2b11118d..593aa05a 100644 --- a/test/environment.ts +++ b/test/environment.ts @@ -15,7 +15,7 @@ export class Environment { private runCommand(command: string, timeout?: number): Bluebird { return new Bluebird((resolve, reject) => { console.log('[ENVIRONMENT] Running: %s', command); - exec(command, (err, stdout, stderr) => { + exec(command, (err: any, stdout: any, stderr: any) => { if(err) { reject(err); return; diff --git a/test/helpers/full-login.ts b/test/helpers/full-login.ts new file mode 100644 index 00000000..5cab80c8 --- /dev/null +++ b/test/helpers/full-login.ts @@ -0,0 +1,12 @@ +import VisitPage from "./visit-page"; +import FillLoginPageWithUserAndPasswordAndClick from "./fill-login-page-and-click"; +import ValidateTotp from "./validate-totp"; +import WaitRedirected from "./wait-redirected"; + +// Validate the two factors! +export default function(driver: any, url: string, user: string, secret: string) { + return VisitPage(driver, `https://login.example.com:8080/?rd=${url}`) + .then(() => FillLoginPageWithUserAndPasswordAndClick(driver, user, 'password')) + .then(() => ValidateTotp(driver, secret)) + .then(() => WaitRedirected(driver, "https://admin.example.com:8080/secret.html")); +} \ No newline at end of file diff --git a/test/helpers/login-and-register-totp.ts b/test/helpers/login-and-register-totp.ts index f4f99bf5..8837254e 100644 --- a/test/helpers/login-and-register-totp.ts +++ b/test/helpers/login-and-register-totp.ts @@ -1,10 +1,9 @@ -import VisitPage from "./visit-page"; -import FillLoginPageAndClick from './fill-login-page-and-click'; import RegisterTotp from './register-totp'; import WaitRedirected from './wait-redirected'; import LoginAs from './login-as'; +import Bluebird = require("bluebird"); -export default function(driver: any, user: string, email?: boolean) { +export default function(driver: any, user: string, email?: boolean): Bluebird { return LoginAs(driver, user) .then(() => WaitRedirected(driver, "https://login.example.com:8080/secondfactor")) .then(() => RegisterTotp(driver, email)); diff --git a/test/helpers/login-as.ts b/test/helpers/login-as.ts index 6a22699b..ebb6707f 100644 --- a/test/helpers/login-as.ts +++ b/test/helpers/login-as.ts @@ -1,7 +1,5 @@ import VisitPage from "./visit-page"; import FillLoginPageAndClick from './fill-login-page-and-click'; -import RegisterTotp from './register-totp'; -import WaitRedirected from './wait-redirected'; export default function(driver: any, user: string) { return VisitPage(driver, "https://login.example.com:8080/") diff --git a/test/minimal-config/bad_password.ts b/test/minimal-config/bad_password.ts index 9a1ff2d1..e629a28a 100644 --- a/test/minimal-config/bad_password.ts +++ b/test/minimal-config/bad_password.ts @@ -1,12 +1,8 @@ -import Bluebird = require("bluebird"); -import SeleniumWebdriver = require("selenium-webdriver"); -import Fs = require("fs"); -import Speakeasy = require("speakeasy"); import WithDriver from '../helpers/with-driver'; import FillLoginPageWithUserAndPasswordAndClick from '../helpers/fill-login-page-and-click'; -import WaitRedirected from '../helpers/wait-redirected'; import VisitPage from '../helpers/visit-page'; import SeeNotification from '../helpers/see-notification'; +import {AUTHENTICATION_FAILED} from '../../shared/UserMessages'; /** * When user provides bad password, @@ -28,7 +24,7 @@ describe("Provide bad password", function() { it('should get a notification message', function() { this.timeout(10000); - return SeeNotification(this.driver, "error", "Authentication failed. Please check your credentials."); + return SeeNotification(this.driver, "error", AUTHENTICATION_FAILED); }); }); }); diff --git a/test/minimal-config/fail_totp.ts b/test/minimal-config/fail_totp.ts index b6c5478e..b347ac47 100644 --- a/test/minimal-config/fail_totp.ts +++ b/test/minimal-config/fail_totp.ts @@ -1,17 +1,11 @@ require("chromedriver"); -import Bluebird = require("bluebird"); -import SeleniumWebdriver = require("selenium-webdriver"); -import Fs = require("fs"); -import Speakeasy = require("speakeasy"); import WithDriver from '../helpers/with-driver'; import FillLoginPageWithUserAndPasswordAndClick from '../helpers/fill-login-page-and-click'; -import WaitRedirected from '../helpers/wait-redirected'; import VisitPage from '../helpers/visit-page'; -import RegisterTotp from '../helpers/register-totp'; import ValidateTotp from '../helpers/validate-totp'; -import AccessSecret from "../helpers/access-secret"; import LoginAndRegisterTotp from '../helpers/login-and-register-totp'; import seeNotification from "../helpers/see-notification"; +import {AUTHENTICATION_TOTP_FAILED} from '../../shared/UserMessages'; /** * Given john has registered a TOTP secret, @@ -24,7 +18,6 @@ describe('Fail TOTP challenge', function() { describe('successfully login as john', function() { before(function() { - const that = this; return LoginAndRegisterTotp(this.driver, "john", true); }); @@ -39,7 +32,7 @@ describe('Fail TOTP challenge', function() { }); it("get a notification message", function() { - return seeNotification(this.driver, "error", "Authentication failed. Have you already registered your secret?"); + return seeNotification(this.driver, "error", AUTHENTICATION_TOTP_FAILED); }); }); }); diff --git a/test/minimal-config/reset_password.ts b/test/minimal-config/reset_password.ts index 880b40cb..3ddfeb04 100644 --- a/test/minimal-config/reset_password.ts +++ b/test/minimal-config/reset_password.ts @@ -1,7 +1,6 @@ require("chromedriver"); import Bluebird = require("bluebird"); import ChildProcess = require("child_process"); -import SeleniumWebdriver = require("selenium-webdriver"); import WithDriver from '../helpers/with-driver'; import VisitPage from '../helpers/visit-page'; diff --git a/test/minimal-config/validate_totp.ts b/test/minimal-config/validate_totp.ts index f1b0d382..4f1a2481 100644 --- a/test/minimal-config/validate_totp.ts +++ b/test/minimal-config/validate_totp.ts @@ -1,13 +1,9 @@ require("chromedriver"); import Bluebird = require("bluebird"); -import SeleniumWebdriver = require("selenium-webdriver"); -import Fs = require("fs"); -import Speakeasy = require("speakeasy"); import WithDriver from '../helpers/with-driver'; import FillLoginPageWithUserAndPasswordAndClick from '../helpers/fill-login-page-and-click'; import WaitRedirected from '../helpers/wait-redirected'; import VisitPage from '../helpers/visit-page'; -import RegisterTotp from '../helpers/register-totp'; import ValidateTotp from '../helpers/validate-totp'; import AccessSecret from "../helpers/access-secret"; import LoginAndRegisterTotp from '../helpers/login-and-register-totp'; @@ -37,15 +33,9 @@ describe('Validate TOTP factor', function() { const driver = this.driver; return VisitPage(driver, "https://login.example.com:8080/?rd=https://admin.example.com:8080/secret.html") - .then(() => { - return FillLoginPageWithUserAndPasswordAndClick(driver, 'john', 'password'); - }) - .then(() => { - return ValidateTotp(driver, secret); - }) - .then(() => { - return WaitRedirected(driver, "https://admin.example.com:8080/secret.html") - }); + .then(() => FillLoginPageWithUserAndPasswordAndClick(driver, 'john', 'password')) + .then(() => ValidateTotp(driver, secret)) + .then(() => WaitRedirected(driver, "https://admin.example.com:8080/secret.html")); }); it("should access the secret", function() {