[RFC] imap-send: escape backslash in password

[RFC] imap-send: escape backslash in password

[Nicolas Morey-Chaisemartin]

Password containing backslashes need to have them doubled to have them properly interpreted by the imap server.

A password terminating with a blackslash used to trigger this error:
IMAP command 'LOGIN <user> <pass>' returned response (BAD) - Missing '"'

Signed-off-by: Nicolas Morey-Chaisemartin <nicolas@morey-chaisemartin.com>
---
 imap-send.c | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/imap-send.c b/imap-send.c
index b2d0b849b..7fde6ec96 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -926,6 +926,32 @@ static int auth_cram_md5(struct imap_store *ctx, struct imap_cmd *cmd, const cha
 	return 0;
 }
 
+static char* imap_escape_password(const char *passwd)
+{
+	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) {

Re: [RFC] imap-send: escape backslash in password

[Junio C Hamano]

Nicolas Morey-Chaisemartin <nicolas@morey-chaisemartin.com> writes:

> Password containing backslashes need to have them doubled to have them properly interpreted by the imap server.

Please wrap this into lines with reasonable lengths like 72 cols.

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.

> +static char* imap_escape_password(const char *passwd)

In our codebase, asterisk sticks to identifier, not typename.  I.e.

	static char *imap_escape(...)

> +{
> +	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);

>  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) {

Thanks.

Re: [RFC] imap-send: escape backslash in password

[Nicolas Morey-Chaisemartin]

Le 04/08/2017 à 21:09, Junio C Hamano a écrit :
> Nicolas Morey-Chaisemartin <nicolas@morey-chaisemartin.com> writes:
>
>> Password containing backslashes need to have them doubled to have them properly interpreted by the imap server.
> Please wrap this into lines with reasonable lengths like 72 cols.

I haven't checked the coding style yet. This was a very quick try that I submitted to get some feedback on the approach.
WIll fix for next time though.

>
> 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.

Didn't found a RFC doc on this. I hit the bug today and looking at the error message, found a few people who add the issue with different client and required escaping backslashes
It probably applies to the username (would be logicial that both string in the line are parsed the same way) not sure if backslashes are allowed in username though.
With password generator, they are more likely to be there.
But it wouldn't hurt to use the escape function for both.

>> +static char* imap_escape_password(const char *passwd)
> In our codebase, asterisk sticks to identifier, not typename.  I.e.
>
> 	static char *imap_escape(...)
Will do. BTW, is there a checkpatch or similar for git ?
Scrolled quickly through the doc and did not see any reference.

>
>> +{
>> +	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);

I looked at the wrappers and wasn't sure if they were to be used for this (one of the main reason this is an RFC).
I guess it would make sense. I'm not familiar with git code, but is there other escape function of this kind that could be factor ?
Or the function is simple enough not to be worth it ?

Thanks

Nicolas

Re: [RFC] imap-send: escape backslash in password

[Andreas Schwab]

On Aug 04 2017, Junio C Hamano <gitster@pobox.com> wrote:

> 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.

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).

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [RFC] imap-send: escape backslash in password

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 997 bytes --]

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.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

Re: [RFC] imap-send: escape backslash in password

[Jeff King]

On Fri, Aug 04, 2017 at 08:06:43PM +0000, brian m. carlson wrote:

> 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.

Yeah. This is an on-the-wire encoding issue, and should happen as part
of forming the protocol string to send. So:

  imap_exec(ctx, NULL, "LOGIN \"%s\" \"%s\"", srvc->user, srvc->pass)

is probably where it needs to happen.

It looks like this issue is present in a lot of other places, too. Just
a few lines below I see:

  imap_exec(ctx, NULL, "CREATE \"%s\"", ctx->name)

As an aside, these are all potential injection vulnerabilities, too.
E.g., if I specify my folder as

  foo"\n. DELETE "bar

then we'd issue an accidental deletion. I doubt it's a big deal in
practice, as it's not common to feed attacker-controlled strings to
imap-send. But we should probably fix it anyway.

The right interface is probably to teach imap_exec() to take a
NULL-terminated list of items (rather than a format string) and then
quote each one appropriately.

-Peff

Re: [RFC] imap-send: escape backslash in password

[Jeff King]

On Fri, Aug 04, 2017 at 09:46:49PM +0200, Andreas Schwab wrote:

> > 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.

-Peff

Re: [RFC] imap-send: escape backslash in password

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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.

Yes, I was scanning the RFC and came to the same conclusion ;-)

> 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?

Re: [RFC] imap-send: escape backslash in password

[Jeff King]

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

Re: [RFC] imap-send: escape backslash in password

[Nicolas Morey-Chaisemartin]

Le 04/08/2017 à 23:22, Jeff King a écrit :
> 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

There is at least one difference right now:
When using --curl, the username/password are loaded from the gitconfig file only.
When using the legacy imap interface, it goes through credential_fill which prompts for a password.

I don't think everyone is ready to store his email account password in a gitconfig file (I know I'm not).
I don't see why it couldn't be fixed. I'll give it a try tomorrow.

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.

But I agree it's probably safer and cleaner to let curl handle everything and drop the legacy stuff once it fully works.

Nicolas

Re: [RFC] imap-send: escape backslash in password

[Junio C Hamano]

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.

Thanks.

Re: [RFC] imap-send: escape backslash in password

[Jeff King]

On Sun, Aug 06, 2017 at 09:12:16PM +0200, Nicolas Morey-Chaisemartin wrote:

> 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.

Yeah, I had thought that the curl implementation _was_ the default if
you have curl. But we just build it by default, and you have to manually
enable it. So I agree it has not gotten nearly as much testing as I had
thought, and as you found, it diverges from the earlier implementation
in quite a few areas.

So I think we would need to take any deprecation much more slowly than I
had first thought (and your patches in the nearby thread are moving in a
good direction).

-Peff

Re: [RFC] imap-send: escape backslash in password

[Jeff King]

On Sun, Aug 06, 2017 at 06:34:22PM -0700, Junio C Hamano wrote:

> 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).

Re: [RFC] imap-send: escape backslash in password

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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).

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?

Re: [RFC] imap-send: escape backslash in password

[Jeff King]

On Tue, Aug 08, 2017 at 09:54:53AM -0700, Junio C Hamano wrote:

> > 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?

Yeah, I think the 7.19.4 breakage was discussed after I wrote that (and
after investigating, I think the 7.16.0 cutoff is probably pretty
reasonable even without that). I do think it's worth addressing. I just
sent a series:

  https://public-inbox.org/git/20170809120024.7phdjzjv54uv5dpz@sigill.intra.peff.net/

-Peff