From: Timo Sirainen Date: Thu, 1 Apr 2021 17:39:27 +0000 (+0300) Subject: lib-imap: imap_bodystructure_write() - Return error on corruption instead of assert... X-Git-Tag: 2.3.15~54 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=2cd90ef78ab9e5fb7b4daf1ee684ab473f59fe67;p=thirdparty%2Fdovecot%2Fcore.git lib-imap: imap_bodystructure_write() - Return error on corruption instead of assert-crash This could happen if broken message_parts came from cache and message_part->data was newly read from the mail input. --- diff --git a/src/lib-imap-storage/imap-msgpart.c b/src/lib-imap-storage/imap-msgpart.c index 024fa6edd5..3bce11754b 100644 --- a/src/lib-imap-storage/imap-msgpart.c +++ b/src/lib-imap-storage/imap-msgpart.c @@ -812,6 +812,7 @@ int imap_msgpart_bodypartstructure(struct mail *mail, { struct message_part *all_parts, *part; string_t *bpstruct; + const char *error; int ret; /* if we start parsing the body in here, make sure we also parse the @@ -840,7 +841,13 @@ int imap_msgpart_bodypartstructure(struct mail *mail, if (ret >= 0) { bpstruct = t_str_new(256); - imap_bodystructure_write(part, bpstruct, TRUE); + if (imap_bodystructure_write(part, bpstruct, TRUE, &error) < 0) { + error = t_strdup_printf( + "Invalid message_part/BODYSTRUCTURE: %s", error); + mail_set_cache_corrupted(mail, MAIL_FETCH_MESSAGE_PARTS, + error); + return -1; + } *bpstruct_r = str_c(bpstruct); } return ret < 0 ? -1 : 1; diff --git a/src/lib-imap/fuzz-imap-bodystructure.c b/src/lib-imap/fuzz-imap-bodystructure.c index 65a3be54ee..a9d4499924 100644 --- a/src/lib-imap/fuzz-imap-bodystructure.c +++ b/src/lib-imap/fuzz-imap-bodystructure.c @@ -28,7 +28,8 @@ FUZZ_BEGIN_STR(const char *str) i_zero(&parts); if (imap_bodystructure_parse(str, pool, &parts, &error) == 0) { - imap_bodystructure_write(&parts, dest, TRUE); + if (imap_bodystructure_write(&parts, dest, TRUE, &error) < 0) + i_panic("Failed to write bodystructure: %s", error); /* The written bodystructure must be parseable *and* it must come out exactly the same again */ if (imap_bodystructure_parse(str_c(dest), pool, &parts, &error) != 0) { @@ -37,7 +38,8 @@ FUZZ_BEGIN_STR(const char *str) } else { const char *new_str = t_strdup(str_c(dest)); str_truncate(dest, 0); - imap_bodystructure_write(&parts, dest, TRUE); + if (imap_bodystructure_write(&parts, dest, TRUE, &error) < 0) + i_panic("Failed to write reparsed bodystructure: %s", error); if (strcmp(str_c(dest), new_str) != 0) { i_panic("Parsed bodystructure '%s' does not match '%s'", str_sanitize_binary(new_str), diff --git a/src/lib-imap/imap-bodystructure.c b/src/lib-imap/imap-bodystructure.c index d00b162c54..82028db654 100644 --- a/src/lib-imap/imap-bodystructure.c +++ b/src/lib-imap/imap-bodystructure.c @@ -60,15 +60,18 @@ params_write(const struct message_part_param *params, str_append_c(str, ')'); } -static void +static int part_write_bodystructure_siblings(const struct message_part *part, - string_t *dest, bool extended) + string_t *dest, bool extended, + const char **error_r) { for (; part != NULL; part = part->next) { str_append_c(dest, '('); - imap_bodystructure_write(part, dest, extended); + if (imap_bodystructure_write(part, dest, extended, error_r) < 0) + return -1; str_append_c(dest, ')'); } + return 0; } static void @@ -111,16 +114,19 @@ part_write_bodystructure_common(const struct message_part_data *data, imap_append_nstring_nolf(str, data->content_location); } -static void part_write_body_multipart(const struct message_part *part, - string_t *str, bool extended) +static int part_write_body_multipart(const struct message_part *part, + string_t *str, bool extended, + const char **error_r) { const struct message_part_data *data = part->data; i_assert(part->data != NULL); - if (part->children != NULL) - part_write_bodystructure_siblings(part->children, str, extended); - else { + if (part->children != NULL) { + if (part_write_bodystructure_siblings(part->children, str, + extended, error_r) < 0) + return -1; + } else { /* no parts in multipart message, that's not allowed. write a single 0-length text/plain structure */ @@ -134,7 +140,7 @@ static void part_write_body_multipart(const struct message_part *part, imap_append_string(str, data->content_subtype); if (!extended) - return; + return 0; /* BODYSTRUCTURE data */ @@ -143,6 +149,7 @@ static void part_write_body_multipart(const struct message_part *part, data->content_type_params_count, str, FALSE); part_write_bodystructure_common(data, str); + return 0; } static bool part_is_truncated(const struct message_part *part) @@ -176,8 +183,8 @@ static bool part_is_truncated(const struct message_part *part) return FALSE; } -static void part_write_body(const struct message_part *part, - string_t *str, bool extended) +static int part_write_body(const struct message_part *part, + string_t *str, bool extended, const char **error_r) { const struct message_part_data *data = part->data; bool text; @@ -206,7 +213,11 @@ static void part_write_body(const struct message_part *part, str_append_c(str, ' '); imap_append_string(str, data->content_subtype); } - i_assert(text == ((part->flags & MESSAGE_PART_FLAG_TEXT) != 0)); + bool part_is_text = (part->flags & MESSAGE_PART_FLAG_TEXT) != 0; + if (text != part_is_text) { + *error_r = "text flag mismatch"; + return -1; + } } /* ("content type param key" "value" ...) */ @@ -241,12 +252,14 @@ static void part_write_body(const struct message_part *part, imap_envelope_write(child_data->envelope, str); str_append(str, ") "); - part_write_bodystructure_siblings(part->children, str, extended); + if (part_write_bodystructure_siblings(part->children, str, + extended, error_r) < 0) + return -1; str_printfa(str, " %u", part->body_size.lines); } if (!extended) - return; + return 0; /* BODYSTRUCTURE data */ @@ -255,15 +268,17 @@ static void part_write_body(const struct message_part *part, str_append_c(str, ' '); imap_append_nstring_nolf(str, data->content_md5); part_write_bodystructure_common(data, str); + return 0; } -void imap_bodystructure_write(const struct message_part *part, - string_t *dest, bool extended) +int imap_bodystructure_write(const struct message_part *part, + string_t *dest, bool extended, + const char **error_r) { if ((part->flags & MESSAGE_PART_FLAG_MULTIPART) != 0) - part_write_body_multipart(part, dest, extended); + return part_write_body_multipart(part, dest, extended, error_r); else - part_write_body(part, dest, extended); + return part_write_body(part, dest, extended, error_r); } /* diff --git a/src/lib-imap/imap-bodystructure.h b/src/lib-imap/imap-bodystructure.h index 7b1390b40a..a7cc6cda7b 100644 --- a/src/lib-imap/imap-bodystructure.h +++ b/src/lib-imap/imap-bodystructure.h @@ -7,9 +7,11 @@ struct imap_arg; /* Write a BODY/BODYSTRUCTURE from given message_part. The message_part->data field must be set. part->body_size.virtual_size and .lines are also used - for writing it. */ -void imap_bodystructure_write(const struct message_part *part, - string_t *dest, bool extended); + for writing it. Returns 0 on success, -1 if parts don't internally match + (e.g. broken cached mime.parts mixed with parsed message). */ +int imap_bodystructure_write(const struct message_part *part, + string_t *dest, bool extended, + const char **error_r); /* Parse BODYSTRUCTURE and save the contents to message_part->data for each message tree node. If the parts argument points to NULL, the message_part diff --git a/src/lib-imap/test-imap-bodystructure.c b/src/lib-imap/test-imap-bodystructure.c index c0474c5db3..6035118abd 100644 --- a/src/lib-imap/test-imap-bodystructure.c +++ b/src/lib-imap/test-imap-bodystructure.c @@ -414,6 +414,7 @@ msg_parse(pool_t pool, const char *message, unsigned int max_nested_mime_parts, static void test_imap_bodystructure_write(void) { struct message_part *parts; + const char *error; unsigned int i; for (i = 0; i < parse_tests_count; i++) T_BEGIN { @@ -424,16 +425,31 @@ static void test_imap_bodystructure_write(void) test_begin(t_strdup_printf("imap bodystructure write [%u]", i)); parts = msg_parse(pool, test->message, 0, 0, TRUE); - imap_bodystructure_write(parts, str, TRUE); + test_assert(imap_bodystructure_write(parts, str, TRUE, &error) == 0); test_assert(strcmp(str_c(str), test->bodystructure) == 0); str_truncate(str, 0); - imap_bodystructure_write(parts, str, FALSE); + test_assert(imap_bodystructure_write(parts, str, FALSE, &error) == 0); test_assert(strcmp(str_c(str), test->body) == 0); pool_unref(&pool); test_end(); } T_END; + + T_BEGIN { + test_begin("imap bodystructure write - corrupted"); + pool_t pool = pool_alloconly_create("imap bodystructure write", 1024); + + parts = msg_parse(pool, "Subject: hello world", 0, 0, TRUE); + i_assert((parts->flags & MESSAGE_PART_FLAG_TEXT) != 0); + parts->flags &= ENUM_NEGATE(MESSAGE_PART_FLAG_TEXT); + + string_t *str = t_str_new(128); + test_assert(imap_bodystructure_write(parts, str, FALSE, &error) < 0); + test_assert_strcmp(error, "text flag mismatch"); + pool_unref(&pool); + test_end(); + } T_END; } static void test_imap_bodystructure_parse(void) @@ -461,7 +477,7 @@ static void test_imap_bodystructure_parse(void) if (ret == 0) { str_truncate(str, 0); - imap_bodystructure_write(parts, str, TRUE); + test_assert(imap_bodystructure_write(parts, str, TRUE, &error) == 0); test_assert(strcmp(str_c(str), test->bodystructure) == 0); } else { i_error("Invalid BODYSTRUCTURE: %s", error); @@ -564,7 +580,7 @@ static void test_imap_bodystructure_parse_full(void) if (ret == 0) { str_truncate(str, 0); - imap_bodystructure_write(parts, str, TRUE); + test_assert(imap_bodystructure_write(parts, str, TRUE, &error) == 0); test_assert(strcmp(str_c(str), test->bodystructure) == 0); } else { i_error("Invalid BODYSTRUCTURE: %s", error); @@ -596,7 +612,7 @@ static void test_imap_bodystructure_normalize(void) if (ret == 0) { str_truncate(str, 0); - imap_bodystructure_write(parts, str, TRUE); + test_assert(imap_bodystructure_write(parts, str, TRUE, &error) == 0); test_assert(strcmp(str_c(str), test->output) == 0); } else { i_error("Invalid BODYSTRUCTURE: %s", error); @@ -685,7 +701,7 @@ static void test_imap_bodystructure_truncation(void) TRUE); /* write out BODYSTRUCTURE and serialize message_parts */ - imap_bodystructure_write(parts, str_body, TRUE); + test_assert(imap_bodystructure_write(parts, str_body, TRUE, &error) == 0); message_part_serialize(parts, str_parts); /* now deserialize message_parts and make sure they can be used diff --git a/src/lib-storage/index/imapc/imapc-mail-fetch.c b/src/lib-storage/index/imapc/imapc-mail-fetch.c index 663748bde7..4275c657f0 100644 --- a/src/lib-storage/index/imapc/imapc-mail-fetch.c +++ b/src/lib-storage/index/imapc/imapc-mail-fetch.c @@ -8,6 +8,7 @@ #include "istream-header-filter.h" #include "message-header-parser.h" #include "imap-arg.h" +#include "imap-util.h" #include "imap-date.h" #include "imap-quote.h" #include "imap-bodystructure.h" @@ -806,7 +807,16 @@ imapc_args_to_bodystructure(struct imapc_mail *mail, ret = NULL; } else { string_t *str = t_str_new(128); - imap_bodystructure_write(parts, str, extended); + if (imap_bodystructure_write(parts, str, extended, &error) < 0) { + /* All the input to imap_bodystructure_write() came + from imap_bodystructure_parse_args(). We should never + get here. Instead, if something is wrong the + parsing should have returned an error already. */ + str_truncate(str, 0); + imap_write_args(str, args); + i_panic("Failed to write parsed BODYSTRUCTURE: %s " + "(original string: '%s')", error, str_c(str)); + } ret = p_strdup(mail->imail.mail.data_pool, str_c(str)); } pool_unref(&pool); diff --git a/src/lib-storage/index/index-mail.c b/src/lib-storage/index/index-mail.c index 5faa62fa42..8804006c5c 100644 --- a/src/lib-storage/index/index-mail.c +++ b/src/lib-storage/index/index-mail.c @@ -808,6 +808,21 @@ static void index_mail_body_parsed_cache_message_parts(struct index_mail *mail) data->messageparts_saved_to_cache = TRUE; } +static int +index_mail_write_bodystructure(struct index_mail *mail, string_t *str, + bool extended) +{ + const char *error; + + if (imap_bodystructure_write(mail->data.parts, str, extended, + &error) < 0) { + mail_set_cache_corrupted(&mail->mail.mail, + MAIL_FETCH_MESSAGE_PARTS, error); + return -1; + } + return 0; +} + static void index_mail_body_parsed_cache_bodystructure(struct index_mail *mail, enum index_cache_field field) @@ -856,12 +871,12 @@ index_mail_body_parsed_cache_bodystructure(struct index_mail *mail, } if (cache_bodystructure) { str = str_new(mail->mail.data_pool, 128); - imap_bodystructure_write(data->parts, str, TRUE); - data->bodystructure = str_c(str); - - index_mail_cache_add(mail, MAIL_CACHE_IMAP_BODYSTRUCTURE, - str_c(str), str_len(str)); - bodystructure_cached = TRUE; + if (index_mail_write_bodystructure(mail, str, TRUE) == 0) { + data->bodystructure = str_c(str); + index_mail_cache_add(mail, MAIL_CACHE_IMAP_BODYSTRUCTURE, + str_c(str), str_len(str)); + bodystructure_cached = TRUE; + } } else { bodystructure_cached = mail_cache_field_exists(_mail->transaction->cache_view, @@ -888,11 +903,11 @@ index_mail_body_parsed_cache_bodystructure(struct index_mail *mail, if (cache_body) { str = str_new(mail->mail.data_pool, 128); - imap_bodystructure_write(data->parts, str, FALSE); - data->body = str_c(str); - - index_mail_cache_add(mail, MAIL_CACHE_IMAP_BODY, - str_c(str), str_len(str)); + if (index_mail_write_bodystructure(mail, str, FALSE) == 0) { + data->body = str_c(str); + index_mail_cache_add(mail, MAIL_CACHE_IMAP_BODY, + str_c(str), str_len(str)); + } } } @@ -1454,6 +1469,16 @@ static int index_mail_parse_bodystructure(struct index_mail *mail, } else { if (index_mail_parse_bodystructure_full(mail, field) < 0) return -1; + if (data->parts == NULL) { + /* Corrupted mime.parts detected. Retry by parsing + the mail. */ + data->parsed_bodystructure = FALSE; + data->parsed_bodystructure_header = FALSE; + data->save_bodystructure_header = TRUE; + data->save_bodystructure_body = TRUE; + if (index_mail_parse_bodystructure_full(mail, field) < 0) + return -1; + } } i_assert(data->parts != NULL); @@ -1463,14 +1488,16 @@ static int index_mail_parse_bodystructure(struct index_mail *mail, case MAIL_CACHE_IMAP_BODY: if (data->body == NULL) { str = str_new(mail->mail.data_pool, 128); - imap_bodystructure_write(data->parts, str, FALSE); + if (index_mail_write_bodystructure(mail, str, FALSE) < 0) + return -1; data->body = str_c(str); } break; case MAIL_CACHE_IMAP_BODYSTRUCTURE: if (data->bodystructure == NULL) { str = str_new(mail->mail.data_pool, 128); - imap_bodystructure_write(data->parts, str, TRUE); + if (index_mail_write_bodystructure(mail, str, TRUE) < 0) + return -1; data->bodystructure = str_c(str); } break;