[RFC] imap-send: escape backslash in password

> +{
> +	const unsigned passwd_len = strlen(passwd);
> +	char *escaped = xmalloc(2 * passwd_len + 1);
> +	const char *passwd_cur = passwd;
> +	char *escaped_cur = escaped;
> +
> +	do {
> +		char *next = strchr(passwd_cur, '\\');
> +
> +		if (!next) {
> +			strcpy(escaped_cur, passwd_cur);
> +		} else {
> +			int len = next - passwd_cur + 1;
> +
> +			memcpy(escaped_cur, passwd_cur, len);
> +			escaped_cur += len;
> +			next++;
> +			*(escaped_cur++) = '\\';
> +		}
> +		passwd_cur = next;
> +	} while(passwd_cur);
> +
> +	return escaped;
> +}

>  static struct imap_store *imap_open_store(struct imap_server_conf *srvc, char *folder)
>  {
>  	struct credential cred = CREDENTIAL_INIT;
> @@ -1090,7 +1116,7 @@ static struct imap_store *imap_open_store(struct imap_server_conf *srvc, char *f
>  			if (!srvc->user)
>  				srvc->user = xstrdup(cred.username);
>  			if (!srvc->pass)
> -				srvc->pass = xstrdup(cred.password);
> +				srvc->pass = imap_escape_password(cred.password);
>  		}
>  
>  		if (srvc->auth_method) {

>
> Is the quoting rules documented somewhere?  If so, please also give
> a reference to it here.  RFC3501 "6.2.3 LOGIN Command" does not say
> much (other parts of the RFC may specify the rules that apply to
> arguments in general, but I didn't look for them).  Without such
> reference, it is hard to judge if this change is sufficient or even
> correct (in an extreme case, the IMAP server you are talking with
> that prompted you to make this change might be in violation).
>
> For example, FRC3501 "9. Formal Syntax" says that both "password"
> and "userid" are "astring"; it looks strange that the code with this
> patch only touches cred.password while sending cred.username as-is.

>
>> +{
>> +	const unsigned passwd_len = strlen(passwd);
>> +	char *escaped = xmalloc(2 * passwd_len + 1);
>> +	const char *passwd_cur = passwd;
>> +	char *escaped_cur = escaped;
>> +
>> +	do {
>> +		char *next = strchr(passwd_cur, '\\');
>> +
>> +		if (!next) {
>> +			strcpy(escaped_cur, passwd_cur);
>> +		} else {
>> +			int len = next - passwd_cur + 1;
>> +
>> +			memcpy(escaped_cur, passwd_cur, len);
>> +			escaped_cur += len;
>> +			next++;
>> +			*(escaped_cur++) = '\\';
>> +		}
>> +		passwd_cur = next;
>> +	} while(passwd_cur);
>> +
>> +	return escaped;
>> +}
> I wonder if we should use strbuf here perhaps like so:
>
> 	struct strbuf encoded = STRBUF_INIT;
> 	const char *p;
>
> 	for (p = passwd; *p; p++) {
> 		if (need_bs_quote(*p))
> 			strbuf_addch(&encoded, '\\');
> 		strbuf_addch(&encoded, *p);
> 	}
> 	return strbuf_detach(&encoded, NULL);

> Is the quoting rules documented somewhere?  If so, please also give
> a reference to it here.  RFC3501 "6.2.3 LOGIN Command" does not say
> much (other parts of the RFC may specify the rules that apply to
> arguments in general, but I didn't look for them).  Without such
> reference, it is hard to judge if this change is sufficient or even
> correct (in an extreme case, the IMAP server you are talking with
> that prompted you to make this change might be in violation).
>
> For example, FRC3501 "9. Formal Syntax" says that both "password"
> and "userid" are "astring"; it looks strange that the code with this
> patch only touches cred.password while sending cred.username as-is.

> > For example, FRC3501 "9. Formal Syntax" says that both "password"
> > and "userid" are "astring"; it looks strange that the code with this
> > patch only touches cred.password while sending cred.username as-is.
> 
> astring         = ... / string
> string          = quoted / ...
> quoted          = DQUOTE *QUOTED-CHAR DQUOTE
> QUOTED-CHAR     = <any TEXT-CHAR except quoted-specials> /
>                   "\" quoted-specials
> quoted-specials = DQUOTE / "\"
> 
> Thus the quoting applies to any element that is a string (and a double
> quote needs to be quoted as well).

> It's been a long time since I've done anything with IMAP, but I think
> another alternative would be to send it as a "literal", like:
>
>   {6}
>   foobar
>
> That's relatively easy to format correctly using the current printf
> specifiers that imap_exec() takes. Though as I said elsewhere in the
> thread, perhaps imap_exec() should provide a different interface.

> I also think it might be reasonable to scrap all of this ad-hoc imap
> code in favor of curl, which already gets these cases right. We already
> have a curl-backed implementation. I think we just left the old code out
> of conservatism. But it seems somewhat buggy and unmaintained, and I
> wonder if we aren't better off to simply encourage people to install
> curl.

> > I also think it might be reasonable to scrap all of this ad-hoc imap
> > code in favor of curl, which already gets these cases right. We already
> > have a curl-backed implementation. I think we just left the old code out
> > of conservatism. But it seems somewhat buggy and unmaintained, and I
> > wonder if we aren't better off to simply encourage people to install
> > curl.
> 
> That is a very attractive direction to go in, especially in the mid
> to longer term.  Perhaps we declare that the ad-hoc hardcoded imap
> is deprecated in the next cycle and drop the support by the end of
> this year?

> On Fri, Aug 04, 2017 at 02:18:13PM -0700, Junio C Hamano wrote:
>
>>> I also think it might be reasonable to scrap all of this ad-hoc imap
>>> code in favor of curl, which already gets these cases right. We already
>>> have a curl-backed implementation. I think we just left the old code out
>>> of conservatism. But it seems somewhat buggy and unmaintained, and I
>>> wonder if we aren't better off to simply encourage people to install
>>> curl.
>> That is a very attractive direction to go in, especially in the mid
>> to longer term.  Perhaps we declare that the ad-hoc hardcoded imap
>> is deprecated in the next cycle and drop the support by the end of
>> this year?
> That is fine by me. AFAIK, we already build the curl support by default
> when a sufficiently-advanced version of curl is available. So if there
> were feature-parity problems hopefully somebody would have reported it.
>
> I think the deprecation here can be relatively fast because we're not
> actually dropping support for any feature. We're just requiring that
> they install curl to get the same functionality (which might be
> inconvenient, but it's a heck of a lot less inconvenient than "there's
> no way to do what you want anymore").
>
> -Peff

> Also it probably make sense to have at least one release where --curl
> is the default. Until your mail I had no idea this option existed so I
> never tried it out.
> Making it the default will make sure almost everyone is using it and
> that there is feature-parity.

> That is fine by me. AFAIK, we already build the curl support by default
> when a sufficiently-advanced version of curl is available. So if there
> were feature-parity problems hopefully somebody would have reported it.
>
> I think the deprecation here can be relatively fast because we're not
> actually dropping support for any feature. We're just requiring that
> they install curl to get the same functionality (which might be
> inconvenient, but it's a heck of a lot less inconvenient than "there's
> no way to do what you want anymore").

> Jeff King <peff@peff.net> writes:
> 
> > That is fine by me. AFAIK, we already build the curl support by default
> > when a sufficiently-advanced version of curl is available. So if there
> > were feature-parity problems hopefully somebody would have reported it.
> >
> > I think the deprecation here can be relatively fast because we're not
> > actually dropping support for any feature. We're just requiring that
> > they install curl to get the same functionality (which might be
> > inconvenient, but it's a heck of a lot less inconvenient than "there's
> > no way to do what you want anymore").
> 
> Yeah, as long as imap-supporting libcurl is not too recent and are
> available everywhere, we should be OK.

> I think we're not quite ready to switch to curl based on comments in the
> nearby thread. But just for reference, since I started looking into
> this...
>
> The defines in the Makefile turn on USE_CURL_FOR_IMAP_SEND want curl
> 7.34.0. That's only from 2013, which is probably recent enough that it
> may cause a problem (I had originally thought it was a few years older,
> but I forgot the curl version hex encoding; 072200 is 7.34.0).
>
> For comparison, nothing older than curl 7.19.4 will work for building
> Git since v2.12.0, as we added some unconditional uses of CURLPROTO_*
> there. Nobody seems to have noticed or complained. I pointed this out a
> few months ago[1] and suggested we clean up some of the more antiquated
> #if blocks in http.c that don't even build. There was some complaint
> that we should keep even these ancient versions working, but the
> compile error is still in "master".
>
> So it's not clear to me that anybody cares about going that far back
> (which is mid-2009), but I'd guess that 2013 might cause some problems.
>
> [1] https://public-inbox.org/git/20170404025438.bgxz5sfmrawqswcj@sigill.intra.peff.net/
>     if you're curious (you were offline for a while at that time, I
>     think).

> > For comparison, nothing older than curl 7.19.4 will work for building
> > Git since v2.12.0, as we added some unconditional uses of CURLPROTO_*
> > there. Nobody seems to have noticed or complained. I pointed this out a
> > few months ago[1] and suggested we clean up some of the more antiquated
> > #if blocks in http.c that don't even build. There was some complaint
> > that we should keep even these ancient versions working, but the
> > compile error is still in "master".
> >
> > So it's not clear to me that anybody cares about going that far back
> > (which is mid-2009), but I'd guess that 2013 might cause some problems.
> >
> > [1] https://public-inbox.org/git/20170404025438.bgxz5sfmrawqswcj@sigill.intra.peff.net/
> >     if you're curious (you were offline for a while at that time, I
> >     think).
> 
> Thanks for digging.  It would not help the issue on this thread at
> all.  While I agree with your conclusion in the quoted thread:
> 
>     I think it might be nice to declare a "too old" version, though,
>     just so we can stop adding _new_ ifdefs. Maybe 7.11.1 is that
>     version now, and in another few years we can bump to 7.16.0. :)
> 
> it appears that we silently declared it to 7.19.4 and found out that
> nobody complained, without us having to wait for a few years?

>  static struct imap_store *imap_open_store(struct imap_server_conf *srvc, char *folder)
>  {
>  	struct credential cred = CREDENTIAL_INIT;
> @@ -1090,7 +1116,7 @@ static struct imap_store *imap_open_store(struct imap_server_conf *srvc, char *f
>  			if (!srvc->user)
>  				srvc->user = xstrdup(cred.username);
>  			if (!srvc->pass)
> -				srvc->pass = xstrdup(cred.password);
> +				srvc->pass = imap_escape_password(cred.password);
>  		}
>  
>  		if (srvc->auth_method) {

> On Fri, Aug 04, 2017 at 06:16:53PM +0200, Nicolas Morey-Chaisemartin wrote:
> >  static struct imap_store *imap_open_store(struct imap_server_conf *srvc, char *folder)
> >  {
> >  	struct credential cred = CREDENTIAL_INIT;
> > @@ -1090,7 +1116,7 @@ static struct imap_store *imap_open_store(struct imap_server_conf *srvc, char *f
> >  			if (!srvc->user)
> >  				srvc->user = xstrdup(cred.username);
> >  			if (!srvc->pass)
> > -				srvc->pass = xstrdup(cred.password);
> > +				srvc->pass = imap_escape_password(cred.password);
> >  		}
> >  
> >  		if (srvc->auth_method) {
> 
> I'm not sure if this is correct.  It looks like this username and
> password are used by whatever authentication method we use, whether
> that's LOGIN or CRAM-MD5.  I don't think we'd want to encode the
> password here before sending it through the CRAM-MD5 authenticator.