Open elliefm opened 6 years ago
elliefm
commented
6 years ago It looks like you're using unixhierarchysep=yes in your imapd.conf?
I wonder if the mboxname_policycheck() isn't handling this properly?
Also, I notice you're logging in as cyrus, which I assume is in your admins. I wonder if admins are allowed to make junk mailbox names that other users aren't.
elliefm
commented
6 years ago Okay, I think I understand this now.
The code forbidding leading . and anywhere-.. are specifically looking for empty paths in the string. At this place in the code, it's in the internal namespace, where the dot is the separator and "real" dots (if any) are ^.
When you're in unixhierarchysep=no, then you just can't have dots in mailbox names, end of story.
If you're in unixhierarchysep=yes, where the separator is /, then you can, and they can be anywhere -- including mailboxes called . and ... When these are written down to disk, they're escaped to ^, so there's no issue.
... except for the sieve handling code, which does not do the .=>^ substitution. timsieved will happily make rubbish paths, and lmtpd will happily try to read them. This does need to be fixed at some point.
slimlv
commented
6 years ago I think the commit in question should be reverted
elliefm
commented
6 years ago So with commit 73af8e1 and upgrading to cyrus 3.0 I'll get folder name "/var/spool/imap/q/user/.test", right ?
No. Mailbox paths on disk have not changed.
If you create a user called ".test" or ".." then its sieve path will be broken, but only its sieve path will be broken. If the user has no sieve scripts, or you do not run the timsieved(8) service, then the broken sieve paths have no effect. The broken sieve paths have existed forever: the only thing commit 73af8e1 does is allow lmtpd to find the sieve scripts, by calculating paths in the same broken way that timsieved does. If you upgrade to Cyrus 3.0.5 at the moment (without this commit) then your sieve scripts will be broken for users with dots in their names, because lmtpd can't find their sieve scripts. This commit fixes 3.0 for people who are already using sieve from an earlier version. (If you are not using sieve scripts, this commit does not affect you at all.)
0 create user/..
Sieve scripts cannot have a "/" in the script name, so even though a user named ".." would have a sieve path containing "/../", the user cannot use the bad path to read or write to arbitrary file locations, they will just get errors. Also, sieve scripts are stored per-user, there is no deeper mailbox hierarchy beneath that, so arbitrary levels of "../../../.." mailboxes won't have any effect on the user's sieve path, it's only their username that has any effect.
Independently of Cyrus usernames, email addresses can't start with a "." anyway, so ".test" and ".." are not useful as real accounts -- they won't be able to receive mail from anywhere.
elliefm
commented
6 years ago I would like this issue to resolve the following questions wrt unixhierarchyseparator:
These are both policy questions, and if changes are made, they would not take effect until a major release (such as 3.2), and would come with a thoroughly documented upgrade path for admins who might have historical folders with these names.
The other thing I would like to come out of this issue is a refactor of the sieve path code, such that all places that calculate a sieve path use the same code, and the sieve path has the dots escaped correctly. This change would not be backward compatible, so it will also be limited to a major release only, and require a proper upgrade path.
But neither of these concerns are addressable on 3.0 or earlier. I'm removing the 2.5 and 3.0 tags from this issue, since the sieve path ".." behaviour can't be fixed retroactively.
slimlv
commented
6 years ago The broken sieve paths have existed forever: the only thing commit 73af8e1 does is allow lmtpd to find the sieve scripts, by calculating paths in the same broken way that timsieved does.
I never met this bug in 2.0 - 2.5 versions where timsieved or lmtpd cant find the script. As far I know, the pathname is always contains "^" symbol instead of dot for both programm. Can you please point me where it is broken in versions prior 3.0 ?
If you upgrade to Cyrus 3.0.5 at the moment (without this commit) then your sieve scripts will be broken for users with dots in their names, because lmtpd can't find their sieve scripts. This commit fixes 3.0 for people who are already using sieve from an earlier version.
Did you think to fix lmtpd instead to keep old pathnames working as before ?
Independently of Cyrus usernames, email addresses can't start with a "." anyway, so ".test" and ".." are not useful as real accounts -- they won't be able to receive mail from anywhere.
you are wrong here, try to google how such mails handled in the real world. There is a bunch of this kind emails in wild internet although they violates RFC indeed.
I believe something was broken in 3.0 branch and you try to ugly fix the issue with no really digging into the problem. You break upgrades from previous versions with no reason.
elliefm
commented
6 years ago I believe something was broken in 3.0 branch
Yes, something was broken in 3.0 branch: lmtpd started doing . => ^ translation when looking for sieve scripts, even though no other part of the sieve system did. This bug was introduced in 3.0.0-beta2, as I've already stated previously.
Did you think to fix lmtpd instead to keep old pathnames working as before ?
This is literally what we did. The fix for #2303 returns lmtpd to its old behaviour: it no longer performs . => ^ translation when looking for sieve scripts. It therefore finds them.
I never met this bug in 2.0 - 2.5 versions where timsieved or lmtpd cant find the script.
You never saw this bug in 2.0-2.5, because the bug never existed until 3.0.0-beta2. All parts of the sieve system prior to 3.0.0-beta2 naively used raw dots, and therefore lmtpd was able to find the sieve files in the same places that timsieved did.
You break upgrades from previous versions with no reason.
Upgrades from previous versions are broken between 3.0.0-beta2 and 3.0.5. The fix, which will be included in 3.0.6, will restore the old behaviour, and therefore upgrades from 2.x => 3.0.6 will work successfully.
I would appreciate an apology for the tone of your previous message.
slimlv
commented
6 years ago I was a bit confused with previous comments. The last comment made things clean. I'm sorry for being annoying and emotional.
Originally reported by @slimlv in #2303, but I'm splitting it out into its own issue. Original text follows:
I'm wondering how mailbox's name is checking against badmboxpatterns in the code. Will newly created account "inbox" (cyrus mailbox "user.inbox") be matched ?
I have cyrus-imapd-2.5.10 - not latest nor 1993 made
then I got in mbailboxes.db the record user.^test%(A %(.test lrswipkxtecdan) I fc7adfae-be93-4579-bf87-faf217e3395b P default V 1522832294 M 1522832536) and folder "/var/spool/imap/q/user/^test" on file system.
So with commit 73af8e1 and upgrading to cyrus 3.0 I'll get folder name "/var/spool/imap/q/user/.test", right ?
and now I can do
May be other version is not affected ?
How you think, may be something is broken here ?