milter: make responses configurable

CHANGELOG.md

@@ -11,6 +11,8 @@ All notable changes to this project will be documented in this file.
 - milter - `AuthResIP` configuration option.
 - milter - `RequireSafeKeys` configuration option.
 - milter - `MinimumKeySizeRSA` configuration option.
+- milter - `ResponseDisabled`, `ResponseUnable`, and `ResponseUnwilling`
+  configuration options.
 
 ### Changed
 - Custom OpenSSL locations must be configured using `OPENSSL_CFLAGS`
@@ -37,6 +39,8 @@ All notable changes to this project will be documented in this file.
   `header.oldest-pass` when appropriate.
 - milter - An `ar-test` program for seeing how `Authentication-Results`
   headers are parsed is built without making you jump through weird hoops.
+- milter - The default behaviour for messages that fail basic validity checks
+  (malformed headers, too many headers) is to reject them.
 
 ### Removed
 - libopenarc - `arc_mail_parse()`

openarc/openarc-config.h

@@ -39,6 +39,9 @@ struct configdef arcf_config[] = {
     {"PermitAuthenticationOverrides", CONFIG_TYPE_BOOLEAN, false},
     {"PidFile",                       CONFIG_TYPE_STRING,  false},
     {"RequireSafeKeys",               CONFIG_TYPE_BOOLEAN, false},
+    {"ResponseDisabled",              CONFIG_TYPE_STRING,  false},
+    {"ResponseUnable",                CONFIG_TYPE_STRING,  false},
+    {"ResponseUnwilling",             CONFIG_TYPE_STRING,  false},
     {"SealHeaderChecks",              CONFIG_TYPE_STRING,  false},
     {"Selector",                      CONFIG_TYPE_STRING,  false},
     {"SignatureAlgorithm",            CONFIG_TYPE_STRING,  false},

openarc/openarc.c

@@ -135,6 +135,9 @@ struct arcf_config
     size_t          conf_keylen;            /* key length */
     int             conf_maxhdrsz;          /* max. header size */
     int             conf_minkeysz;          /* min. key size */
+    int             conf_ret_disabled;      /* configured not to process */
+    int             conf_ret_unable;        /* internal error */
+    int             conf_ret_unwilling;     /* badly formed message */
     struct config  *conf_data;              /* configuration data */
     ARC_LIB        *conf_libopenarc;        /* shared library instance */
     struct conflist conf_peers;             /* peers hosts */
@@ -221,6 +224,14 @@ struct nametable arcf_chainstates[] = {
     {NULL,   -1            }
 };
 
+struct nametable arcf_responses[] = {
+    {"accept",   SMFIS_ACCEPT  },
+    {"discard",  SMFIS_DISCARD },
+    {"reject",   SMFIS_REJECT  },
+    {"tempfail", SMFIS_TEMPFAIL},
+    {NULL,       -1            }
+};
+
 /* PROTOTYPES */
 sfsistat      mlfi_abort(SMFICTX *);
 sfsistat      mlfi_close(SMFICTX *);
@@ -1156,6 +1167,10 @@ arcf_config_new(void)
     new->conf_safekeys = true;
     new->conf_authresip = true;
 
+    new->conf_ret_disabled = SMFIS_ACCEPT;
+    new->conf_ret_unable = SMFIS_TEMPFAIL;
+    new->conf_ret_unwilling = SMFIS_REJECT;
+
     LIST_INIT(&new->conf_peers);
     LIST_INIT(&new->conf_internal);
 
@@ -1543,6 +1558,52 @@ arcf_config_load(struct config      *data,
             conf->conf_fixedtime = strtoul(str, &end, 10);
         }
 
+        str = NULL;
+        config_get(data, "ResponseDisabled", &str, sizeof str);
+        if (str)
+        {
+            int resp = arc_name_to_code(arcf_responses, str);
+            if (resp == -1)
+            {
+                snprintf(err, errlen, "%s: invalid response value", str);
+            }
+            else
+            {
+                conf->conf_ret_disabled = arc_name_to_code(arcf_responses, str);
+            }
+        }
+
+        str = NULL;
+        config_get(data, "ResponseUnable", &str, sizeof str);
+        if (str)
+        {
+            int resp = arc_name_to_code(arcf_responses, str);
+            if (resp == -1)
+            {
+                snprintf(err, errlen, "%s: invalid response value", str);
+            }
+            else
+            {
+                conf->conf_ret_unable = arc_name_to_code(arcf_responses, str);
+            }
+        }
+
+        str = NULL;
+        config_get(data, "ResponseUnwilling", &str, sizeof str);
+        if (str)
+        {
+            int resp = arc_name_to_code(arcf_responses, str);
+            if (resp == -1)
+            {
+                snprintf(err, errlen, "%s: invalid response value", str);
+            }
+            else
+            {
+                conf->conf_ret_unwilling = arc_name_to_code(arcf_responses,
+                                                            str);
+            }
+        }
+
         (void) config_get(data, "TestKeys", &conf->conf_testkeys,
                           sizeof conf->conf_testkeys);
 
@@ -2785,10 +2846,10 @@ mlfi_connect(SMFICTX *ctx, char *host, _SOCK_ADDR *ip)
                 syslog(LOG_ERR, "%s malloc(): %s", host, strerror(errno));
             }
 
