IMAP memory issues with use of ConstStr

Had a coredump in IMAP Search parsing.  Not sure of root cause, but parameter string parsing
  of IMAP Search coredumped on ConstStr pointer content access.

When ConstStr are created, pointer is null and length is zero.  Some code accesses the pointer
  contents without checking length first.  Not sure how parameter parsing passed a zero length
parameter, but added length checks before accessing throughout IMAP code to be safe.

While adding checks, found a typo in checking string for end of string in imap_list().
  Was using address of character instead of looking for null termination.  Corrected.

diff --git a/citadel/server/modules/imap/imap_list.c b/citadel/server/modules/imap/imap_list.c
index 7565d8e94bb37ad3e5d2047609d52b042ef1a82f..33b714912ea767506f3002ed259a1dfe32c5f267 100644 (file)
--- a/citadel/server/modules/imap/imap_list.c
+++ b/citadel/server/modules/imap/imap_list.c
@@ -236,14 +236,16 @@ void imap_list(int num_parms, ConstStr *Params)
         * selection options.  Extract their exact position, and then modify our
         * expectation of where the root folder will be specified.
         */
-       if (Params[2].Key[0] == '(') {
+       if (Params[2].len && (Params[2].Key[0] == '(')) {
                extended_list_in_use = 1;
                selection_left = 2;
                paren_nest = 0;
                for (i=2; i<num_parms; ++i) {
-                       for (j=0; Params[i].Key[j]; ++j) {
-                               if (Params[i].Key[j] == '(') ++paren_nest;
-                               if (Params[i].Key[j] == ')') --paren_nest;
+                       if (Params[i].len) {
+                               for (j=0; Params[i].Key[j]; ++j) {
+                                       if (Params[i].Key[j] == '(') ++paren_nest;
+                                       if (Params[i].Key[j] == ')') --paren_nest;
+                               }
                        }
                        if (paren_nest == 0) {
                                selection_right = i;    /* found end of selection options */
@@ -258,12 +260,12 @@ void imap_list(int num_parms, ConstStr *Params)
        if ((selection_left > 0) && (selection_right >= selection_left)) {
 
                /* Strip off the outer parentheses */
-               if (Params[selection_left].Key[0] == '(') {
+               if (Params[selection_left].len && (Params[selection_left].Key[0] == '(')) {
                        TokenCutLeft(&Imap->Cmd, 
                                     &Params[selection_left], 
                                     1);
                }
-               if (Params[selection_right].Key[Params[selection_right].len-1] == ')') {
+               if (Params[selection_right].len && (Params[selection_right].Key[Params[selection_right].len-1] == ')')) {
                        TokenCutRight(&Imap->Cmd, 
                                      &Params[selection_right], 
                                      1);
@@ -293,13 +295,15 @@ void imap_list(int num_parms, ConstStr *Params)
        patterns_left = root_pos + 1;
        patterns_right = root_pos + 1;
 
-       if (Params[patterns_left].Key[0] == '(') {
+       if (Params[patterns_left].len && (Params[patterns_left].Key[0] == '(')) {
                extended_list_in_use = 1;
                paren_nest = 0;
                for (i=patterns_left; i<num_parms; ++i) {
-                       for (j=0; &Params[i].Key[j]; ++j) {
-                               if (Params[i].Key[j] == '(') ++paren_nest;
-                               if (Params[i].Key[j] == ')') --paren_nest;
+                       if (Params[i].len) {
+                               for (j=0; Params[i].Key[j]; ++j) {
+                                       if (Params[i].Key[j] == '(') ++paren_nest;
+                                       if (Params[i].Key[j] == ')') --paren_nest;
+                               }
                        }
                        if (paren_nest == 0) {
                                patterns_right = i;     /* found end of patterns */
@@ -352,29 +356,31 @@ void imap_list(int num_parms, ConstStr *Params)
                extended_list_in_use = 1;
                paren_nest = 0;
                for (i=return_left; i<num_parms; ++i) {
-                       for (j=0;   Params[i].Key[j]; ++j) {
-                               if (Params[i].Key[j] == '(') ++paren_nest;
-                               if (Params[i].Key[j] == ')') --paren_nest;
-                       }
+                       if (Params[i].len) {
+                               for (j=0;   Params[i].Key[j]; ++j) {
+                                       if (Params[i].Key[j] == '(') ++paren_nest;
+                                       if (Params[i].Key[j] == ')') --paren_nest;
+                               }
 
-                       /* Might as well look for these while we're in here... */
-                       if (Params[i].Key[0] == '(') 
-                               TokenCutLeft(&Imap->Cmd, 
-                                            &Params[i], 
-                                            1);
-                       if (Params[i].Key[Params[i].len-1] == ')')
-                           TokenCutRight(&Imap->Cmd, 
-                                         &Params[i], 
-                                         1);
+                               /* Might as well look for these while we're in here... */
+                               if (Params[i].Key[0] == '(') 
+                                       TokenCutLeft(&Imap->Cmd, 
+                                               &Params[i], 
+                                               1);
+                               if (Params[i].len && (Params[i].Key[Params[i].len-1] == ')'))
+                               TokenCutRight(&Imap->Cmd, 
+                                               &Params[i], 
+                                               1);
 
-                       syslog(LOG_DEBUG, "evaluating <%s>", Params[i].Key);
+                               syslog(LOG_DEBUG, "evaluating <%s>", Params[i].Key);
 
-                       if (!strcasecmp(Params[i].Key, "SUBSCRIBED")) {
-                               ImapFilter.return_subscribed = 1;
-                       }
+                               if (!strcasecmp(Params[i].Key, "SUBSCRIBED")) {
+                                       ImapFilter.return_subscribed = 1;
+                               }
 
-                       else if (!strcasecmp(Params[i].Key, "CHILDREN")) {
-                               ImapFilter.return_children = 1;
+                               else if (!strcasecmp(Params[i].Key, "CHILDREN")) {
+                                       ImapFilter.return_children = 1;
+                               }
                        }
 
                        if (paren_nest == 0) {

diff --git a/citadel/server/modules/imap/imap_misc.c b/citadel/server/modules/imap/imap_misc.c
index e91d748aaa0b1a77aa55697ea9092357757d27cd..e5efc0f1e353e0a3bb9d15cb8d51e6c88e847537 100644 (file)
--- a/citadel/server/modules/imap/imap_misc.c
+++ b/citadel/server/modules/imap/imap_misc.c
@@ -264,7 +264,7 @@ void imap_do_append_flags(long new_msgnum, char *new_message_flags) {
  * This function is called by the main command loop.
  */
 void imap_append(int num_parms, ConstStr *Params) {
-       long literal_length;
+       long literal_length=0;
        struct CtdlMessage *msg = NULL;
        long new_msgnum = (-1L);
        int ret = 0;
@@ -282,7 +282,7 @@ void imap_append(int num_parms, ConstStr *Params) {
                return;
        }
 
-       if ( (Params[num_parms-1].Key[0] != '{')
+       if ( !Params[num_parms-1].len || (Params[num_parms-1].Key[0] != '{')
           || (Params[num_parms-1].Key[Params[num_parms-1].len-1] != '}') )  {
                IReply("BAD no message literal supplied");
                return;
@@ -303,7 +303,9 @@ void imap_append(int num_parms, ConstStr *Params) {
         * }
         */
 
-       literal_length = atol(&Params[num_parms-1].Key[1]);
+       if (Params[num_parms-1].len>1) {
+               literal_length = atol(&Params[num_parms-1].Key[1]);
+       }
        if (literal_length < 1) {
                IReply("BAD Message length must be at least 1.");
                return;

diff --git a/citadel/server/modules/imap/imap_search.c b/citadel/server/modules/imap/imap_search.c
index 72b57ba10e7c64b2ec3132bca57b9f44f5c217a8..03678d296db47ab9819d4b22036f1d407d01fa13 100644 (file)
--- a/citadel/server/modules/imap/imap_search.c
+++ b/citadel/server/modules/imap/imap_search.c
@@ -535,10 +535,10 @@ void imap_do_search(int num_items, ConstStr *itemlist, int is_uid) {
         * client software.  Revisit later...
         */
        for (i=0; i<num_items; ++i) {
-               if (itemlist[i].Key[0] == '(') {
+               if (itemlist[i].len && (itemlist[i].Key[0] == '(')) {
                        TokenCutLeft(&Imap->Cmd, &itemlist[i], 1);
                }
-               if (itemlist[i].Key[itemlist[i].len-1] == ')') {
+               if (itemlist[i].len && (itemlist[i].Key[itemlist[i].len-1] == ')')) {
                        TokenCutRight(&Imap->Cmd, &itemlist[i], 1);
                }
        }

diff --git a/citadel/server/modules/imap/serv_imap.c b/citadel/server/modules/imap/serv_imap.c
index fd0b93dd60218eb48043af23b3459a7e8b2537b2..30e7b2587382e2123c87465f8c0f9eeddc53dedc 100644 (file)
--- a/citadel/server/modules/imap/serv_imap.c
+++ b/citadel/server/modules/imap/serv_imap.c
@@ -555,7 +555,7 @@ void imap_login(int num_parms, ConstStr *Params) {
 
        switch (num_parms) {
        case 3:
-               if (Params[2].Key[0] == '{') {
+               if (Params[2].len && (Params[2].Key[0] == '{')) {
                        IAPuts("+ go ahead\r\n");
                        IMAP->authstate = imap_as_expecting_multilineusername;
                        strcpy(IMAP->authseq, Params[0].Key);