smtpd doesn't parse server responses correctly

[Nils Frohberg]

Hi,

smtpd currently ignores the server capabilities advertised on a
singleline or the last line of a multiline response:

[...]
<<< 220 mail.tako.de ESMTP
>>> EHLO something.tako.de
<<< 250-mail.tako.de
<<< 250-PIPELINING
<<< 250-8BITMIME
<<< 250-SIZE 0
<<< 250 AUTH LOGIN PLAIN CRAM-MD5
mta: leaving smtp phase
mta: new status for nilsf@...: AUTH not available
[...]

Note that although AUTH is advertised, smtpd misses it.

I have included a patch that fixes the problem and that also only
parses the server reply (for AUTH and STARTTLS) during the EHLO
phase.

Nils


Index: client.c
===================================================================
RCS file: /cvs/src/usr.sbin/smtpd/client.c,v
retrieving revision 1.9
diff -u -r1.9 client.c
--- client.c 25 Oct 2009 20:43:29 -0000 1.9
+++ client.c 9 Nov 2009 20:00:02 -0000
@@ -859,7 +859,12 @@
 
  sp->reply[0] = '\0';
 
- /* get a reply, dealing with multiline responses */
+ /* get a reply, dealing with multiline responses
+ * multiline responses are provided by 3 digits followed by '-', and
+ * are terminated the same as single line responses: either 3 digits,
+ * or 3 digits followed by ' '.
+ * all responses can have additional data starting at the 4th char.
+ */
  for (;;) {
  errno = 0;
  if ((ln = buf_getln(&sp->r)) == NULL) {
@@ -875,17 +880,28 @@
  if (sp->verbose)
  fprintf(sp->verbose, "<<< %s\n", ln);
 
- if (strlen(ln) == 3 || ln[3] == ' ')
+ /* if we have 3 chars, there's no multiline or data string */
+ if (strlen(ln) == 3)
  break;
- else if (ln[3] != '-') {
- cause = "150 garbled multiline reply";
+ else if (strlen(ln) < 4 || (ln[3] != ' ' && ln[3] != '-')) {
+ /* we didn't get the entire smtp reply code,
+ * or we got a wrong single-/multiline character.
+ */
+ cause = "150 garbled smtp reply";
  goto done;
  }
 
- if (strcmp(ln + 4, "STARTTLS") == 0)
- sp->exts[CLIENT_EXT_STARTTLS].have = 1;
- if (strncmp(ln + 4, "AUTH", 4) == 0)
- sp->exts[CLIENT_EXT_AUTH].have = 1;
+ /* save some useful server capabilities during ehlo reply */
+ if (sp->state == CLIENT_EHLO) {
+ if (strcmp(ln + 4, "STARTTLS") == 0)
+ sp->exts[CLIENT_EXT_STARTTLS].have = 1;
+ else if (strncmp(ln + 4, "AUTH", 4) == 0)
+ sp->exts[CLIENT_EXT_AUTH].have = 1;
+ }
+
+ /* this is either single line or the last multiline reply */
+ if (ln[3] == ' ')
+ break;
  }
 
  /* validate reply code */

[Gilles Chehade-5]

On Mon, Nov 09, 2009 at 10:01:27PM +0100, Nils Frohberg wrote:
> Hi,
>

Hi Nils,

> smtpd currently ignores the server capabilities advertised on a
> singleline or the last line of a multiline response:
>
> [...]
> <<< 220 mail.tako.de ESMTP
> >>> EHLO something.tako.de
> <<< 250-mail.tako.de
> <<< 250-PIPELINING
> <<< 250-8BITMIME
> <<< 250-SIZE 0
> <<< 250 AUTH LOGIN PLAIN CRAM-MD5
> mta: leaving smtp phase
> mta: new status for nilsf@...: AUTH not available
> [...]
>
> Note that although AUTH is advertised, smtpd misses it.
>

good catch

 
> I have included a patch that fixes the problem and that also only
> parses the server reply (for AUTH and STARTTLS) during the EHLO
> phase.
>

diff looks ok by me, i'll wait for jacekm@ to get a chance to review
it as he may have local changes to client.c before I commit it
tomorrow.

Thanks !

Gilles


--
Gilles Chehade
freelance developer/sysadmin/consultant

                   http://www.poolp.org

[Jacek Masiulaniec-2]

On Mon, Nov 09, 2009 at 10:01:27PM +0100, Nils Frohberg wrote:

> Hi,
>
> smtpd currently ignores the server capabilities advertised on a
> singleline or the last line of a multiline response:
>
> [...]
> <<< 220 mail.tako.de ESMTP
> >>> EHLO something.tako.de
> <<< 250-mail.tako.de
> <<< 250-PIPELINING
> <<< 250-8BITMIME
> <<< 250-SIZE 0
> <<< 250 AUTH LOGIN PLAIN CRAM-MD5
> mta: leaving smtp phase
> mta: new status for nilsf@...: AUTH not available
> [...]
>
> Note that although AUTH is advertised, smtpd misses it.
>
> I have included a patch that fixes the problem and that also only
> parses the server reply (for AUTH and STARTTLS) during the EHLO
> phase.

nice diff, committed, thanks!

>
> Nils
>
>
> Index: client.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/smtpd/client.c,v
> retrieving revision 1.9
> diff -u -r1.9 client.c
> --- client.c 25 Oct 2009 20:43:29 -0000 1.9
> +++ client.c 9 Nov 2009 20:00:02 -0000
> @@ -859,7 +859,12 @@
>  
>   sp->reply[0] = '\0';
>  
> - /* get a reply, dealing with multiline responses */
> + /* get a reply, dealing with multiline responses
> + * multiline responses are provided by 3 digits followed by '-', and
> + * are terminated the same as single line responses: either 3 digits,
> + * or 3 digits followed by ' '.
> + * all responses can have additional data starting at the 4th char.
> + */
>   for (;;) {
>   errno = 0;
>   if ((ln = buf_getln(&sp->r)) == NULL) {
> @@ -875,17 +880,28 @@
>   if (sp->verbose)
>   fprintf(sp->verbose, "<<< %s\n", ln);
>  
> - if (strlen(ln) == 3 || ln[3] == ' ')
> + /* if we have 3 chars, there's no multiline or data string */
> + if (strlen(ln) == 3)
>   break;
> - else if (ln[3] != '-') {
> - cause = "150 garbled multiline reply";
> + else if (strlen(ln) < 4 || (ln[3] != ' ' && ln[3] != '-')) {
> + /* we didn't get the entire smtp reply code,
> + * or we got a wrong single-/multiline character.
> + */
> + cause = "150 garbled smtp reply";
>   goto done;
>   }
>  
> - if (strcmp(ln + 4, "STARTTLS") == 0)
> - sp->exts[CLIENT_EXT_STARTTLS].have = 1;
> - if (strncmp(ln + 4, "AUTH", 4) == 0)
> - sp->exts[CLIENT_EXT_AUTH].have = 1;
> + /* save some useful server capabilities during ehlo reply */
> + if (sp->state == CLIENT_EHLO) {
> + if (strcmp(ln + 4, "STARTTLS") == 0)
> + sp->exts[CLIENT_EXT_STARTTLS].have = 1;
> + else if (strncmp(ln + 4, "AUTH", 4) == 0)
> + sp->exts[CLIENT_EXT_AUTH].have = 1;
> + }
> +
> + /* this is either single line or the last multiline reply */
> + if (ln[3] == ' ')
> + break;
>   }
>  
>   /* validate reply code */