From c4e46fc77781fef1a108f60e6879806693a9d3d1 Mon Sep 17 00:00:00 2001 From: Bob Hageman Date: Wed, 29 Nov 2023 13:10:47 +0100 Subject: [PATCH] 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 | 1 + server/keyshare/email.go | 29 ++++++++++++++----- .../keyshareserver/server_email_test.go | 4 +-- server/keyshare/myirmaserver/server.go | 2 +- .../myirmaserver/server_email_test.go | 18 ++++++------ server/keyshare/myirmaserver/server_test.go | 8 ++--- server/keyshare/tasks/tasks_test.go | 6 ++-- 7 files changed, 41 insertions(+), 27 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d353da098..d94b0ce4f 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/server/keyshare/email.go b/server/keyshare/email.go index b8a01471a..6f9910c57 100644 --- a/server/keyshare/email.go +++ b/server/keyshare/email.go @@ -150,13 +150,13 @@ 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 @@ -164,7 +164,20 @@ func VerifyMXRecord(email string) error { 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 } diff --git a/server/keyshare/keyshareserver/server_email_test.go b/server/keyshare/keyshareserver/server_email_test.go index 4852f6889..931266483 100644 --- a/server/keyshare/keyshareserver/server_email_test.go +++ b/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, ) diff --git a/server/keyshare/myirmaserver/server.go b/server/keyshare/myirmaserver/server.go index c80c40a96..be4fe0a27 100644 --- a/server/keyshare/myirmaserver/server.go +++ b/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() diff --git a/server/keyshare/myirmaserver/server_email_test.go b/server/keyshare/myirmaserver/server_email_test.go index cb1011e26..418eb0ff7 100644 --- a/server/keyshare/myirmaserver/server_email_test.go +++ b/server/keyshare/myirmaserver/server_email_test.go @@ -16,7 +16,7 @@ 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, @@ -24,7 +24,7 @@ func TestServerLoginEmail(t *testing.T) { }, }, 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) } diff --git a/server/keyshare/myirmaserver/server_test.go b/server/keyshare/myirmaserver/server_test.go index 56ec63f45..f5ab81400 100644 --- a/server/keyshare/myirmaserver/server_test.go +++ b/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) diff --git a/server/keyshare/tasks/tasks_test.go b/server/keyshare/tasks/tasks_test.go index b355d239e..794a376d8 100644 --- a/server/keyshare/tasks/tasks_test.go +++ b/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{