From fe00750e39d718b8f991a2d6fd10cedddd4c1193 Mon Sep 17 00:00:00 2001
From: Alberto Bertogli <albertito@blitiri.com.ar>
Date: Thu, 5 Jan 2017 14:45:50 -0300
Subject: [PATCH] sts: Treat missing/empty "mx" list as invalid

The "mx" field is required, a policy without it is invalid, so add a
check for it.

See
https://mailarchive.ietf.org/arch/msg/uta/Omqo1Bw6rJbrTMl2Zo69IJr35Qo
for more background, in particular the following paragraph:

> The "mx" field is required, so if it is missing, the policy is invalid
> and should not be honored. (It doesn't make sense to honor the policy
> anyway, I would say, since a policy without allowed MXs is essentially a
> way of saying, "There should be TLS and the server identity should match
> the MX, whatever the MX is." I guess this prevents SSL stripping, but
> doesn't prevent DNS injection, so it's of relatively little value.)
---
 internal/sts/sts.go      | 8 +++++++-
 internal/sts/sts_test.go | 9 ++++++---
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/internal/sts/sts.go b/internal/sts/sts.go
index df78758..8d91c20 100644
--- a/internal/sts/sts.go
+++ b/internal/sts/sts.go
@@ -58,6 +58,7 @@ var (
 	ErrUnknownVersion = errors.New("unknown policy version")
 	ErrInvalidMaxAge  = errors.New("invalid max_age")
 	ErrInvalidMode    = errors.New("invalid mode")
+	ErrInvalidMX      = errors.New("invalid mx")
 )
 
 // Check that the policy contents are valid.
@@ -73,13 +74,18 @@ func (p *Policy) Check() error {
 		return ErrInvalidMode
 	}
 
+	// "mx" field is required, and the policy is invalid if it's not present.
+	// https://mailarchive.ietf.org/arch/msg/uta/Omqo1Bw6rJbrTMl2Zo69IJr35Qo
+	if len(p.MXs) == 0 {
+		return ErrInvalidMX
+	}
+
 	return nil
 }
 
 // MXMatches checks if the given MX is allowed, according to the policy.
 // https://tools.ietf.org/html/draft-ietf-uta-mta-sts-02#section-4.1
 func (p *Policy) MXIsAllowed(mx string) bool {
-	// TODO: Clarify how we should treat an empty MX list.
 	for _, pattern := range p.MXs {
 		if matchDomain(mx, pattern) {
 			return true
diff --git a/internal/sts/sts_test.go b/internal/sts/sts_test.go
index aab8b6b..bfd90e4 100644
--- a/internal/sts/sts_test.go
+++ b/internal/sts/sts_test.go
@@ -24,10 +24,10 @@ func TestParsePolicy(t *testing.T) {
 
 func TestCheckPolicy(t *testing.T) {
 	validPs := []Policy{
-		{Version: "STSv1", Mode: "enforce", MaxAge: 1 * time.Hour},
-		{Version: "STSv1", Mode: "report", MaxAge: 1 * time.Hour},
-		{Version: "STSv1", Mode: "report", MaxAge: 1 * time.Hour,
+		{Version: "STSv1", Mode: "enforce", MaxAge: 1 * time.Hour,
 			MXs: []string{"mx1", "mx2"}},
+		{Version: "STSv1", Mode: "report", MaxAge: 1 * time.Hour,
+			MXs: []string{"mx1"}},
 	}
 	for i, p := range validPs {
 		if err := p.Check(); err != nil {
@@ -42,6 +42,9 @@ func TestCheckPolicy(t *testing.T) {
 		{Policy{Version: "STSv2"}, ErrUnknownVersion},
 		{Policy{Version: "STSv1"}, ErrInvalidMaxAge},
 		{Policy{Version: "STSv1", MaxAge: 1, Mode: "blah"}, ErrInvalidMode},
+		{Policy{Version: "STSv1", MaxAge: 1, Mode: "enforce"}, ErrInvalidMX},
+		{Policy{Version: "STSv1", MaxAge: 1, Mode: "enforce", MXs: []string{}},
+			ErrInvalidMX},
 	}
 	for i, c := range invalid {
 		if err := c.p.Check(); err != c.expected {