Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check MX record hostname validity #358

Merged
merged 9 commits into from
Nov 29, 2023
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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`
Expand Down
29 changes: 21 additions & 8 deletions server/keyshare/email.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
ivard marked this conversation as resolved.
Show resolved Hide resolved
hasValidHost = true
break
}
}

if !hasValidHost {
return ErrInvalidEmailDomain
}
ivard marked this conversation as resolved.
Show resolved Hide resolved

return nil
}
4 changes: 2 additions & 2 deletions server/keyshare/keyshareserver/server_email_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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,
)

Expand Down
6 changes: 3 additions & 3 deletions server/keyshare/myirmaserver/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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 {
ivard marked this conversation as resolved.
Show resolved Hide resolved
// already logged
keyshare.WriteError(w, err)
return
Expand Down
16 changes: 8 additions & 8 deletions server/keyshare/myirmaserver/server_email_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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": "[email protected]", "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)
}
Expand Down
8 changes: 4 additions & 4 deletions server/keyshare/myirmaserver/server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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, "")
Expand All @@ -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)
Expand Down
6 changes: 3 additions & 3 deletions server/keyshare/tasks/tasks_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down