smtpd: change PATH for filters

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

smtpd: change PATH for filters

Martijn van Duren-5
With filters most likely defaulting to /usr/local/libexec/smtpd in the  
near future I would like to add this as the default PATH, followed by
the usual suspects. I left the usual suspects in place because people
have shown they already implement filters in awk and probably will do
so in /bin/sh in the future, which will need all the other paths.

I put it in PATH_FILTER, so it can easily be altered for portable
where sysadmins may decide to change the path for filters or for
us if we ever want anything in base under /usr/libexec/smtpd.

OK?

martijn@

Index: smtpd.c
===================================================================
RCS file: /cvs/src/usr.sbin/smtpd/smtpd.c,v
retrieving revision 1.324
diff -u -p -r1.324 smtpd.c
--- smtpd.c 26 Jul 2019 07:08:34 -0000 1.324
+++ smtpd.c 2 Sep 2019 15:00:47 -0000
@@ -1350,6 +1350,7 @@ fork_processor(const char *name, const c
  */
  if (read(STDERR_FILENO, &buf, 1) != 0)
  errx(1, "lka didn't properly close write end of error socket");
+ setenv("PATH", PATH_FILTER, 1);
  if (system(command) == -1)
  err(1, NULL);
 
Index: smtpd.h
===================================================================
RCS file: /cvs/src/usr.sbin/smtpd/smtpd.h,v
retrieving revision 1.633
diff -u -p -r1.633 smtpd.h
--- smtpd.h 28 Aug 2019 15:50:36 -0000 1.633
+++ smtpd.h 2 Sep 2019 15:00:47 -0000
@@ -56,6 +56,7 @@
 #define SMTPD_BACKLOG 5
 
 #define PATH_SMTPCTL "/usr/sbin/smtpctl"
+#define PATH_FILTER "/usr/local/libexec/smtpd:" _PATH_DEFPATH
 
 #define PATH_OFFLINE "/offline"
 #define PATH_PURGE "/purge"

Reply | Threaded
Open this post in threaded view
|

Re: smtpd: change PATH for filters

Theo de Raadt-2
This seems really unconvenitional.

PATH is normally used by interactive commands, or scripts which want
easily accessible programs.  libexec has traditionally been excluded.
The idea is that programs which need things in libexec, must hardcode
the path.  Intentionally.   This is a subdirectory of libexec, probably
trying to follow the same pattern.  So why is it not excluded in the same
way?

Second questoin: are filter programs going to be in /bin or /usr/sbin?
If not, why is /bin on this PATH you are defining?

It smell execvp abuse.

Martijn van Duren <[hidden email]> wrote:

> With filters most likely defaulting to /usr/local/libexec/smtpd in the  
> near future I would like to add this as the default PATH, followed by
> the usual suspects. I left the usual suspects in place because people
> have shown they already implement filters in awk and probably will do
> so in /bin/sh in the future, which will need all the other paths.
>
> I put it in PATH_FILTER, so it can easily be altered for portable
> where sysadmins may decide to change the path for filters or for
> us if we ever want anything in base under /usr/libexec/smtpd.
>
> OK?
>
> martijn@
>
> Index: smtpd.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/smtpd/smtpd.c,v
> retrieving revision 1.324
> diff -u -p -r1.324 smtpd.c
> --- smtpd.c 26 Jul 2019 07:08:34 -0000 1.324
> +++ smtpd.c 2 Sep 2019 15:00:47 -0000
> @@ -1350,6 +1350,7 @@ fork_processor(const char *name, const c
>   */
>   if (read(STDERR_FILENO, &buf, 1) != 0)
>   errx(1, "lka didn't properly close write end of error socket");
> + setenv("PATH", PATH_FILTER, 1);
>   if (system(command) == -1)
>   err(1, NULL);
>  
> Index: smtpd.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/smtpd/smtpd.h,v
> retrieving revision 1.633
> diff -u -p -r1.633 smtpd.h
> --- smtpd.h 28 Aug 2019 15:50:36 -0000 1.633
> +++ smtpd.h 2 Sep 2019 15:00:47 -0000
> @@ -56,6 +56,7 @@
>  #define SMTPD_BACKLOG 5
>  
>  #define PATH_SMTPCTL "/usr/sbin/smtpctl"
> +#define PATH_FILTER "/usr/local/libexec/smtpd:" _PATH_DEFPATH
>  
>  #define PATH_OFFLINE "/offline"
>  #define PATH_PURGE "/purge"
>

Reply | Threaded
Open this post in threaded view
|

Re: smtpd: change PATH for filters

Martijn van Duren-5
Gilles should probably elaborate, but the way things are now is that
system(3) is used to start the filters, allowing us to run any arbitrary
(set of) command(s) as a filter.

Since the filters now in ports are non-interactive commands I proposed
to move them to /usr/local/libexec/smtpd, which gilles@ is a proponent
of. This however means that all filters need to be specified by a full
path, which is not something I would promote.

Hence the proposition of this diff.

On 9/2/19 5:13 PM, Theo de Raadt wrote:

> This seems really unconvenitional.
>
> PATH is normally used by interactive commands, or scripts which want
> easily accessible programs.  libexec has traditionally been excluded.
> The idea is that programs which need things in libexec, must hardcode
> the path.  Intentionally.   This is a subdirectory of libexec, probably
> trying to follow the same pattern.  So why is it not excluded in the same
> way?
>
> Second questoin: are filter programs going to be in /bin or /usr/sbin?
> If not, why is /bin on this PATH you are defining?
>
> It smell execvp abuse.
>
> Martijn van Duren <[hidden email]> wrote:
>
>> With filters most likely defaulting to /usr/local/libexec/smtpd in the  
>> near future I would like to add this as the default PATH, followed by
>> the usual suspects. I left the usual suspects in place because people
>> have shown they already implement filters in awk and probably will do
>> so in /bin/sh in the future, which will need all the other paths.
>>
>> I put it in PATH_FILTER, so it can easily be altered for portable
>> where sysadmins may decide to change the path for filters or for
>> us if we ever want anything in base under /usr/libexec/smtpd.
>>
>> OK?
>>
>> martijn@
>>
>> Index: smtpd.c
>> ===================================================================
>> RCS file: /cvs/src/usr.sbin/smtpd/smtpd.c,v
>> retrieving revision 1.324
>> diff -u -p -r1.324 smtpd.c
>> --- smtpd.c 26 Jul 2019 07:08:34 -0000 1.324
>> +++ smtpd.c 2 Sep 2019 15:00:47 -0000
>> @@ -1350,6 +1350,7 @@ fork_processor(const char *name, const c
>>   */
>>   if (read(STDERR_FILENO, &buf, 1) != 0)
>>   errx(1, "lka didn't properly close write end of error socket");
>> + setenv("PATH", PATH_FILTER, 1);
>>   if (system(command) == -1)
>>   err(1, NULL);
>>  
>> Index: smtpd.h
>> ===================================================================
>> RCS file: /cvs/src/usr.sbin/smtpd/smtpd.h,v
>> retrieving revision 1.633
>> diff -u -p -r1.633 smtpd.h
>> --- smtpd.h 28 Aug 2019 15:50:36 -0000 1.633
>> +++ smtpd.h 2 Sep 2019 15:00:47 -0000
>> @@ -56,6 +56,7 @@
>>  #define SMTPD_BACKLOG 5
>>  
>>  #define PATH_SMTPCTL "/usr/sbin/smtpctl"
>> +#define PATH_FILTER "/usr/local/libexec/smtpd:" _PATH_DEFPATH
>>  
>>  #define PATH_OFFLINE "/offline"
>>  #define PATH_PURGE "/purge"
>>

Reply | Threaded
Open this post in threaded view
|

Re: smtpd: change PATH for filters

Theo de Raadt-2
Martijn van Duren <[hidden email]> wrote:

> Gilles should probably elaborate, but the way things are now is that
> system(3) is used to start the filters, allowing us to run any arbitrary
> (set of) command(s) as a filter.
>
> Since the filters now in ports are non-interactive commands I proposed
> to move them to /usr/local/libexec/smtpd, which gilles@ is a proponent
> of. This however means that all filters need to be specified by a full
> path, which is not something I would promote.
>
> Hence the proposition of this diff.

None of that prevents using an absolute path.

absolute path execution is "the rule" when it comes to libexec.

For instance, inetd.conf

Reply | Threaded
Open this post in threaded view
|

Re: smtpd: change PATH for filters

Gilles Chehade-7
In reply to this post by Martijn van Duren-5
September 2, 2019 5:23 PM, "Martijn van Duren" <[hidden email]> wrote:

> Gilles should probably elaborate, but the way things are now is that
> system(3) is used to start the filters, allowing us to run any arbitrary
> (set of) command(s) as a filter.
>
> Since the filters now in ports are non-interactive commands I proposed
> to move them to /usr/local/libexec/smtpd, which gilles@ is a proponent
> of. This however means that all filters need to be specified by a full
> path, which is not something I would promote.
>
> Hence the proposition of this diff.
>

I don't feel comfortable adding that path to PATH, even if we're going
to call system() right behind.

Why not detect if the command starts with '/' and prepend libexec directory
if that's not the case ?



> On 9/2/19 5:13 PM, Theo de Raadt wrote:
>
>> This seems really unconvenitional.
>>
>> PATH is normally used by interactive commands, or scripts which want
>> easily accessible programs. libexec has traditionally been excluded.
>> The idea is that programs which need things in libexec, must hardcode
>> the path. Intentionally. This is a subdirectory of libexec, probably
>> trying to follow the same pattern. So why is it not excluded in the same
>> way?
>>
>> Second questoin: are filter programs going to be in /bin or /usr/sbin?
>> If not, why is /bin on this PATH you are defining?
>>
>> It smell execvp abuse.
>>
>> Martijn van Duren <[hidden email]> wrote:
>>
>>> With filters most likely defaulting to /usr/local/libexec/smtpd in the
>>> near future I would like to add this as the default PATH, followed by
>>> the usual suspects. I left the usual suspects in place because people
>>> have shown they already implement filters in awk and probably will do
>>> so in /bin/sh in the future, which will need all the other paths.
>>>
>>> I put it in PATH_FILTER, so it can easily be altered for portable
>>> where sysadmins may decide to change the path for filters or for
>>> us if we ever want anything in base under /usr/libexec/smtpd.
>>>
>>> OK?
>>>
>>> martijn@
>>>
>>> Index: smtpd.c
>>> ===================================================================
>>> RCS file: /cvs/src/usr.sbin/smtpd/smtpd.c,v
>>> retrieving revision 1.324
>>> diff -u -p -r1.324 smtpd.c
>>> --- smtpd.c 26 Jul 2019 07:08:34 -0000 1.324
>>> +++ smtpd.c 2 Sep 2019 15:00:47 -0000
>>> @@ -1350,6 +1350,7 @@ fork_processor(const char *name, const c
>>> */
>>> if (read(STDERR_FILENO, &buf, 1) != 0)
>>> errx(1, "lka didn't properly close write end of error socket");
>>> + setenv("PATH", PATH_FILTER, 1);
>>> if (system(command) == -1)
>>> err(1, NULL);
>>>
>>> Index: smtpd.h
>>> ===================================================================
>>> RCS file: /cvs/src/usr.sbin/smtpd/smtpd.h,v
>>> retrieving revision 1.633
>>> diff -u -p -r1.633 smtpd.h
>>> --- smtpd.h 28 Aug 2019 15:50:36 -0000 1.633
>>> +++ smtpd.h 2 Sep 2019 15:00:47 -0000
>>> @@ -56,6 +56,7 @@
>>> #define SMTPD_BACKLOG 5
>>>
>>> #define PATH_SMTPCTL "/usr/sbin/smtpctl"
>>> +#define PATH_FILTER "/usr/local/libexec/smtpd:" _PATH_DEFPATH
>>>
>>> #define PATH_OFFLINE "/offline"
>>> #define PATH_PURGE "/purge"

Reply | Threaded
Open this post in threaded view
|

Re: smtpd: change PATH for filters

Gilles Chehade-7
September 2, 2019 5:55 PM, [hidden email] wrote:

> September 2, 2019 5:23 PM, "Martijn van Duren" <[hidden email]> wrote:
>
>> Gilles should probably elaborate, but the way things are now is that
>> system(3) is used to start the filters, allowing us to run any arbitrary
>> (set of) command(s) as a filter.
>>
>> Since the filters now in ports are non-interactive commands I proposed
>> to move them to /usr/local/libexec/smtpd, which gilles@ is a proponent
>> of. This however means that all filters need to be specified by a full
>> path, which is not something I would promote.
>>
>> Hence the proposition of this diff.
>
> I don't feel comfortable adding that path to PATH, even if we're going
> to call system() right behind.
>
> Why not detect if the command starts with '/' and prepend libexec directory
> if that's not the case ?
>

I also want to rework the command line before it's passed to system() so we
exec and save some unnecessary processes which are only waiting for a child
to exit its infinite loop.

To do that, we are going to copy the command anyways so checking if command
starts with a / and prepending the absolute path is going to be easy.

Reply | Threaded
Open this post in threaded view
|

Re: smtpd: change PATH for filters

Theo de Raadt-2
In reply to this post by Gilles Chehade-7
[hidden email] wrote:

> September 2, 2019 5:23 PM, "Martijn van Duren" <[hidden email]> wrote:
>
> > Gilles should probably elaborate, but the way things are now is that
> > system(3) is used to start the filters, allowing us to run any arbitrary
> > (set of) command(s) as a filter.
> >
> > Since the filters now in ports are non-interactive commands I proposed
> > to move them to /usr/local/libexec/smtpd, which gilles@ is a proponent
> > of. This however means that all filters need to be specified by a full
> > path, which is not something I would promote.
> >
> > Hence the proposition of this diff.
> >
>
> I don't feel comfortable adding that path to PATH, even if we're going
> to call system() right behind.

The main problem with adding it to PATH, is the transitive nature of the
environment.  If the first command being run does a system of something
else, it will be seen again.  If it runs a shell script, there it is.
Easily reachable code fragments becomes available by virtue of being at
the head of the path.  That is improper.

Reply | Threaded
Open this post in threaded view
|

Re: smtpd: change PATH for filters

Martijn van Duren-5
In reply to this post by Gilles Chehade-7
On 9/2/19 6:00 PM, [hidden email] wrote:

> September 2, 2019 5:55 PM, [hidden email] wrote:
>
>> September 2, 2019 5:23 PM, "Martijn van Duren" <[hidden email]> wrote:
>>
>>> Gilles should probably elaborate, but the way things are now is that
>>> system(3) is used to start the filters, allowing us to run any arbitrary
>>> (set of) command(s) as a filter.
>>>
>>> Since the filters now in ports are non-interactive commands I proposed
>>> to move them to /usr/local/libexec/smtpd, which gilles@ is a proponent
>>> of. This however means that all filters need to be specified by a full
>>> path, which is not something I would promote.
>>>
>>> Hence the proposition of this diff.
>>
>> I don't feel comfortable adding that path to PATH, even if we're going
>> to call system() right behind.
>>
>> Why not detect if the command starts with '/' and prepend libexec directory
>> if that's not the case ?
>>
>
> I also want to rework the command line before it's passed to system() so we
> exec and save some unnecessary processes which are only waiting for a child
> to exit its infinite loop.
>
> To do that, we are going to copy the command anyways so checking if command
> starts with a / and prepending the absolute path is going to be easy.
>
Something like this?

Index: smtpd.c
===================================================================
RCS file: /cvs/src/usr.sbin/smtpd/smtpd.c,v
retrieving revision 1.324
diff -u -p -r1.324 smtpd.c
--- smtpd.c 26 Jul 2019 07:08:34 -0000 1.324
+++ smtpd.c 2 Sep 2019 17:27:31 -0000
@@ -1277,6 +1277,8 @@ fork_processor(const char *name, const c
  int sp[2], errfd[2];
  struct passwd *pw;
  struct group *gr;
+ char exec[_POSIX_ARG_MAX];
+ int execr;
 
  if (user == NULL)
  user = SMTPD_USER;
@@ -1340,6 +1342,14 @@ fork_processor(const char *name, const c
     signal(SIGHUP, SIG_DFL) == SIG_ERR)
  err(1, "signal");
 
+ if (command[0] == '/')
+ execr = snprintf(exec, sizeof(exec), "exec %s", command);
+ else
+ execr = snprintf(exec, sizeof(exec), "exec %s/filter-%s",
+    PATH_LIBEXEC, command);
+ if (execr >= (int) sizeof(exec))
+ errx(1, "%s: exec path too long", name);
+
  /*
  * Wait for lka to acknowledge that it received the fd.
  * This prevents a race condition between the filter sending an error
@@ -1350,7 +1360,7 @@ fork_processor(const char *name, const c
  */
  if (read(STDERR_FILENO, &buf, 1) != 0)
  errx(1, "lka didn't properly close write end of error socket");
- if (system(command) == -1)
+ if (system(exec) == -1)
  err(1, NULL);
 
  /* there's no successful exit from a processor */
Index: smtpd.conf.5
===================================================================
RCS file: /cvs/src/usr.sbin/smtpd/smtpd.conf.5,v
retrieving revision 1.221
diff -u -p -r1.221 smtpd.conf.5
--- smtpd.conf.5 17 Aug 2019 14:43:21 -0000 1.221
+++ smtpd.conf.5 2 Sep 2019 17:27:31 -0000
@@ -383,6 +383,11 @@ filter
 .Ar filter-name
 from
 .Ar command .
+If
+.Ar command
+starts with a slash it is executed with an absolute path,
+else it will be prepended with
+.Dq /usr/local/libexec/smtpd/filter- .
 .It Ic include Qq Ar pathname
 Replace this directive with the content of the additional configuration
 file at the absolute
@@ -757,6 +762,11 @@ Register an external process named
 from
 .Ar command .
 Such processes may be used to share the same instance between multiple filters.
+If
+.Ar command
+starts with a slash it is executed with an absolute path,
+else it will be prepended with
+.Dq /usr/local/libexec/smtpd/filter- .
 .It Ic queue Cm compression
 Store queue files in a compressed format.
 This may be useful to save disk space.

Reply | Threaded
Open this post in threaded view
|

Re: smtpd: change PATH for filters

Gilles Chehade-7
September 2, 2019 7:29 PM, "Martijn van Duren" <[hidden email]> wrote:

> On 9/2/19 6:00 PM, [hidden email] wrote:
>
>> September 2, 2019 5:55 PM, [hidden email] wrote:
>>
>>> September 2, 2019 5:23 PM, "Martijn van Duren" <[hidden email]> wrote:
>>
>> Gilles should probably elaborate, but the way things are now is that
>> system(3) is used to start the filters, allowing us to run any arbitrary
>> (set of) command(s) as a filter.
>>
>> Since the filters now in ports are non-interactive commands I proposed
>> to move them to /usr/local/libexec/smtpd, which gilles@ is a proponent
>> of. This however means that all filters need to be specified by a full
>> path, which is not something I would promote.
>>
>> Hence the proposition of this diff.
>>> I don't feel comfortable adding that path to PATH, even if we're going
>>> to call system() right behind.
>>>
>>> Why not detect if the command starts with '/' and prepend libexec directory
>>> if that's not the case ?
>>
>> I also want to rework the command line before it's passed to system() so we
>> exec and save some unnecessary processes which are only waiting for a child
>> to exit its infinite loop.
>>
>> To do that, we are going to copy the command anyways so checking if command
>> starts with a / and prepending the absolute path is going to be easy.
>
> Something like this?
>

can't test right now but comments inlined:


> Index: smtpd.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/smtpd/smtpd.c,v
> retrieving revision 1.324
> diff -u -p -r1.324 smtpd.c
> --- smtpd.c 26 Jul 2019 07:08:34 -0000 1.324
> +++ smtpd.c 2 Sep 2019 17:27:31 -0000
> @@ -1277,6 +1277,8 @@ fork_processor(const char *name, const c
> int sp[2], errfd[2];
> struct passwd *pw;
> struct group *gr;
> + char exec[_POSIX_ARG_MAX];

I don't know if _POSIX_ARG_MAX is the proper define to use,
I genuinely don't know so someone more knowledgeable should jump in.


> + int execr;
>
> if (user == NULL)
> user = SMTPD_USER;
> @@ -1340,6 +1342,14 @@ fork_processor(const char *name, const c
> signal(SIGHUP, SIG_DFL) == SIG_ERR)
> err(1, "signal");
>
> + if (command[0] == '/')
> + execr = snprintf(exec, sizeof(exec), "exec %s", command);
> + else
> + execr = snprintf(exec, sizeof(exec), "exec %s/filter-%s",
> + PATH_LIBEXEC, command);

I don't really like that 'filter-' is implicit.

So if I specify full path it's filter-rspamd, when i don't it's rspamd,
which is confusing because that's also the name of a software on my
system.

I think people can live with typing 'filter-' and we keep it explicit.



> + if (execr >= (int) sizeof(exec))
> + errx(1, "%s: exec path too long", name);
> +
> /*
> * Wait for lka to acknowledge that it received the fd.
> * This prevents a race condition between the filter sending an error
> @@ -1350,7 +1360,7 @@ fork_processor(const char *name, const c
> */
> if (read(STDERR_FILENO, &buf, 1) != 0)
> errx(1, "lka didn't properly close write end of error socket");
> - if (system(command) == -1)
> + if (system(exec) == -1)
> err(1, NULL);
>
> /* there's no successful exit from a processor */
> Index: smtpd.conf.5
> ===================================================================
> RCS file: /cvs/src/usr.sbin/smtpd/smtpd.conf.5,v
> retrieving revision 1.221
> diff -u -p -r1.221 smtpd.conf.5
> --- smtpd.conf.5 17 Aug 2019 14:43:21 -0000 1.221
> +++ smtpd.conf.5 2 Sep 2019 17:27:31 -0000
> @@ -383,6 +383,11 @@ filter
> .Ar filter-name
> from
> .Ar command .
> +If
> +.Ar command
> +starts with a slash it is executed with an absolute path,
> +else it will be prepended with
> +.Dq /usr/local/libexec/smtpd/filter- .
> .It Ic include Qq Ar pathname
> Replace this directive with the content of the additional configuration
> file at the absolute
> @@ -757,6 +762,11 @@ Register an external process named
> from
> .Ar command .
> Such processes may be used to share the same instance between multiple filters.
> +If
> +.Ar command
> +starts with a slash it is executed with an absolute path,
> +else it will be prepended with
> +.Dq /usr/local/libexec/smtpd/filter- .
> .It Ic queue Cm compression
> Store queue files in a compressed format.
> This may be useful to save disk space.

Reply | Threaded
Open this post in threaded view
|

Re: smtpd: change PATH for filters

Martijn van Duren-5
On 9/2/19 9:10 PM, [hidden email] wrote:

> September 2, 2019 7:29 PM, "Martijn van Duren" <[hidden email]> wrote:
>
>> On 9/2/19 6:00 PM, [hidden email] wrote:
>>
>>> September 2, 2019 5:55 PM, [hidden email] wrote:
>>>
>>>> September 2, 2019 5:23 PM, "Martijn van Duren" <[hidden email]> wrote:
>>>
>>> Gilles should probably elaborate, but the way things are now is that
>>> system(3) is used to start the filters, allowing us to run any arbitrary
>>> (set of) command(s) as a filter.
>>>
>>> Since the filters now in ports are non-interactive commands I proposed
>>> to move them to /usr/local/libexec/smtpd, which gilles@ is a proponent
>>> of. This however means that all filters need to be specified by a full
>>> path, which is not something I would promote.
>>>
>>> Hence the proposition of this diff.
>>>> I don't feel comfortable adding that path to PATH, even if we're going
>>>> to call system() right behind.
>>>>
>>>> Why not detect if the command starts with '/' and prepend libexec directory
>>>> if that's not the case ?
>>>
>>> I also want to rework the command line before it's passed to system() so we
>>> exec and save some unnecessary processes which are only waiting for a child
>>> to exit its infinite loop.
>>>
>>> To do that, we are going to copy the command anyways so checking if command
>>> starts with a / and prepending the absolute path is going to be easy.
>>
>> Something like this?
>>
>
> can't test right now but comments inlined:
>
>
>> Index: smtpd.c
>> ===================================================================
>> RCS file: /cvs/src/usr.sbin/smtpd/smtpd.c,v
>> retrieving revision 1.324
>> diff -u -p -r1.324 smtpd.c
>> --- smtpd.c 26 Jul 2019 07:08:34 -0000 1.324
>> +++ smtpd.c 2 Sep 2019 17:27:31 -0000
>> @@ -1277,6 +1277,8 @@ fork_processor(const char *name, const c
>> int sp[2], errfd[2];
>> struct passwd *pw;
>> struct group *gr;
>> + char exec[_POSIX_ARG_MAX];
>
> I don't know if _POSIX_ARG_MAX is the proper define to use,
> I genuinely don't know so someone more knowledgeable should jump in.

https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/limits.h.html
{_POSIX_ARG_MAX}
    Maximum length of argument to the exec functions including environment data.
    Value: 4 096

>
>
>> + int execr;
>>
>> if (user == NULL)
>> user = SMTPD_USER;
>> @@ -1340,6 +1342,14 @@ fork_processor(const char *name, const c
>> signal(SIGHUP, SIG_DFL) == SIG_ERR)
>> err(1, "signal");
>>
>> + if (command[0] == '/')
>> + execr = snprintf(exec, sizeof(exec), "exec %s", command);
>> + else
>> + execr = snprintf(exec, sizeof(exec), "exec %s/filter-%s",
>> + PATH_LIBEXEC, command);
>
> I don't really like that 'filter-' is implicit.
>
> So if I specify full path it's filter-rspamd, when i don't it's rspamd,
> which is confusing because that's also the name of a software on my
> system.
>
> I think people can live with typing 'filter-' and we keep it explicit.

Sure, no problem. I choose this approach to be more in line with
queue-*, scheduler-*, and table-*.
I'll change it before commit.

>
>
>
>> + if (execr >= (int) sizeof(exec))
>> + errx(1, "%s: exec path too long", name);
>> +
>> /*
>> * Wait for lka to acknowledge that it received the fd.
>> * This prevents a race condition between the filter sending an error
>> @@ -1350,7 +1360,7 @@ fork_processor(const char *name, const c
>> */
>> if (read(STDERR_FILENO, &buf, 1) != 0)
>> errx(1, "lka didn't properly close write end of error socket");
>> - if (system(command) == -1)
>> + if (system(exec) == -1)
>> err(1, NULL);
>>
>> /* there's no successful exit from a processor */
>> Index: smtpd.conf.5
>> ===================================================================
>> RCS file: /cvs/src/usr.sbin/smtpd/smtpd.conf.5,v
>> retrieving revision 1.221
>> diff -u -p -r1.221 smtpd.conf.5
>> --- smtpd.conf.5 17 Aug 2019 14:43:21 -0000 1.221
>> +++ smtpd.conf.5 2 Sep 2019 17:27:31 -0000
>> @@ -383,6 +383,11 @@ filter
>> .Ar filter-name
>> from
>> .Ar command .
>> +If
>> +.Ar command
>> +starts with a slash it is executed with an absolute path,
>> +else it will be prepended with
>> +.Dq /usr/local/libexec/smtpd/filter- .
>> .It Ic include Qq Ar pathname
>> Replace this directive with the content of the additional configuration
>> file at the absolute
>> @@ -757,6 +762,11 @@ Register an external process named
>> from
>> .Ar command .
>> Such processes may be used to share the same instance between multiple filters.
>> +If
>> +.Ar command
>> +starts with a slash it is executed with an absolute path,
>> +else it will be prepended with
>> +.Dq /usr/local/libexec/smtpd/filter- .
>> .It Ic queue Cm compression
>> Store queue files in a compressed format.
>> This may be useful to save disk space.

Reply | Threaded
Open this post in threaded view
|

Re: smtpd: change PATH for filters

Gilles Chehade-7
September 2, 2019 9:15 PM, "Martijn van Duren" <[hidden email]> wrote:

> On 9/2/19 9:10 PM, [hidden email] wrote:
>
>> September 2, 2019 7:29 PM, "Martijn van Duren" <[hidden email]> wrote:
>>
>>> On 9/2/19 6:00 PM, [hidden email] wrote:
>>
>> September 2, 2019 5:55 PM, [hidden email] wrote:
>>
>> September 2, 2019 5:23 PM, "Martijn van Duren" <[hidden email]> wrote:
>>
>> Gilles should probably elaborate, but the way things are now is that
>> system(3) is used to start the filters, allowing us to run any arbitrary
>> (set of) command(s) as a filter.
>>
>> Since the filters now in ports are non-interactive commands I proposed
>> to move them to /usr/local/libexec/smtpd, which gilles@ is a proponent
>> of. This however means that all filters need to be specified by a full
>> path, which is not something I would promote.
>>
>> Hence the proposition of this diff.
>> I don't feel comfortable adding that path to PATH, even if we're going
>> to call system() right behind.
>>
>> Why not detect if the command starts with '/' and prepend libexec directory
>> if that's not the case ?
>>
>> I also want to rework the command line before it's passed to system() so we
>> exec and save some unnecessary processes which are only waiting for a child
>> to exit its infinite loop.
>>
>> To do that, we are going to copy the command anyways so checking if command
>> starts with a / and prepending the absolute path is going to be easy.
>>> Something like this?
>>
>> can't test right now but comments inlined:
>>
>>> Index: smtpd.c
>>> ===================================================================
>>> RCS file: /cvs/src/usr.sbin/smtpd/smtpd.c,v
>>> retrieving revision 1.324
>>> diff -u -p -r1.324 smtpd.c
>>> --- smtpd.c 26 Jul 2019 07:08:34 -0000 1.324
>>> +++ smtpd.c 2 Sep 2019 17:27:31 -0000
>>> @@ -1277,6 +1277,8 @@ fork_processor(const char *name, const c
>>> int sp[2], errfd[2];
>>> struct passwd *pw;
>>> struct group *gr;
>>> + char exec[_POSIX_ARG_MAX];
>>
>> I don't know if _POSIX_ARG_MAX is the proper define to use,
>> I genuinely don't know so someone more knowledgeable should jump in.
>
> https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/limits.h.html
> {_POSIX_ARG_MAX}
> Maximum length of argument to the exec functions including environment data.
> Value: 4 096
>

looks good to me unless someone objects


>>> + int execr;
>>>
>>> if (user == NULL)
>>> user = SMTPD_USER;
>>> @@ -1340,6 +1342,14 @@ fork_processor(const char *name, const c
>>> signal(SIGHUP, SIG_DFL) == SIG_ERR)
>>> err(1, "signal");
>>>
>>> + if (command[0] == '/')
>>> + execr = snprintf(exec, sizeof(exec), "exec %s", command);
>>> + else
>>> + execr = snprintf(exec, sizeof(exec), "exec %s/filter-%s",
>>> + PATH_LIBEXEC, command);
>>
>> I don't really like that 'filter-' is implicit.
>>
>> So if I specify full path it's filter-rspamd, when i don't it's rspamd,
>> which is confusing because that's also the name of a software on my
>> system.
>>
>> I think people can live with typing 'filter-' and we keep it explicit.
>
> Sure, no problem. I choose this approach to be more in line with
> queue-*, scheduler-*, and table-*.
> I'll change it before commit.
>

okie dokie

I dislike that we did this for queue-*, scheduler-* and table-* too,
I'd rather see if we can change that in the future.

I have a plan for 2020 to switch queue / scheduler / table to an API
like filters' so we might want to revisit then :-)


>>> + if (execr >= (int) sizeof(exec))
>>> + errx(1, "%s: exec path too long", name);
>>> +
>>> /*
>>> * Wait for lka to acknowledge that it received the fd.
>>> * This prevents a race condition between the filter sending an error
>>> @@ -1350,7 +1360,7 @@ fork_processor(const char *name, const c
>>> */
>>> if (read(STDERR_FILENO, &buf, 1) != 0)
>>> errx(1, "lka didn't properly close write end of error socket");
>>> - if (system(command) == -1)
>>> + if (system(exec) == -1)
>>> err(1, NULL);
>>>
>>> /* there's no successful exit from a processor */
>>> Index: smtpd.conf.5
>>> ===================================================================
>>> RCS file: /cvs/src/usr.sbin/smtpd/smtpd.conf.5,v
>>> retrieving revision 1.221
>>> diff -u -p -r1.221 smtpd.conf.5
>>> --- smtpd.conf.5 17 Aug 2019 14:43:21 -0000 1.221
>>> +++ smtpd.conf.5 2 Sep 2019 17:27:31 -0000
>>> @@ -383,6 +383,11 @@ filter
>>> .Ar filter-name
>>> from
>>> .Ar command .
>>> +If
>>> +.Ar command
>>> +starts with a slash it is executed with an absolute path,
>>> +else it will be prepended with
>>> +.Dq /usr/local/libexec/smtpd/filter- .
>>> .It Ic include Qq Ar pathname
>>> Replace this directive with the content of the additional configuration
>>> file at the absolute
>>> @@ -757,6 +762,11 @@ Register an external process named
>>> from
>>> .Ar command .
>>> Such processes may be used to share the same instance between multiple filters.
>>> +If
>>> +.Ar command
>>> +starts with a slash it is executed with an absolute path,
>>> +else it will be prepended with
>>> +.Dq /usr/local/libexec/smtpd/filter- .
>>> .It Ic queue Cm compression
>>> Store queue files in a compressed format.
>>> This may be useful to save disk space.

Reply | Threaded
Open this post in threaded view
|

Re: smtpd: change PATH for filters

Todd C. Miller-3
In reply to this post by Martijn van Duren-5
On Mon, 02 Sep 2019 21:15:23 +0200, Martijn van Duren wrote:

> https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/limits.h.html
> {_POSIX_ARG_MAX}
>     Maximum length of argument to the exec functions including environment da
> ta.
>     Value: 4 096

Note that this is the minimum value POSIX requires, implementations
are allowed to have larger values.  The _actual_ value on OpenBSD
is ARG_MAX which is much higher, 256 * 1024.

Personally, I'd just use asprintf() here.  The actual limit is going
to be closer to ARG_MAX - (strlen("sh) + strlen("-c") so I don't
think you are likely to get it exactly.  The kernel should enforce
that limit, not smtpd.

 - todd

Reply | Threaded
Open this post in threaded view
|

Re: smtpd: change PATH for filters

Martijn van Duren-5
On 9/3/19 4:30 PM, Todd C. Miller wrote:

> On Mon, 02 Sep 2019 21:15:23 +0200, Martijn van Duren wrote:
>
>> https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/limits.h.html
>> {_POSIX_ARG_MAX}
>>     Maximum length of argument to the exec functions including environment da
>> ta.
>>     Value: 4 096
>
> Note that this is the minimum value POSIX requires, implementations
> are allowed to have larger values.  The _actual_ value on OpenBSD
> is ARG_MAX which is much higher, 256 * 1024.
>
> Personally, I'd just use asprintf() here.  The actual limit is going
> to be closer to ARG_MAX - (strlen("sh) + strlen("-c") so I don't
> think you are likely to get it exactly.  The kernel should enforce
> that limit, not smtpd.
>
>  - todd
>
I choose this value because I hit the maximum command length of the
shell before. This way I'm somewhat confident that the shell doesn't
do something weird with my command if we ever overflow it's internal
buffer. So I based it on the shell, not the kernel.

Also, I deem it extremely unlikely that any configuration ever
has a filter command which reaches over _POSIX_ARG_MAX.

If you feel strongly about changing it asprintf I'll prepare a diff,
I just want my reasoning out there first.

Reply | Threaded
Open this post in threaded view
|

Re: smtpd: change PATH for filters

Todd C. Miller-3
On Tue, 03 Sep 2019 17:11:22 +0200, Martijn van Duren wrote:

> I choose this value because I hit the maximum command length of the
> shell before. This way I'm somewhat confident that the shell doesn't
> do something weird with my command if we ever overflow it's internal
> buffer. So I based it on the shell, not the kernel.
>
> Also, I deem it extremely unlikely that any configuration ever
> has a filter command which reaches over _POSIX_ARG_MAX.

OK, that's fine.  We can revisit this later if system(3) is removed
from the equation.

 - todd