Fully parse Received-SPF, and only use it if it includes the envelope

sender against which the result was calculated.

RELEASE_NOTES

@@ -4,28 +4,30 @@ This listing shows the versions of the OpenDMARC package, the date of
 release, and a summary of the changes in that release.
 
 1.4.1		2021/03/??
-	Addition of defines for MUSL C Library. (#129/#133)
-		Patches by Marco Rebhan ([email protected]).
+	Addition of defines for MUSL C Library. (#129/#133).  Patches by
+		Marco Rebhan.
 	Fixes to MySQL Schema (#98/#99).  Patch by Bond Keevil.
 	LIBSPF2 calls would not compile on OpenBSD due to OpenBSD not 
 		having the ns_type definition in arpa/resolv.h.
 		Added detection to configure script.  (#134)
-	Reworked hcreate_r functions to hcreate to compile natively on
-		OpenBSD, MacOS. (Part of #94)  Reported by Rupert Gallagher.
+	Reworked hcreate_r calls to use hcreate, to compile natively on
+		OpenBSD and MacOS. (Part of #94)  Reported by Rupert Gallagher.
 	Add compatibility with AutoConf 2.70. (#95)  
-	LIBOPENDMARC: Fix bug #50: Ignore all RRTYPEs other than TXT.
-		Problem reported by Jan Bouwhuis.
-	LIBOPENDMARC: Fix bug #89: Repair absurd RRTYPE test in SPF code.
-	LIBOPENDMARC: Fix bug #104: Fix bogus header field parsing code.
 	Documentation updates about SourceForge being deprecated.  (#101)
+	Only accept results from Received-SPF fields that indicate clearly
+		which identifier was being evaluated, since DMARC specifically
+		only wants results based on MAIL FROM.
         Many build-time fixes (#100, #91, #90, #86, #85, #84, #83, #82, #81)
 		Patches provided by Rupert Gallagher ([email protected])
 	Added config option HoldQuarantinedMessages (default false), which
 		controls if messages with p=quarantine will be passed on to 
 		the mail stream (if False) or placed in the MTA's "hold" 
-		queue. (if True).  Issue #105.  Patch by Marcos Moraes, on
+		queue (if True).  Issue #105.  Patch by Marcos Moraes, on
 		the OpenDMARC mailing list.
-
+	LIBOPENDMARC: Fix bug #50: Ignore all RRTYPEs other than TXT.
+		Problem reported by Jan Bouwhuis.
+	LIBOPENDMARC: Fix bug #89: Repair absurd RRTYPE test in SPF code.
+	LIBOPENDMARC: Fix bug #104: Fix bogus header field parsing code.
 
 1.4.0		2021/01/28
 	Add ARC support.  Extensive work contributed by ValiMail, with patches

opendmarc/opendmarc.c

@@ -422,99 +422,147 @@ dmarcf_getsymval(SMFICTX *ctx, char *sym)
 **
 **  Parameters:
 **  	str -- the value of the Received-SPF field to analyze
+**  	envfrom -- envelope sender against which to test
 **
 **  Return value:
 **  	A ARES_RESULT_* constant.
+**
+**  Notes:
+**  	We will not accept a result delivered via a discovered Received-SPF
+**  	header field unless (a) it includes the "identity" key and its
+**  	value is "mailfrom", AND (b) it includes the "envelope-from" key and
+**  	its value matches the envelope sender we got via milter.  If either
+**  	of those tests fails, a "pass" or a "fail" is interpreted as "neutral".
+**  	This is necessary to be compliant with RFC 7489 Section 4.1,
+**  	which says the SPF evaluation of MAIL FROM is what DMARC consumes.
 */
 
 int
-dmarcf_parse_received_spf(char *str)
+dmarcf_parse_received_spf(char *str, char *envfrom)
 {
-	_Bool copying = FALSE;
+	_Bool in_result = TRUE;
 	_Bool escaped = FALSE;
 	int parens = 0;
 	char *p;
 	char *r;
 	char *end;
 	char result[MAXSPFRESULT + 1];
+	char spf_envfrom[BUFRSZ + 1];
+	char key[BUFRSZ + 1];
+	char value[BUFRSZ + 1];
+	char identity[BUFRSZ + 1];
 
 	assert(str != NULL);
 
+	memset(spf_envfrom, '\0', sizeof spf_envfrom);
+	memset(key, '\0', sizeof key);
+	memset(value, '\0', sizeof value);
+	memset(identity, '\0', sizeof identity);
 	memset(result, '\0', sizeof result);
+
+	/* first thing we get is the result token */
 	r = result;
 	end = &result[sizeof result - 1];
 
 	for (p = str; *p != '\0'; p++)
 	{
-		if (escaped)
+		if (*p == '(')
 		{
-			if (copying)
-			{
-				if (r < end)
-					*r++ = *p;
-			}
-
-			escaped = FALSE;
+			parens++;
 		}
-		else if (copying)
+		else if (*p == ')' && parens > 0)
 		{
-			if (!escaped && *p == '\\')
-			{
-				escaped = TRUE;
-			}
-			else if (*p == '(')
-			{
-				copying = FALSE;
-				parens++;
-			}
- 			else if (isascii(*p) && isspace(*p))
-			{
-				copying = FALSE;
-			}
-			else if (r < end)
-			{
-				*r++ = *p;
-			}
+			parens--;
 		}
-		else if (*p == '(')
+		else if (escaped)
 		{
-			parens++;
+			if (parens == 0 && r < end)
+				*r++ = *p;
+			escaped = FALSE;
 		}
-		else if (*p == ')' && parens > 0)
+		else if (*p == '\\')
 		{
-			parens--;
+			escaped = TRUE;
 		}
 		else if (parens == 0)
 		{
+			/* a possibly meaningful character */
 			if (isascii(*p) && isspace(*p))
+			{
+				if (in_result)
+				{
+					in_result = FALSE;
+					r = key;
+					end = &key[sizeof key - 1];
+				}
 				continue;
+			}
 
-			if (!copying)
+			if (!in_result && *p == '=')
 			{
-				if (result[0] != '\0')
-					break;
-
-				copying = TRUE;
-				if (r < end)
-					*r++ = *p;
+				r = value;
+				end = &value[sizeof value - 1];
+			}
+			else if (!in_result && *p == ';')
+			{
+				if (strcasecmp(key, "identity") == 0)
+					strlcpy(identity, value, sizeof identity);
+				if (strcasecmp(key, "envelope-from") == 0)
+					strlcpy(spf_envfrom, value, sizeof spf_envfrom);
+				memset(key, '\0', sizeof key);
+				memset(value, '\0', sizeof value);
+
+				r = key;
+				end = &key[sizeof key - 1];
+			}
+			else if (r < end)
+			{
+				*r++ = *p;
 			}
 		}
 	}
 
-	if (strcasecmp(result, "pass") == 0)
+	if (key[0] != '\0')
+	{
+		if (strcasecmp(key, "identity") == 0)
+			strlcpy(identity, value, sizeof identity);
+		if (strcasecmp(key, "envelope-from") == 0)
+			strlcpy(spf_envfrom, value, sizeof spf_envfrom);
+	}
+
+	if (strcasecmp(identity, "mailfrom") != 0 ||
+            strcasecmp(spf_envfrom, envfrom) != 0)
+	{
+		return ARES_RESULT_NEUTRAL;
+	}
+	else if (strcasecmp(result, "pass") == 0)
+	{
 		return ARES_RESULT_PASS;
+	}
 	else if (strcasecmp(result, "fail") == 0)
+	{
 		return ARES_RESULT_FAIL;
+	}
 	else if (strcasecmp(result, "softfail") == 0)
+	{
 		return ARES_RESULT_SOFTFAIL;
+	}
 	else if (strcasecmp(result, "neutral") == 0)
+	{
 		return ARES_RESULT_NEUTRAL;
+	}
 	else if (strcasecmp(result, "temperror") == 0)
+	{
 		return ARES_RESULT_TEMPERROR;
+	}
 	else if (strcasecmp(result, "none") == 0)
+	{
 		return ARES_RESULT_NONE;
+	}
 	else
+	{
 		return ARES_RESULT_PERMERROR;
+	}
 }
 
 /*
@@ -2889,7 +2937,8 @@ mlfi_eom(SMFICTX *ctx)
 				else
 					spfmode = DMARC_POLICY_SPF_ORIGIN_MAILFROM;
 
-				spfres = dmarcf_parse_received_spf(hdr->hdr_value);
+				spfres = dmarcf_parse_received_spf(hdr->hdr_value,
+				                                   dfc->mctx_envfrom);
 
 				dmarcf_dstring_printf(dfc->mctx_histbuf,
 				                      "spf %d\n", spfres);

opendmarc/tests/Makefile.am

@@ -3,7 +3,8 @@
 check_SCRIPTS = 
 
 if LIVE_TESTS
-check_SCRIPTS += t-verify-nodata t-verify-nodata-reject t-verify-unspec
+check_SCRIPTS += t-verify-nodata t-verify-nodata-reject t-verify-unspec \
+	t-verify-received-spf-good
 endif
 
 TESTS = $(check_SCRIPTS)
@@ -12,6 +13,8 @@ EXTRA_DIST = \
 	t-verify-nodata t-verify-nodata.conf t-verify-nodata.lua \
 	t-verify-nodata-reject t-verify-nodata-reject.conf \
 		t-verify-nodata-reject.lua \
-	t-verify-unspec t-verify-unspec.conf t-verify-unspec.lua
+	t-verify-unspec t-verify-unspec.conf t-verify-unspec.lua \
+	t-verify-received-spf-good t-verify-received-spf-good.conf \
+		t-verify-received-spf-good.lua
 
 MOSTLYCLEANFILES=

opendmarc/tests/t-verify-received-spf-bad

@@ -0,0 +1,13 @@
+#!/bin/sh
+#
+# Test a message with a Received-SPF header that we can't trust
+#
+# DMARC only wants SPF results based on MAIL FROM.  This is the positive
+# case where we have a Received-SPF that is definitely what we want.
+
+if [ x"$srcdir" = x"" ]
+then
+	srcdir=`pwd`
+fi
+
+miltertest -s $srcdir/t-verify-received-spf-bad.lua

opendmarc/tests/t-verify-received-spf-bad.conf

@@ -0,0 +1 @@
+Background		No

opendmarc/tests/t-verify-received-spf-bad.lua

@@ -0,0 +1,120 @@
+-- Copyright (c) 2021, The Trusted Domain Project.  All rights reserved.
+
+-- Test message with a not-valid (the way DMARC wants it) Received-SPF field
+-- 
+-- Confirms that a message with a Received-SPF field that indicates a "pass"
+-- but does not include "identity=mailfrom" and "envelope-from" with the
+-- right value will not be trusted.
+
+mt.echo("*** Received-SPF test (bad)")
+
+-- setup
+sock = "unix:" .. mt.getcwd() .. "/t-verify-received-spf-bad.sock"
+binpath = mt.getcwd() .. "/.."
+if os.getenv("srcdir") ~= nil then
+	mt.chdir(os.getenv("srcdir"))
+end
+
+-- try to start the filter
+mt.startfilter(binpath .. "/opendmarc", "-l",
+               "-c", "t-verify-received-spf-bad.conf", "-p", sock)
+
+-- try to connect to it
+conn = mt.connect(sock, 40, 0.05)
+if conn == nil then
+	error("mt.connect() failed")
+end
+
+-- send connection information
+-- mt.negotiate() is called implicitly
+if mt.conninfo(conn, "localhost2", "127.0.0.2") ~= nil then
+	error("mt.conninfo() failed")
+end
+if mt.getreply(conn) ~= SMFIR_CONTINUE then
+	error("mt.conninfo() unexpected reply")
+end
+
+-- send envelope macros and sender data
+-- mt.helo() is called implicitly
+mt.macro(conn, SMFIC_MAIL, "i", "t-verify-received-spf-bad")
+if mt.mailfrom(conn, "[email protected]") ~= nil then
+	error("mt.mailfrom() failed")
+end
+if mt.getreply(conn) ~= SMFIR_CONTINUE then
+	error("mt.mailfrom() unexpected reply")
+end
+
+-- send headers
+-- mt.rcptto() is called implicitly
+if mt.header(conn, "Received-SPF", "pass") ~= nil then
+	error("mt.header(Received-SPF) failed")
+end
+if mt.getreply(conn) ~= SMFIR_CONTINUE then
+	error("mt.header(Received-SPF) unexpected reply")
+end
+if mt.header(conn, "From", "[email protected]") ~= nil then
+	error("mt.header(From) failed")
+end
+if mt.getreply(conn) ~= SMFIR_CONTINUE then
+	error("mt.header(From) unexpected reply")
+end
+if mt.header(conn, "To", "[email protected]") ~= nil then
+	error("mt.header(To) failed")
+end
+if mt.getreply(conn) ~= SMFIR_CONTINUE then
+	error("mt.header(To) unexpected reply")
+end
+if mt.header(conn, "Date", "Tue, 22 Dec 2009 13:04:12 -0800") ~= nil then
+	error("mt.header(Date) failed")
+end
+if mt.getreply(conn) ~= SMFIR_CONTINUE then
+	error("mt.header(Date) unexpected reply")
+end
+if mt.header(conn, "Subject", "DMARC test") ~= nil then
+	error("mt.header(Subject) failed")
+end
+if mt.getreply(conn) ~= SMFIR_CONTINUE then
+	error("mt.header(Subject) unexpected reply")
+end
+
+-- send EOH
+if mt.eoh(conn) ~= nil then
+	error("mt.eoh() failed")
+end
+if mt.getreply(conn) ~= SMFIR_CONTINUE then
+	error("mt.eoh() unexpected reply")
+end
+
+-- end of message; let the filter react
+if mt.eom(conn) ~= nil then
+	error("mt.eom() failed")
+end
+if mt.getreply(conn) ~= SMFIR_ACCEPT then
+	error("mt.eom() unexpected reply")
+end
+
+-- verify that an Authentication-Results header field got added
+if not mt.eom_check(conn, MT_HDRINSERT, "Authentication-Results") and
+   not mt.eom_check(conn, MT_HDRADD, "Authentication-Results") then
+	error("no Authentication-Results added")
+end
+
+-- verify that a DMARC pass result was added
+n = 0
+found = 0
+while true do
+	ar = mt.getheader(conn, "Authentication-Results", n)
+	if ar == nil then
+		break
+	end
+	if string.find(ar, "dmarc=fail", 1, true) ~= nil then
+		found = 1
+		break
+	end
+	n = n + 1
+end
+if found == 0 then
+	error("incorrect DMARC result")
+end
+
+mt.disconnect(conn)