From 57e1ad1d06dd5808aa23f2c807f76a4183238d5d Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Wed, 20 Nov 2024 00:02:55 -0300 Subject: [PATCH] Custom domain: check CNAME when adding domains (#11747) If the user owns the domain, they can delete the CNAME before adding the domain to their project. Hashed domains won't be needed anymore to prevent domain hijacking (but still be something we should do), since users trying to add a domain with an old CNAME will need to delete that CNAME first, and only domain owners can do that. It may be a little of a pain for some users that first add the CNAME before creating the domain on .org, but that's only on .org, since on .com the users doesn't know the CNAME before creating the domain. Ref https://github.com/readthedocs/meta/discussions/157 --- docs/user/guides/custom-domains.rst | 19 ++++++ readthedocs/projects/forms.py | 67 +++++++++++++++++++ .../projects/tests/test_domain_views.py | 64 ++++++++++++++++++ requirements/deploy.txt | 2 + requirements/docker.txt | 2 + requirements/pip.in | 2 + requirements/pip.txt | 2 + requirements/testing.txt | 2 + 8 files changed, 160 insertions(+) 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