imapserver: allow creating mailboxes with characters &#*%, and encode mailbox names in imap with imaputf7 when needed

the imapserver started with imap4rev2-only and utf8=only. to prevent potential issues with imaputf7, which makes "&" special, we refused any mailbox with an "&" in the name. we already tried decoding utf7, falling back to using a mailbox name verbatim. that behaviour wasn't great. we now treat the enabled extensions IMAP4rev2 and/or UTF8=ACCEPT as indication whether mailbox names are in imaputf7. if they are, the encoding must be correct. we now also send mailbox names in imaputf7 when imap4rev2/utf8=accept isn't enabled. and we now allow "*" and "%" (wildcard characters for matching) in mailbox names. not ideal for IMAP LIST with patterns, but not enough reason to refuse them in mailbox names. people that migrate may run into this, possibly as blocker. we also allow "#" in mailbox names, but not as first character, to prevent potential clashes with IMAP namespaces in the future. based on report from Damian Poddebniak using https://github.com/duesee/imap-flow and issue #110, thanks for reporting!
2024-12-26 00:13:47 +03:00 · 2024-01-01 13:15:25 +01:00 · 2024-01-01 13:15:25 +01:00 · d84c96eca5
commit d84c96eca5
parent a9940f9855
7 changed files with 150 additions and 51 deletions
--- a/imapserver/create_test.go
+++ b/imapserver/create_test.go
@ -67,8 +67,13 @@ func TestCreate(t *testing.T) {
 	tc.transactf("bad", `create "\x9f"`)
 	tc.transactf("bad", `create "\u2028"`)
 	tc.transactf("bad", `create "\u2029"`)
-	tc.transactf("no", `create "%%"`)
-	tc.transactf("no", `create "*"`)
-	tc.transactf("no", `create "#"`)
-	tc.transactf("no", `create "&"`)
+	tc.transactf("ok", `create "%%"`)
+	tc.transactf("ok", `create "*"`)
+	tc.transactf("no", `create "#"`) // Leading hash not allowed.
+	tc.transactf("ok", `create "test#"`)
+
+	// UTF-7 checks are only for IMAP4 before rev2 and without UTF8=ACCEPT.
+	tc.transactf("ok", `create "&"`)      // Interpreted as UTF-8, no UTF-7.
+	tc2.transactf("bad", `create "&"`)    // Bad UTF-7.
+	tc2.transactf("ok", `create "&Jjo-"`) // ☺, valid UTF-7.
 }
