On Mon 2 Sep 2013 at 12:01:35PM +0200, Oswald Buddenhagen wrote: > On Mon, Sep 02, 2013 at 03:25:07AM -0500, guns wrote: > > On Mon 2 Sep 2013 at 09:17:29AM +0200, Oswald Buddenhagen wrote: > > > patches welcome. > > > > The patch is trivial, but agreement is harder won. > > > it's not entirely trivial. the nice implementation would be building > auto-quoting into imap_exec(), which would mean a custom printf() > implementation. the easier alternative is an imap_quote() function, but > that needs to be manually called, either with fixed-size buffers or with > subsequent free()s - either variant will look ugly.
Yes, I slightly oversold the simplicity of the patch. Looking over the code base, the assumption that quoted strings do not contain escaped \\ and \" chars is widespread, but relatively easy to fix. The first assumption is that strchr( string + 1, '"' ) correctly finds the closing quote from the start of a double quoted string. A new function unescaped_strchr() is introduced, which is like strchr, but skips characters following a backslash. Patch [1/4] adds this function, but only changes a couple of strchr() calls to unescaped_strchr(). This new function should be used in more places, but I am attempting to be conservative in my changes. Patch [2/4] changes the handling of quoted arguments in config.c:get_arg(). Previously, double quotes simply served as a way to include whitespace in a config argument; the presence of a double quote toggled the parser's "whitespace allowed" flag. Characters were otherwise read verbatim with no escaping. The new behavior is more conventional: a config argument that begins with a double quote must end in a double quote, and all contained backslashes and double quotes must be backslash escaped. No escape sequences aside from \\ and \" are accepted. The resulting value is an unescaped version of the string without surrounding quotes. A config argument that does not begin with a double quote is read literally, and neither " nor \ have any special meaning. This is a breaking change, but only for people who have struggled to write literal \ and " characters in their config files. The intended purpose of this change is to ensure that all string values in config structs are unescaped strings which can be reliably re-escaped on emission. Patch [3/4] adds the imap_quote() function you suggested, with fixed destination buffers. The reason I decided on this instead of augmenting imap_exec() is that extra metadata is required to determine which args in the printf list should be quoted, since not every argument is double-quoted. An imap_quote(char *dst, const char *s, size_t n) in constrast only adds one stack variable + one function call for each format argument to be quoted. I think this is simpler and more composable IMHO. There exist many instances of \"%s\" in imap_exec() format strings, but I only added quoting for the LOGIN call since this is the most likely place for this to be an issue, and I am not currently aware of the consequences of changing the others. Patch [4/4] simply skips backslash escaped characters when reading quoted LIST responses. This allows proper parsing of unfortunately named mailboxes with double quotes and backslashes. All four patches are attached as a single file, and can also be accessed at this URL: https://github.com/guns/isync/compare/imap-quoted-strings.patch The changes are done conservatively, and the style is in line with the rest of the code. I am happy to make any and all amendments that you propose and will be awaiting your response. Thank you again, Sung Pae
From 57c7b6dc9bb0f4c3aab15ca6151df0a5b497080b Mon Sep 17 00:00:00 2001 From: guns <[email protected]> Date: Tue, 3 Sep 2013 13:26:52 -0500 Subject: [PATCH 1/4] unescaped_strchr() for searching within quoted strings --- src/drv_imap.c | 4 ++-- src/isync.h | 1 + src/util.c | 22 ++++++++++++++++++++++ 3 files changed, 25 insertions(+), 2 deletions(-) diff --git a/src/drv_imap.c b/src/drv_imap.c index abfc760..1886efa 100644 --- a/src/drv_imap.c +++ b/src/drv_imap.c @@ -473,7 +473,7 @@ next_arg( char **s ) if (**s == '"') { ++*s; ret = *s; - *s = strchr( *s, '"' ); + *s = unescaped_strchr( *s, '"' ); } else { ret = *s; while (**s && !isspace( (unsigned char) **s )) @@ -1136,7 +1136,7 @@ imap_socket_read( void *aux ) cmd2->gen.param.high_prio = 1; p = strchr( cmdp->cmd, '"' ); if (imap_exec( ctx, &cmd2->gen, get_cmd_result_p2, - "CREATE %.*s", strchr( p + 1, '"' ) - p + 1, p ) < 0) + "CREATE %.*s", unescaped_strchr( p + 1, '"' ) - p + 1, p ) < 0) return; continue; } diff --git a/src/isync.h b/src/isync.h index 661c1ee..c3e950e 100644 --- a/src/isync.h +++ b/src/isync.h @@ -433,6 +433,7 @@ void free_generic_messages( message_t * ); #ifndef HAVE_MEMRCHR void *memrchr( const void *s, int c, size_t n ); #endif +char *unescaped_strchr( const char *s, int c ); void *nfmalloc( size_t sz ); void *nfcalloc( size_t sz ); diff --git a/src/util.c b/src/util.c index 2b89a7d..814dc28 100644 --- a/src/util.c +++ b/src/util.c @@ -210,6 +210,28 @@ memrchr( const void *s, int c, size_t n ) } #endif +/* Like strchr, but skips instances of c preceded by a backslash */ +char * +unescaped_strchr( const char *s, int c ) +{ + char *p = (char *)s; + char accept[2]; + + accept[0] = '\\'; + accept[1] = c; + + while ((p = strpbrk( p, accept )) != NULL) { + if (*p == '\\') { + /* Advance twice to skip the escaped char */ + if (*++p == 0) { p = NULL; break; } + if (*++p == 0) { p = NULL; break; } + } else + break; + } + + return p; +} + void oob( void ) { -- 1.8.4 From 94052fe63a7bc5b4d132b3a9c3cdac436bdc4ba6 Mon Sep 17 00:00:00 2001 From: guns <[email protected]> Date: Tue, 3 Sep 2013 13:30:13 -0500 Subject: [PATCH 2/4] Dequote and unescape double quoted strings in get_arg() --- src/config.c | 35 ++++++++++++++++++++++++++--------- 1 file changed, 26 insertions(+), 9 deletions(-) diff --git a/src/config.c b/src/config.c index 2c2a086..698cbe3 100644 --- a/src/config.c +++ b/src/config.c @@ -63,22 +63,39 @@ get_arg( conffile_t *cfile, int required, int *comment ) cfile->err = 1; } ret = 0; - } else { - for (quoted = 0, ret = t = p; c; c = *p) { - p++; - if (c == '"') - quoted ^= 1; - else if (!quoted && isspace( (unsigned char) c )) + } else if (c == '"') { + /* Strip quotes and unescape \\ and \" */ + quoted = 1; + ret = t = ++p; + while ((c = *p++) != 0) { + if (c == '\\') { + c = *p++; + if (c == '\\' || c == '"') + *t++ = c; + else { + error( "%s:%d: unsupported escape sequence '\\%c'\n", + cfile->file, cfile->line, c ); + cfile->err = 1; + quoted = 0; + ret = p = 0; + break; + } + } else if (c == '"') { + *t = quoted = 0; break; - else + } else *t++ = c; } - *t = 0; if (quoted) { error( "%s:%d: missing closing quote\n", cfile->file, cfile->line ); cfile->err = 1; - ret = 0; + ret = p = 0; } + } else { + ret = p; + while (*p != 0 && !isspace( (unsigned char) *p )) + p++; + *p++ = 0; } cfile->rest = p; return ret; -- 1.8.4 From 7222200794346a989a2f617c35d2aea3173c3185 Mon Sep 17 00:00:00 2001 From: guns <[email protected]> Date: Tue, 3 Sep 2013 13:35:12 -0500 Subject: [PATCH 3/4] Implementation of imap_quote() with fixed input buffer Quotes src string and copies to dst buffer, which must be at least twice the length of the src string to account for the worst case password consisting of only \ and " chars, plus three for the surrounding quotes and the terminal null byte. This patch only quotes input for the LOGIN imap_exec(), although there exist other imap_exec() calls with \"%s\" in their format strings. --- src/drv_imap.c | 38 ++++++++++++++++++++++++++++++++++---- 1 file changed, 34 insertions(+), 4 deletions(-) diff --git a/src/drv_imap.c b/src/drv_imap.c index 1886efa..dff414d 100644 --- a/src/drv_imap.c +++ b/src/drv_imap.c @@ -48,6 +48,10 @@ typedef struct imap_server_conf { #endif } imap_server_conf_t; +/* Worst case quoted length: every char escaped + 2 quote chars + \0 */ +#define MAX_PASS_LEN 80 +#define MAX_QUOTED_PASS_LEN (MAX_PASS_LEN * 2) + 3 + typedef struct imap_store_conf { store_conf_t gen; imap_server_conf_t *server; @@ -355,6 +359,30 @@ submit_imap_cmd( imap_store_t *ctx, struct imap_cmd *cmd ) return send_imap_cmd( ctx, cmd ); } +/* RFC 3501 quoted string: + * + * quoted = DQUOTE *QUOTED-CHAR DQUOTE + * QUOTED-CHAR = <any TEXT-CHAR except quoted-specials> / "\" quoted-specials + * quoted-specials = DQUOTE / "\" + */ +void +imap_quote( char *p, const char *s, size_t len ) +{ + char c; + size_t i = 0, j = 0; + + p[j++] = '"'; + + while (j < len - 2 && (c = s[i++]) != 0) { + if (c == '\\' || c == '"') + p[j++] = '\\'; + p[j++] = c; + } + + p[j++] = '"'; + p[j] = 0; +} + static int imap_exec( imap_store_t *ctx, struct imap_cmd *cmdp, void (*done)( imap_store_t *ctx, struct imap_cmd *cmd, int response ), @@ -1491,7 +1519,7 @@ imap_open_store_authenticate2( imap_store_t *ctx ) { imap_store_conf_t *cfg = (imap_store_conf_t *)ctx->gen.conf; imap_server_conf_t *srvc = cfg->server; - char *arg; + char *arg, user[MAX_QUOTED_PASS_LEN], pass[MAX_QUOTED_PASS_LEN]; info ("Logging in...\n"); if (!srvc->user) { @@ -1501,7 +1529,7 @@ imap_open_store_authenticate2( imap_store_t *ctx ) if (srvc->pass_cmd) { FILE *fp; int ret; - char buffer[80]; + char buffer[MAX_PASS_LEN]; if (!(fp = popen( srvc->pass_cmd, "r" ))) { pipeerr: @@ -1527,7 +1555,7 @@ imap_open_store_authenticate2( imap_store_t *ctx ) free( srvc->pass ); /* From previous runs */ srvc->pass = nfstrdup( buffer ); } else if (!srvc->pass) { - char prompt[80]; + char prompt[MAX_PASS_LEN]; sprintf( prompt, "Password (%s): ", srvc->name ); arg = getpass( prompt ); if (!arg) { @@ -1566,8 +1594,10 @@ imap_open_store_authenticate2( imap_store_t *ctx ) if (!ctx->conn.ssl) #endif warn( "*** IMAP Warning *** Password is being sent in the clear\n" ); + imap_quote(user, srvc->user, MAX_QUOTED_PASS_LEN); + imap_quote(pass, srvc->pass, MAX_QUOTED_PASS_LEN); imap_exec( ctx, 0, imap_open_store_authenticate2_p2, - "LOGIN \"%s\" \"%s\"", srvc->user, srvc->pass ); + "LOGIN %s %s", user, pass ); return; bail: -- 1.8.4 From 738f899fd50c37b1d4449641b21124c972e1575d Mon Sep 17 00:00:00 2001 From: guns <[email protected]> Date: Tue, 3 Sep 2013 22:48:57 -0500 Subject: [PATCH 4/4] Skip escaped chars in LIST responses This allows proper parsing of mailboxes with backslashes and double quotes. --- src/drv_imap.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/drv_imap.c b/src/drv_imap.c index dff414d..17b6528 100644 --- a/src/drv_imap.c +++ b/src/drv_imap.c @@ -620,9 +620,12 @@ parse_imap_list( imap_store_t *ctx, char **sp, parse_list_state_t *sts ) /* quoted string */ s++; p = s; - for (; *s != '"'; s++) + for (; *s != '"'; s++) { + if (*s == '\\') + s++; if (!*s) goto bail; + } cur->len = s - p; s++; cur->val = nfmalloc( cur->len + 1 ); -- 1.8.4
pgpnluZsgpzUa.pgp
Description: PGP signature
------------------------------------------------------------------------------ Learn the latest--Visual Studio 2012, SharePoint 2013, SQL 2012, more! Discover the easy way to master current and previous Microsoft technologies and advance your career. Get an incredible 1,500+ hours of step-by-step tutorial videos with LearnDevNow. Subscribe today and save! http://pubads.g.doubleclick.net/gampad/clk?id=58040911&iu=/4140/ostg.clktrk
_______________________________________________ isync-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/isync-devel
