[PATCH v2 0/7] Fixes for lib-mail message-address

Pali Rohár

5 Jun 2016 5 Jun '16

4:48 p.m.

Hello,

I borrowed dovecot parser for email addresses and I'm using it in my new perl module Email::Address::XS for parsing & formatting list of email addresses. That perl module is available at github [1].

During implementation and testing I found bugs in dovecot code. So I'm sending my patches (together with tests) which I'm using in my perl module.

Lot of other (normal & corner) test cases are part of that perl module [2], so if you are interested feel free to reuse them. Because writing new test cases for dovecot is hard for me, I'm not going to do it. In perl for perl modules it is a lot of easier for me.

[1] - https://github.com/pali/Email-Address-XS [2] - https://github.com/pali/Email-Address-XS/blob/master/t/Email-Address-XS.t

Changes since v1:

Updated description with test example
Rebased on top of master branch

Pali Rohár (7): lib-mail: message_address_write: Fix generating empty group list lib-mail: message_address_write: Fix generating group list with empty name lib-mail: parse_addr_spec: Like in rfc822_skip_comment() check if last_comment is not NULL lib-mail: parse_addr_spec: Email address without local-part is invalid lib-mail: parse_mailbox: Set display name instead mailbox when parsing failed lib-mail: message_address_write: Quote and escape strings if needed lib-mail: Update tests for message address

src/lib-mail/message-address.c | 99 +++++++++++++++++++++++++++++++---- src/lib-mail/test-message-address.c | 11 +++- 2 files changed, 98 insertions(+), 12 deletions(-)

-- 1.7.9.5

Show replies by date

Pali Rohár

5 Jun 5 Jun

4:48 p.m.

New subject: [PATCH v2 1/7] lib-mail: message_address_write: Fix generating empty group list

