smtpd: empty envelope sender is fatal to mda

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

smtpd: empty envelope sender is fatal to mda

Lauri Tirkkonen-2
Hi, I noticed in my maillog that smtpd is dropping bounce messages
destined for me on the floor, like so:

    Nov 27 16:04:44 quickman smtpd[66508]: 0000000000000000 mda delivery evpid=75062f6aefbfab27 from=<> to=<[hidden email]> rcpt=<[hidden email]> user=lotheac delay=0s result=PermFail stat=Error ("smtpd: mda command line could not be expanded: No such file or directory")

my smtpd.conf is as follows:

    # $OpenBSD: smtpd.conf,v 1.11 2018/06/04 21:10:58 jmc Exp $

    # This is the smtpd server system-wide configuration file.
    # See smtpd.conf(5) for more information.

    table aliases file:/etc/mail/aliases

    pki quickman.lotheac.fi cert "/etc/ssl/quickman.lotheac.fi.fullchain.pem"
    pki quickman.lotheac.fi key "/etc/ssl/private/quickman.lotheac.fi.key"

    # To accept external mail, replace with: listen on all
    #
    listen on all tls
    listen on lo0 port 10028 tag DKIM

    #action "local" maildir alias <aliases>
    action "local" mda "/etc/mail/mda.sh '%{sender}'"
    action "relay" relay
    action "relay_dkim" relay host smtp://127.0.0.1:10027

    table maildest { "lotheac.fi" "quickman.lotheac.fi" }
    # Uncomment the following to accept external mail for domain "example.org"
    #
    match from any for domain <maildest> action "local"
    match for local action "local"
    match tag DKIM for any action "relay"
    match for any action "relay_dkim"


so, it seems that the %{sender} expansion is failing somehow. From
smtpd.conf(5):

    %{sender}            sender email address, may be empty string

clearly, in this case it *should* expand to the empty string - there is
no envelope sender. However mda_expand_token returns 0 if the string
ends up empty:

204         /* expanded string is empty */
205         i = strlen(string);
206         if (i == 0)
207                 return 0;

and it does, because above 'string' has been set to the empty string
(lines 119-121):

115         if (!strcasecmp("sender", rtoken)) {
116                 if (snprintf(tmp, sizeof tmp, "%s@%s",
117                         dlv->sender.user, dlv->sender.domain) >= (int)sizeof tmp)
118                         return -1;
119                 if (strcmp(tmp, "@") == 0)
120                         (void)strlcpy(tmp, "", sizeof tmp);
121                 string = tmp;
122         }

As an aside the error exit happens with err(), but mda_expand_format
doesn't actually set errno, so ENOENT is not actually what is happening.
Maybe that's another bug.

So, mda_expand_format() assumes that a 0 retval from mda_expand_token()
is an error, even though it's a valid value (ie. the length of the empty
string). Maybe it should return (size_t)-1 instead for errors? Diff for
that below, it makes the expansion work for me.

I found this on 6.4, but also applies to -CURRENT.

