>From 463272eab866a36162fe51813327ca7af2f37ca0 Mon Sep 17 00:00:00 2001 From: Oswald Buddenhagen Date: Wed, 24 Nov 2021 19:21:48 +0100 Subject: [PATCH 1/5] CVE-2021-3657: reject excessively large IMAP literals we didn't limit the 32-bit size of literals so far, which, given that we use int-sized lengths & offsets, permitted all kinds of buffer overflows. malicious/compromised servers may have been able to exploit this. actual email senders would be constrained by size limits for delivered mails, and to cause more than a crash they'd have to predict the exact size of the final message. we now limit to 2GB, which, given that we use unsigned ints since e2d3b4d55 (v1.4.0), gives the handlers downstream plenty of headroom. an alternative would have been using 64-bit offsets, but this seems like major overkill, even if IMAP4rev2 recently mandated it (we talk only IMAP4rev1, so we can ignore it). --- src/drv_imap.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/drv_imap.c b/src/drv_imap.c index bd7a0f2..bb71506 100644 --- a/src/drv_imap.c +++ b/src/drv_imap.c @@ -877,6 +877,11 @@ parse_imap_list( imap_store_t *ctx, char **sp, parse_list_state_t *sts ) bytes = (int)(cur->len = strtoul( s + 1, &s, 10 )); if (*s != '}' || *++s) goto bail; + if ((uint)bytes >= INT_MAX) { + error( "IMAP error: excessively large literal from %s " + "- THIS MIGHT BE AN ATTEMPT TO HACK YOU!\n", ctx->conn.name ); + goto bail; + } s = cur->val = nfmalloc( cur->len + 1 ); s[cur->len] = 0; -- 2.33.1.11.g2e4d00c830 >From ba13362a52d8749731ba645e5e50e47862a5b91d Mon Sep 17 00:00:00 2001 From: Oswald Buddenhagen Date: Wed, 24 Nov 2021 17:22:04 +0100 Subject: [PATCH 2/5] deal with oversized messages in maildirs don't try to read messages > 2G, as that will only lead to trouble down the line. this wouldn't have worked on linux anyway (we read in one chunk, and that is limited to (2^31 - 2^12) on all architectures), but on platforms were big reads work, this was a security problem if one synchronized other users' maildirs. as a minor fix on the side, we now also clip the reported message size, so MaxSize works for excessively big messages. --- src/drv_maildir.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/drv_maildir.c b/src/drv_maildir.c index ea4195d..f916632 100644 --- a/src/drv_maildir.c +++ b/src/drv_maildir.c @@ -1168,7 +1168,8 @@ maildir_scan( maildir_store_t *ctx, msg_t_array_alloc_t *msglist ) } goto retry; } - entry->size = (uint)st.st_size; + // The clipped value is good enough for MaxSize comparisons. + entry->size = st.st_size > UINT_MAX ? UINT_MAX : (uint)st.st_size; } if (want_tuid || want_msgid) { if (!(f = fopen( buf, "r" ))) { @@ -1563,12 +1564,17 @@ maildir_fetch_msg( store_t *gctx, message_t *gmsg, msg_data_t *data, int minimal } } fstat( fd, &st ); + if (st.st_size > INT_MAX) { + error( "Maildir error: %s is too big", buf ); + goto mbad; + } data->len = st.st_size; if (data->date == -1) data->date = st.st_mtime; data->data = nfmalloc( data->len ); if (read( fd, data->data, data->len ) != data->len) { sys_error( "Maildir error: cannot read %s", buf ); + mbad: close( fd ); cb( DRV_MSG_BAD, aux ); return; -- 2.33.1.11.g2e4d00c830 >From bc15e571b650270b87e9758916f93eab04992cef Mon Sep 17 00:00:00 2001 From: Oswald Buddenhagen Date: Wed, 24 Nov 2021 17:46:43 +0100 Subject: [PATCH 3/5] report conversion errors directly in copy_msg_convert() that makes it easier to report various conditions without introducing separate error codes. --- src/sync.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/sync.c b/src/sync.c index 79dc223..978521c 100644 --- a/src/sync.c +++ b/src/sync.c @@ -406,7 +406,7 @@ copy_msg_bytes( char **out_ptr, const char *in_buf, uint *in_idx, uint in_len, i } static int -copy_msg_convert( int in_cr, int out_cr, copy_vars_t *vars ) +copy_msg_convert( int in_cr, int out_cr, copy_vars_t *vars, int t ) { char *in_buf = vars->data.data; uint in_len = vars->data.len; @@ -451,7 +451,8 @@ copy_msg_convert( int in_cr, int out_cr, copy_vars_t *vars ) goto nloop; } } - /* invalid message */ + warn( "Warning: message %u from %s has incomplete header; skipping.\n", + vars->msg->uid, str_fn[1-t] ); free( in_buf ); return 0; oke: @@ -556,9 +557,7 @@ msg_fetched( int sts, void *aux ) scr = (svars->drv[1-t]->get_caps( svars->ctx[1-t] ) / DRV_CRLF) & 1; tcr = (svars->drv[t]->get_caps( svars->ctx[t] ) / DRV_CRLF) & 1; if (vars->srec || scr != tcr) { - if (!copy_msg_convert( scr, tcr, vars )) { - warn( "Warning: message %u from %s has incomplete header.\n", - vars->msg->uid, str_fn[1-t] ); + if (!copy_msg_convert( scr, tcr, vars, t )) { vars->cb( SYNC_NOGOOD, 0, vars ); return; } -- 2.33.1.11.g2e4d00c830 >From 92921b1d3b7262eaa0fbb095cc714098b431c2f9 Mon Sep 17 00:00:00 2001 From: Oswald Buddenhagen Date: Wed, 24 Nov 2021 17:51:06 +0100 Subject: [PATCH 4/5] reject messages that grow too large due to conversion that shouldn't really be a problem, as we have 2GB of headroom, and most growth would happen when sending an all-newlines message from maildir to imap (due to CR additions), which is mostly non-critical. but better safe than sorry. --- src/sync.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/sync.c b/src/sync.c index 978521c..32d4fa1 100644 --- a/src/sync.c +++ b/src/sync.c @@ -494,6 +494,12 @@ copy_msg_convert( int in_cr, int out_cr, copy_vars_t *vars, int t ) } vars->data.len = in_len + extra; + if (vars->data.len > INT_MAX) { + warn( "Warning: message %u from %s is too big after conversion; skipping.\n", + vars->msg->uid, str_fn[1-t] ); + free( in_buf ); + return 0; + } char *out_buf = vars->data.data = nfmalloc( vars->data.len ); idx = 0; if (vars->srec) { -- 2.33.1.11.g2e4d00c830 >From 127003ee37e3eb6d914782be43097338baa32d2b Mon Sep 17 00:00:00 2001 From: Oswald Buddenhagen Date: Wed, 24 Nov 2021 18:24:00 +0100 Subject: [PATCH 5/5] reject unreasonably long mailbox names from IMAP LIST this wasn't really a security problem, as the name mapping we actually do does not change the string length, and the iteration was already safe after the literal length fix, but it's still better to catch weird input. --- src/drv_imap.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/drv_imap.c b/src/drv_imap.c index bb71506..c5a7aed 100644 --- a/src/drv_imap.c +++ b/src/drv_imap.c @@ -1439,6 +1439,10 @@ parse_list_rsp_p2( imap_store_t *ctx, list_t *list, char *cmd ATTR_UNUSED ) } arg = list->val; argl = (int)list->len; + if (argl > 1000) { + warn( "IMAP warning: ignoring unreasonably long mailbox name '%.100s[...]'\n", arg ); + return LIST_OK; + } // The server might be weird and have a non-uppercase INBOX. It // may legitimately do so, but we need the canonical spelling. normalize_INBOX( ctx, arg, argl ); -- 2.33.1.11.g2e4d00c830