basic SMTP recipient checking #233

schmonz · 2022-02-05T12:26:12Z

Give mail admins some basic SMTP recipient-checking tools:

An API for external programs to reject recipients (@jaysoffian's RCPTCHECK patch)
A program that calls the API and provides the API, giving a way to list a sequence of recipient-checkers in control/rcptchecks (my qmail-rcptcheck program)
Two such recipient-checkers: qmail-rcptcheck-realrcptto derived from Paul Jarc's qmail-realrcptto patch, and qmail-rcptcheck-badrcptto derived from Ward Vandewege's badrcptto patch (was GPL'd, now CC0 -- thanks @cure!)

Notably not included:

Any other recipient-checkers, such as rejectutils' qmail-rcptcheck-qregex (never needed it myself) or something for vpopmail users
Enabling any of this by default (we probably should, in some future release)
Adapting RCPTCHECK to qmail-qmtpd (probably straightforward, just needs someone who cares)
Automated tests (I've run the code in production for years; still, sorry about this, and happy to add tests later)
All the other programmability of qmail-spp (but qmail-rcptcheck will happily run in that environment and recipient-checkers won't need to change at all)

Original: https://www.soffian.org/downloads/qmail/qmail-smtpd.patch This patch modifies qmail-smtpd to check for an environment variable, $RCPTCHECK. If it is set, for each 'rcpt to:' that qmail-smtpd receives, it will fork/exec $RCPTCHECK. $RCPTCHECK is run in the same environment as qmail-smtpd (Note that qmail-smtpd changes its CWD to qmail-home [typically /var/qmail] at startup). Additionally $SENDER is set to the envelope from (mail from:) and $RECIPIENT is set to the envelope recipient (for the current rcpt to:). Based on the return code (exit value) of $RCPTCHECK, the rcpt to: address will either be accepted or rejected as follows: - 100: recipient is rejected with "553 sorry, no mailbox here by that name. (#5.1.1)" - 111: connection is dropped with a temporary error "421 unable to verify recipient (#4.3.0)" - 120: connection is dropped with a temporary error "421 unable to execute recipient check (#4.3.0)" - All others: recipient is accepted. 120 is used internally (by the patch) if $RCPTCHECK cannot be executed. Useful for validating recipient to addresses with an arbitrary program (such as fastforward in '-n' mode).

Original: qmail-1.03-realrcptto-2006.12.10.patch The qmail-realrcptto patch copies logic from qmail-send, qmail-lspawn, qmail-getpw, and qmail-local into qmail-smtpd and qmail-qmtpd, so that if a local delivery (i.e., one for a domain in /var/qmail/control/locals or virtualdomains) would eventually bounce due to a missing .qmail file, then that recipient address is rejected during the SMTP or QMTP protocol conversation. There are other qmail patches around that do similar jobs (badrcptto, etc.); the focus of this patch is to get this functionality with no additional administrative effort, rather than qmail-smtpd's running speed. You just set up your .qmail files, as you would have to do anyway, and the rest is automatic, though slower than a CDB lookup. Addresses which use the default delivery instructions are never rejected by this patch, because they would never be bounced due to the lack of a .qmail file. This patch is less effective when there are .qmail-default files, or when qmaild does not have sufficient filesystem permissions to stat() users' .qmail files, since in those cases, qmail-smtpd cannot know that the delivery would later bounce due to the lack of a .qmail file. For example, I'm told that this patch is not useful for virtual domains managed with vpopmail, since an applicable .qmail-default file always exists. (I'm not familiar with vpopmail myself.) The patch may still be useful for other local or virtual domains on the same server, if they are not managed by vpopmail.

- Remove realrcptto references from qmail-qmtpd(8) and qmail-smtpd(8) - Add qmail-rcptcheck-realrcptto(8) program and manual page - Adjust realrcptto.c messages for the new context Note that this leaves qmail-qmtpd(8) without realrcptto functionality. Porting the RCPTCHECK patch would probably do the trick.

qmail-rcptcheck runs an administrator-defined sequence of programs to check SMTP envelope senders and recipients. Checks must adhere to the RCPTCHECK interface. If any check rejects, the message is rejected. qmail-rcptcheck is most commonly invoked from qmail-smtpd via RCPTCHECK, but also runs under qmail-spp (not documented here), so if and when we switch to that, recipient-checking programs won't need to be changed.

Original: badrcptto-v1.02.netqmail.patch The badrcptto patch SMTP-rejects email for specific recipients in an email domain you otherwise accept mail for. List these "bad recipients" in control/badrcptto, one per line. Rejection occurs during the smtp envelope (rcpt) phase, before the email headers and body can be transmitted. Alex Kramarov added code to skip badrcptto checks if RELAYCLIENT is set, so that one could prevent remote users from sending email to certain local accounts while permitting local users to do so. This has been merged. Andrew McCarthy added logging for badrcptto hits. This has also been merged.

lgtm-com · 2022-02-05T12:29:22Z

This pull request introduces 1 alert when merging 44a63b4 into a943036 - view on LGTM.com

new alerts:

1 for Comparison result is always the same

DerDakon · 2022-02-05T15:10:21Z

realrcptto.c

+extern void die_nomem();
+extern void die_control();
+extern void die_cdb();
+extern void die_sys();


Why aren't they in realrcptto.h? They also need _noreturn annotations.

DerDakon · 2022-02-05T15:10:55Z

qmail-rcptcheck-realrcptto.c

+void accept_recipient() { _exit(  0); }
+void reject_recipient() { _exit(100); }
+void unable_to_verify() { _exit(111); }


They should be annotated __noreturn.

DerDakon · 2022-02-05T15:11:25Z

qmail-rcptcheck.c

+  char *(*get_sender)();
+  char *(*get_recipient)();


The return type should be const to make it clear that the caller may not modify the value.

DerDakon · 2022-02-05T15:13:01Z

qmail-rcptcheck.c

+}
+
+static int looks_like_spp() {
+  char *x = env_get("SMTPRCPTTO");


Should be const as well.

DerDakon · 2022-02-05T15:15:32Z

qmail-rcptcheck.c

+
+  switch(pid = fork()) {
+    case -1:
+      rcptcheck.unable_to_execute();


The callbacks are expected to be noreturn here, otherwise one would fallthrough. Without annotations this will lead to compiler warnings.

I wonder if it isn't less error prone to expect the callbacks to return and instead exit in the caller. That way an unexpected return from such a callback will not screw up the program logic.

- Remove references from qmail-smtpd(8) - Extract logic to badrcptto.c - Add qmail-rcptcheck-badrcptto(8) program and manual page

lgtm-com · 2022-02-05T20:54:43Z

This pull request introduces 1 alert when merging 1ef1c15 into a943036 - view on LGTM.com

new alerts:

1 for Comparison result is always the same

josuah

That is a great addition, enters the game of providing an API, and a simple and sane one, for making patches unneeded. Kudos for bringing that up.

Letting you adjust the __noreturn and char -> char const as proposed by @DerDakon.

I will open an issue for the other points I discussed.

Thank you!

josuah · 2022-05-05T16:52:26Z

badrcptto.c

+static void _badrcptto_log_rejection(char *recipient)
+{
+  char smtpdpid[32];
+  char *remoteip = env_get("TCPREMOTEIP");


What is best between using env_get or getenv for new code?

The getenv() function shall search the environment of the calling process for the environment variable name if it exists and return a pointer to the value of the environment variable. If the specified environment variable cannot be found, a null pointer shall be returned.

env_get returns the value of the first variable whose name is name, or 0 if there is no such variable.

Eventually, env_get can be #defined as getenv() in env.h.

josuah · 2022-05-05T16:55:45Z

qmail-rcptcheck.c

+    case -1:
+      rcptcheck.unable_to_execute();
+    case 0:
+      execv(*rcptcheck_program,rcptcheck_program);


I second the use of execv instead of execvp as it is a path relative to control.

josuah · 2022-05-05T16:59:16Z

qmail-rcptcheck.c

+  rcptcheck.get_sender        = rcptcheck_get_sender;
+  rcptcheck.get_recipient     = rcptcheck_get_recipient;
+  rcptcheck.accept_recipient  = rcptcheck_accept_recipient;
+  rcptcheck.reject_recipient  = rcptcheck_reject_recipient;
+  rcptcheck.unable_to_verify  = rcptcheck_unable_to_verify;
+  rcptcheck.unable_to_execute = rcptcheck_unable_to_execute;


The overhead of assigning function pointers might be extremely small, great way to do it!

josuah · 2022-05-05T17:02:24Z

qmail-rcptcheck.c

+
+  if (looks_like_spp()) run_rcptchecks_under_spp();
+  else run_rcptchecks_under_rcptcheck();
+


At this point, we should not have any empty function pointer.
Checking... Each run_rcptchecks_under_* sets 6 function pointers, the struct has 6 function pointers, run in an if-else... Function pointers not called before... Looks all good.

josuah · 2022-05-05T17:07:05Z

badrcptto.c

+  return ((j < address.len) && (constmap(&map,address.s + j,address.len - j - 1)));
+}
+
+static int _badrcptto_reject_string(char *string)


A char * string passed without a length looks suspicious: would char *string be written to without a length? Of course not, it is only read.

Making it char const * saves the time it takes to check it, and make it more obvious that the function returns 0 or 1 depending on string.

josuah · 2022-05-05T17:07:39Z

badrcptto.c

+  char smtpdpid[32];
+  char *remoteip = env_get("TCPREMOTEIP");
+  if (!remoteip) remoteip = "unknown";
+  str_copy(smtpdpid + fmt_ulong(smtpdpid,getppid())," ");


Same question with str_copy vs strcpy.

josuah · 2022-05-05T17:15:34Z

badrcptto.c

+  stralloc addr = {0};
+  stralloc brt = {0};


I think I missed when those two are set stralloc_free(). Or does it not matter since it is a short-lived program?

I realize that there is not even any stralloc_free() in DJB code. It could be introduced later.

The benefits of using stralloc are as follows: [...]

If every heap-allocated object is represented by a stralloc, then it is very easy to identify what pointer is in the heap. When you stop using a pointer p, should you free it ? Sometimes it's not easy to find out. When you stop using a stralloc sa, you know you must call stralloc_free(&sa). Store your strong references as strallocs and weak references as simple pointers, and never free simple pointers. This policy allows me to boast that no skarnet.org software has ever leaked memory. -- https://skarnet.org/software/skalibs/libstddjb/stralloc.html

Other wraparound of DJB code do include stralloc_free, like fefe's https://www.fefe.de/libowfat/:

#include <stdlib.h> #include "stralloc.h" void stralloc_free(stralloc *sa) { if (sa->s) free(sa->s); sa->s=0; sa->a=sa->len=0; }

Jay Soffian and others added 8 commits February 2, 2022 15:27

Document RCPTCHECK in qmail-smtpd.8.

b6bb341

Rebase realrcptto. Fix build and clean.

f15549b

Add CHANGES entry.

1422071

schmonz requested review from DerDakon, josuah, leahneukirchen and mbhangui as code owners February 5, 2022 12:26

schmonz linked an issue Feb 5, 2022 that may be closed by this pull request

Configurable validation of SMTP recipients #49

Open

schmonz added this to the 1.09 milestone Feb 5, 2022

schmonz added the enhancement New feature or request label Feb 5, 2022

schmonz requested review from jlmuir and xenotrope February 5, 2022 12:27

DerDakon requested changes Feb 5, 2022

View reviewed changes

schmonz added 2 commits February 5, 2022 21:49

Convert badrcptto to a RCPTCHECK-style program.

1189eda

- Remove references from qmail-smtpd(8) - Extract logic to badrcptto.c - Add qmail-rcptcheck-badrcptto(8) program and manual page

No need to generate qmail-rcptcheck-realrcptto(8).

1ef1c15

josuah requested changes May 5, 2022

View reviewed changes

josuah mentioned this pull request May 5, 2022

No stralloc_free and not freeing memory #236

Open

schmonz modified the milestones: 1.09, 1.90 Sep 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

basic SMTP recipient checking #233

basic SMTP recipient checking #233

schmonz commented Feb 5, 2022 •

edited

lgtm-com bot commented Feb 5, 2022

DerDakon Feb 5, 2022

DerDakon Feb 5, 2022

DerDakon Feb 5, 2022

DerDakon Feb 5, 2022

DerDakon Feb 5, 2022

lgtm-com bot commented Feb 5, 2022

josuah left a comment

josuah May 5, 2022

josuah May 5, 2022

josuah May 5, 2022

josuah May 5, 2022

josuah May 5, 2022

josuah May 5, 2022

josuah May 5, 2022

josuah May 5, 2022

josuah May 5, 2022


		if (looks_like_spp()) run_rcptchecks_under_spp();
		else run_rcptchecks_under_rcptcheck();

basic SMTP recipient checking #233

Are you sure you want to change the base?

basic SMTP recipient checking #233

Conversation

schmonz commented Feb 5, 2022 • edited

lgtm-com bot commented Feb 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented Feb 5, 2022

josuah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schmonz commented Feb 5, 2022 •

edited