+            int retval = curconf->conf_ret_unable;
             pthread_mutex_unlock(&conf_lock);
 
-            /* XXX -- result should be selectable */
-            return SMFIS_TEMPFAIL;
+            return retval;
         }
 
         pthread_mutex_lock(&conf_lock);
@@ -2836,9 +2897,11 @@ mlfi_connect(SMFICTX *ctx, char *host, _SOCK_ADDR *ip)
     {
         if (curconf->conf_dolog)
         {
-            syslog(LOG_INFO, "ignoring connection from %s", host);
+            syslog(
+                LOG_INFO, "peer connection from %s, returning %s", host,
+                arc_code_to_name(arcf_responses, curconf->conf_ret_disabled));
         }
-        return SMFIS_ACCEPT;
+        return curconf->conf_ret_disabled;
     }
 
     /* infer operating mode if not explicitly set */
@@ -2991,15 +3054,13 @@ mlfi_header(SMFICTX *ctx, char *headerf, char *headerv)
         afc->mctx_hdrbytes + strlen(headerf) + strlen(headerv) + 2 >
             conf->conf_maxhdrsz)
     {
-        /* FIXME: this should be configurable, and it might be better to
-         * default to rejecting the message.
-         */
         if (conf->conf_dolog)
         {
-            syslog(LOG_NOTICE, "too much header data; accepting");
+            syslog(LOG_NOTICE, "too much header data, returning %s",
+                   arc_code_to_name(arcf_responses, conf->conf_ret_unwilling));
         }
 
-        return SMFIS_ACCEPT;
+        return conf->conf_ret_unwilling;
     }
 
     /*
@@ -3026,7 +3087,7 @@ mlfi_header(SMFICTX *ctx, char *headerf, char *headerv)
         }
 
         arcf_cleanup(ctx);
-        return SMFIS_TEMPFAIL;
+        return conf->conf_ret_unable;
     }
 
     newhdr->hdr_hdr = ARC_STRDUP(headerf);
@@ -3046,7 +3107,7 @@ mlfi_header(SMFICTX *ctx, char *headerf, char *headerv)
 
             arcf_cleanup(ctx);
 
-            return SMFIS_TEMPFAIL;
+            return conf->conf_ret_unable;
         }
     }
     else
@@ -3111,7 +3172,7 @@ mlfi_header(SMFICTX *ctx, char *headerf, char *headerv)
         ARC_FREE(newhdr->hdr_val);
         ARC_FREE(newhdr);
         arcf_cleanup(ctx);
-        return SMFIS_TEMPFAIL;
+        return conf->conf_ret_unable;
     }
 
     afc->mctx_hdrbytes += strlen(newhdr->hdr_hdr) + 1;
@@ -3359,7 +3420,7 @@ mlfi_eoh(SMFICTX *ctx)
                        afc->mctx_jobid);
             }
 
-            return SMFIS_ACCEPT;
+            return conf->conf_ret_disabled;
         }
     }
 #endif /* USE_JANSSON */
@@ -3381,7 +3442,7 @@ mlfi_eoh(SMFICTX *ctx)
                    afc->mctx_jobid, err);
         }
 
-        return SMFIS_TEMPFAIL;
+        return conf->conf_ret_unable;
     }
 
     for (hdr = afc->mctx_hqhead; hdr != NULL; hdr = hdr->hdr_next)
@@ -3397,7 +3458,7 @@ mlfi_eoh(SMFICTX *ctx)
                            afc->mctx_jobid);
                 }
 
-                return SMFIS_TEMPFAIL;
+                return conf->conf_ret_unable;
             }
         }
         else
@@ -3438,7 +3499,11 @@ mlfi_eoh(SMFICTX *ctx)
                        afc->mctx_jobid, hdr->hdr_hdr);
             }
 
-            return SMFIS_TEMPFAIL;
+            if (status == ARC_STAT_SYNTAX)
+            {
+                return conf->conf_ret_unwilling;
+            }
+            return conf->conf_ret_unable;
         }
     }
 
@@ -3512,7 +3577,7 @@ mlfi_body(SMFICTX *ctx, unsigned char *bodyp, size_t bodylen)
                        afc->mctx_jobid);
             }
 
-            return SMFIS_TEMPFAIL;
+            return conf->conf_ret_unable;
         }
     }
 
@@ -3657,7 +3722,7 @@ mlfi_eom(SMFICTX *ctx)
                 syslog(LOG_ERR, "arc_dstring_new() failed");
             }
 
-            return SMFIS_TEMPFAIL;
+            return conf->conf_ret_unable;
         }
     }
 
@@ -3693,7 +3758,7 @@ mlfi_eom(SMFICTX *ctx)
                    afc->mctx_jobid);
         }
 
