diff --git a/docs/user/guides/custom-domains.rst b/docs/user/guides/custom-domains.rst index d14ba329b6e..450f1f0870d 100644 --- a/docs/user/guides/custom-domains.rst +++ b/docs/user/guides/custom-domains.rst @@ -112,6 +112,25 @@ this can cause a delay in validating because there is an exponential back-off in Loading the domain details in the Read the Docs dashboard and saving the domain again will force a revalidation. +Disallowed DNS configurations +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +In order to prevent some common cases of domain hijacking, we disallow some DNS configurations: + +- CNAME records pointing to another CNAME record + (``doc.example.com -> docs.example.com -> readthedocs.io``). +- CNAME records pointing to the APEX domain + (``www.example.com -> example.com -> readthedocs.io``). + +This prevents attackers from taking over unused domains with CNAME records pointing to domains that are on Read the Docs. + +.. warning:: + + Users shouldn't rely on these restrictions to prevent domain hijacking. + We recommend regularly reviewing your DNS records, + removing any that are no longer needed or that don't exist on Read the Docs, + or registering all valid domains in your project. + The validation process period has ended ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ diff --git a/readthedocs/projects/forms.py b/readthedocs/projects/forms.py index ca2c7b24455..e7547a7bf16 100644 --- a/readthedocs/projects/forms.py +++ b/readthedocs/projects/forms.py @@ -6,6 +6,8 @@ from re import fullmatch from urllib.parse import urlparse +import dns.name +import dns.resolver import pytz from allauth.socialaccount.models import SocialAccount from django import forms @@ -1036,8 +1038,73 @@ def clean_domain(self): if invalid_domain and domain_string.endswith(invalid_domain): raise forms.ValidationError(f"{invalid_domain} is not a valid domain.") + self._check_for_suspicious_cname(domain_string) + return domain_string + def _check_for_suspicious_cname(self, domain): + """ + Check if a domain has a suspicious CNAME record. + + The domain is suspicious if: + + - Has a CNAME pointing to another CNAME. + This prevents the subdomain from being hijacked if the last subdomain is on RTD, + but the user didn't register the other subdomain. + Example: doc.example.com -> docs.example.com -> readthedocs.io, + We don't want to allow doc.example.com to be added. + - Has a CNAME pointing to the APEX domain. + This prevents a subdomain from being hijacked if the APEX domain is on RTD. + A common case is `www` pointing to the APEX domain, but users didn't register the + `www` subdomain, only the APEX domain. + Example: www.example.com -> example.com -> readthedocs.io, + we don't want to allow www.example.com to be added. + """ + cname = self._get_cname(domain) + # Doesn't have a CNAME record, we are good. + if not cname: + return + + # If the domain has a CNAME pointing to the APEX domain, that's not good. + # This check isn't perfect, but it's a good enoug heuristic + # to dectect CNAMES like www.example.com -> example.com. + if f"{domain}.".endswith(f".{cname}"): + raise forms.ValidationError( + _( + "This domain has a CNAME record pointing to the APEX domain. " + "Please remove the CNAME before adding the domain.", + ), + ) + + second_cname = self._get_cname(cname) + # The domain has a CNAME pointing to another CNAME, That's not good. + if second_cname: + raise forms.ValidationError( + _( + "This domain has a CNAME record pointing to another CNAME. " + "Please remove the CNAME before adding the domain.", + ), + ) + + def _get_cname(self, domain): + try: + answers = dns.resolver.resolve(domain, "CNAME", lifetime=1) + # dnspython doesn't recursively resolve CNAME records. + # We always have one response or none. + return str(answers[0].target) + except (dns.resolver.NoAnswer, dns.resolver.NXDOMAIN): + return None + except dns.resolver.LifetimeTimeout: + raise forms.ValidationError( + _( + "DNS resolution timed out. Make sure the domain is correct, or try again later." + ), + ) + except dns.name.EmptyLabel: + raise forms.ValidationError( + _("The domain is not valid."), + ) + def clean_canonical(self): canonical = self.cleaned_data["canonical"] pk = self.instance.pk diff --git a/readthedocs/projects/tests/test_domain_views.py b/readthedocs/projects/tests/test_domain_views.py index 10ed1de63d4..822d5f6539f 100644 --- a/readthedocs/projects/tests/test_domain_views.py +++ b/readthedocs/projects/tests/test_domain_views.py @@ -1,3 +1,6 @@ +from unittest import mock + +import dns.resolver from django.contrib.auth.models import User from django.contrib.messages import get_messages from django.test import TestCase, override_settings @@ -109,6 +112,67 @@ def test_edit_domain_on_subproject(self): self.assertEqual(domain.domain, "test.example.com") self.assertEqual(domain.canonical, False) + @mock.patch("readthedocs.projects.forms.dns.resolver.resolve") + def test_create_domain_with_chained_cname_record(self, dns_resolve_mock): + dns_resolve_mock.side_effect = [ + [mock.MagicMock(target="docs.example.com.")], + [mock.MagicMock(target="readthedocs.io.")], + ] + resp = self.client.post( + reverse("projects_domains_create", args=[self.project.slug]), + data={"domain": "docs2.example.com"}, + ) + assert resp.status_code == 200 + form = resp.context_data["form"] + assert not form.is_valid() + assert ( + "This domain has a CNAME record pointing to another CNAME" + in form.errors["domain"][0] + ) + + @mock.patch("readthedocs.projects.forms.dns.resolver.resolve") + def test_create_domain_with_cname_record_to_apex_domain(self, dns_resolve_mock): + dns_resolve_mock.side_effect = [ + [mock.MagicMock(target="example.com.")], + ] + resp = self.client.post( + reverse("projects_domains_create", args=[self.project.slug]), + data={"domain": "www.example.com"}, + ) + assert resp.status_code == 200 + form = resp.context_data["form"] + assert not form.is_valid() + assert ( + "This domain has a CNAME record pointing to the APEX domain" + in form.errors["domain"][0] + ) + + @mock.patch("readthedocs.projects.forms.dns.resolver.resolve") + def test_create_domain_cname_timeout(self, dns_resolve_mock): + dns_resolve_mock.side_effect = dns.resolver.LifetimeTimeout + resp = self.client.post( + reverse("projects_domains_create", args=[self.project.slug]), + data={"domain": "docs.example.com"}, + ) + assert resp.status_code == 200 + form = resp.context_data["form"] + assert not form.is_valid() + assert "DNS resolution timed out" in form.errors["domain"][0] + + @mock.patch("readthedocs.projects.forms.dns.resolver.resolve") + def test_create_domain_with_single_cname(self, dns_resolve_mock): + dns_resolve_mock.side_effect = [ + [mock.MagicMock(target="readthedocs.io.")], + dns.resolver.NoAnswer, + ] + resp = self.client.post( + reverse("projects_domains_create", args=[self.project.slug]), + data={"domain": "docs.example.com"}, + ) + assert resp.status_code == 302 + domain = self.project.domains.first() + assert domain.domain == "docs.example.com" + @override_settings(RTD_ALLOW_ORGANIZATIONS=True) class TestDomainViewsWithOrganizations(TestDomainViews): diff --git a/requirements/deploy.txt b/requirements/deploy.txt index 70b00886fd7..408db1ffc18 100644 --- a/requirements/deploy.txt +++ b/requirements/deploy.txt @@ -181,6 +181,8 @@ djangorestframework-api-key==3.0.0 # via -r requirements/pip.txt djangorestframework-jsonp==1.0.2 # via -r requirements/pip.txt +dnspython==2.7.0 + # via -r requirements/pip.txt docker==6.1.2 # via -r requirements/pip.txt docutils==0.21.2 diff --git a/requirements/docker.txt b/requirements/docker.txt index 2ffd3c9035f..a11a6cfa1c8 100644 --- a/requirements/docker.txt +++ b/requirements/docker.txt @@ -191,6 +191,8 @@ djangorestframework-api-key==3.0.0 # via -r requirements/pip.txt djangorestframework-jsonp==1.0.2 # via -r requirements/pip.txt +dnspython==2.7.0 + # via -r requirements/pip.txt docker==6.1.2 # via -r requirements/pip.txt docutils==0.21.2 diff --git a/requirements/pip.in b/requirements/pip.in index dc637d8b8d0..6813aeb4975 100644 --- a/requirements/pip.in +++ b/requirements/pip.in @@ -46,6 +46,8 @@ slumber pyyaml Pygments +dnspython + # Used for Redis cache Django backend (`django.core.cache.backends.redis.RedisCache`) redis diff --git a/requirements/pip.txt b/requirements/pip.txt index ded646e2a8b..cd86f2aff50 100644 --- a/requirements/pip.txt +++ b/requirements/pip.txt @@ -143,6 +143,8 @@ djangorestframework-api-key==3.0.0 # via -r requirements/pip.in djangorestframework-jsonp==1.0.2 # via -r requirements/pip.in +dnspython==2.7.0 + # via -r requirements/pip.in docker==6.1.2 # via -r requirements/pip.in docutils==0.21.2 diff --git a/requirements/testing.txt b/requirements/testing.txt index b108fdaa3a3..4ff3e486734 100644 --- a/requirements/testing.txt +++ b/requirements/testing.txt @@ -188,6 +188,8 @@ djangorestframework-api-key==3.0.0 # via -r requirements/pip.txt djangorestframework-jsonp==1.0.2 # via -r requirements/pip.txt +dnspython==2.7.0 + # via -r requirements/pip.txt docker==6.1.2 # via -r requirements/pip.txt docutils==0.21.2