lobo/isync
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

CVE-2021-3578: fix handling of unexpected APPENDUID response code

Browse Source 
if the code was sent in response to anything but a STORE, we'd overwrite
a data pointer in one of our imap_cmd subclasses, an allocator data
structure, or the start of the next allocation, with an int that was
completely under the server's control. it's plausible that this could be
exploited for remote code execution.

to avoid this, we could ensure that the object is of the right type
prior to casting, by using a new flag in the parameter block. but it's
easier to just dispose of the out_uid field altogether and reuse the uid
field that is present in the parameter block anyway, but was used only
for FETCH commands so far.

this problem was found by Lukas Braun <koomi@moshbit.net> using a
fuzzer.
This commit is contained in:
Oswald Buddenhagen 2021-04-14 16:58:27 +02:00
parent d55ced04ed
commit 589d2ed428
1 changed files with 14 additions and 5 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

19
src/drv_imap.c
View File

@ -181,7 +181,6 @@ typedef struct {
imap_cmd_t gen; imap_cmd_t gen;
void (*callback)( int sts, uint uid, void *aux ); void (*callback)( int sts, uint uid, void *aux );
void *callback_aux; void *callback_aux;
uint out_uid;
} imap_cmd_out_uid_t; } imap_cmd_out_uid_t;
typedef struct { typedef struct {
@ -1184,11 +1183,22 @@ parse_response_code( imap_store_t *ctx, imap_cmd_t *cmd, char *s )
*/ */
for (; isspace( (uchar)*p ); p++); for (; isspace( (uchar)*p ); p++);
error( "*** IMAP ALERT *** %s\n", p ); error( "*** IMAP ALERT *** %s\n", p );
} else if (cmd && !strcmp( "APPENDUID", arg )) { } else if (!strcmp( "APPENDUID", arg )) {
// The checks ensure that:
// - cmd => this is the final tagged response of a command, at which
// point cmd was already removed from ctx->in_progress, so param.uid
// is available for reuse.
// - !param.uid => the command isn't actually a FETCH. This doesn't
// really matter, as the field is safe to overwrite given the
// previous condition; it just has no effect for non-APPENDs.
if (!cmd || cmd->param.uid) {
error( "IMAP error: unexpected APPENDUID status\n" );
return RESP_CANCEL;
}
if (!(arg = next_arg( &s )) || if (!(arg = next_arg( &s )) ||
(ctx->uidvalidity = strtoul( arg, &earg, 10 ), *earg) || (ctx->uidvalidity = strtoul( arg, &earg, 10 ), *earg) ||
!(arg = next_arg( &s )) || !(arg = next_arg( &s )) ||
(((imap_cmd_out_uid_t *)cmd)->out_uid = strtoul( arg, &earg, 10 ), *earg)) (cmd->param.uid = strtoul( arg, &earg, 10 ), *earg))
{ {
error( "IMAP error: malformed APPENDUID status\n" ); error( "IMAP error: malformed APPENDUID status\n" );
return RESP_CANCEL; return RESP_CANCEL;
@ -2957,7 +2967,6 @@ imap_store_msg( store_t *gctx, msg_data_t *data, int to_trash,
ctx->buffer_mem += data->len; ctx->buffer_mem += data->len;
cmd->gen.param.data_len = data->len; cmd->gen.param.data_len = data->len;
cmd->gen.param.data = data->data; cmd->gen.param.data = data->data;
cmd->out_uid = 0;
if (to_trash) { if (to_trash) {
cmd->gen.param.create = 1; cmd->gen.param.create = 1;
@ -2990,7 +2999,7 @@ imap_store_msg_p2( imap_store_t *ctx ATTR_UNUSED, imap_cmd_t *cmd, int response
imap_cmd_out_uid_t *cmdp = (imap_cmd_out_uid_t *)cmd; imap_cmd_out_uid_t *cmdp = (imap_cmd_out_uid_t *)cmd;
transform_msg_response( &response ); transform_msg_response( &response );
cmdp->callback( response, cmdp->out_uid, cmdp->callback_aux ); cmdp->callback( response, cmdp->gen.param.uid, cmdp->callback_aux );
} }
/******************* imap_find_new_msgs *******************/ /******************* imap_find_new_msgs *******************/