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

[Jeff King <peff@peff.net>]

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