Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Add a config option for validating 'next_link' parameters against a domain whitelist #8275

Merged
merged 12 commits into from
Sep 8, 2020
1 change: 1 addition & 0 deletions changelog.d/8275.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add a config option to specify a whitelist of domains that a user can be redirected to after validating their email or phone number.
18 changes: 18 additions & 0 deletions docs/sample_config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -432,6 +432,24 @@ retention:
#
#request_token_inhibit_3pid_errors: true

# A list of domains that the domain portion of 'next_link' parameters
# must match.
#
# This parameter is optionally provided by clients while requesting
# validation of an email or phone number, and maps to a link that
# users will be automatically redirected to after validation
# succeeds. Clients can make use this parameter to aid the validation
# process.
#
# The whitelist is applied whether the homeserver or an
# identity server is handling validation.
#
# The default value is no whitelist functionality; all domains are
# allowed. Setting this value to an empty list will instead disallow
# all domains.
#
#next_link_domain_whitelist: ["matrix.org"]


## TLS ##

Expand Down
33 changes: 32 additions & 1 deletion synapse/config/server.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
import os.path
import re
from textwrap import indent
from typing import Any, Dict, Iterable, List, Optional
from typing import Any, Dict, Iterable, List, Optional, Set

import attr
import yaml
Expand Down Expand Up @@ -542,6 +542,19 @@ class LimitRemoteRoomsConfig:
users_new_default_push_rules
) # type: set

# Whitelist of domain names that given next_link parameters must have
next_link_domain_whitelist = config.get(
"next_link_domain_whitelist"
) # type: Optional[List[str]]

self.next_link_domain_whitelist = None # type: Optional[Set[str]]
if next_link_domain_whitelist is not None:
if not isinstance(next_link_domain_whitelist, list):
raise ConfigError("'next_link_domain_whitelist' must be a list")

# Turn the list into a set to improve lookup speed.
self.next_link_domain_whitelist = set(next_link_domain_whitelist)

def has_tls_listener(self) -> bool:
return any(listener.tls for listener in self.listeners)

