search

trusteddomainproject / OpenDMARC

This is the Trusted Domain Project's impementation of the DMARC protocol libary and mail filter, called OpenDMARC. A "milter" connects to unix-based mailers (originally, sendmail, but now many) and provides a standard filtering API.
Other
98 stars 52 forks source link

Segfault with invalid or long values in ARC-Authentication-Results headers #242

Open KIC-8462852 opened 1 year ago

KIC-8462852 commented 1 year ago

Hi, This issue is similar to #183 but this time involving invalid or very long values in ARC-Authentication-Results headers (instead of ARC-Seal). The patch fixes passing NULL value to strlcpy() when parsing invalid or very long values (>=OPENDMARC_ARCARES_MAX_TOKEN_LEN) in ARC-Authentication-Results headers with opendmarc_arcares_parse() and opendmarc_arcares_arc_parse(). Thx. opendmarc-arcares.patch.txt

glts commented 1 year ago

@KIC-8462852 Looking at this new patch, I noticed that in your earlier linked patch you used return 0;. Those should probably be return -1; as is done in this patch. Cheers

KIC-8462852 commented 1 year ago

Hi @glts Indeed, surely, no doubt. I remember that two years ago, when I wrote the patch for issue #183, I came across this question. Formally, in case of a parsing failure, the return should be -1. But, as at the time there was already a similar test in the code that returned 0, I ended up following the same logic for consistency. Anyway, I think there's no big deal with this. First, it is not relevant to the DMARC assessment. In fact, the return from these parsers does not in any way affect the DMARC assessment and its results. Second, this just influences the info shown in the DMARC reports. The difference in the way the code proceeds after the parsing is: when returning 0, the parsing results, even partial ones, are stored for later use in aggregated DMARC reports; when returning -1, these results are simply ignored. So, for what matters in both patches, which is solving the Segfault issues, this is a minor question. IMHO, this being the sole purpose of this parsing, it would be best not to parse the ARC headers at all. That's the job of OpenARC (or other ARC software) and also because these parsers still have other issues. The OpenDMARC, even when using ARC results to override a DMARC failure, does not use any information in these ARC headers. It relies on the ARes header with the required ARC evaluation (see #182). Anyway, in this new patch for the same issue now with ARC-ARes headers (as you know #183 is about ARC-Seal headers), I decided to use the more correct way by returning -1. But we agree that it should be -1 in both cases. Cheers.