Make sure setacl can never set a username which breaks the DB format

[brong]

from the mailing list: ouch:

After running setaclmailbox some/mailbox "group:name with spaces" all,
all attempts to access the mailbox in any way either result in an
"Invalid identifier" error message, or can't even find the mailbox.

We shouldn't allow that!

[brong]

Would be good to backport this one to both 3.0 and 2.5.

[elliefm]

I think 3.0 and 3.1 are probably going to crash on this, rather than drop junk into the database. Look at how they handle the identifier:

cyrus-imapd/imap/mboxlist.c

Lines 1986 to 1989 in ad3b3e5

	/* round trip identifier to potentially strip domain */
	mbname_t *mbname = mbname_from_userid(identifier);
	/* XXX - enforce cross domain restrictions */
	identifier = mbname_userid(mbname);

Pretty sure that's gonna result in identifier = NULL?

Creating a Cassandane test for this to compare across versions...

[elliefm]

#1851 looks related to this

[elliefm]

Do we have any documentation or knowledge on how groups with Cyrus are expected to work? OP on the mailing list reckons this wasn't a problem for 2.4

[brong]

Ouch! For a start, that's totally b0rkened in 3.0+, because it doesn't handle groups at all.

Second: in 2.4 it used tabs to separate the ACLs, which I guess is how that worked.

In 2.5, it's a dlist - so it SHOULD be storing correctly, but maybe it's not serialising correctly.

[elliefm]

Oh I see -- the lib/acl* code still uses tab delimitations internally, but _write_acl() in imap/mboxlist.c converts those into dlist format.

[elliefm]

At this point it's not really clear to me whether it's bad data being written into the mailboxes.db, or good data that's being misparsed later.

If it's the latter, the bug might be in the dlist_parsesax implementation. It's never changed since its initial commit (other than the tabs->spaces), and nothing else uses it, so an edge-case here wouldn't turn up anywhere else that uses dlists.

Really need some data! Will ask OP on the list.

[creshal]

cyr_dbtool show spits out the following:

some.mailbox	%(A %(mailbox lrswipkxtecda group:group1 lrswipkxtecdan group:group2 lrswipkxtecda group:group with spaces lrswipkxtecdan) P default M 1527515381)
some.mailbox.Sub	%(A %(mailbox lrswipkxtecda group:group1 lrswipkxtecdan group:group2 lrswipkxtecda group:group with spaces lrswipkxtecdan) P default M 1527515381)
some.mailbox.Sub2	%(A %(mailbox lrswipkxtecda group:group1 lrswipkxtecdan group:group2 lrswipkxtecda group:group with spaces lrswipkxtecdan) P default M 1527515381)
some.mailbox.Sub3	%(A %(mailbox lrswipkxtecda group:group1 lrswipkxtecdan group:group2 lrswipkxtecda group:group with spaces lrswipkxtecdan) P default M 1527515382)
some.mailbox.Sub4	%(A %(mailbox lrswipkxtecda group:group1 lrswipkxtecdan group:group2 lrswipkxtecda group:group with spaces lrswipkxtecdan) P default M 1527515382)
some.mailbox.Sub5	%(A %(mailbox lrswipkxtecda group:group1 lrswipkxtecdan group:group2 lrswipkxtecda group:group with spaces lrswipkxtecdan) P default M 1527515382)

cyrus.header looks like this:

Cyrus mailbox header
"The best thing about this system was that it had lots of goals."
	--Jim Morris on Andrew
	118c711353ba6de0
$Forwarded Junk NonJunk
mailbox	lrswipkxtecda	group:group1	lrswipkxtecdan	group:group2	lrswipkxtecda	group:group with spaces	lrswipkxtecdan	

(In case github eats formatting: Records are separated with \t, spaces in the group name are spaces, and there's a trailing \t at the end of the line)

---

I managed to redirect incoming mails to a new mailbox, and nobody currently needs the mails in this one, so I'm in no rush to repair it. If you need any other data, just poke me here or via email.

[creshal]

OP on the mailing list reckons this wasn't a problem for 2.4

No, I don't think it ever worked in older versions. I can't remember trying, at least.

[karagian]

No, I don't think it ever worked in older versions. I can't remember trying, at least.

I can confirm that there was no problem with acls regarding groups contains spaces in version 2.4. I had tested it when I reported #1851

[elliefm]

some.mailbox %(A %(mailbox lrswipkxtecda group:group1 lrswipkxtecdan group:group2 lrswipkxtecda group:group with spaces lrswipkxtecdan) P default M 1527515381)

Okay, that looks like it's being written badly to the mailboxes db, which surprises me because reading the code, I thought it was tabs in there too. So the deserialisation is breaking because the serialisation was bad. That's progress, thanks! :)

The cyrus.header looks fine to me, but I don't think it gets looked at on this code path.

[elliefm]

Okay I've found the cause, 2.5 is currently missing this commit: 0565460

In the dlist for the A entry, the identifier/rights pairs are key/value pairs, but 2.5 dlist_print() doesn't atomise keys (only values), so when the key contains spaces it gets corrupted.

[elliefm]

That commit is now backported to the 2.5 branch as 59ee719, and will be in the next 2.5 release. I think it will probably apply cleanly onto 2.5.10 sources in the meantime, but ymmv.

Now to unbork 3.0/master...

[elliefm]

I've added some tests, and verified that 3.0 and master are fine :)