Empty group list ends with ": " not with ", ". Test case: { { name = NULL, mailbox = "group", domain = NULL }, { name = NULL, mailbox = NULL, domain = NULL } } converts to: group:; --- src/lib-mail/message-address.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/lib-mail/message-address.c b/src/lib-mail/message-address.c index 36cb483..efa91fd 100644 --- a/src/lib-mail/message-address.c +++ b/src/lib-mail/message-address.c @@ -340,6 +340,7 @@ message_address_parse(pool_t pool, const unsigned char *data, size_t size, void message_address_write(string_t *str, const struct message_address *addr) { + const char *tmp; bool first = TRUE, in_group = FALSE; /* a) mailbox@domain @@ -365,7 +366,12 @@ void message_address_write(string_t *str, const struct message_address *addr) i_assert(addr->mailbox == NULL); /* cut out the ", " */ - str_truncate(str, str_len(str)-2); + tmp = str_c(str)+str_len(str)-2; + i_assert((tmp[0] == ',' || tmp[0] == ':') && tmp[1] == ' '); + if (tmp[0] == ',' && tmp[1] == ' ') + str_truncate(str, str_len(str)-2); + else if (tmp[0] == ':' && tmp[1] == ' ') + str_truncate(str, str_len(str)-1); str_append_c(str, ';'); } -- 1.7.9.5

Pali Rohár

4:48 p.m.

New subject: [PATCH v2 2/7] lib-mail: message_address_write: Fix generating group list with empty name

Empty name for group list must be quoted. Test case: { { name = NULL, mailbox = "", domain = NULL }, { name = NULL, mailbox = NULL, domain = NULL } } converts to: "":; --- src/lib-mail/message-address.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/lib-mail/message-address.c b/src/lib-mail/message-address.c index efa91fd..9ce4a55 100644 --- a/src/lib-mail/message-address.c +++ b/src/lib-mail/message-address.c @@ -357,8 +357,12 @@ void message_address_write(string_t *str, const struct message_address *addr) if (!in_group) { /* beginning of group. mailbox is the group name, others are NULL. */ - if (addr->mailbox != NULL) + if (addr->mailbox != NULL && *addr->mailbox != '\0') { str_append(str, addr->mailbox); + } else { + /* empty group name needs to be quoted */ + str_append(str, "\"\""); + } str_append(str, ": "); first = TRUE; } else { -- 1.7.9.5

Pali Rohár

4:48 p.m.

New subject: [PATCH v2 3/7] lib-mail: parse_addr_spec: Like in rfc822_skip_comment() check if last_comment is not NULL

This will fix possible NULL pointer dereference when caller does not set last_comment. --- src/lib-mail/message-address.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/lib-mail/message-address.c b/src/lib-mail/message-address.c index 9ce4a55..4e86185 100644 --- a/src/lib-mail/message-address.c +++ b/src/lib-mail/message-address.c @@ -158,7 +158,8 @@ static int parse_addr_spec(struct message_address_parser_context *ctx) /* addr-spec = local-part "@" domain */ int ret, ret2; - str_truncate(ctx->parser.last_comment, 0); + if (ctx->parser.last_comment != NULL) + str_truncate(ctx->parser.last_comment, 0); ret = parse_local_part(ctx); if (ret != 0 && *ctx->parser.data == '@') { @@ -167,9 +168,11 @@ static int parse_addr_spec(struct message_address_parser_context *ctx) ret = ret2; } - if (str_len(ctx->parser.last_comment) > 0) { - ctx->addr.name = - p_strdup(ctx->pool, str_c(ctx->parser.last_comment)); + if (ctx->parser.last_comment != NULL) { + if (str_len(ctx->parser.last_comment) > 0) { + ctx->addr.name = + p_strdup(ctx->pool, str_c(ctx->parser.last_comment)); + } } return ret; } -- 1.7.9.5

Pali Rohár

4:48 p.m.

New subject: [PATCH v2 4/7] lib-mail: parse_addr_spec: Email address without local-part is invalid

Add explicit invalid_syntax flag also when end of input occure because address is without domain invalid and in this case it was not correctly propagated. --- src/lib-mail/message-address.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/lib-mail/message-address.c b/src/lib-mail/message-address.c index 4e86185..93b7c83 100644 --- a/src/lib-mail/message-address.c +++ b/src/lib-mail/message-address.c @@ -162,6 +162,10 @@ static int parse_addr_spec(struct message_address_parser_context *ctx) str_truncate(ctx->parser.last_comment, 0); ret = parse_local_part(ctx); + if (ret <= 0) { + /* end of input or parsing local-part failed */ + ctx->addr.invalid_syntax = TRUE; + } if (ret != 0 && *ctx->parser.data == '@') { ret2 = parse_domain(ctx); if (ret2 <= 0) -- 1.7.9.5

Pali Rohár

4:48 p.m.

New subject: [PATCH v2 5/7] lib-mail: parse_mailbox: Set display name instead mailbox when parsing failed

It does not make sense to set mailbox without domain on incorrect input. Rather set display name which is more likely useable value. Test case: test is parsed as: { name = "test", mailbox = NULL, domain = NULL } --- src/lib-mail/message-address.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/lib-mail/message-address.c b/src/lib-mail/message-address.c index 93b7c83..54d4ee1 100644 --- a/src/lib-mail/message-address.c +++ b/src/lib-mail/message-address.c @@ -205,6 +205,10 @@ static int parse_mailbox(struct message_address_parser_context *ctx) /* nope, should be addr-spec */ ctx->parser.data = start; ret = parse_addr_spec(ctx); + if (ctx->addr.invalid_syntax && !ctx->addr.name && ctx->addr.mailbox && !ctx->addr.domain) { + ctx->addr.name = ctx->addr.mailbox; + ctx->addr.mailbox = NULL; + } } if (ret < 0) -- 1.7.9.5

Pali Rohár

4:48 p.m.

New subject: [PATCH v2 6/7] lib-mail: message_address_write: Quote and escape strings if needed

ATEXT characters must be properly quoted when are in phrase. Test case: { name = "test\"test", mailbox = "user", domain = "host" } converts to: "test\"test" --- src/lib-mail/message-address.c | 66 +++++++++++++++++++++++++++++++++++++--- 1 file changed, 62 insertions(+), 4 deletions(-) diff --git a/src/lib-mail/message-address.c b/src/lib-mail/message-address.c index 54d4ee1..7d6356c 100644 --- a/src/lib-mail/message-address.c +++ b/src/lib-mail/message-address.c @@ -2,6 +2,7 @@ #include "lib.h" #include "str.h" +#include "strescape.h" #include "message-parser.h" #include "message-address.h" #include "rfc822-parser.h" @@ -32,6 +33,49 @@ static void add_address(struct message_address_parser_context *ctx) ctx->last_addr = addr; } +/* quote with "" and escape all '\', '"' and "'" characters if need */ +static void str_append_maybe_escape(string_t *dest, const char *cstr, bool escape_dot) +{ + const char *p; + + /* see if we need to quote it */ + for (p = cstr; *p != '\0'; p++) { + if (!IS_ATEXT(*p) && (escape_dot || *p != '.')) + break; + } + + if (*p == '\0') { + str_append_data(dest, cstr, (size_t) (p - cstr)); + return; + } + + /* see if we need to escape it */ + for (p = cstr; *p != '\0'; p++) { + if (IS_ESCAPED_CHAR(*p)) + break; + } + + if (*p == '\0') { + /* only quote */ + str_append_c(dest, '"'); + str_append_data(dest, cstr, (size_t) (p - cstr)); + str_append_c(dest, '"'); + return; + } + + /* quote and escape */ + str_append_c(dest, '"'); + str_append_data(dest, cstr, (size_t) (p - cstr)); + + for (; *p != '\0'; p++) { + if (IS_ESCAPED_CHAR(*p)) + str_append_c(dest, '\\'); + str_append_c(dest, *p); + } + + str_append_c(dest, '"'); +} + static int parse_local_part(struct message_address_parser_context *ctx) { int ret; @@ -369,7 +413,14 @@ void message_address_write(string_t *str, const struct message_address *addr) /* beginning of group. mailbox is the group name, others are NULL. */ if (addr->mailbox != NULL && *addr->mailbox != '\0') { - str_append(str, addr->mailbox); + /* check for MIME encoded-word */ + if (strstr(addr->mailbox, "=?")) + /* MIME encoded-word MUST NOT appear within a 'quoted-string' + so escaping and quoting of phrase is not possible, instead + use obsolete RFC822 phrase syntax which allow spaces */ + str_append(str, addr->mailbox); + else + str_append_maybe_escape(str, addr->mailbox, TRUE); } else { /* empty group name needs to be quoted */ str_append(str, "\"\""); @@ -396,7 +447,7 @@ void message_address_write(string_t *str, const struct message_address *addr) /* no name and no route. use only mailbox@domain */ i_assert(addr->mailbox != NULL); - str_append(str, addr->mailbox); + str_append_maybe_escape(str, addr->mailbox, FALSE); str_append_c(str, '@'); str_append(str, addr->domain); } else { @@ -404,7 +455,14 @@ void message_address_write(string_t *str, const struct message_address *addr) i_assert(addr->mailbox != NULL); if (addr->name != NULL) { - str_append(str, addr->name); + /* check for MIME encoded-word */ + if (strstr(addr->name, "=?")) + /* MIME encoded-word MUST NOT appear within a 'quoted-string' + so escaping and quoting of phrase is not possible, instead + use obsolete RFC822 phrase syntax which allow spaces */ + str_append(str, addr->name); + else + str_append_maybe_escape(str, addr->name, TRUE); str_append_c(str, ' '); } str_append_c(str, '<'); @@ -412,7 +470,7 @@ void message_address_write(string_t *str, const struct message_address *addr) str_append(str, addr->route); str_append_c(str, ':'); } - str_append(str, addr->mailbox); + str_append_maybe_escape(str, addr->mailbox, FALSE); str_append_c(str, '@'); str_append(str, addr->domain); str_append_c(str, '>'); -- 1.7.9.5

Pali Rohár

4:48 p.m.

New subject: [PATCH v2 7/7] lib-mail: Update tests for message address

--- src/lib-mail/test-message-address.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/lib-mail/test-message-address.c b/src/lib-mail/test-message-address.c index 9bbf29e..bf85806 100644 --- a/src/lib-mail/test-message-address.c +++ b/src/lib-mail/test-message-address.c @@ -20,11 +20,13 @@ static void test_message_address(void) static const char *input[] = { "user@domain", NULL, "", "user@domain", - "foo bar ", NULL, - "\"foo bar\" ", "foo bar ", + "foo bar ", "\"foo bar\" ", + "\"foo bar\" ", NULL, + "\"foo: ;,\" ", NULL, "<@route:user@domain>", NULL, "<@route@route2:user@domain>", "<@route,@route2:user@domain>", "hello <@route ,@route2:user@domain>", "hello <@route,@route2:user@domain>", + "hello", NULL, "user (hello)", NULL, "hello <user>", NULL, "@domain", NULL @@ -40,9 +42,11 @@ static void test_message_address(void) { NULL, NULL, NULL, "user", "domain", FALSE }, { NULL, "foo bar", NULL, "user", "domain", FALSE }, { NULL, "foo bar", NULL, "user", "domain", FALSE }, + { NULL, "foo: ;,", NULL, "user", "domain", FALSE }, { NULL, NULL, "@route", "user", "domain", FALSE }, { NULL, NULL, "@route,@route2", "user", "domain", FALSE }, { NULL, "hello", "@route,@route2", "user", "domain", FALSE }, + { NULL, "hello", NULL, "", "", TRUE }, { NULL, "hello", NULL, "user", "", TRUE }, { NULL, "hello", NULL, "user", "", TRUE }, { NULL, NULL, NULL, "", "domain", TRUE } @@ -104,10 +108,13 @@ static void test_message_address(void) str_append(group, "group:;"); addr = message_address_parse(pool_datastack_create(), str_data(group), str_len(group), UINT_MAX, FALSE); + str_truncate(str, 0); + message_address_write(str, addr); test_assert(addr != NULL && cmp_addr(addr, &group_prefix)); addr = addr->next; test_assert(addr != NULL && addr->next == NULL && cmp_addr(addr, &group_suffix)); + test_assert(strcmp(str_c(str), "group:;") == 0); test_end(); } -- 1.7.9.5

3093

Age (days ago)

3093

Last active (days ago)

List overview

7 comments

1 participants

participants (1)

Pali Rohár