Expand Down Expand Up @@ -1014,6 +1027,24 @@ def generate_config_section(
# act as if no error happened and return a fake session ID ('sid') to clients.
#
#request_token_inhibit_3pid_errors: true

# A list of domains that the domain portion of 'next_link' parameters
# must match.
#
# This parameter is optionally provided by clients while requesting
# validation of an email or phone number, and maps to a link that
# users will be automatically redirected to after validation
# succeeds. Clients can make use this parameter to aid the validation
# process.
#
# The whitelist is applied whether the homeserver or an
# identity server is handling validation.
#
# The default value is no whitelist functionality; all domains are
# allowed. Setting this value to an empty list will instead disallow
# all domains.
#
#next_link_domain_whitelist: ["matrix.org"]
"""
% locals()
)
Expand Down
66 changes: 57 additions & 9 deletions synapse/rest/client/v2_alpha/account.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,11 @@
import logging
import random
from http import HTTPStatus
from typing import TYPE_CHECKING
from urllib.parse import urlparse

if TYPE_CHECKING:
from synapse.app.homeserver import HomeServer

from synapse.api.constants import LoginType
from synapse.api.errors import (
Expand Down Expand Up @@ -98,6 +103,9 @@ async def on_POST(self, request):
Codes.THREEPID_DENIED,
)

# Raise if the provided next_link value isn't valid
assert_valid_next_link(self.hs, next_link)

# The email will be sent to the stored address.
# This avoids a potential account hijack by requesting a password reset to
# an email address which is controlled by the attacker but which, after
Expand Down Expand Up @@ -446,6 +454,9 @@ async def on_POST(self, request):
Codes.THREEPID_DENIED,
)

# Raise if the provided next_link value isn't valid
assert_valid_next_link(self.hs, next_link)

existing_user_id = await self.store.get_user_id_by_threepid("email", email)

if existing_user_id is not None:
Expand Down Expand Up @@ -517,6 +528,9 @@ async def on_POST(self, request):
Codes.THREEPID_DENIED,
)

# Raise if the provided next_link value isn't valid
assert_valid_next_link(self.hs, next_link)

existing_user_id = await self.store.get_user_id_by_threepid("msisdn", msisdn)

if existing_user_id is not None:
Expand Down Expand Up @@ -603,15 +617,10 @@ async def on_GET(self, request):

# Perform a 302 redirect if next_link is set
if next_link:
if next_link.startswith("file:///"):
logger.warning(
"Not redirecting to next_link as it is a local file: address"
)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check now happens implicitly in assert_valid_next_link, as only http and https next_link values are allowed.

Additionally, next_link can only be specified during the call to /requestToken. Having this check in /submit_token, blocking the user after already approving their /requestToken call, is not great UX.

else:
request.setResponseCode(302)
request.setHeader("Location", next_link)
finish_request(request)
return None
request.setResponseCode(302)
request.setHeader("Location", next_link)
finish_request(request)
return None

# Otherwise show the success template
html = self.config.email_add_threepid_template_success_html_content
Expand Down Expand Up @@ -875,6 +884,45 @@ async def on_POST(self, request):
return 200, {"id_server_unbind_result": id_server_unbind_result}


def assert_valid_next_link(hs: "HomeServer", next_link: str):
"""
Raises a SynapseError if a given next_link value is invalid

next_link is valid if the scheme is http(s) and the next_link.domain_whitelist config
option is either empty or contains a domain that matches the one in the given next_link

Args:
hs: The homeserver object
next_link: The next_link value given by the client

Raises:
SynapseError: If the next_link is invalid
"""
valid = True

# Parse the contents of the URL
next_link_parsed = urlparse(next_link)

# Scheme must not point to the local drive
if next_link_parsed.scheme == "file":
valid = False
anoadragon453 marked this conversation as resolved.
Show resolved Hide resolved

# If the domain whitelist is set, the domain must be in it
if (
valid
and hs.config.next_link_domain_whitelist is not None
and next_link_parsed.hostname not in hs.config.next_link_domain_whitelist
):
valid = False

if not valid:
raise SynapseError(
400,
"'next_link' domain not included in whitelist, or not http(s)",
errcode=Codes.INVALID_PARAM,
)


class WhoamiRestServlet(RestServlet):
PATTERNS = client_patterns("/account/whoami$")

Expand Down
103 changes: 96 additions & 7 deletions tests/rest/client/v2_alpha/test_account.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,11 @@
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

import json
import os
import re
from email.parser import Parser
from typing import Optional

import pkg_resources

Expand All @@ -29,6 +29,7 @@
from synapse.rest.client.v2_alpha import account, register

from tests import unittest
from tests.unittest import override_config


class PasswordResetTestCase(unittest.HomeserverTestCase):
Expand Down Expand Up @@ -668,16 +669,104 @@ def test_no_valid_token(self):
self.assertEqual(200, int(channel.result["code"]), msg=channel.result["body"])
self.assertFalse(channel.json_body["threepids"])

def _request_token(self, email, client_secret):
@override_config({"next_link_domain_whitelist": None})
def test_next_link(self):
"""Tests a valid next_link parameter value with no whitelist (good case)"""
self._request_token(
"[email protected]",
"some_secret",
next_link="https://example.com/a/good/site",
expect_code=200,
)

@override_config({"next_link_domain_whitelist": None})
def test_next_link_exotic_protocol(self):
"""Tests using a esoteric protocol as a next_link parameter value.
Someone may be hosting a client on IPFS etc.
"""
self._request_token(
"[email protected]",
"some_secret",
next_link="some-protocol://abcdefghijklmopqrstuvwxyz",
expect_code=200,
)

@override_config({"next_link_domain_whitelist": None})
def test_next_link_file_uri(self):
"""Tests next_link parameters cannot be file URI"""
# Attempt to use a next_link value that points to the local disk
self._request_token(
"[email protected]",
"some_secret",
next_link="file:///host/path",
expect_code=400,
)

@override_config({"next_link_domain_whitelist": ["example.com", "example.org"]})
def test_next_link_domain_whitelist(self):
"""Tests next_link parameters must fit the whitelist if provided"""
self._request_token(
"[email protected]",
"some_secret",
next_link="https://example.com/some/good/page",
expect_code=200,
)

self._request_token(
"[email protected]",
"some_secret",
next_link="https://example.org/some/also/good/page",
expect_code=200,
)

self._request_token(
"[email protected]",
"some_secret",
next_link="https://bad.example.org/some/bad/page",
expect_code=400,
)

@override_config({"next_link_domain_whitelist": []})
anoadragon453 marked this conversation as resolved.
Show resolved Hide resolved
def test_empty_next_link_domain_whitelist(self):
"""Tests an empty next_lint_domain_whitelist value, meaning next_link is essentially
disallowed
"""
self._request_token(
"[email protected]",
"some_secret",
next_link="https://example.com/a/page",
expect_code=400,
)

def _request_token(
self,
email: str,
client_secret: str,
next_link: Optional[str] = None,
expect_code: int = 200,
) -> str:
"""Request a validation token to add an email address to a user's account

Args:
email: The email address to validate
client_secret: A secret string
next_link: A link to redirect the user to after validation
expect_code: Expected return code of the call

Returns:
The ID of the new threepid validation session
"""
body = {"client_secret": client_secret, "email": email, "send_attempt": 1}
if next_link:
body["next_link"] = next_link

request, channel = self.make_request(
"POST",
b"account/3pid/email/requestToken",
{"client_secret": client_secret, "email": email, "send_attempt": 1},
"POST", b"account/3pid/email/requestToken", body,
)
self.render(request)
self.assertEquals(200, channel.code, channel.result)
self.assertEquals(expect_code, channel.code, channel.result)

return channel.json_body["sid"]
return channel.json_body.get("sid")

def _request_token_invalid_email(
self, email, expected_errcode, expected_error, client_secret="foobar",
Expand Down