Re: [RFC] imap-send: escape backslash in password
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.bgxz5sfmrawqs...@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.7phdjzjv54uv5...@sigill.intra.peff.net/ -Peff
Re: [RFC] imap-send: escape backslash in password
Jeff King 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.bgxz5sfmrawqs...@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
On Sun, Aug 06, 2017 at 06:34:22PM -0700, Junio C Hamano wrote: > Jeff King 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.bgxz5sfmrawqs...@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
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 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
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
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
Jeff King 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
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 = / > "\" 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
On Fri, Aug 04, 2017 at 08:06:43PM +, 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
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 signature.asc Description: PGP signature
Re: [RFC] imap-send: escape backslash in password
On Aug 04 2017, Junio C Hamano 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 = / "\" 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, sch...@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
Le 04/08/2017 à 21:09, Junio C Hamano a écrit : > Nicolas Morey-Chaisemartin 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
Nicolas Morey-Chaisemartin 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.
[RFC] imap-send: escape backslash in password
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 ' returned response (BAD) - Missing '"' Signed-off-by: Nicolas Morey-Chaisemartin --- 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) {