From b66d51b7bac2004ac436c287bb25a4bfa5c5ea95 Mon Sep 17 00:00:00 2001 From: Harlow Solutions Date: Thu, 20 Jul 2023 14:40:56 -0400 Subject: [PATCH] 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. --- citadel/server/modules/imap/imap_list.c | 64 +++++++++++++---------- citadel/server/modules/imap/imap_misc.c | 8 +-- citadel/server/modules/imap/imap_search.c | 4 +- citadel/server/modules/imap/serv_imap.c | 2 +- 4 files changed, 43 insertions(+), 35 deletions(-) diff --git a/citadel/server/modules/imap/imap_list.c b/citadel/server/modules/imap/imap_list.c index 7565d8e94..33b714912 100644 --- 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 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; iCmd, - &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 e91d748aa..e5efc0f1e 100644 --- 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 72b57ba10..03678d296 100644 --- 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; iCmd, &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 fd0b93dd6..30e7b2587 100644 --- 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); -- 2.39.2