From 9508aa7713632ed40124a933d91d5766cf2369c2 Mon Sep 17 00:00:00 2001
From: Gusted <postmaster@gusted.xyz>
Date: Mon, 12 Aug 2024 20:01:09 +0200
Subject: [PATCH] Improve usage of HMAC output for mailer tokens

- If the incoming mail feature is enabled, tokens are being sent with
outgoing mails. These tokens contains information about what type of
action is allow with such token (such as replying to a certain issue
ID), to verify these tokens the code uses the HMAC-SHA256 construction.
- The output of the HMAC is truncated to 80 bits, because this is
recommended by RFC2104, but RFC2104 actually doesn't recommend this. It
recommends, if truncation should need to take place, it should use
max(80, hash_len/2) of the leftmost bits. For HMAC-SHA256 this works out
to 128 bits instead of the currently used 80 bits.
- Update to token version 2 and disallow any usage of token version 1,
token version 2 are generated with 128 bits of HMAC output.
- Add test to verify the deprecation of token version 1 and a general
MAC check test.
---
 services/mailer/token/token.go           | 16 +++++++--
 tests/integration/incoming_email_test.go | 46 ++++++++++++++++++++++++
 2 files changed, 59 insertions(+), 3 deletions(-)

diff --git a/services/mailer/token/token.go b/services/mailer/token/token.go
index 8a5a762d6b..1a52bce803 100644
--- a/services/mailer/token/token.go
+++ b/services/mailer/token/token.go
@@ -22,9 +22,16 @@ import (
 //
 // The payload is verifiable by the generated HMAC using the user secret. It contains:
 // | Timestamp | Action/Handler Type | Action/Handler Data |
+//
+//
+// Version changelog
+//
+// v1 -> v2:
+// Use 128 instead of 80 bits of the HMAC-SHA256 output.
 
 const (
 	tokenVersion1        byte = 1
+	tokenVersion2        byte = 2
 	tokenLifetimeInYears int  = 1
 )
 
@@ -70,7 +77,7 @@ func CreateToken(ht HandlerType, user *user_model.User, data []byte) (string, er
 		return "", err
 	}
 
-	return encodingWithoutPadding.EncodeToString(append([]byte{tokenVersion1}, packagedData...)), nil
+	return encodingWithoutPadding.EncodeToString(append([]byte{tokenVersion2}, packagedData...)), nil
 }
 
 // ExtractToken extracts the action/user tuple from the token and verifies the content
@@ -84,7 +91,7 @@ func ExtractToken(ctx context.Context, token string) (HandlerType, *user_model.U
 		return UnknownHandlerType, nil, nil, &ErrToken{"no data"}
 	}
 
-	if data[0] != tokenVersion1 {
+	if data[0] != tokenVersion2 {
 		return UnknownHandlerType, nil, nil, &ErrToken{fmt.Sprintf("unsupported token version: %v", data[0])}
 	}
 
@@ -124,5 +131,8 @@ func generateHmac(secret, payload []byte) []byte {
 	mac.Write(payload)
 	hmac := mac.Sum(nil)
 
-	return hmac[:10] // RFC2104 recommends not using less then 80 bits
+	// RFC2104 section 5 recommends that if you do HMAC truncation, you should use
+	// the max(80, hash_len/2) of the leftmost bits.
+	// For SHA256 this works out to using 128 of the leftmost bits.
+	return hmac[:16]
 }
diff --git a/tests/integration/incoming_email_test.go b/tests/integration/incoming_email_test.go
index fdc0425b54..66f833b28d 100644
--- a/tests/integration/incoming_email_test.go
+++ b/tests/integration/incoming_email_test.go
@@ -4,6 +4,7 @@
 package integration
 
 import (
+	"encoding/base32"
 	"io"
 	"net"
 	"net/smtp"
@@ -75,6 +76,51 @@ func TestIncomingEmail(t *testing.T) {
 		assert.Equal(t, payload, p)
 	})
 
+	tokenEncoding := base32.StdEncoding.WithPadding(base32.NoPadding)
+	t.Run("Deprecated token version", func(t *testing.T) {
+		defer tests.PrintCurrentTest(t)()
+
+		payload := []byte{1, 2, 3, 4, 5}
+
+		token, err := token_service.CreateToken(token_service.ReplyHandlerType, user, payload)
+		require.NoError(t, err)
+		assert.NotEmpty(t, token)
+
+		// Set the token to version 1.
+		unencodedToken, err := tokenEncoding.DecodeString(token)
+		require.NoError(t, err)
+		unencodedToken[0] = 1
+		token = tokenEncoding.EncodeToString(unencodedToken)
+
+		ht, u, p, err := token_service.ExtractToken(db.DefaultContext, token)
+		require.ErrorContains(t, err, "unsupported token version: 1")
+		assert.Equal(t, token_service.UnknownHandlerType, ht)
+		assert.Nil(t, u)
+		assert.Nil(t, p)
+	})
+
+	t.Run("MAC check", func(t *testing.T) {
+		defer tests.PrintCurrentTest(t)()
+
+		payload := []byte{1, 2, 3, 4, 5}
+
+		token, err := token_service.CreateToken(token_service.ReplyHandlerType, user, payload)
+		require.NoError(t, err)
+		assert.NotEmpty(t, token)
+
+		// Modify the MAC.
+		unencodedToken, err := tokenEncoding.DecodeString(token)
+		require.NoError(t, err)
+		unencodedToken[len(unencodedToken)-1] ^= 0x01
+		token = tokenEncoding.EncodeToString(unencodedToken)
+
+		ht, u, p, err := token_service.ExtractToken(db.DefaultContext, token)
+		require.ErrorContains(t, err, "verification failed")
+		assert.Equal(t, token_service.UnknownHandlerType, ht)
+		assert.Nil(t, u)
+		assert.Nil(t, p)
+	})
+
 	t.Run("Handler", func(t *testing.T) {
 		t.Run("Reply", func(t *testing.T) {
 			checkReply := func(t *testing.T, payload []byte, issue *issues_model.Issue, commentType issues_model.CommentType) {