git » chasquid » commit fe00750

sts: Treat missing/empty "mx" list as invalid

author Alberto Bertogli
2017-01-05 17:45:50 UTC
committer Alberto Bertogli
2017-02-28 22:27:15 UTC
parent 933ab54cd8dd09e9fa988ab3a2f9002cc4efa87a
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 +7 -1
internal/sts/sts_test.go +6 -3 
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 {