From 5b53a150c0f3d6e0b75f9451c898553e2d739cb4 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. (cherry picked from commit 9508aa7713632ed40124a933d91d5766cf2369c2) --- 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) {