OMEMO key/tag fix

e2ee/omemo/envelope.go

@@ -32,7 +32,11 @@ import (
 //     plaintext body. The GCM tag is stripped from the ciphertext and
 //     instead appended to the key (key||tag, 32 bytes) before that blob is
 //     Signal-encrypted per recipient; <payload> carries ciphertext only,
-//     <iv> carries the GCM nonce.
+//     <iv> carries the GCM nonce. This is what we (and current real
+//     clients) produce; decryptPayloadOmemo0 additionally accepts an
+//     older, non-conformant 16-byte-key-only layout some real-world
+//     senders still use (see its own doc comment) - real-world interop,
+//     confirmed against actual client behavior/history, not assumed.
 //
 //   - Omemo1 and Omemo2 share the same payload crypto (introduced at
 //     Omemo1/XEP-0384 0.4.0, unchanged since): the plaintext is first
@@ -152,12 +156,41 @@ func encryptPayloadOmemo0(plaintext []byte) (ciphertext, iv, contentKey []byte,
 	return ciphertext, iv, contentKey, nil
 }
 
+// decryptPayloadOmemo0 accepts two real-world wire layouts for the
+// per-device content key, dispatching on its length:
+//
+//   - 32 bytes (omemo0KeyLen+omemo0TagLen): the spec-correct, current
+//     layout this package also produces (see encryptPayloadOmemo0) - key
+//     and GCM tag concatenated in <key>, <payload> is ciphertext only.
+//     Confirmed as what current real clients use (python-oldmemo, the
+//     backend behind Gajim's OMEMO plugin and slixmpp-omemo; Profanity;
+//     the xmpp-parsers crate's doc comment) - not assumed.
+//   - 16 bytes (omemo0KeyLen): an older, non-conformant layout some
+//     real-world senders still use, where <key> is just the raw AES key
+//     and the GCM tag instead rides along appended to <payload>
+//     (ciphertext||tag - the natural output of a plain AEAD Seal call).
+//     Confirmed via Conversations' own git history (commit e38a9cd729bf,
+//     "no longer accept auth tag appended to payload", ~2020-01 - meaning
+//     Conversations itself accepted exactly this for years before that)
+//     and Dino's dino#303 (a bug from mishandling this same split) - not
+//     a hypothetical. Accepted here defensively for interop, the same way
+//     Conversations itself did.
 func decryptPayloadOmemo0(ciphertext, iv, contentKey []byte) ([]byte, error) {
-	if len(contentKey) != omemo0KeyLen+omemo0TagLen {
+	switch len(contentKey) {
-		return nil, fmt.Errorf("omemo: omemo0 content key must be %d bytes, got %d", omemo0KeyLen+omemo0TagLen, len(contentKey))
+	case omemo0KeyLen + omemo0TagLen:
+		return openOmemo0GCM(ciphertext, iv, contentKey[:omemo0KeyLen], contentKey[omemo0KeyLen:])
+	case omemo0KeyLen:
+		if len(ciphertext) < omemo0TagLen {
+			return nil, fmt.Errorf("omemo: omemo0 payload too short to contain a trailing GCM tag: %d bytes", len(ciphertext))
+		}
+		tag := ciphertext[len(ciphertext)-omemo0TagLen:]
+		return openOmemo0GCM(ciphertext[:len(ciphertext)-omemo0TagLen], iv, contentKey, tag)
+	default:
+		return nil, fmt.Errorf("omemo: omemo0 content key must be %d or %d bytes, got %d", omemo0KeyLen, omemo0KeyLen+omemo0TagLen, len(contentKey))
 	}
-	key := contentKey[:omemo0KeyLen]
+}
-	tag := contentKey[omemo0KeyLen:]
+
+func openOmemo0GCM(ciphertext, iv, key, tag []byte) ([]byte, error) {
 	block, err := aes.NewCipher(key)
 	if err != nil {
 		return nil, err

e2ee/omemo/envelope_test.go

@@ -2,10 +2,53 @@ package omemo
 
 import (
 	"bytes"
+	"crypto/aes"
+	"crypto/cipher"
+	"crypto/rand"
 	"reflect"
 	"testing"
 )
 
+// TestDecryptPayloadOmemo0LegacyKeyLayout covers the real-world interop
+// fallback in decryptPayloadOmemo0: some senders (confirmed via
+// Conversations' own git history and dino#303 - see that function's doc
+// comment) put only the raw 16-byte AES key in <key> and let the GCM tag
+// ride along appended to <payload> instead, rather than the current
+// key||tag (32-byte) layout this package itself produces. This constructs
+// that legacy layout directly (bypassing encryptPayloadOmemo0, which never
+// produces it) to prove decryptPayloadOmemo0 still accepts it.
+func TestDecryptPayloadOmemo0LegacyKeyLayout(t *testing.T) {
+	plaintext := []byte("hello from an older client")
+
+	key := make([]byte, omemo0KeyLen)
+	if _, err := rand.Read(key); err != nil {
+		t.Fatalf("rand.Read key: %v", err)
+	}
+	iv := make([]byte, omemo0IVLen)
+	if _, err := rand.Read(iv); err != nil {
+		t.Fatalf("rand.Read iv: %v", err)
+	}
+	block, err := aes.NewCipher(key)
+	if err != nil {
+		t.Fatalf("aes.NewCipher: %v", err)
+	}
+	gcm, err := cipher.NewGCM(block)
+	if err != nil {
+		t.Fatalf("cipher.NewGCM: %v", err)
+	}
+	// The legacy layout: <payload> = ciphertext||tag (GCM's Seal output,
+	// kept whole), <key> = the raw 16-byte key (no tag appended).
+	sealed := gcm.Seal(nil, iv, plaintext, nil)
+
+	got, err := decryptPayloadOmemo0(sealed, iv, key)
+	if err != nil {
+		t.Fatalf("decryptPayloadOmemo0: %v", err)
+	}
+	if !bytes.Equal(got, plaintext) {
+		t.Fatalf("got %q, want %q", got, plaintext)
+	}
+}
+
 func TestEncryptOuterRoundTrip(t *testing.T) {
 	for _, tc := range []struct {
 		name    string