-        return SMFIS_TEMPFAIL;
+        return conf->conf_ret_unable;
     }
 
     if (BITSET(ARC_MODE_SIGN, cc->cctx_mode))
@@ -3806,7 +3871,7 @@ mlfi_eom(SMFICTX *ctx)
                        afc->mctx_jobid);
             }
 
-            return SMFIS_TEMPFAIL;
+            return conf->conf_ret_unable;
         }
 
         for (sealhdr = seal; sealhdr != NULL; sealhdr = arc_hdr_next(sealhdr))
@@ -3858,7 +3923,7 @@ mlfi_eom(SMFICTX *ctx)
                        afc->mctx_jobid, "");
             }
 
-            return SMFIS_TEMPFAIL;
+            return conf->conf_ret_unable;
         }
 
         arc_dstring_blank(afc->mctx_tmpstr);

openarc/openarc.conf.5.in

@@ -248,6 +248,34 @@ containing the process ID.
 Controls whether the filter requires that private keys have safe file
 permissions.
 
+.It Cm ResponseDisabled Pq string
+.Brq Cm accept | discard | reject | tempfail
+
+Response to send after determining that this message is one that
+the filter is configured not to process.
+See
+.Cm SealHeaderChecks
+and
+.Cm PeerList .
+The default is
+.Cm accept .
+
+.It Cm ResponseUnable Pq string
+.Brq Cm accept | discard | reject | tempfail
+
+Response to send after an internal error occurs that makes it
+impossible to finish processing the message.
+The default is
+.Cm tempfail .
+
+.It Cm ResponseUnwilling Pq string
+.Brq Cm accept | discard | reject | tempfail
+
+Response to send after a message fails basic validity checks, such as
+.Cm MaximumHeaders .
+The default is
+.Cm reject .
+
 .It Cm SealHeaderChecks Pq string
 Identifies a file containing header checks that should be done to determine
 whether to seal a message.
@@ -268,7 +296,7 @@ Use
 .Ql openarc \-V
 to see the list of supported algorithms.
 The default is
-.Cm rsa-sha256.
+.Cm rsa-sha256 .
 Other values are not useful if you are intending to interoperate with other
 implementers of the ARC protocol.
 

openarc/openarc.conf.sample

@@ -216,6 +216,30 @@ KeyFile			/var/db/dkim/example.private
 
 # RequireSafeKeys yes
 
+##  ResponseDisabled { accept | discard | reject | tempfail }
+##      default "accept"
+##
+##  Response to send after determining that this message is one that
+##  the filter is configured not to process.
+
+# ResponseDisabled accept
+
+##  ResponseUnable { accept | discard | reject | tempfail }
+##      default "tempfail"
+##
+##  Response to send after an internal error occurs that makes it impossible
+##  to finish processing the message.
+
+# ResponseUnable tempfail
+
+##  ResponseUnwilling { accept | discard | reject | tempfail }
+##      default "reject"
+##
+##  Response to send after a message fails basic validity checks, such as
+##  MaximumHeaders.
+
+# ResponseUnwilling reject
+
 ##  SealHeaderChecks filename
 ##  	default (none)
 ##

test/files/test_milter_maximumheaders.conf

@@ -0,0 +1,3 @@
+{
+  "MaximumHeaders": "1"
+}

test/files/test_milter_responsedisabled.conf

@@ -0,0 +1,4 @@
+{
+  "PeerList": "peerlist",
+  "ResponseDisabled": "reject"
+}

test/files/test_milter_responseunwilling.conf

@@ -0,0 +1,4 @@
+{
+  "MaximumHeaders": "1",
+  "ResponseUnwilling": "accept"
+}

test/test_milter.py

@@ -564,6 +564,12 @@ def test_milter_finalreceiver(run_miltertest):
     assert res['headers'][0][1] == ' example.com; arc=pass header.oldest-pass=0 smtp.remote-ip=127.0.0.1 arc.chain="example.com:example.com"'
 
 
+def test_milter_maximumheaders(run_miltertest):
+    """Oversized headers result in message rejection"""
+    with pytest.raises(miltertest.MilterError, match="Unexpected reply to L: \\('r'"):
+        run_miltertest()
+
+
 def test_milter_minimum_key_bits(run_miltertest):
     """A 2048-bit key passes when that is the minimum"""
     res = run_miltertest()
@@ -584,6 +590,18 @@ def test_milter_peerlist(run_miltertest):
         run_miltertest()
 
 
+def test_milter_responsedisabled(run_miltertest):
+    """Configured to reject messages from peers"""
+    with pytest.raises(miltertest.MilterError, match='unexpected response: r'):
+        run_miltertest()
+
+
+def test_milter_responseunwilling(run_miltertest):
+    """Configured to accept messages with too many headers"""
+    with pytest.raises(miltertest.MilterError, match="Unexpected reply to L: \\('a'"):
+        run_miltertest()
+
+
 def test_milter_softwareheader(run_miltertest):
     """Advertise software name, version"""
     res = run_miltertest()