Check MX record hostname validity (#358)

* improve: check MX record hostname validity

* chore: update changelog

* fix: invalid mx record example.com makes tests fail

* improve: polish mx record host check

* improve: LookupHost must return no error AND one or more addresses to be valid

* improve: do return a 400 when domain is invalid

CHANGELOG.md

@@ -6,6 +6,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## Unreleased
 ### Fixed
+- Invalid hostname specified in MX record bypasses e-mail address revalidation
 - Background revocation tasks not stopped when closing an `irmaclient`
 
 ### Internal

server/keyshare/email.go

@@ -150,21 +150,34 @@ func VerifyMXRecord(email string) error {
 
 	host := email[strings.LastIndex(email, "@")+1:]
 
-	if records, err := net.LookupMX(host); err != nil || len(records) == 0 {
-		if err != nil {
-			if derr, ok := err.(*net.DNSError); ok && (derr.IsTemporary || derr.IsTimeout) {
-				// When DNS is not resolving or there is no active network connection
-				server.Logger.WithField("error", err).Error("No active network connection")
-				return ErrNoNetwork
-			}
+	records, err := net.LookupMX(host)
+
+	if err != nil || len(records) == 0 {
+		if derr, ok := err.(*net.DNSError); ok && (derr.IsTemporary || derr.IsTimeout) {
+			// When DNS is not resolving or there is no active network connection
+			server.Logger.WithField("error", err).Error("No active network connection")
+			return ErrNoNetwork
 		}
 
 		// Check if there is a valid A or AAAA record which is used as fallback by mailservers
 		// when there are no MX records present
 		if records, err := net.LookupIP(host); err != nil || len(records) == 0 {
 			return ErrInvalidEmailDomain
 		}
-		return nil
 	}
+
+	hasValidHost := false
+	for _, h := range records {
+		// Check if host specified at MX record is valid
+		if addr, err := net.LookupHost(h.Host); err == nil && len(addr) > 0 {
+			hasValidHost = true
+			break
+		}
+	}
+
+	if !hasValidHost {
+		return ErrInvalidEmailDomain
+	}
+
 	return nil
 }

server/keyshare/keyshareserver/server_email_test.go

@@ -14,7 +14,7 @@ func TestServerRegistrationWithEmail(t *testing.T) {
 	defer StopKeyshareServer(t, keyshareServer, httpServer)
 
 	test.HTTPPost(t, nil, "http://localhost:8080/client/register",
-		`{"pin":"testpin","email":"test@example.com","language":"en"}`, nil,
+		`{"pin":"testpin","email":"test@github.com","language":"en"}`, nil,
 		200, nil,
 	)
 	test.HTTPPost(t, nil, "http://localhost:8080/client/register",
@@ -26,7 +26,7 @@ func TestServerRegistrationWithEmail(t *testing.T) {
 	// rejecting the registration would be too severe. So the registration is accepted and the
 	// server falls back to its default language.
 	test.HTTPPost(t, nil, "http://localhost:8080/client/register",
-		`{"pin":"testpin","email":"test@example.com","language":"nonexistinglanguage"}`, nil,
+		`{"pin":"testpin","email":"test@github.com","language":"nonexistinglanguage"}`, nil,
 		200, nil,
 	)
 

server/keyshare/myirmaserver/server.go

@@ -277,7 +277,7 @@ func (s *Server) sendLoginEmail(ctx context.Context, request emailLoginRequest)
 	}
 
 	if err := keyshare.VerifyMXRecord(request.Email); err != nil {
-		return keyshare.ErrInvalidEmail
+		return keyshare.ErrInvalidEmailDomain
 	}
 
 	token := common.NewSessionToken()

server/keyshare/myirmaserver/server_email_test.go

@@ -16,15 +16,15 @@ func TestServerLoginEmail(t *testing.T) {
 			"testuser": {
 				id:         15,
 				lastActive: time.Unix(0, 0),
-				email:      []string{"test@example.com"},
+				email:      []string{"test@github.com"},
 			},
 			"noemail": {
 				id:         17,
 				lastActive: time.Unix(0, 0),
 			},
 		},
 		loginEmailTokens: map[string]string{
-			"testtoken": "test@example.com",
+			"testtoken": "test@github.com",
 		},
 		verifyEmailTokens: map[string]int64{
 			"testemailtoken": 15,
@@ -33,24 +33,24 @@ func TestServerLoginEmail(t *testing.T) {
 	myirmaServer, httpServer := StartMyIrmaServer(t, db, "localhost:1025")
 	defer StopMyIrmaServer(t, myirmaServer, httpServer)
 
-	test.HTTPPost(t, nil, "http://localhost:8081/login/email", `{"email": "nonexistinglanguage", "language": "en"}`, nil, 400, nil)
+	test.HTTPPost(t, nil, "http://localhost:8081/login/email", `{"email": "nonexistingemail", "language": "en"}`, nil, 400, nil)
 
-	test.HTTPPost(t, nil, "http://localhost:8081/login/email", `{"email": "test@example.com", "language":"en"}`, nil, 204, nil)
+	test.HTTPPost(t, nil, "http://localhost:8081/login/email", `{"email": "test@github.com", "language":"en"}`, nil, 204, nil)
 
-	// Non-existing email addresses should not give an error.
-	test.HTTPPost(t, nil, "http://localhost:8081/login/email", `{"email": "nonexisting@example.com", "language":"en"}`, nil, 204, nil)
+	// Non-working, but valid email addresses should not give an error.
+	test.HTTPPost(t, nil, "http://localhost:8081/login/email", `{"email": "nonexisting@github.com", "language":"en"}`, nil, 204, nil)
 
 	// Rate limited requests should not give an error.
-	test.HTTPPost(t, nil, "http://localhost:8081/login/email", `{"email": "test@example.com", "language":"en"}`, nil, 204, nil)
+	test.HTTPPost(t, nil, "http://localhost:8081/login/email", `{"email": "test@github.com", "language":"en"}`, nil, 204, nil)
 
 	// When unknown language is used, we should use the fallback language.
-	test.HTTPPost(t, nil, "http://localhost:8081/login/email", `{"email": "test@example.com", "language":"nonexistinglanguage"}`, nil, 204, nil)
+	test.HTTPPost(t, nil, "http://localhost:8081/login/email", `{"email": "test@github.com", "language":"nonexistinglanguage"}`, nil, 204, nil)
 
 	client := test.NewHTTPClient()
 
 	test.HTTPPost(t, client, "http://localhost:8081/login/token", `{"username":"testuser", "token":"testtoken"}`, nil, 204, nil)
 
-	test.HTTPPost(t, client, "http://localhost:8081/email/remove", "test@example.com", nil, 204, nil)
+	test.HTTPPost(t, client, "http://localhost:8081/email/remove", "test@github.com", nil, 204, nil)
 
 	test.HTTPPost(t, client, "http://localhost:8081/user/delete", "", nil, 204, nil)
 }

server/keyshare/myirmaserver/server_test.go

@@ -158,7 +158,7 @@ func TestServerUserData(t *testing.T) {
 			"testuser": {
 				id:         15,
 				lastActive: time.Unix(0, 0),
-				email:      []string{"test@example.com"},
+				email:      []string{"test@github.com"},
 				logEntries: []logEntry{
 					{
 						Timestamp: 110,
@@ -174,7 +174,7 @@ func TestServerUserData(t *testing.T) {
 			},
 		},
 		loginEmailTokens: map[string]string{
-			"testtoken": "test@example.com",
+			"testtoken": "test@github.com",
 		},
 	}
 	myirmaServer, httpServer := StartMyIrmaServer(t, db, "")
@@ -186,9 +186,9 @@ func TestServerUserData(t *testing.T) {
 
 	var userdata user
 	test.HTTPGet(t, client, "http://localhost:8081/user", nil, 200, &userdata)
-	assert.Equal(t, []userEmail{{Email: "test@example.com", DeleteInProgress: false}}, userdata.Emails)
+	assert.Equal(t, []userEmail{{Email: "test@github.com", DeleteInProgress: false}}, userdata.Emails)
 
-	test.HTTPPost(t, client, "http://localhost:8081/email/remove", "test@example.com", textPlainHeader(), 204, nil)
+	test.HTTPPost(t, client, "http://localhost:8081/email/remove", "test@github.com", textPlainHeader(), 204, nil)
 
 	userdata = user{}
 	test.HTTPGet(t, client, "http://localhost:8081/user", nil, 200, &userdata)

server/keyshare/tasks/tasks_test.go

@@ -145,9 +145,9 @@ func TestExpireAccounts(t *testing.T) {
 		xTimesEntry(12, "(%s, 'ExpiredUser%s', '', '', 0, 0, 0), ")+
 		"(28, 'ExpiredUserWithoutMail', '', '', 0, 0, 0)", time.Now().Unix())
 	require.NoError(t, err)
-	_, err = db.Exec("INSERT INTO irma.emails (user_id, email, delete_on) VALUES (15, 'test@example.com', NULL), " +
-		xTimesEntry(12, "(%s, 'test%s@example.com', NULL), ") +
-		"(28, 'test@example.com', NULL)")
+	_, err = db.Exec("INSERT INTO irma.emails (user_id, email, delete_on) VALUES (15, 'test@github.com', NULL), " +
+		xTimesEntry(12, "(%s, 'test%s@github.com', NULL), ") +
+		"(28, 'test@github.com', NULL)")
 	require.NoError(t, err)
 
 	th, err := newHandler(&Configuration{