--- a/imapserver/list.go
+++ b/imapserver/list.go
@ -212,7 +212,7 @@ func (c *conn) cmdList(tag, cmd string, p *parser) {
 				if extended != nil {
 					extStr = " " + extended.pack(c)
 				}
-				line := fmt.Sprintf(`* LIST %s "/" %s%s`, flags.pack(c), astring(name).pack(c), extStr)
+				line := fmt.Sprintf(`* LIST %s "/" %s%s`, flags.pack(c), astring(c.encodeMailbox(name)).pack(c), extStr)
 				responseLines = append(responseLines, line)

 				if retStatusAttrs != nil && info.mailbox != nil {
--- a/imapserver/parse.go
+++ b/imapserver/parse.go
@ -7,8 +7,6 @@ import (
 	"strconv"
 	"strings"
 	"time"
-
-	"golang.org/x/exp/slog"
 )

 var (
@ -392,30 +390,37 @@ func (p *parser) xatom() string {
 	return p.xtakechars(atomChar, "atom")
 }

-func (p *parser) xmailbox() string {
-	s := p.xastring()
-	// UTF-7 is deprecated in IMAP4rev2. IMAP4rev1 does not fully forbid
-	// UTF-8 returned in mailbox names. We'll do our best by attempting to
-	// decode utf-7. But if that doesn't work, we'll just use the original
-	// string.
-	// ../rfc/3501:964
-	if !p.conn.enabled[capIMAP4rev2] {
+func (p *parser) xdecodeMailbox(s string) string {
+	// UTF-7 is deprecated for IMAP4rev2-only clients, and not used with UTF8=ACCEPT.
+	// The future should be without UTF-7, we don't encode/decode it with modern
+	// clients. Most clients are IMAP4rev1, we need to handle UTF-7.
+	// ../rfc/3501:964 ../rfc/9051:7885
+	// Thunderbird will enable UTF8=ACCEPT and send "&" unencoded. ../rfc/9051:7953
+	if p.conn.utf8strings() {
+		return s
+	}
 	ns, err := utf7decode(s)
 	if err != nil {
-			p.conn.log.Infox("decoding utf7 or mailbox name", err, slog.String("name", s))
-		} else {
-			s = ns
+		p.xerrorf("decoding utf7 mailbox name: %v", err)
 	}
+	return ns
 }
-	return s
+
+func (p *parser) xmailbox() string {
+	s := p.xastring()
+	return p.xdecodeMailbox(s)
 }

 // ../rfc/9051:6605
 func (p *parser) xlistMailbox() string {
+	var s string
 	if p.hasPrefix(`"`) || p.hasPrefix("{") {
-		return p.xstring()
+		s = p.xstring()
+	} else {
+		s = p.xtakechars(atomChar+listWildcards+respSpecials, "list-char")
 	}
-	return p.xtakechars(atomChar+listWildcards+respSpecials, "list-char")
+	// Presumably UTF-7 encoding applies to mailbox patterns too.
+	return p.xdecodeMailbox(s)
 }

 // ../rfc/9051:6707 ../rfc/9051:6848 ../rfc/5258:1095 ../rfc/5258:1169 ../rfc/5258:1196
--- a/imapserver/server.go
+++ b/imapserver/server.go
@ -13,12 +13,9 @@ LIST-EXTENDED, SPECIAL-USE, MOVE, UTF8=ONLY.

 We take a liberty with UTF8=ONLY. We are supposed to wait for ENABLE of
 UTF8=ACCEPT or IMAP4rev2 before we respond with quoted strings that contain
-non-ASCII UTF-8. But we will unconditionally accept UTF-8 at the moment. See
+non-ASCII UTF-8. Until that's enabled, we do use UTF-7 for mailbox names. See
 ../rfc/6855:251

-We always respond with utf8 mailbox names. We do parse utf7 (only in IMAP4rev1,
-not in IMAP4rev2). ../rfc/3501:964
-
 - We never execute multiple commands at the same time for a connection. We expect a client to open multiple connections instead. ../rfc/9051:1110
 - Do not write output on a connection with an account lock held. Writing can block, a slow client could block account operations.
 - When handling commands that modify the selected mailbox, always check that the mailbox is not opened readonly. And always revalidate the selected mailbox, another session may have deleted the mailbox.
@ -161,7 +158,7 @@ var authFailDelay = time.Second  // After authentication failure.
 // TLS. The client should not be selecting PLUS variants on non-TLS connections,
 // instead opting to do the bare SCRAM variant without indicating the server claims
 // to support the PLUS variant (skipping the server downgrade detection check).
-const serverCapabilities = "IMAP4rev2 IMAP4rev1 ENABLE LITERAL+ IDLE SASL-IR BINARY UNSELECT UIDPLUS ESEARCH SEARCHRES MOVE UTF8=ONLY LIST-EXTENDED SPECIAL-USE LIST-STATUS AUTH=SCRAM-SHA-256-PLUS AUTH=SCRAM-SHA-256 AUTH=SCRAM-SHA-1-PLUS AUTH=SCRAM-SHA-1 AUTH=CRAM-MD5 ID APPENDLIMIT=9223372036854775807 CONDSTORE QRESYNC"
+const serverCapabilities = "IMAP4rev2 IMAP4rev1 ENABLE LITERAL+ IDLE SASL-IR BINARY UNSELECT UIDPLUS ESEARCH SEARCHRES MOVE UTF8=ACCEPT LIST-EXTENDED SPECIAL-USE LIST-STATUS AUTH=SCRAM-SHA-256-PLUS AUTH=SCRAM-SHA-256 AUTH=SCRAM-SHA-1-PLUS AUTH=SCRAM-SHA-1 AUTH=CRAM-MD5 ID APPENDLIMIT=9223372036854775807 CONDSTORE QRESYNC"

 type conn struct {
 	cid               int64
@ -387,6 +384,13 @@ func (c *conn) utf8strings() bool {
 	return c.enabled[capIMAP4rev2] || c.enabled[capUTF8Accept]
 }

+func (c *conn) encodeMailbox(s string) string {
+	if c.utf8strings() {
+		return s
+	}
+	return utf7encode(s)
+}
+
 func (c *conn) xdbwrite(fn func(tx *bstore.Tx)) {
 	err := c.account.DB.Write(context.TODO(), func(tx *bstore.Tx) error {
 		fn(tx)
@ -1311,19 +1315,19 @@ func (c *conn) applyChanges(changes []store.Change, initial bool) {
 			// unrecognized \NonExistent and interpret this as a newly created mailbox, while
 			// the goal was to remove it...
 			if c.enabled[capIMAP4rev2] {
-				c.bwritelinef(`* LIST (\NonExistent) "/" %s`, astring(ch.Name).pack(c))
+				c.bwritelinef(`* LIST (\NonExistent) "/" %s`, astring(c.encodeMailbox(ch.Name)).pack(c))
 			}
 		case store.ChangeAddMailbox:
-			c.bwritelinef(`* LIST (%s) "/" %s`, strings.Join(ch.Flags, " "), astring(ch.Mailbox.Name).pack(c))
+			c.bwritelinef(`* LIST (%s) "/" %s`, strings.Join(ch.Flags, " "), astring(c.encodeMailbox(ch.Mailbox.Name)).pack(c))
 		case store.ChangeRenameMailbox:
 			// OLDNAME only with IMAP4rev2 or NOTIFY ../rfc/9051:2726 ../rfc/5465:628
 			var oldname string
 			if c.enabled[capIMAP4rev2] {
-				oldname = fmt.Sprintf(` ("OLDNAME" (%s))`, string0(ch.OldName).pack(c))
+				oldname = fmt.Sprintf(` ("OLDNAME" (%s))`, string0(c.encodeMailbox(ch.OldName)).pack(c))
 			}
-			c.bwritelinef(`* LIST (%s) "/" %s%s`, strings.Join(ch.Flags, " "), astring(ch.NewName).pack(c), oldname)
+			c.bwritelinef(`* LIST (%s) "/" %s%s`, strings.Join(ch.Flags, " "), astring(c.encodeMailbox(ch.NewName)).pack(c), oldname)
 		case store.ChangeAddSubscription:
-			c.bwritelinef(`* LIST (%s) "/" %s`, strings.Join(append([]string{`\Subscribed`}, ch.Flags...), " "), astring(ch.Name).pack(c))
+			c.bwritelinef(`* LIST (%s) "/" %s`, strings.Join(append([]string{`\Subscribed`}, ch.Flags...), " "), astring(c.encodeMailbox(ch.Name)).pack(c))
 		default:
 			panic(fmt.Sprintf("internal error, missing case for %#v", change))
 		}
@ -2041,7 +2045,7 @@ func (c *conn) cmdSelectExamine(isselect bool, tag, cmd string, p *parser) {
 	}
 	c.bwritelinef(`* OK [UIDVALIDITY %d] x`, mb.UIDValidity)
 	c.bwritelinef(`* OK [UIDNEXT %d] x`, mb.UIDNext)
-	c.bwritelinef(`* LIST () "/" %s`, astring(mb.Name).pack(c))
+	c.bwritelinef(`* LIST () "/" %s`, astring(c.encodeMailbox(mb.Name)).pack(c))
 	if c.enabled[capCondstore] {
 		// ../rfc/7162:417
 		// ../rfc/7162-eid5055 ../rfc/7162:484 ../rfc/7162:1167
@ -2239,9 +2243,9 @@ func (c *conn) cmdCreate(tag, cmd string, p *parser) {
 		var oldname string
 		// OLDNAME only with IMAP4rev2 or NOTIFY ../rfc/9051:2726 ../rfc/5465:628
 		if c.enabled[capIMAP4rev2] && n == name && name != origName && !(name == "Inbox" || strings.HasPrefix(name, "Inbox/")) {
-			oldname = fmt.Sprintf(` ("OLDNAME" (%s))`, string0(origName).pack(c))
+			oldname = fmt.Sprintf(` ("OLDNAME" (%s))`, string0(c.encodeMailbox(origName)).pack(c))
 		}
-		c.bwritelinef(`* LIST (\Subscribed) "/" %s%s`, astring(n).pack(c), oldname)
+		c.bwritelinef(`* LIST (\Subscribed) "/" %s%s`, astring(c.encodeMailbox(n)).pack(c), oldname)
 	}
 	c.ok(tag, cmd)
 }
@ -2526,7 +2530,7 @@ func (c *conn) cmdLsub(tag, cmd string, p *parser) {
 				continue
 			}
 			have[name] = true
-			line := fmt.Sprintf(`* LSUB () "/" %s`, astring(name).pack(c))
+			line := fmt.Sprintf(`* LSUB () "/" %s`, astring(c.encodeMailbox(name)).pack(c))
 			lines = append(lines, line)

 		}
@ -2541,7 +2545,7 @@ func (c *conn) cmdLsub(tag, cmd string, p *parser) {
 			if have[mb.Name] || !subscribedKids[mb.Name] || !re.MatchString(mb.Name) {
 				return nil
 			}
-			line := fmt.Sprintf(`* LSUB (\NoSelect) "/" %s`, astring(mb.Name).pack(c))
+			line := fmt.Sprintf(`* LSUB (\NoSelect) "/" %s`, astring(c.encodeMailbox(mb.Name)).pack(c))
 			lines = append(lines, line)
 			return nil
 		})
@ -2639,7 +2643,7 @@ func (c *conn) xstatusLine(tx *bstore.Tx, mb store.Mailbox, attrs []string) stri
 			xsyntaxErrorf("unknown attribute %q", a)
 		}
 	}
-	return fmt.Sprintf("* STATUS %s (%s)", astring(mb.Name).pack(c), strings.Join(status, " "))
+	return fmt.Sprintf("* STATUS %s (%s)", astring(c.encodeMailbox(mb.Name)).pack(c), strings.Join(status, " "))
 }

 func flaglist(fl store.Flags, keywords []string) listspace {
--- a/imapserver/utf7.go
+++ b/imapserver/utf7.go
@ -1,11 +1,17 @@
 package imapserver

 import (
+	"bytes"
 	"encoding/base64"
 	"errors"
 	"fmt"
+	"unicode/utf16"
 )

+// IMAP4rev1 uses a modified version of UTF-7.
+// ../rfc/3501:1050
+// ../rfc/2152:69
+
 const utf7chars = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+,"

 var utf7encoding = base64.NewEncoding(utf7chars).WithPadding(base64.NoPadding)
@ -16,6 +22,7 @@ var (
 	errUTF7OddSized         = errors.New("utf7: odd-sized data")
 	errUTF7UnneededShift    = errors.New("utf7: unneeded shift")
 	errUTF7UnfinishedShift  = errors.New("utf7: unfinished shift")
+	errUTF7BadSurrogate     = errors.New("utf7: bad utf16 surrogates")
 )

 func utf7decode(s string) (string, error) {
@ -60,24 +67,80 @@ func utf7decode(s string) (string, error) {

 		x := make([]rune, len(buf)/2)
 		j := 0
+		trymerge := false
 		for i := 0; i < len(buf); i += 2 {
 			x[j] = rune(buf[i])<<8 | rune(buf[i+1])
-			j++
+			if trymerge {
+				s0 := utf16.IsSurrogate(x[j-1])
+				s1 := utf16.IsSurrogate(x[j])
+				if s0 && s1 {
+					c := utf16.DecodeRune(x[j-1], x[j])
+					if c == 0xfffd {
+						return "", fmt.Errorf("%w: decoding %x %x", errUTF7BadSurrogate, x[j-1], x[j])
 					}
+					x[j-1] = c
+					trymerge = false
+					continue
+				} else if s0 != s1 {
+					return "", fmt.Errorf("%w: not both surrogate: %x %x", errUTF7BadSurrogate, x[j-1], x[j])
+				}
+			}
+			j++
+			trymerge = true
+		}
+		x = x[:j]

-		need := false
 		for _, c := range x {
 			if c < 0x20 || c > 0x7e || c == '&' {
-				need = true
-			}
 				r += string(c)
-		}
-		if !need {
+			} else {
+				// ../rfc/3501:1057
 				return "", errUTF7UnneededShift
 			}
 		}
+	}
 	if shifted {
 		return "", errUTF7UnfinishedShift
 	}
 	return r, nil
 }
+
+func utf7encode(s string) string {
+	var r string
+	var code string
+
+	flushcode := func() {
+		if code == "" {
+			return
+		}
+		var b bytes.Buffer
+		for _, c := range code {
+			high, low := utf16.EncodeRune(c)
+			if high == 0xfffd && low == 0xfffd {
+				b.WriteByte(byte(c >> 8))
+				b.WriteByte(byte(c >> 0))
+			} else {
+				b.WriteByte(byte(high >> 8))
+				b.WriteByte(byte(high >> 0))
+				b.WriteByte(byte(low >> 8))
+				b.WriteByte(byte(low >> 0))
+			}
+		}
+		r += "&" + utf7encoding.EncodeToString(b.Bytes()) + "-"
+		code = ""
+	}
+
+	for _, c := range s {
+		if c == '&' {
+			flushcode()
+			r += "&-"
+		} else if c >= ' ' && c < 0x7f {
+			flushcode()
+			r += string(c)
+		} else {
+			code += string(c)
+		}
+	}
+	flushcode()
+	return r
+}
--- a/imapserver/utf7_test.go
+++ b/imapserver/utf7_test.go
@ -2,6 +2,7 @@ package imapserver

 import (
 	"errors"
+	"fmt"
 	"testing"
 )

@ -16,6 +17,12 @@ func TestUTF7(t *testing.T) {
 		if (expErr == nil) != (err == nil) || err != nil && !errors.Is(err, expErr) {
 			t.Fatalf("got err %v, expected %v", err, expErr)
 		}
+		if expErr == nil {
+			expInput := utf7encode(output)
+			if expInput != input {
+				t.Fatalf("encoding, got %s, expected %s", expInput, input)
+			}
+		}
 	}

 	check("plain", "plain", nil)
@ -24,10 +31,19 @@ func TestUTF7(t *testing.T) {
 	check("&Jjo-test&Jjo-", "☺test☺", nil)
 	check("&Jjo-test", "☺test", nil)
 	check("&-", "&", nil)
-	check("&-", "&", nil)
 	check("&Jjo", "", errUTF7UnfinishedShift)     // missing closing -
 	check("&Jjo-&-", "", errUTF7SuperfluousShift) // shift just after unshift not allowed, should have been a single shift.
 	check("&AGE-", "", errUTF7UnneededShift)      // Just 'a', does not need utf7.
 	check("&☺-", "", errUTF7Base64)
 	check("&YQ-", "", errUTF7OddSized) // Just a single byte 'a'
+	check("&2AHcNw-", "𐐷", nil)
+	check(fmt.Sprintf("&%s-", utf7encoding.EncodeToString([]byte{0xdc, 0x00, 0xd8, 0x00})), "", errUTF7BadSurrogate) // Low & high surrogate swapped.
+	check(fmt.Sprintf("&%s-", utf7encoding.EncodeToString([]byte{0, 1, 0xdc, 0x00})), "", errUTF7BadSurrogate)       // ASCII + high surrogate.
+	check(fmt.Sprintf("&%s-", utf7encoding.EncodeToString([]byte{0, 1, 0xd8, 0x00})), "", errUTF7BadSurrogate)       // ASCII + low surrogate.
+	check(fmt.Sprintf("&%s-", utf7encoding.EncodeToString([]byte{0xd8, 0x00, 0, 1})), "", errUTF7BadSurrogate)       // low surrogate + ASCII.
+	check(fmt.Sprintf("&%s-", utf7encoding.EncodeToString([]byte{0xdc, 0x00, 0, 1})), "", errUTF7BadSurrogate)       // high surrogate + ASCII.
+
+	// ../rfc/9051:7967
+	check("~peter/mail/&U,BTFw-/&ZeVnLIqe-", "~peter/mail/台北/日本語", nil)
+	check("&U,BTFw-&ZeVnLIqe-", "", errUTF7SuperfluousShift)
 }
--- a/store/account.go
+++ b/store/account.go
@ -2599,12 +2599,18 @@ func CheckMailboxName(name string, allowInbox bool) (normalizedName string, isIn
 	if strings.HasPrefix(name, "/") || strings.HasSuffix(name, "/") || strings.Contains(name, "//") {
 		return "", false, errors.New("bad slashes in mailbox name")
 	}
-	for _, c := range name {
-		switch c {
-		case '%', '*', '#', '&':
-			return "", false, fmt.Errorf("character %c not allowed in mailbox name", c)
+
+	// "%" and "*" are difficult to use with the IMAP LIST command, but we allow mostly
+	// allow them. ../rfc/3501:1002 ../rfc/9051:983
+	if strings.HasPrefix(name, "#") {
+		return "", false, errors.New("mailbox name cannot start with hash due to conflict with imap namespaces")
 	}
-		// ../rfc/6855:192
+
+	// "#" and "&" are special in IMAP mailbox names. "#" for namespaces, "&" for
+	// IMAP-UTF-7 encoding. We do allow them. ../rfc/3501:1018 ../rfc/9051:991
+
+	for _, c := range name {
+		// ../rfc/3501:999 ../rfc/6855:192 ../rfc/9051:979
 		if c <= 0x1f || c >= 0x7f && c <= 0x9f || c == 0x2028 || c == 0x2029 {
 			return "", false, errors.New("control characters not allowed in mailbox name")
 		}