diff --git a/CHANGELOG.md b/CHANGELOG.md index b6efbf0..cd77347 100644 --- a/CHANGELOG.md +++ b/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()` diff --git a/openarc/openarc-config.h b/openarc/openarc-config.h index dad2b41..449254a 100644 --- a/openarc/openarc-config.h +++ b/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}, diff --git a/openarc/openarc.c b/openarc/openarc.c index dd1ac42..1989ed3 100644 --- a/openarc/openarc.c +++ b/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); diff --git a/openarc/openarc.conf.5.in b/openarc/openarc.conf.5.in index e469fe5..17a2c8e 100644 --- a/openarc/openarc.conf.5.in +++ b/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. diff --git a/openarc/openarc.conf.sample b/openarc/openarc.conf.sample index 39c6eeb..3000b3e 100644 --- a/openarc/openarc.conf.sample +++ b/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) ## diff --git a/test/files/test_milter_maximumheaders.conf b/test/files/test_milter_maximumheaders.conf new file mode 100644 index 0000000..62e40f3 --- /dev/null +++ b/test/files/test_milter_maximumheaders.conf @@ -0,0 +1,3 @@ +{ + "MaximumHeaders": "1" +} diff --git a/test/files/test_milter_responsedisabled.conf b/test/files/test_milter_responsedisabled.conf new file mode 100644 index 0000000..5d11207 --- /dev/null +++ b/test/files/test_milter_responsedisabled.conf @@ -0,0 +1,4 @@ +{ + "PeerList": "peerlist", + "ResponseDisabled": "reject" +} diff --git a/test/files/test_milter_responseunwilling.conf b/test/files/test_milter_responseunwilling.conf new file mode 100644 index 0000000..2e58276 --- /dev/null +++ b/test/files/test_milter_responseunwilling.conf @@ -0,0 +1,4 @@ +{ + "MaximumHeaders": "1", + "ResponseUnwilling": "accept" +} diff --git a/test/test_milter.py b/test/test_milter.py index 5cfe066..779ce3a 100644 --- a/test/test_milter.py +++ b/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()