Your Cyrus imapd ACL patch

[Greg Banks]

G'day,

Resending to the list in hopes of reaching Guilherme; the email address 
in the commit message is bouncing.

-------- Original Message --------
Subject: 	Your Cyrus imapd ACL patch
Date: 	Tue, 09 Aug 2011 16:47:23 +1000
From: 	Greg Banks <gnb at fastmail.fm>
To: 	Guilherme Maciel Ferreira <guilherme.maciel.ferreira at intra2net.com>
CC: 	Jeroen van Meeuwen (Kolab Systems) <vanmeeuwen at kolabsys.com>



G'day,

I've been using the Cassandane test suite to test a version of Cyrus
imapd containing your patch

http://git.cyrusimap.org/cyrus-imapd/commit/?id=4412656e218a42559964ccdce06e8daefb8197c5

(cc'ing Jeroen who committed the patch) and I'm seeing the following
Valgrind errors

Conditional jump or move depends on uninitialised value(s)
    at 0x45056E: mboxlist_is_owner (mboxlist.c:1354)
    by 0x450A3D: mboxlist_setacl (mboxlist.c:1451)
    by 0x41DEFB: cmd_setacl (imapd.c:7188)
    by 0x40DA48: cmdloop (imapd.c:1915)
    by 0x40A928: service_main (imapd.c:970)
    by 0x4094CD: main (service.c:570)

Conditional jump or move depends on uninitialised value(s)
    at 0x450584: mboxlist_is_owner (mboxlist.c:1354)
    by 0x450A3D: mboxlist_setacl (mboxlist.c:1451)
    by 0x41DEFB: cmd_setacl (imapd.c:7188)
    by 0x40DA48: cmdloop (imapd.c:1915)
    by 0x40A928: service_main (imapd.c:970)
    by 0x4094CD: main (service.c:570)



where

1346 static int mboxlist_is_owner(const char *name, int domainlen,
1347                              const char *user, int userlen)
1348 {
1349     char *cp = NULL;
1350
1351     int enough_length = !strncmp(name+domainlen, "user.", 5);
1352     int user_complete = (!(cp = strchr(user, '.')) || (cp - user)>   userlen);
1353     int match_strings = !strncmp(name+domainlen+5, user, userlen);
1354     int match_length = (name[domainlen+5+userlen] == '\0' ||<---------
1355                         name[domainlen+5+userlen] == '.');
1356
1357     return (enough_length&&   user_complete&&   match_strings&&   match_length);
1358 }

and

1449     /* checks if the mailbox belongs to the user who is trying to change the
1450        access rights */
1451     if (mboxlist_is_owner(name, domainlen, userid, useridlen)) {
1452         isusermbox = 1;
1453     }
1454     anyoneuseracl = config_getswitch(IMAPOPT_ANYONEUSERACL);
1455
1456     /* checks if the identifier is the mailbox owner */
1457     if (mboxlist_is_owner(name, domainlen, identifier, identifierlen)) {<-------
1458         isidentifiermbox = 1;
1459     }
1460
1461     /* who is the mailbox owner? */
1462     if (isusermbox) {
1463         mailbox_owner = userid;
1464     }
1465     else if (isidentifiermbox) {
1466         mailbox_owner = identifier;
1467     }



I can't quite work out what the code in mboxlist_setacl() thinks it's
supposed to be doing.  The variable 'identifierlen' is initially set to
strlen(identifier) but then the variable 'identifier' is reassigned on
some paths without changing 'identifierlen', and then both 'identifier'
and 'identifierlen' are passed to mboxlist_is_owner() which uses one to
index the other.  Something is clearly broken but I can't quite tell what.

To help your debugging, I instrumented imapd to list the arguments to
cmd_setacl() to the Valgrind log:

**29768** cmd_setacl(tag="3" name="user.cassandane" identifier="admin" rights="lrswipkxtecda") imapd_userid="admin"
**29768** cmd_setacl(tag="4" name="user.cassandane" identifier="cassandane" rights="lrswipkxtecd") imapd_userid="admin"
**29768** cmd_setacl(tag="5" name="user.cassandane" identifier="anyone" rights="p") imapd_userid="admin"
**29768** cmd_setacl(tag="5" name="user.base" identifier="admin" rights="lrswipkxtecda") imapd_userid="admin"
==29768== Conditional jump or move depends on uninitialised value(s)
==29768==    at 0x45082E: mboxlist_is_owner (mboxlist.c:1354)
==29768==    by 0x450CFD: mboxlist_setacl (mboxlist.c:1451)
==29768==    by 0x41E1B9: cmd_setacl (imapd.c:7192)
==29768==    by 0x40DCC6: cmdloop (imapd.c:1916)
==29768==    by 0x40ABA6: service_main (imapd.c:971)
==29768==    by 0x4094CD: main (service.c:570)
==29768==
==29768== Conditional jump or move depends on uninitialised value(s)
==29768==    at 0x450844: mboxlist_is_owner (mboxlist.c:1354)
==29768==    by 0x450CFD: mboxlist_setacl (mboxlist.c:1451)
==29768==    by 0x41E1B9: cmd_setacl (imapd.c:7192)
==29768==    by 0x40DCC6: cmdloop (imapd.c:1916)
==29768==    by 0x40ABA6: service_main (imapd.c:971)
==29768==    by 0x4094CD: main (service.c:570)
==29768==
==29768== Conditional jump or move depends on uninitialised value(s)
==29768==    at 0x45082E: mboxlist_is_owner (mboxlist.c:1354)
==29768==    by 0x450D3F: mboxlist_setacl (mboxlist.c:1457)
==29768==    by 0x41E1B9: cmd_setacl (imapd.c:7192)
==29768==    by 0x40DCC6: cmdloop (imapd.c:1916)
==29768==    by 0x40ABA6: service_main (imapd.c:971)
==29768==    by 0x4094CD: main (service.c:570)
==29768==
==29768== Conditional jump or move depends on uninitialised value(s)
==29768==    at 0x450844: mboxlist_is_owner (mboxlist.c:1354)
==29768==    by 0x450D3F: mboxlist_setacl (mboxlist.c:1457)
==29768==    by 0x41E1B9: cmd_setacl (imapd.c:7192)
==29768==    by 0x40DCC6: cmdloop (imapd.c:1916)
==29768==    by 0x40ABA6: service_main (imapd.c:971)
==29768==    by 0x4094CD: main (service.c:570)
==29768==
**29768** cmd_setacl(tag="6" name="user.base.subdir2" identifier="admin" rights="lrswipkxtecda") imapd_userid="admin"
**29768** cmd_setacl(tag="26" name="user.baseplus" identifier="admin" rights="lrswipkxtecda") imapd_userid="admin"
**29768** cmd_setacl(tag="27" name="user.baseplus.subdir" identifier="admin" rights="lrswipkxtecda") imapd_userid="admin"


Hmm, it looks like perhaps mboxlist_is_owner() is walking off the end of
the 'name' string when the folder name is user.foo and the length of
'foo' is less than the length of the current logged in user.

Any chance you could fix this?

-- 
Greg.


-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.andrew.cmu.edu/pipermail/cyrus-devel/attachments/20110809/3164da36/attachment.html