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 readthedocs/meta#157

Author: stsewd

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
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 

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

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):

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

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

requirements/pip.in

@@ -46,6 +46,8 @@ slumber
 pyyaml
 Pygments
 
+dnspython
+
 # Used for Redis cache Django backend (`django.core.cache.backends.redis.RedisCache`)
 redis
 

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

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