diff --git a/usr.sbin/smtpd/mda_variables.c b/usr.sbin/smtpd/mda_variables.c
index a23112c71e8..0a1605772f2 100644
--- a/usr.sbin/smtpd/mda_variables.c
+++ b/usr.sbin/smtpd/mda_variables.c
@@ -75,14 +75,14 @@ mda_expand_token(char *dest, size_t len, const char *token,
  mods = NULL;
 
  if (strlcpy(rtoken, token, sizeof rtoken) >= sizeof rtoken)
- return 0;
+ return -1;
 
  /* token[x[:y]] -> extracts optional x and y, converts into offsets */
  if ((lbracket = strchr(rtoken, '[')) &&
     (rbracket = strchr(rtoken, ']'))) {
  /* ] before [ ... or empty */
  if (rbracket < lbracket || rbracket - lbracket <= 1)
- return 0;
+ return -1;
 
  *lbracket = *rbracket = '\0';
  content  = lbracket + 1;
@@ -102,7 +102,7 @@ mda_expand_token(char *dest, size_t len, const char *token,
  }
  }
  if (errstr)
- return 0;
+ return -1;
 
  /* token:mod_1,mod_2,mod_n -> extract modifiers */
  mods = strchr(rbracket + 1, ':');
@@ -115,7 +115,7 @@ mda_expand_token(char *dest, size_t len, const char *token,
  if (!strcasecmp("sender", rtoken)) {
  if (snprintf(tmp, sizeof tmp, "%s@%s",
  dlv->sender.user, dlv->sender.domain) >= (int)sizeof tmp)
- return 0;
+ return -1;
  if (strcmp(tmp, "@") == 0)
  (void)strlcpy(tmp, "", sizeof tmp);
  string = tmp;
@@ -123,7 +123,7 @@ mda_expand_token(char *dest, size_t len, const char *token,
  else if (!strcasecmp("rcpt", rtoken)) {
  if (snprintf(tmp, sizeof tmp, "%s@%s",
  dlv->rcpt.user, dlv->rcpt.domain) >= (int)sizeof tmp)
- return 0;
+ return -1;
  if (strcmp(tmp, "@") == 0)
  (void)strlcpy(tmp, "", sizeof tmp);
  string = tmp;
@@ -131,7 +131,7 @@ mda_expand_token(char *dest, size_t len, const char *token,
  else if (!strcasecmp("dest", rtoken)) {
  if (snprintf(tmp, sizeof tmp, "%s@%s",
  dlv->dest.user, dlv->dest.domain) >= (int)sizeof tmp)
- return 0;
+ return -1;
  if (strcmp(tmp, "@") == 0)
  (void)strlcpy(tmp, "", sizeof tmp);
  string = tmp;
@@ -161,17 +161,17 @@ mda_expand_token(char *dest, size_t len, const char *token,
  else if (!strcasecmp("mbox.from", rtoken)) {
  if (snprintf(tmp, sizeof tmp, "%s@%s",
  dlv->sender.user, dlv->sender.domain) >= (int)sizeof tmp)
- return 0;
+ return -1;
  if (strcmp(tmp, "@") == 0)
  (void)strlcpy(tmp, "MAILER-DAEMON", sizeof tmp);
  string = tmp;
  }
  else
- return 0;
+ return -1;
 
  if (string != tmp) {
  if (strlcpy(tmp, string, sizeof tmp) >= sizeof tmp)
- return 0;
+ return -1;
  string = tmp;
  }
 
@@ -187,12 +187,12 @@ mda_expand_token(char *dest, size_t len, const char *token,
  break;
  }
  if (!token_modifiers[i].f(tmp, sizeof tmp))
- return 0; /* modifier error */
+ return -1; /* modifier error */
  break;
  }
  }
  if ((size_t)i == nitems(token_modifiers))
- return 0; /* modifier not found */
+ return -1; /* modifier not found */
  } while ((mods = sep) != NULL);
  }
 
@@ -208,7 +208,7 @@ mda_expand_token(char *dest, size_t len, const char *token,
 
  /* begin offset beyond end of string */
  if (begoff >= i)
- return 0;
+ return -1;
 
  /* end offset beyond end of string, make it end of string */
  if (endoff >= i)
@@ -225,13 +225,13 @@ mda_expand_token(char *dest, size_t len, const char *token,
 
  /* check that final offsets are valid */
  if (begoff < 0 || endoff < 0 || endoff < begoff)
- return 0;
+ return -1;
  endoff += 1; /* end offset is inclusive */
 
  /* check that substring does not exceed destination buffer length */
  i = endoff - begoff;
  if ((size_t)i + 1 >= len)
- return 0;
+ return -1;
 
  string += begoff;
  for (; i; i--) {
@@ -308,7 +308,7 @@ mda_expand_format(char *buf, size_t len, const struct deliver *dlv,
 
  exptoklen = mda_expand_token(exptok, sizeof exptok, token, dlv,
     ui, mda_command);
- if (exptoklen == 0)
+ if (exptoklen == (size_t)-1)
  return 0;
 
  /* writing expanded token at ptmp will overflow tmpbuf */

--
Lauri Tirkkonen | lotheac @ IRCnet

Reply | Threaded
Open this post in threaded view
|

Re: smtpd: empty envelope sender is fatal to mda

Gilles Chehade-7
Hi,

I will get back to you tomorrow as I have a very busy day today.

There is a work-around which is to use the %{mbox.from} variable
which is what we do on mbox and lmtp to avoid empty senders, but
your diff makes sense from a fast read, I need to assess if this
comes with side-effects or not before committing.

Thanks for your mail !

Gilles


On Tue, Nov 27, 2018 at 06:47:14PM +0200, Lauri Tirkkonen wrote:

> Hi, I noticed in my maillog that smtpd is dropping bounce messages
> destined for me on the floor, like so:
>
>     Nov 27 16:04:44 quickman smtpd[66508]: 0000000000000000 mda delivery evpid=75062f6aefbfab27 from=<> to=<[hidden email]> rcpt=<[hidden email]> user=lotheac delay=0s result=PermFail stat=Error ("smtpd: mda command line could not be expanded: No such file or directory")
>
> my smtpd.conf is as follows:
>
>     # $OpenBSD: smtpd.conf,v 1.11 2018/06/04 21:10:58 jmc Exp $
>
>     # This is the smtpd server system-wide configuration file.
>     # See smtpd.conf(5) for more information.
>
>     table aliases file:/etc/mail/aliases
>
>     pki quickman.lotheac.fi cert "/etc/ssl/quickman.lotheac.fi.fullchain.pem"
>     pki quickman.lotheac.fi key "/etc/ssl/private/quickman.lotheac.fi.key"
>
>     # To accept external mail, replace with: listen on all
>     #
>     listen on all tls
>     listen on lo0 port 10028 tag DKIM
>
>     #action "local" maildir alias <aliases>
>     action "local" mda "/etc/mail/mda.sh '%{sender}'"
>     action "relay" relay
>     action "relay_dkim" relay host smtp://127.0.0.1:10027
>
>     table maildest { "lotheac.fi" "quickman.lotheac.fi" }
>     # Uncomment the following to accept external mail for domain "example.org"
>     #
>     match from any for domain <maildest> action "local"
>     match for local action "local"
>     match tag DKIM for any action "relay"
>     match for any action "relay_dkim"
>
>
> so, it seems that the %{sender} expansion is failing somehow. From
> smtpd.conf(5):
>
>     %{sender}            sender email address, may be empty string
>
> clearly, in this case it *should* expand to the empty string - there is
> no envelope sender. However mda_expand_token returns 0 if the string
> ends up empty:
>
> 204         /* expanded string is empty */
> 205         i = strlen(string);
> 206         if (i == 0)
> 207                 return 0;
>
> and it does, because above 'string' has been set to the empty string
> (lines 119-121):
>
> 115         if (!strcasecmp("sender", rtoken)) {
> 116                 if (snprintf(tmp, sizeof tmp, "%s@%s",
> 117                         dlv->sender.user, dlv->sender.domain) >= (int)sizeof tmp)
> 118                         return -1;
> 119                 if (strcmp(tmp, "@") == 0)
> 120                         (void)strlcpy(tmp, "", sizeof tmp);
> 121                 string = tmp;
> 122         }
>
> As an aside the error exit happens with err(), but mda_expand_format
> doesn't actually set errno, so ENOENT is not actually what is happening.
> Maybe that's another bug.
>
> So, mda_expand_format() assumes that a 0 retval from mda_expand_token()
> is an error, even though it's a valid value (ie. the length of the empty
> string). Maybe it should return (size_t)-1 instead for errors? Diff for
> that below, it makes the expansion work for me.
>
> I found this on 6.4, but also applies to -CURRENT.
>
> diff --git a/usr.sbin/smtpd/mda_variables.c b/usr.sbin/smtpd/mda_variables.c
> index a23112c71e8..0a1605772f2 100644
> --- a/usr.sbin/smtpd/mda_variables.c
> +++ b/usr.sbin/smtpd/mda_variables.c
> @@ -75,14 +75,14 @@ mda_expand_token(char *dest, size_t len, const char *token,
>   mods = NULL;
>  
>   if (strlcpy(rtoken, token, sizeof rtoken) >= sizeof rtoken)
> - return 0;
> + return -1;
>  
>   /* token[x[:y]] -> extracts optional x and y, converts into offsets */
>   if ((lbracket = strchr(rtoken, '[')) &&
>      (rbracket = strchr(rtoken, ']'))) {
>   /* ] before [ ... or empty */
>   if (rbracket < lbracket || rbracket - lbracket <= 1)
> - return 0;
> + return -1;
>  
>   *lbracket = *rbracket = '\0';
>   content  = lbracket + 1;
> @@ -102,7 +102,7 @@ mda_expand_token(char *dest, size_t len, const char *token,
>   }
>   }
>   if (errstr)
> - return 0;
> + return -1;
>  
>   /* token:mod_1,mod_2,mod_n -> extract modifiers */
>   mods = strchr(rbracket + 1, ':');
> @@ -115,7 +115,7 @@ mda_expand_token(char *dest, size_t len, const char *token,
>   if (!strcasecmp("sender", rtoken)) {
>   if (snprintf(tmp, sizeof tmp, "%s@%s",
>   dlv->sender.user, dlv->sender.domain) >= (int)sizeof tmp)
> - return 0;
> + return -1;
>   if (strcmp(tmp, "@") == 0)
>   (void)strlcpy(tmp, "", sizeof tmp);
>   string = tmp;
> @@ -123,7 +123,7 @@ mda_expand_token(char *dest, size_t len, const char *token,
>   else if (!strcasecmp("rcpt", rtoken)) {
>   if (snprintf(tmp, sizeof tmp, "%s@%s",
>   dlv->rcpt.user, dlv->rcpt.domain) >= (int)sizeof tmp)
> - return 0;
> + return -1;
>   if (strcmp(tmp, "@") == 0)
>   (void)strlcpy(tmp, "", sizeof tmp);
>   string = tmp;
> @@ -131,7 +131,7 @@ mda_expand_token(char *dest, size_t len, const char *token,
>   else if (!strcasecmp("dest", rtoken)) {
>   if (snprintf(tmp, sizeof tmp, "%s@%s",
>   dlv->dest.user, dlv->dest.domain) >= (int)sizeof tmp)
> - return 0;
> + return -1;
>   if (strcmp(tmp, "@") == 0)
>   (void)strlcpy(tmp, "", sizeof tmp);
>   string = tmp;
> @@ -161,17 +161,17 @@ mda_expand_token(char *dest, size_t len, const char *token,
>   else if (!strcasecmp("mbox.from", rtoken)) {
>   if (snprintf(tmp, sizeof tmp, "%s@%s",
>   dlv->sender.user, dlv->sender.domain) >= (int)sizeof tmp)
> - return 0;
> + return -1;
>   if (strcmp(tmp, "@") == 0)
>   (void)strlcpy(tmp, "MAILER-DAEMON", sizeof tmp);
>   string = tmp;
>   }
>   else
> - return 0;
> + return -1;
>  
>   if (string != tmp) {
>   if (strlcpy(tmp, string, sizeof tmp) >= sizeof tmp)
> - return 0;
> + return -1;
>   string = tmp;
>   }
>  
> @@ -187,12 +187,12 @@ mda_expand_token(char *dest, size_t len, const char *token,
>   break;
>   }
>   if (!token_modifiers[i].f(tmp, sizeof tmp))
> - return 0; /* modifier error */
> + return -1; /* modifier error */
>   break;
>   }
>   }
>   if ((size_t)i == nitems(token_modifiers))
> - return 0; /* modifier not found */
> + return -1; /* modifier not found */
>   } while ((mods = sep) != NULL);
>   }
>  
> @@ -208,7 +208,7 @@ mda_expand_token(char *dest, size_t len, const char *token,
>  
>   /* begin offset beyond end of string */
>   if (begoff >= i)
> - return 0;
> + return -1;
>  
>   /* end offset beyond end of string, make it end of string */
>   if (endoff >= i)
> @@ -225,13 +225,13 @@ mda_expand_token(char *dest, size_t len, const char *token,
>  
>   /* check that final offsets are valid */
>   if (begoff < 0 || endoff < 0 || endoff < begoff)
> - return 0;
> + return -1;
>   endoff += 1; /* end offset is inclusive */
>  
>   /* check that substring does not exceed destination buffer length */
>   i = endoff - begoff;
>   if ((size_t)i + 1 >= len)
> - return 0;
> + return -1;
>  
>   string += begoff;
>   for (; i; i--) {
> @@ -308,7 +308,7 @@ mda_expand_format(char *buf, size_t len, const struct deliver *dlv,
>  
>   exptoklen = mda_expand_token(exptok, sizeof exptok, token, dlv,
>      ui, mda_command);
> - if (exptoklen == 0)
> + if (exptoklen == (size_t)-1)
>   return 0;
>  
>   /* writing expanded token at ptmp will overflow tmpbuf */
>
> --
> Lauri Tirkkonen | lotheac @ IRCnet
>

--
Gilles Chehade       @poolpOrg

https://www.poolp.org                 tip me: https://paypal.me/poolpOrg

Reply | Threaded
Open this post in threaded view
|

Re: smtpd: empty envelope sender is fatal to mda

Lauri Tirkkonen-2
In reply to this post by Lauri Tirkkonen-2
On Tue, Nov 27 2018 18:47:14 +0200, Lauri Tirkkonen wrote:
> As an aside the error exit happens with err(), but mda_expand_format
> doesn't actually set errno, so ENOENT is not actually what is happening.
> Maybe that's another bug.

diff to fix that below. I would also change the last lines of this
function from perror("execle"); _exit(1); to err(1, "execle"), but maybe
there's some reason for _exit instead of exit that I'm not seeing?

diff --git a/usr.sbin/smtpd/mda_unpriv.c b/usr.sbin/smtpd/mda_unpriv.c
index 641034f7734..84fdc5dd622 100644
--- a/usr.sbin/smtpd/mda_unpriv.c
+++ b/usr.sbin/smtpd/mda_unpriv.c
@@ -54,11 +54,11 @@ mda_unpriv(struct dispatcher *dsp, struct deliver *deliver,
 
  if (strlcpy(mda_exec, mda_command, sizeof (mda_exec))
     >= sizeof (mda_exec))
- err(1, "mda command line too long");
+ errx(1, "mda command line too long");
 
  if (! mda_expand_format(mda_exec, sizeof mda_exec, deliver,
  &deliver->userinfo, NULL))
- err(1, "mda command line could not be expanded");
+ errx(1, "mda command line could not be expanded");
 
  mda_command = mda_exec;
 
@@ -83,16 +83,16 @@ mda_unpriv(struct dispatcher *dsp, struct deliver *deliver,
  mda_command_wrap = dict_get(env->sc_mda_wrappers,
     dsp->u.local.mda_wrapper);
  if (mda_command_wrap == NULL)
- err(1, "could not find wrapper %s",
+ errx(1, "could not find wrapper %s",
     dsp->u.local.mda_wrapper);
 
  if (strlcpy(mda_wrapper, mda_command_wrap, sizeof (mda_wrapper))
     >= sizeof (mda_wrapper))
- err(1, "mda command line too long");
+ errx(1, "mda command line too long");
 
  if (! mda_expand_format(mda_wrapper, sizeof mda_wrapper, deliver,
  &deliver->userinfo, mda_command))
- err(1, "mda command line could not be expanded");
+ errx(1, "mda command line could not be expanded");
  mda_command = mda_wrapper;
  }
  execle("/bin/sh", "/bin/sh", "-c", mda_command, (char *)NULL,

--
Lauri Tirkkonen | lotheac @ IRCnet