Re: [diff] src/usr.sbin/smtpd: add a forward-file option
> On 20 Dec 2020, at 18:15, Chris Bennett > wrote: > > On Sun, Dec 20, 2020 at 09:51:35AM +0100, Gilles CHEHADE wrote: >> >> >>> On 20 Dec 2020, at 07:13, Sebastien Marie wrote: >>> >>> On Sat, Dec 19, 2020 at 10:36:32PM +, gil...@poolp.org wrote: Hello, Whenever a rule with a local action (mbox, maildir, lmtp or mda) is matched, smtpd will attempt to search for a ~/.forward file in the recipient directory and process it. This may be convenient for some setups but it is an implicit behavior that's not overridable and not always wanted. This diff changes this behavior by requiring the admins to explicitly allow the forward files processing in the actions when desired: action "local_users" maildir forward-file With this diff, if forward-file is not specified, code to request parent process for an fd is bypassed and the expansion layer just pretends parent couldn't find one. This let the code fallback in an already existing code path with the proper behavior and is very uninvasive. >>> >>> if I could understood the direction (which is fine as it makes the >>> daemon less behaviour dependant on a user settings), the default seems >>> wrong to me (at least for now, and for OpenBSD base specifically). >>> >>> Currently, root@ mail delivery is based on /root/.forward file: >>> install is writing this file to redirect root@ mail to user (if user >>> was created at install-time). It is done this way since 2011 (see >>> distrib/miniroot/install.sh rev 1.218). So I assume that all installs >>> which were done with a user configured, since 2011, could use it. >> >> Yes, the default would need to be changed as follows: >> >> mini$ diff -uNp smtpd.conf smtpd.conf.new >> >> >> --- smtpd.conf Mon Dec 14 22:13:04 2020 >> +++ smtpd.conf.new Sun Dec 20 09:43:22 2020 >> @@ -11,7 +11,7 @@ listen on socket >> # >> listen on all hostname debug.poolp.org >> >> -action "local_mail" maildir alias >> +action "local_mail" maildir alias forward-file >> action "outbound" relay >> > > My src tree still has mbox as the default. There was talk of changing > from mbox to maildir as default. Is this now going forward also? Nope, sorry, this was just an example from my machine which uses maildir so that you get the idea, I don’t propose we name machines “debug.poolp.org” by default either ;-) > While mbox is simple, once I moved to Dovecot for IMAP, changing mbox to > maildir was easy, but needed, amongst some other non-mbox choices. > > I think new users will be much happier learning maildir and skipping the > whole mbox thing. > > My 2 cents. :^) I don’t need to be convinced as I was the one who launched the maildir debate, however base is not ready and mail doesn’t support maildir to begin with. If there was a switch to maildir, the first effect would be that you could not read the mail sent to root at install and the daily, weekly and monthly mails, including the insecurity report.
Re: [diff] src/usr.sbin/smtpd: add a forward-file option
On Sun, Dec 20, 2020 at 09:51:35AM +0100, Gilles CHEHADE wrote: > > > > On 20 Dec 2020, at 07:13, Sebastien Marie wrote: > > > > On Sat, Dec 19, 2020 at 10:36:32PM +, gil...@poolp.org wrote: > >> Hello, > >> > >> Whenever a rule with a local action (mbox, maildir, lmtp or mda) is > >> matched, smtpd will > >> attempt to search for a ~/.forward file in the recipient directory and > >> process it. This > >> may be convenient for some setups but it is an implicit behavior that's > >> not overridable > >> and not always wanted. > >> > >> This diff changes this behavior by requiring the admins to explicitly > >> allow the forward > >> files processing in the actions when desired: > >> > >>action "local_users" maildir forward-file > >> > >> > >> With this diff, if forward-file is not specified, code to request parent > >> process for an > >> fd is bypassed and the expansion layer just pretends parent couldn't find > >> one. This let > >> the code fallback in an already existing code path with the proper > >> behavior and is very > >> uninvasive. > >> > > > > if I could understood the direction (which is fine as it makes the > > daemon less behaviour dependant on a user settings), the default seems > > wrong to me (at least for now, and for OpenBSD base specifically). > > > > Currently, root@ mail delivery is based on /root/.forward file: > > install is writing this file to redirect root@ mail to user (if user > > was created at install-time). It is done this way since 2011 (see > > distrib/miniroot/install.sh rev 1.218). So I assume that all installs > > which were done with a user configured, since 2011, could use it. > > Yes, the default would need to be changed as follows: > > mini$ diff -uNp smtpd.conf smtpd.conf.new > > > > --- smtpd.confMon Dec 14 22:13:04 2020 > +++ smtpd.conf.newSun Dec 20 09:43:22 2020 > @@ -11,7 +11,7 @@ listen on socket > # > listen on all hostname debug.poolp.org > > -action "local_mail" maildir alias > +action "local_mail" maildir alias forward-file > action "outbound" relay > My src tree still has mbox as the default. There was talk of changing from mbox to maildir as default. Is this now going forward also? While mbox is simple, once I moved to Dovecot for IMAP, changing mbox to maildir was easy, but needed, amongst some other non-mbox choices. I think new users will be much happier learning maildir and skipping the whole mbox thing. My 2 cents. :^) Chris Bennett
Re: [diff] src/usr.sbin/smtpd: add a forward-file option
> On 20 Dec 2020, at 10:14, Sebastien Marie wrote: > > On Sat, Dec 19, 2020 at 11:19:10PM -0700, Theo de Raadt wrote: >> There are thousands of people with smtpd configurations, and sysmerge >> is not going to handle this. >> >> We cannot expect them all to change their files. This is madness. > > Well, it wouldn't be the first time. But I agree that such changes > should be rare and have really good reason for. > > So yes, even if the option is desirable and being off-by-default would > be a good default, the flag-day way for handling it is complex. I really want to emphasise that I don’t suggest off-by-default on OpenBSD but just making the feature explicit. The default smtpd.conf could still have the option to retain the default behaviour. > Regarding the option itself, if I recall correctly some descriptions > made by Gilles about smtpd, opening ~/.forward is one of the few tasks > done by the priviligied process of smtpd. So it could make sense to > avoid it if not need. > > Gilles, could you confirm that having an option to remove .forward > capability (whatever the default value of the option is) could > effectively help to reduce the attack surface of smtpd ? Yes, this is one of the benefits. Setups that don’t ask for forward-file don’t go through the parent process at every recipient submitted in a session, that’s one imsg less handled by the privileged process. > For example, as immediate consequence, I see no reason for smtpd > priviliegied process to keep a full filesystem view: it might be > possible to restricted it to few directories with unveil(2) (I assume > priviliegied process is still need for bsd_auth, and bsd_auth will > have some requirements). Yes !
Re: [diff] src/usr.sbin/smtpd: add a forward-file option
> On 20 Dec 2020, at 10:03, Gilles CHEHADE wrote: > > >> On 20 Dec 2020, at 07:19, Theo de Raadt wrote: >> >> There are thousands of people with smtpd configurations, and sysmerge >> is not going to handle this. >> >> We cannot expect them all to change their files. This is madness. >> >> Gilles, I think you should be adding an option that blocks it optionally, >> and then some operators can use that. If they wish. I am surprised you >> think this can be a default, when as Sebastien points out the base system >> uses it today... >> > > I know that this isn’t convenient and my first version of the diff was a > “disalllow-forward-file” option but: > > The diff was to discuss what I think is the right way of doing it, not the > one I find the most convenient. > If this is not desired, I can submit a diff for the convenient way but I > would have hated not showing what I think is right first. > > In addition, my diff is a turn on a feature explicitly whereas the > “disallow-forward-file” option is a turn off an implicit behaviour, > and when I see that some people don’t even know that .forward files are a > thing, I feel it’s the wrong way around. People who > want forward files know they exist and can ask for it, whereas people who > don’t know they exist or who don’t request it will > get it behind their backs. > > As I said to semarie@ and millert@, the default configuration could be > adapted to add forward-file to the mbox action, > and this diff could be adapted to not ignore .forward files but warn that > they are used on a rule without the keyword to > give people two releases to adapt since we can’t expect everyone to change > their files but we can expect them to upgrade > at least every two releases. > > Also, what doesn’t show on this diff is that if we rely on the implicit > behaviour and a “disallow-forward-file” it kind of makes > other features backwards too in terms of configuration. > > Assuming disallow-forward-file, then do we add an option to disallow > execution of an mda or do we add an option to allow it ? > Does the default behaviour of forward files is to execute custom commands or > not ? > If not, then how do we express it if there’s no option visible in the conf ? > > It makes the grammar very weird :-/ I’d like to add something I forgot to mention, there are two bonus benefits to do this: 1- the forward file handling requires an indirection through the parent process to obtain an fd to the .forward file, an indirection through a privileged process which would be completely bypassed for all users who do not explicitly require it. 2- if we make the feature explicit, then it becomes easier to add some security safe-guards in the parent process right before execution of an MDA: if it is asked to execute a custom command but the configuration states there Is no .forward file allowed, then we can detect something is fishy and refuse to fork a child process for delivery.
Re: [diff] src/usr.sbin/smtpd: add a forward-file option
> On 20 Dec 2020, at 03:21, Theo de Raadt wrote: > > Todd C. Miller wrote: > >> I like this direction but I worry about breaking existing configs. >> How are we going to alert existing users that they need to update >> their configs if the behavior silently changes? > > I think the configuration is backwards. > > Every endpoint box will need these new stanza. > > Very strange. I’m really unsure about that. It does make your endpoints and mine need the option because we receive mail on machines where users have shells, but from what I observe this is not the common case. From what I observe, most people expose mail through IMAP, the forward files are not accessible to recipients. I could be wrong but I think we are biased on this.
Re: [diff] src/usr.sbin/smtpd: add a forward-file option
> On 20 Dec 2020, at 07:19, Theo de Raadt wrote: > > There are thousands of people with smtpd configurations, and sysmerge > is not going to handle this. > > We cannot expect them all to change their files. This is madness. > > Gilles, I think you should be adding an option that blocks it optionally, > and then some operators can use that. If they wish. I am surprised you > think this can be a default, when as Sebastien points out the base system > uses it today... > I know that this isn’t convenient and my first version of the diff was a “disalllow-forward-file” option but: The diff was to discuss what I think is the right way of doing it, not the one I find the most convenient. If this is not desired, I can submit a diff for the convenient way but I would have hated not showing what I think is right first. In addition, my diff is a turn on a feature explicitly whereas the “disallow-forward-file” option is a turn off an implicit behaviour, and when I see that some people don’t even know that .forward files are a thing, I feel it’s the wrong way around. People who want forward files know they exist and can ask for it, whereas people who don’t know they exist or who don’t request it will get it behind their backs. As I said to semarie@ and millert@, the default configuration could be adapted to add forward-file to the mbox action, and this diff could be adapted to not ignore .forward files but warn that they are used on a rule without the keyword to give people two releases to adapt since we can’t expect everyone to change their files but we can expect them to upgrade at least every two releases. Also, what doesn’t show on this diff is that if we rely on the implicit behaviour and a “disallow-forward-file” it kind of makes other features backwards too in terms of configuration. Assuming disallow-forward-file, then do we add an option to disallow execution of an mda or do we add an option to allow it ? Does the default behaviour of forward files is to execute custom commands or not ? If not, then how do we express it if there’s no option visible in the conf ? It makes the grammar very weird :-/
Re: [diff] src/usr.sbin/smtpd: add a forward-file option
> On 20 Dec 2020, at 07:13, Sebastien Marie wrote: > > On Sat, Dec 19, 2020 at 10:36:32PM +, gil...@poolp.org wrote: >> Hello, >> >> Whenever a rule with a local action (mbox, maildir, lmtp or mda) is matched, >> smtpd will >> attempt to search for a ~/.forward file in the recipient directory and >> process it. This >> may be convenient for some setups but it is an implicit behavior that's not >> overridable >> and not always wanted. >> >> This diff changes this behavior by requiring the admins to explicitly allow >> the forward >> files processing in the actions when desired: >> >>action "local_users" maildir forward-file >> >> >> With this diff, if forward-file is not specified, code to request parent >> process for an >> fd is bypassed and the expansion layer just pretends parent couldn't find >> one. This let >> the code fallback in an already existing code path with the proper behavior >> and is very >> uninvasive. >> > > if I could understood the direction (which is fine as it makes the > daemon less behaviour dependant on a user settings), the default seems > wrong to me (at least for now, and for OpenBSD base specifically). > > Currently, root@ mail delivery is based on /root/.forward file: > install is writing this file to redirect root@ mail to user (if user > was created at install-time). It is done this way since 2011 (see > distrib/miniroot/install.sh rev 1.218). So I assume that all installs > which were done with a user configured, since 2011, could use it. Yes, the default would need to be changed as follows: mini$ diff -uNp smtpd.conf smtpd.conf.new --- smtpd.conf Mon Dec 14 22:13:04 2020 +++ smtpd.conf.new Sun Dec 20 09:43:22 2020 @@ -11,7 +11,7 @@ listen on socket # listen on all hostname debug.poolp.org -action "local_mail" maildir alias +action "local_mail" maildir alias forward-file action "outbound" relay # Uncomment the following to accept external mail for domain "example.org" mini$ > At first step, I would keep the default smtpd.conf with "forward-file" > option set. It would make smtpd(1) to default to no "forward-file" if > not set (what your diff do), but set the default to with > "forward-file" for OpenBSD base. > > Admin could remove the option if he/she doesn't use it. Yes, I agree and I was showing the idea more than suggesting a default configuration for OpenBSD base. If the default config had the diff I showed above, which is what you suggest, then there would be no behaviour change on a default install. As for existing setups, as I answered to millert@, there could even be a two release plan so that not having the keyword would not break forward file but just warn the admin in logs that the keyword is now needed whenever a .forward file is used. This would push them into adapting their configuration file before it breaks a year from now. Since there’s currently no way of not having forward files, then this does not break setups as not doing anything keeps the old behaviour + warnings.
Re: [diff] src/usr.sbin/smtpd: add a forward-file option
> On 20 Dec 2020, at 02:09, Todd C. Miller wrote: > > I like this direction but I worry about breaking existing configs. > How are we going to alert existing users that they need to update > their configs if the behavior silently changes? > > - todd I agree and this diff was more to suggest a direction and spark discussion than a request to get this in. Today there’s no way to disable forward files and OpenBSD supports two releases. If we agreed this is the right direction then we could have a two-release plan: 1- introduce the keyword but not require it yet 2- add the keyword to the default configuration file 3- throw in a warning in logs whenever a .forward file is used with an action that doesn’t have the keyword set With this, existing setups would not break but start warning that a configuration file change is required. If the configuration change is made, the warnings stop right away. People get two releases to fix their configuration before the keyword becomes mandatory. I’ll address other concerns raised by semarie@ and deraadt@ as a reply to their mail.
Re: [diff] src/usr.sbin/smtpd: add a forward-file option
On Sat, Dec 19, 2020 at 11:19:10PM -0700, Theo de Raadt wrote: > There are thousands of people with smtpd configurations, and sysmerge > is not going to handle this. > > We cannot expect them all to change their files. This is madness. Well, it wouldn't be the first time. But I agree that such changes should be rare and have really good reason for. So yes, even if the option is desirable and being off-by-default would be a good default, the flag-day way for handling it is complex. Regarding the option itself, if I recall correctly some descriptions made by Gilles about smtpd, opening ~/.forward is one of the few tasks done by the priviligied process of smtpd. So it could make sense to avoid it if not need. Gilles, could you confirm that having an option to remove .forward capability (whatever the default value of the option is) could effectively help to reduce the attack surface of smtpd ? For example, as immediate consequence, I see no reason for smtpd priviliegied process to keep a full filesystem view: it might be possible to restricted it to few directories with unveil(2) (I assume priviliegied process is still need for bsd_auth, and bsd_auth will have some requirements). Thanks. -- Sebastien Marie
Re: [diff] src/usr.sbin/smtpd: add a forward-file option
There are thousands of people with smtpd configurations, and sysmerge is not going to handle this. We cannot expect them all to change their files. This is madness. Gilles, I think you should be adding an option that blocks it optionally, and then some operators can use that. If they wish. I am surprised you think this can be a default, when as Sebastien points out the base system uses it today... Sebastien Marie wrote: > On Sat, Dec 19, 2020 at 10:36:32PM +, gil...@poolp.org wrote: > > Hello, > > > > Whenever a rule with a local action (mbox, maildir, lmtp or mda) is > > matched, smtpd will > > attempt to search for a ~/.forward file in the recipient directory and > > process it. This > > may be convenient for some setups but it is an implicit behavior that's not > > overridable > > and not always wanted. > > > > This diff changes this behavior by requiring the admins to explicitly allow > > the forward > > files processing in the actions when desired: > > > > action "local_users" maildir forward-file > > > > > > With this diff, if forward-file is not specified, code to request parent > > process for an > > fd is bypassed and the expansion layer just pretends parent couldn't find > > one. This let > > the code fallback in an already existing code path with the proper behavior > > and is very > > uninvasive. > > > > if I could understood the direction (which is fine as it makes the > daemon less behaviour dependant on a user settings), the default seems > wrong to me (at least for now, and for OpenBSD base specifically). > > Currently, root@ mail delivery is based on /root/.forward file: > install is writing this file to redirect root@ mail to user (if user > was created at install-time). It is done this way since 2011 (see > distrib/miniroot/install.sh rev 1.218). So I assume that all installs > which were done with a user configured, since 2011, could use it. > > At first step, I would keep the default smtpd.conf with "forward-file" > option set. It would make smtpd(1) to default to no "forward-file" if > not set (what your diff do), but set the default to with > "forward-file" for OpenBSD base. > > Admin could remove the option if he/she doesn't use it. > > Thanks. > -- > Sebastien Marie >
Re: [diff] src/usr.sbin/smtpd: add a forward-file option
On Sat, Dec 19, 2020 at 10:36:32PM +, gil...@poolp.org wrote: > Hello, > > Whenever a rule with a local action (mbox, maildir, lmtp or mda) is matched, > smtpd will > attempt to search for a ~/.forward file in the recipient directory and > process it. This > may be convenient for some setups but it is an implicit behavior that's not > overridable > and not always wanted. > > This diff changes this behavior by requiring the admins to explicitly allow > the forward > files processing in the actions when desired: > > action "local_users" maildir forward-file > > > With this diff, if forward-file is not specified, code to request parent > process for an > fd is bypassed and the expansion layer just pretends parent couldn't find > one. This let > the code fallback in an already existing code path with the proper behavior > and is very > uninvasive. > if I could understood the direction (which is fine as it makes the daemon less behaviour dependant on a user settings), the default seems wrong to me (at least for now, and for OpenBSD base specifically). Currently, root@ mail delivery is based on /root/.forward file: install is writing this file to redirect root@ mail to user (if user was created at install-time). It is done this way since 2011 (see distrib/miniroot/install.sh rev 1.218). So I assume that all installs which were done with a user configured, since 2011, could use it. At first step, I would keep the default smtpd.conf with "forward-file" option set. It would make smtpd(1) to default to no "forward-file" if not set (what your diff do), but set the default to with "forward-file" for OpenBSD base. Admin could remove the option if he/she doesn't use it. Thanks. -- Sebastien Marie
[diff] src/usr.sbin/smtpd: add a forward-file option
Hello, Whenever a rule with a local action (mbox, maildir, lmtp or mda) is matched, smtpd will attempt to search for a ~/.forward file in the recipient directory and process it. This may be convenient for some setups but it is an implicit behavior that's not overridable and not always wanted. This diff changes this behavior by requiring the admins to explicitly allow the forward files processing in the actions when desired: action "local_users" maildir forward-file With this diff, if forward-file is not specified, code to request parent process for an fd is bypassed and the expansion layer just pretends parent couldn't find one. This let the code fallback in an already existing code path with the proper behavior and is very uninvasive. diff --git a/usr.sbin/smtpd/lka_session.c b/usr.sbin/smtpd/lka_session.c index ed1fd36fafd..ff328441957 100644 --- a/usr.sbin/smtpd/lka_session.c +++ b/usr.sbin/smtpd/lka_session.c @@ -434,9 +434,17 @@ lka_expand(struct lka_session *lks, struct rule *rule, struct expandnode *xn) fwreq.uid = lk.userinfo.uid; fwreq.gid = lk.userinfo.gid; - m_compose(p_parent, IMSG_LKA_OPEN_FORWARD, 0, 0, -1, - , sizeof(fwreq)); - lks->flags |= F_WAITING; + if (dsp->u.local.forward_file) { + log_debug("OPENING FORWARD FILE"); + m_compose(p_parent, IMSG_LKA_OPEN_FORWARD, 0, 0, -1, + , sizeof(fwreq)); + lks->flags |= F_WAITING; + } else { + log_debug("BYPASSING FORWARD FILE"); + fwreq.status = 1; + lks->flags |= F_WAITING; + lka_session_forward_reply(, -1); + } break; case EXPAND_FILENAME: diff --git a/usr.sbin/smtpd/parse.y b/usr.sbin/smtpd/parse.y index 9f1cb52ec98..752c3376b77 100644 --- a/usr.sbin/smtpd/parse.y +++ b/usr.sbin/smtpd/parse.y @@ -178,7 +178,7 @@ typedef struct { %token CA CERT CHAIN CHROOT CIPHERS COMMIT COMPRESSION CONNECT %token DATA DATA_LINE DHE DISCONNECT DOMAIN %token EHLO ENABLE ENCRYPTION ERROR EXPAND_ONLY -%token FCRDNS FILTER FOR FORWARD_ONLY FROM +%token FCRDNS FILTER FOR FORWARD_FILE FORWARD_ONLY FROM %token GROUP %token HELO HELO_SRC HOST HOSTNAME HOSTNAMES %token INCLUDE INET4 INET6 @@ -669,6 +669,13 @@ USER STRING { } dispatcher->u.local.mda_wrapper = $2; } +| FORWARD_FILE { + if (dispatcher->u.local.forward_file) { + yyerror("forward-file already specified for this dispatcher"); + YYERROR; + } + dispatcher->u.local.forward_file = 1; +} ; dispatcher_local_options: @@ -2646,6 +2653,7 @@ lookup(char *s) { "fcrdns", FCRDNS }, { "filter", FILTER }, { "for",FOR }, + { "forward-file", FORWARD_FILE }, { "forward-only", FORWARD_ONLY }, { "from", FROM }, { "group", GROUP }, diff --git a/usr.sbin/smtpd/smtpd.conf.5 b/usr.sbin/smtpd/smtpd.conf.5 index 36207c39a1e..fa98e13e158 100644 --- a/usr.sbin/smtpd/smtpd.conf.5 +++ b/usr.sbin/smtpd/smtpd.conf.5 @@ -173,6 +173,8 @@ Use the mapping for .Xr aliases 5 expansion. +.It Cm forward-file +Allow the use of a .forward file in user home directory . .It Xo .Cm ttl .Sm off diff --git a/usr.sbin/smtpd/smtpd.h b/usr.sbin/smtpd/smtpd.h index 529ff683f76..8225f3ff157 100644 --- a/usr.sbin/smtpd/smtpd.h +++ b/usr.sbin/smtpd/smtpd.h @@ -1159,6 +1159,7 @@ struct dispatcher_local { uint8_t expand_only; uint8_t forward_only; + uint8_t forward_file; char*mda_wrapper; char*command;
Re: [diff] src/usr.sbin/smtpd: add a forward-file option
Todd C. Miller wrote: > I like this direction but I worry about breaking existing configs. > How are we going to alert existing users that they need to update > their configs if the behavior silently changes? I think the configuration is backwards. Every endpoint box will need these new stanza. Very strange.
Re: [diff] src/usr.sbin/smtpd: add a forward-file option
I like this direction but I worry about breaking existing configs. How are we going to alert existing users that they need to update their configs if the behavior silently changes? - todd