Re: smtpd: allow arguments on NOOP
On Fri, 23 Jun 2023 11:58:47 +0200, Omar Polo wrote: > another diff from the -portable repo: > > https://github.com/OpenSMTPD/OpenSMTPD/pull/1150 > > per rfc-5321 § 4.1.1.9 the NOOP command allows optionally one argument > that we SHOULD ignore. > > The original diff set the check function in the commands table to NULL > because NOOP is not a phase that can have filters. However, I prefer > to stay on the safe side and add a smtp_check_noop() function. > Alternatively, we could allow a NULL check function in > smtp_session_imsg(). > > the rfc specifies only one optional string, while here for semplicity > it's relaxed to allow anything. This is a case where being liberal in what you accept seems fine. OK millert@ - todd
Re: smtpd: allow arguments on NOOP
June 23, 2023 11:58 AM, "Omar Polo" wrote:
> another diff from the -portable repo:
>
> https://github.com/OpenSMTPD/OpenSMTPD/pull/1150
>
> per rfc-5321 § 4.1.1.9 the NOOP command allows optionally one argument
> that we SHOULD ignore.
>
> The original diff set the check function in the commands table to NULL
> because NOOP is not a phase that can have filters. However, I prefer
> to stay on the safe side and add a smtp_check_noop() function.
> Alternatively, we could allow a NULL check function in
> smtp_session_imsg().
>
> the rfc specifies only one optional string, while here for semplicity
> it's relaxed to allow anything.
>
> diff /usr/src
> commit - 8def1c1c2777f0b5175283f8116e1eaab1f1962a
> path + /usr/src
> blob - 1686f03e96deeb5e6ea8b065456e04c27c752c8c
> file + usr.sbin/smtpd/smtp_session.c
> --- usr.sbin/smtpd/smtp_session.c
> +++ usr.sbin/smtpd/smtp_session.c
> @@ -212,6 +212,7 @@ static int smtp_check_noparam(struct smtp_session *,
> static int smtp_check_mail_from(struct smtp_session *, const char *);
> static int smtp_check_rcpt_to(struct smtp_session *, const char *);
> static int smtp_check_data(struct smtp_session *, const char *);
> +static int smtp_check_noop(struct smtp_session *, const char *);
> static int smtp_check_noparam(struct smtp_session *, const char *);
>
> static void smtp_filter_phase(enum filter_phase, struct smtp_session *, const
> char *);
> @@ -276,7 +277,7 @@ static struct {
> { CMD_DATA, FILTER_DATA, "DATA", smtp_check_data, smtp_proceed_data },
> { CMD_RSET, FILTER_RSET, "RSET", smtp_check_rset, smtp_proceed_rset },
> { CMD_QUIT, FILTER_QUIT, "QUIT", smtp_check_noparam, smtp_proceed_quit },
> - { CMD_NOOP, FILTER_NOOP, "NOOP", smtp_check_noparam, smtp_proceed_noop },
> + { CMD_NOOP, FILTER_NOOP, "NOOP", smtp_check_noop, smtp_proceed_noop },
> { CMD_HELP, FILTER_HELP, "HELP", smtp_check_noparam, smtp_proceed_help },
> { CMD_WIZ, FILTER_WIZ, "WIZ", smtp_check_noparam, smtp_proceed_wiz },
> { CMD_COMMIT, FILTER_COMMIT, ".", smtp_check_noparam, smtp_proceed_commit },
> @@ -1343,8 +1344,8 @@ smtp_command(struct smtp_session *s, char *line)
> break;
>
> case CMD_NOOP:
> - if (!smtp_check_noparam(s, args))
> - break;
> + if (!smtp_check_noop(s, args))
> + break;
> smtp_filter_phase(FILTER_NOOP, s, NULL);
> break;
>
> @@ -1631,6 +1632,12 @@ smtp_check_noparam(struct smtp_session *s, const char
> }
>
> static int
> +smtp_check_noop(struct smtp_session *s, const char *args)
> +{
> + return 1;
> +}
> +
> +static int
> smtp_check_noparam(struct smtp_session *s, const char *args)
> {
> if (args != NULL) {
This reads fine and you did well adding an smtp_check_noop() because it leaves
room
for hooking this command in filters which is something OpenSMTPD could benefit
from
as it would allow a filter to detect people doing NOOP loops and kick them.
Just my 2cents
smtpd: allow arguments on NOOP
another diff from the -portable repo:
https://github.com/OpenSMTPD/OpenSMTPD/pull/1150
per rfc-5321 § 4.1.1.9 the NOOP command allows optionally one argument
that we SHOULD ignore.
The original diff set the check function in the commands table to NULL
because NOOP is not a phase that can have filters. However, I prefer
to stay on the safe side and add a smtp_check_noop() function.
Alternatively, we could allow a NULL check function in
smtp_session_imsg().
the rfc specifies only one optional string, while here for semplicity
it's relaxed to allow anything.
diff /usr/src
commit - 8def1c1c2777f0b5175283f8116e1eaab1f1962a
path + /usr/src
blob - 1686f03e96deeb5e6ea8b065456e04c27c752c8c
file + usr.sbin/smtpd/smtp_session.c
--- usr.sbin/smtpd/smtp_session.c
+++ usr.sbin/smtpd/smtp_session.c
@@ -212,6 +212,7 @@ static int smtp_check_noparam(struct smtp_session *,
static int smtp_check_mail_from(struct smtp_session *, const char *);
static int smtp_check_rcpt_to(struct smtp_session *, const char *);
static int smtp_check_data(struct smtp_session *, const char *);
+static int smtp_check_noop(struct smtp_session *, const char *);
static int smtp_check_noparam(struct smtp_session *, const char *);
static void smtp_filter_phase(enum filter_phase, struct smtp_session *, const
char *);
@@ -276,7 +277,7 @@ static struct {
{ CMD_DATA, FILTER_DATA,"DATA",
smtp_check_data,smtp_proceed_data },
{ CMD_RSET, FILTER_RSET,"RSET",
smtp_check_rset,smtp_proceed_rset },
{ CMD_QUIT, FILTER_QUIT,"QUIT",
smtp_check_noparam, smtp_proceed_quit },
- { CMD_NOOP, FILTER_NOOP,"NOOP",
smtp_check_noparam, smtp_proceed_noop },
+ { CMD_NOOP, FILTER_NOOP,"NOOP",
smtp_check_noop,smtp_proceed_noop },
{ CMD_HELP, FILTER_HELP,"HELP",
smtp_check_noparam, smtp_proceed_help },
{ CMD_WIZ, FILTER_WIZ, "WIZ",
smtp_check_noparam, smtp_proceed_wiz },
{ CMD_COMMIT, FILTER_COMMIT, ".",
smtp_check_noparam, smtp_proceed_commit },
@@ -1343,8 +1344,8 @@ smtp_command(struct smtp_session *s, char *line)
break;
case CMD_NOOP:
- if (!smtp_check_noparam(s, args))
- break;
+ if (!smtp_check_noop(s, args))
+ break;
smtp_filter_phase(FILTER_NOOP, s, NULL);
break;
@@ -1631,6 +1632,12 @@ smtp_check_noparam(struct smtp_session *s, const char
}
static int
+smtp_check_noop(struct smtp_session *s, const char *args)
+{
+ return 1;
+}
+
+static int
smtp_check_noparam(struct smtp_session *s, const char *args)
{
if (args != NULL) {
