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

From: Nicolas Morey-Chaisemartin <nicolas@morey-chaisemartin.com>
Date: Aug 4, 2017, 19:32 UTC
Message-ID: <90b60e41-f59f-a141-1c83-3605444333b4@morey-chaisemartin.com>
In-Reply-To: <xmqqbmnvtain.fsf@gitster.mtv.corp.google.com>

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.

Show 12 quoted lines

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

Show 36 quoted lines

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