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
