From 33c8b2177ed92bb2168e974c8d9a4a53597add1f Mon Sep 17 00:00:00 2001
From: Shivaram Lingamneni <slingamn@cs.stanford.edu>
Date: Wed, 25 Dec 2019 15:06:26 -0500
Subject: [PATCH] fix a bug

In the previous commit, the client would receive a failure message but would
actually remain logged in after an authzid/authcid mismatch. This was a
correctness rather than a security issue, but now it's fixed so that the client
never logs in in the first place.
---
 irc/accounts.go |  6 +++++-
 irc/errors.go   |  1 +
 irc/handlers.go | 38 +++++++++++++++++++++-----------------
 irc/nickserv.go |  2 +-
 4 files changed, 28 insertions(+), 19 deletions(-)

diff --git a/irc/accounts.go b/irc/accounts.go
index 64a406c7..01e164a9 100644
--- a/irc/accounts.go
+++ b/irc/accounts.go
@@ -971,7 +971,7 @@ func (am *AccountManager) ChannelsForAccount(account string) (channels []string)
 	return unmarshalRegisteredChannels(channelStr)
 }
 
-func (am *AccountManager) AuthenticateByCertFP(client *Client) error {
+func (am *AccountManager) AuthenticateByCertFP(client *Client, authzid string) error {
 	if client.certfp == "" {
 		return errAccountInvalidCredentials
 	}
@@ -991,6 +991,10 @@ func (am *AccountManager) AuthenticateByCertFP(client *Client) error {
 		return err
 	}
 
+	if authzid != "" && authzid != account {
+		return errAuthzidAuthcidMismatch
+	}
+
 	// ok, we found an account corresponding to their certificate
 	clientAccount, err := am.LoadAccount(account)
 	if err != nil {
diff --git a/irc/errors.go b/irc/errors.go
index 00d64851..15f030dc 100644
--- a/irc/errors.go
+++ b/irc/errors.go
@@ -27,6 +27,7 @@ var (
 	errAccountVerificationInvalidCode = errors.New("Invalid account verification code")
 	errAccountUpdateFailed            = errors.New(`Error while updating your account information`)
 	errAccountMustHoldNick            = errors.New(`You must hold that nickname in order to register it`)
+	errAuthzidAuthcidMismatch         = errors.New(`authcid and authzid must be the same`)
 	errCallbackFailed                 = errors.New("Account verification could not be sent")
 	errCertfpAlreadyExists            = errors.New(`An account already exists for your certificate fingerprint`)
 	errChannelNotOwnedByAccount       = errors.New("Channel not owned by the specified account")
diff --git a/irc/handlers.go b/irc/handlers.go
index 8816931e..e1b400cf 100644
--- a/irc/handlers.go
+++ b/irc/handlers.go
@@ -446,13 +446,14 @@ func authPlainHandler(server *Server, client *Client, mechanism string, value []
 }
 
 func authErrorToMessage(server *Server, err error) (msg string) {
-	if err == errAccountDoesNotExist || err == errAccountUnverified || err == errAccountInvalidCredentials {
-		msg = err.Error()
-	} else {
+	switch err {
+	case errAccountDoesNotExist, errAccountUnverified, errAccountInvalidCredentials, errAuthzidAuthcidMismatch:
+		return err.Error()
+	default:
+		// don't expose arbitrary error messages to the user
 		server.logger.Error("internal", "sasl authentication failure", err.Error())
-		msg = "Unknown"
+		return "Unknown"
 	}
-	return
 }
 
 // AUTHENTICATE EXTERNAL
@@ -462,24 +463,27 @@ func authExternalHandler(server *Server, client *Client, mechanism string, value
 		return false
 	}
 
-	err := server.accounts.AuthenticateByCertFP(client)
+	// EXTERNAL doesn't carry an authentication ID (this is determined from the
+	// certificate), but does carry an optional authorization ID.
+	var authzid string
+	var err error
+	if len(value) != 0 {
+		authzid, err = CasefoldName(string(value))
+		if err != nil {
+			err = errAuthzidAuthcidMismatch
+		}
+	}
+
+	if err == nil {
+		err = server.accounts.AuthenticateByCertFP(client, authzid)
+	}
+
 	if err != nil {
 		msg := authErrorToMessage(server, err)
 		rb.Add(nil, server.name, ERR_SASLFAIL, client.nick, fmt.Sprintf("%s: %s", client.t("SASL authentication failed"), client.t(msg)))
 		return false
 	}
 
-	// EXTERNAL doesn't carry an authentication ID (this is determined from the
-	// certificate), but does carry an optional authorization ID.
-	if len(value) != 0 {
-		authcid := client.Account()
-		cfAuthzid, err := CasefoldName(string(value))
-		if err != nil || cfAuthzid != authcid {
-			rb.Add(nil, server.name, ERR_SASLFAIL, client.Nick(), client.t("SASL authentication failed: authcid and authzid should be the same"))
-			return false
-		}
-	}
-
 	sendSuccessfulAccountAuth(client, rb, false, true)
 	return false
 }
diff --git a/irc/nickserv.go b/irc/nickserv.go
index f3da0a9f..7a5b7609 100644
--- a/irc/nickserv.go
+++ b/irc/nickserv.go
@@ -536,7 +536,7 @@ func nsIdentifyHandler(server *Server, client *Client, command string, params []
 
 	// try certfp
 	if !loginSuccessful && client.certfp != "" {
-		err := server.accounts.AuthenticateByCertFP(client)
+		err := server.accounts.AuthenticateByCertFP(client, "")
 		loginSuccessful = (err == nil)
 	}