Cannot subscribe shared folder if destination mailbox name is a prefix of source mailbox name

[MartySvec]

Hello,

we’ve found a bug in Cyrus 3.6 where it is impossible to share a mailbox folder from one mailbox to another when the destination mailbox name is a prefix of the source mailbox name. It’s reproducible by these steps:

1. Create two mailboxes, [email protected] and [email protected]. It’s crucial that martin is a prefix of martin123.
2. Create a subfolder [email protected]/SharedFolder.
3. Set ACL so that [email protected] has access to [email protected]/SharedFolder. I.e. cyradm listacl looks like:

user/martin123/[email protected]:
  [email protected] lrswipkxtecd
  [email protected] lrswipkxtecdan

4. Try to subscribe SharedFolder from [email protected]. Then, although ACL is properly set, the subscription doesn’t happen and [email protected] doesn’t see the shared folder.

While investigating this issue we noticed that there’s a strange record in martin.sub subscriptions file: instead of the expected Nsomedomain.com.user.martin123.SharedFolder, there’s NINBOX123.SharedFolder. That is, the common prefix of the two mailboxes was replaced by string INBOX. If we copy .sub file from another mailbox that contains Nsomedomain.com.user.martin123.SharedFolder , subscription of the shared folder works fine.

After quick inspection of the source code I think the issue is caused by function mboxlist_dbname_to_key() in mboxlist.c. This function rewrites mbname to "INBOX" based on a trivial strncmp() comparison and ignores the possibility that mbname can be a partial prefix of a longer mailbox name. Then, when called from mboxlist_changesub(), dbname points to a different mailbox than userid and mboxlist_dbname_to_key() rewrites dbname to a nonexisting bogus name.

I think that mboxlist_dbname_to_key() should rewrite mbname to "INBOX" only if it’s followed by a separator or end of string. However, mboxlist_dbname_to_key() is called from a number of other places, so I don’t want to propose a patch without deeper knowledge of the code. Finally, note that there’s similar strncmp()-based string replacement of "INBOX" to mbname in mboxlist_dbname_from_key(). Perhaps this function should perform prefix sanity checks as well?

We’ve found this bug in Cyrus IMAP 3.6 (3.6.1-4+deb12u3) in Debian 12. Cyrus IMAP 3.4.x and older seem to be unaffected. Master branch on github contains the same mboxlist_dbname_to_key() and mboxlist_changesub() code so I assume that all versions after 3.6 have this bug too. Note that our setup uses options altnamespace: yes and mailbox_legacy_dirs: yes (not sure if it’s relevant).

Best regards,
Martin

[elliefm]

@ksmurchison I haven't looked at code yet, but Martin's analysis here smells right.

However, mboxlist_dbname_to_key() is called from a number of other places, so I don’t want to propose a patch without deeper knowledge of the code.

Any thoughts on this concern? I'm not real familiar with these newer APIs yet, but it'd be easy enough for me to write a test case to reproduce the problem, so I'm happy to have a go at tackling a fix. Not sure what kind of dragons to expect though!

[ksmurchison]

Any thoughts on this concern? I'm not real familiar with these newer APIs yet, but it'd be easy enough for me to write a test case to reproduce the problem, so I'm happy to have a go at tackling a fix. Not sure what kind of dragons to expect though!

@elliefm The analysis sounds correct. If you want to take a shot at it, feel free. Let me know if you run into issues.

[MartySvec]

Ellie, Ken, thank you for quick response. My primary concern regarding any fixes inside mboxlist_dbname_to/from_key() is that such a fix must not break compatibility with existing keys saved in other cyrusdb databases. That would be a nightmare for Cyrus upgrades. (I can imagine a mailbox name with an incorrectly rewritten prefix which is silently rewritten back to correct one by the present implementation, but the fixed code wouldn't be able to "correct" it.) Personally, I believe that the incorrect prefix rewriting happens only in special cases like the cross-mailbox folder subscription and other cyrusdb databases are unaffected. But this should be reviewed by somebody familiar with Cyrus internals, considering all possible inputs to mboxlist_dbname_to/from_key() and usages of these keys.

[elliefm]

Looking over how mboxlist_dbname_to/from_key are used, it looks like the userid argument is always NULL unless we're doing something with subscriptions, so this INBOX replacement behaviour seems to be specific to that. Which I think means that fixing the prefix behaviour won't cause problems anywhere else.

Once the actual bug is fixed, it might be possible to find and fix up bad subscriptions where this has occurred by looking for subscriptions to e.g. INBOX123.SharedFolder where the name starts with INBOX and is not immediately followed by a separator. If that's not a real mailbox, but reversing the INBOX conversion produces a thing that is a real mailbox, then it can be fixed and the subscription replaced. Do we have an existing tool for repairing subscriptions that this can be added to? I'm skimming reconstruct but don't see anything for subscriptions in there.

[elliefm]

I've got a test now that confirms the problem still happens on the master branch.

We might be able to automatically repair the bad subscriptions data from mboxlist_upgrade_subs() and friends, which is called every time a subscriptions db is opened. Might require bumping DB_VERSION_STR, which smells funny but looks like it's only used for the subscriptions databases?

[elliefm]

I think I would rename DB_VERSION_KEY and DB_VERSION_STR to SUBDB_... to reflect their specificity, rather than try to synchronise all the other mboxlist stuff with the change. If we want to start numbering mailboxes.db versions as well, that feels like a separate piece of work, not part of this.

[elliefm]

It looks like the SUBSCRIBE command succeeds, but since the record created is rubbish, it's a bit like being subscribed to a mailbox that doesn't exist, it doesn't show up in LSUB or LIST responses.

I wonder what the UNSUBSCRIBE behaviour is...

[elliefm]

Ahh interesting, you can unsubscribe from one of these mailboxes, spelling the mailbox name correctly, and it will find and remove the bad entry from the subscriptions file. Which makes sense because it's only the db-level operations that have the bad conversion, the rest of the time the mailbox name is correct.