From 3129c57bb8f81461f49855e4c5a7d4d06946a91f Mon Sep 17 00:00:00 2001 From: Roland Bracewell Shoemaker Date: Mon, 28 Jan 2019 17:05:58 -0800 Subject: [PATCH] Require email domains end in a IANA suffix (#4037) --- iana/iana.go | 32 ++++++++++++++++++++ iana/iana_test.go | 65 ++++++++++++++++++++++++++++++++++++++++ policy/pa.go | 31 ++----------------- policy/pa_test.go | 62 -------------------------------------- ra/ra.go | 4 +++ ra/ra_test.go | 12 ++++++++ test/integration-test.py | 2 +- 7 files changed, 117 insertions(+), 91 deletions(-) create mode 100644 iana/iana.go create mode 100644 iana/iana_test.go diff --git a/iana/iana.go b/iana/iana.go new file mode 100644 index 000000000..8e138e1db --- /dev/null +++ b/iana/iana.go @@ -0,0 +1,32 @@ +package iana + +import ( + "fmt" + + "github.com/weppos/publicsuffix-go/publicsuffix" +) + +// ExtractSuffix returns the public suffix of the domain using only the "ICANN" +// section of the Public Suffix List database. +// If the domain does not end in a suffix that belongs to an IANA-assigned +// domain, ExtractSuffix returns an error. +func ExtractSuffix(name string) (string, error) { + if name == "" { + return "", fmt.Errorf("Blank name argument passed to ExtractSuffix") + } + + rule := publicsuffix.DefaultList.Find(name, &publicsuffix.FindOptions{IgnorePrivate: true, DefaultRule: nil}) + if rule == nil { + return "", fmt.Errorf("Domain %s has no IANA TLD", name) + } + + suffix := rule.Decompose(name)[1] + + // If the TLD is empty, it means name is actually a suffix. + // In fact, decompose returns an array of empty strings in this case. + if suffix == "" { + suffix = name + } + + return suffix, nil +} diff --git a/iana/iana_test.go b/iana/iana_test.go new file mode 100644 index 000000000..214952abc --- /dev/null +++ b/iana/iana_test.go @@ -0,0 +1,65 @@ +package iana + +import "testing" + +func TestExtractSuffix_Valid(t *testing.T) { + testCases := []struct { + domain, want string + }{ + // TLD with only 1 rule. + {"biz", "biz"}, + {"domain.biz", "biz"}, + {"b.domain.biz", "biz"}, + + // The relevant {kobe,kyoto}.jp rules are: + // jp + // *.kobe.jp + // !city.kobe.jp + // kyoto.jp + // ide.kyoto.jp + {"jp", "jp"}, + {"kobe.jp", "jp"}, + {"c.kobe.jp", "c.kobe.jp"}, + {"b.c.kobe.jp", "c.kobe.jp"}, + {"a.b.c.kobe.jp", "c.kobe.jp"}, + {"city.kobe.jp", "kobe.jp"}, + {"www.city.kobe.jp", "kobe.jp"}, + {"kyoto.jp", "kyoto.jp"}, + {"test.kyoto.jp", "kyoto.jp"}, + {"ide.kyoto.jp", "ide.kyoto.jp"}, + {"b.ide.kyoto.jp", "ide.kyoto.jp"}, + {"a.b.ide.kyoto.jp", "ide.kyoto.jp"}, + + // Domain with a private public suffix should return the ICANN public suffix. + {"foo.compute-1.amazonaws.com", "com"}, + // Domain equal to a private public suffix should return the ICANN public + // suffix. + {"cloudapp.net", "net"}, + } + + for _, tc := range testCases { + got, err := ExtractSuffix(tc.domain) + if err != nil { + t.Errorf("%q: returned error", tc.domain) + continue + } + if got != tc.want { + t.Errorf("%q: got %q, want %q", tc.domain, got, tc.want) + } + } +} + +func TestExtractSuffix_Invalid(t *testing.T) { + testCases := []string{ + "", + "example", + "example.example", + } + + for _, tc := range testCases { + _, err := ExtractSuffix(tc) + if err == nil { + t.Errorf("%q: expected err, got none", tc) + } + } +} diff --git a/policy/pa.go b/policy/pa.go index 7f7aecbb0..15776a70b 100644 --- a/policy/pa.go +++ b/policy/pa.go @@ -11,13 +11,13 @@ import ( "strings" "sync" - "github.com/weppos/publicsuffix-go/publicsuffix" "golang.org/x/net/idna" "golang.org/x/text/unicode/norm" "github.com/letsencrypt/boulder/core" berrors "github.com/letsencrypt/boulder/errors" "github.com/letsencrypt/boulder/features" + "github.com/letsencrypt/boulder/iana" blog "github.com/letsencrypt/boulder/log" "github.com/letsencrypt/boulder/reloader" ) @@ -289,7 +289,7 @@ func (pa *AuthorityImpl) WillingToIssue(id core.AcmeIdentifier) error { } // Names must end in an ICANN TLD, but they must not be equal to an ICANN TLD. - icannTLD, err := extractDomainIANASuffix(domain) + icannTLD, err := iana.ExtractSuffix(domain) if err != nil { return errNonPublic } @@ -342,7 +342,7 @@ func (pa *AuthorityImpl) WillingToIssueWildcard(ident core.AcmeIdentifier) error // The base domain is the wildcard request with the `*.` prefix removed baseDomain := strings.TrimPrefix(rawDomain, "*.") // Names must end in an ICANN TLD, but they must not be equal to an ICANN TLD. - icannTLD, err := extractDomainIANASuffix(baseDomain) + icannTLD, err := iana.ExtractSuffix(baseDomain) if err != nil { return errNonPublic } @@ -481,31 +481,6 @@ func (pa *AuthorityImpl) ChallengesFor(identifier core.AcmeIdentifier, regID int return shuffled, shuffledCombos, nil } -// ExtractDomainIANASuffix returns the public suffix of the domain using only the "ICANN" -// section of the Public Suffix List database. -// If the domain does not end in a suffix that belongs to an IANA-assigned -// domain, ExtractDomainIANASuffix returns an error. -func extractDomainIANASuffix(name string) (string, error) { - if name == "" { - return "", fmt.Errorf("Blank name argument passed to ExtractDomainIANASuffix") - } - - rule := publicsuffix.DefaultList.Find(name, &publicsuffix.FindOptions{IgnorePrivate: true, DefaultRule: nil}) - if rule == nil { - return "", fmt.Errorf("Domain %s has no IANA TLD", name) - } - - suffix := rule.Decompose(name)[1] - - // If the TLD is empty, it means name is actually a suffix. - // In fact, decompose returns an array of empty strings in this case. - if suffix == "" { - suffix = name - } - - return suffix, nil -} - // ChallengeTypeEnabled returns whether the specified challenge type is enabled func (pa *AuthorityImpl) ChallengeTypeEnabled(t string, regID int64) bool { pa.blacklistMu.RLock() diff --git a/policy/pa_test.go b/policy/pa_test.go index 05b34cdbb..43c022c13 100644 --- a/policy/pa_test.go +++ b/policy/pa_test.go @@ -401,68 +401,6 @@ func TestChallengesForWildcard(t *testing.T) { test.AssertEquals(t, challenges[0].Type, core.ChallengeTypeDNS01) } -func TestExtractDomainIANASuffix_Valid(t *testing.T) { - testCases := []struct { - domain, want string - }{ - // TLD with only 1 rule. - {"biz", "biz"}, - {"domain.biz", "biz"}, - {"b.domain.biz", "biz"}, - - // The relevant {kobe,kyoto}.jp rules are: - // jp - // *.kobe.jp - // !city.kobe.jp - // kyoto.jp - // ide.kyoto.jp - {"jp", "jp"}, - {"kobe.jp", "jp"}, - {"c.kobe.jp", "c.kobe.jp"}, - {"b.c.kobe.jp", "c.kobe.jp"}, - {"a.b.c.kobe.jp", "c.kobe.jp"}, - {"city.kobe.jp", "kobe.jp"}, - {"www.city.kobe.jp", "kobe.jp"}, - {"kyoto.jp", "kyoto.jp"}, - {"test.kyoto.jp", "kyoto.jp"}, - {"ide.kyoto.jp", "ide.kyoto.jp"}, - {"b.ide.kyoto.jp", "ide.kyoto.jp"}, - {"a.b.ide.kyoto.jp", "ide.kyoto.jp"}, - - // Domain with a private public suffix should return the ICANN public suffix. - {"foo.compute-1.amazonaws.com", "com"}, - // Domain equal to a private public suffix should return the ICANN public - // suffix. - {"cloudapp.net", "net"}, - } - - for _, tc := range testCases { - got, err := extractDomainIANASuffix(tc.domain) - if err != nil { - t.Errorf("%q: returned error", tc.domain) - continue - } - if got != tc.want { - t.Errorf("%q: got %q, want %q", tc.domain, got, tc.want) - } - } -} - -func TestExtractDomainIANASuffix_Invalid(t *testing.T) { - testCases := []string{ - "", - "example", - "example.example", - } - - for _, tc := range testCases { - _, err := extractDomainIANASuffix(tc) - if err == nil { - t.Errorf("%q: expected err, got none", tc) - } - } -} - // TestMalformedExactBlacklist tests that loading a JSON policy file with an // invalid exact blacklist entry will fail as expected. func TestMalformedExactBlacklist(t *testing.T) { diff --git a/ra/ra.go b/ra/ra.go index 420b80789..ca1601dcc 100644 --- a/ra/ra.go +++ b/ra/ra.go @@ -23,6 +23,7 @@ import ( "github.com/letsencrypt/boulder/features" "github.com/letsencrypt/boulder/goodkey" bgrpc "github.com/letsencrypt/boulder/grpc" + "github.com/letsencrypt/boulder/iana" blog "github.com/letsencrypt/boulder/log" "github.com/letsencrypt/boulder/metrics" "github.com/letsencrypt/boulder/probs" @@ -385,6 +386,9 @@ func validateEmail(address string) error { "invalid contact domain. Contact emails @%s are forbidden", domain) } + if _, err := iana.ExtractSuffix(domain); err != nil { + return berrors.InvalidEmailError("email domain name does not end in a IANA suffix") + } return nil } diff --git a/ra/ra_test.go b/ra/ra_test.go index 6d607d6c0..fc776f840 100644 --- a/ra/ra_test.go +++ b/ra/ra_test.go @@ -365,6 +365,18 @@ func TestValidateContacts(t *testing.T) { err = ra.validateContacts(context.Background(), &[]string{forbidden}) test.AssertError(t, err, "Forbidden email") + + err = ra.validateContacts(context.Background(), &[]string{"mailto:admin@localhost"}) + test.AssertError(t, err, "Forbidden email") + + err = ra.validateContacts(context.Background(), &[]string{"mailto:admin@example.not.a.iana.suffix"}) + test.AssertError(t, err, "Forbidden email") + + err = ra.validateContacts(context.Background(), &[]string{"mailto:admin@1.2.3.4"}) + test.AssertError(t, err, "Forbidden email") + + err = ra.validateContacts(context.Background(), &[]string{"mailto:admin@[1.2.3.4]"}) + test.AssertError(t, err, "Forbidden email") } func TestNewRegistration(t *testing.T) { diff --git a/test/integration-test.py b/test/integration-test.py index 8e05ce9a9..e3f86a21f 100644 --- a/test/integration-test.py +++ b/test/integration-test.py @@ -418,7 +418,7 @@ def random_domain(): return "rand.%x.xyz" % random.randrange(2**32) def test_expiration_mailer(): - email_addr = "integration.%x@boulder" % random.randrange(2**16) + email_addr = "integration.%x@letsencrypt.org" % random.randrange(2**16) cert, _ = auth_and_issue([random_domain()], email=email_addr) # Check that the expiration mailer sends a reminder expiry = datetime.datetime.strptime(cert.body.get_notAfter(), '%Y%m%d%H%M%SZ')