From cb1d6769895acab06ad4b806e920aa70e97c9088 Mon Sep 17 00:00:00 2001 From: Bob Hageman Date: Tue, 21 Nov 2023 11:55:42 +0100 Subject: [PATCH 1/6] improve: check MX record hostname validity --- server/keyshare/email.go | 28 ++++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-) diff --git a/server/keyshare/email.go b/server/keyshare/email.go index b8a01471..9e5e6d66 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,19 @@ func VerifyMXRecord(email string) error { if records, err := net.LookupIP(host); err != nil || len(records) == 0 { return ErrInvalidEmailDomain } - return nil } + + invalidHosts := 0 + for _, h := range records { + // Check if host specified at MX record valid + if addr, err := net.LookupHost(h.Host); err != nil || len(addr) == 0 { + invalidHosts++ + } + } + + if invalidHosts >= len(records) { + return ErrInvalidEmailDomain + } + return nil } From 510289763ff7ec773ff70e9205a78990d50db0be Mon Sep 17 00:00:00 2001 From: Bob Hageman Date: Tue, 21 Nov 2023 12:04:17 +0100 Subject: [PATCH 2/6] chore: update changelog --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index f91f7b16..76403da4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,10 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## Unreleased +### Fixed +- Invalid hostname specified in MX record bypasses e-mail address revalidation + ## [0.14.2] - 2023-10-25 ### Fixed - IRMA session gets stuck in communicating status when user is requested to confirm PIN in `irmaclient` From 39c09589628e85b1d3445b8afee849acfd76f6f9 Mon Sep 17 00:00:00 2001 From: Bob Hageman Date: Thu, 23 Nov 2023 15:49:07 +0100 Subject: [PATCH 3/6] fix: invalid mx record example.com makes tests fail --- .../keyshare/keyshareserver/server_email_test.go | 4 ++-- server/keyshare/myirmaserver/server.go | 6 +++--- .../keyshare/myirmaserver/server_email_test.go | 16 ++++++++-------- server/keyshare/myirmaserver/server_test.go | 8 ++++---- server/keyshare/tasks/tasks_test.go | 6 +++--- 5 files changed, 20 insertions(+), 20 deletions(-) diff --git a/server/keyshare/keyshareserver/server_email_test.go b/server/keyshare/keyshareserver/server_email_test.go index 4852f688..93126648 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 c80c40a9..4a51a419 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() @@ -311,10 +311,10 @@ func (s *Server) handleEmailLogin(w http.ResponseWriter, r *http.Request) { return } - // In case sendLoginEmail fails with errEmailNotFound or errTooManyRequests, then we + // In case sendLoginEmail fails with errEmailNotFound, errTooManyRequests or ErrInvalidEmailDomain, then we // should not write an error. Otherwise, we would leak information about our user base. err := s.sendLoginEmail(r.Context(), request) - if err != nil && err != errEmailNotFound && err != errTooManyTokens { + if err != nil && err != errEmailNotFound && err != errTooManyTokens && err != keyshare.ErrInvalidEmailDomain { // already logged keyshare.WriteError(w, err) return diff --git a/server/keyshare/myirmaserver/server_email_test.go b/server/keyshare/myirmaserver/server_email_test.go index cb1011e2..b70ba450 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. + // Non-working, but valid 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) // 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 56ec63f4..f5ab8140 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 b355d239..794a376d 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{ From eaf97f5a9e9145bb593ea696cf1f98497fa0d7e2 Mon Sep 17 00:00:00 2001 From: Bob Hageman Date: Thu, 23 Nov 2023 15:52:41 +0100 Subject: [PATCH 4/6] improve: polish mx record host check --- server/keyshare/email.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/server/keyshare/email.go b/server/keyshare/email.go index 9e5e6d66..2f314085 100644 --- a/server/keyshare/email.go +++ b/server/keyshare/email.go @@ -166,15 +166,16 @@ func VerifyMXRecord(email string) error { } } - invalidHosts := 0 + hasValidHost := false for _, h := range records { - // Check if host specified at MX record valid - if addr, err := net.LookupHost(h.Host); err != nil || len(addr) == 0 { - invalidHosts++ + // 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 invalidHosts >= len(records) { + if !hasValidHost { return ErrInvalidEmailDomain } From 5df4daf3c6e3e42153f91703af9a19ff49f9a802 Mon Sep 17 00:00:00 2001 From: Bob Hageman Date: Tue, 28 Nov 2023 14:36:03 +0100 Subject: [PATCH 5/6] improve: LookupHost must return no error AND one or more addresses to be valid --- server/keyshare/email.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/keyshare/email.go b/server/keyshare/email.go index 2f314085..6f9910c5 100644 --- a/server/keyshare/email.go +++ b/server/keyshare/email.go @@ -169,7 +169,7 @@ func VerifyMXRecord(email string) error { 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 { + if addr, err := net.LookupHost(h.Host); err == nil && len(addr) > 0 { hasValidHost = true break } From 92a9d2522d25a2fa706f426460897163bcc3c206 Mon Sep 17 00:00:00 2001 From: Bob Hageman Date: Tue, 28 Nov 2023 14:46:06 +0100 Subject: [PATCH 6/6] improve: do return a 400 when domain is invalid --- server/keyshare/myirmaserver/server.go | 4 ++-- server/keyshare/myirmaserver/server_email_test.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/server/keyshare/myirmaserver/server.go b/server/keyshare/myirmaserver/server.go index 4a51a419..be4fe0a2 100644 --- a/server/keyshare/myirmaserver/server.go +++ b/server/keyshare/myirmaserver/server.go @@ -311,10 +311,10 @@ func (s *Server) handleEmailLogin(w http.ResponseWriter, r *http.Request) { return } - // In case sendLoginEmail fails with errEmailNotFound, errTooManyRequests or ErrInvalidEmailDomain, then we + // In case sendLoginEmail fails with errEmailNotFound or errTooManyRequests, then we // should not write an error. Otherwise, we would leak information about our user base. err := s.sendLoginEmail(r.Context(), request) - if err != nil && err != errEmailNotFound && err != errTooManyTokens && err != keyshare.ErrInvalidEmailDomain { + if err != nil && err != errEmailNotFound && err != errTooManyTokens { // already logged keyshare.WriteError(w, err) return diff --git a/server/keyshare/myirmaserver/server_email_test.go b/server/keyshare/myirmaserver/server_email_test.go index b70ba450..418eb0ff 100644 --- a/server/keyshare/myirmaserver/server_email_test.go +++ b/server/keyshare/myirmaserver/server_email_test.go @@ -38,7 +38,7 @@ func TestServerLoginEmail(t *testing.T) { test.HTTPPost(t, nil, "http://localhost:8081/login/email", `{"email": "test@github.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@example.com", "language":"en"}`, nil, 204, nil) + 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@github.com", "language":"en"}`, nil, 204, nil)