Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
KIC-8462852 opened this issue May 30, 2023 · 2 comments

Comments

@KIC-8462852
Copy link

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

@KIC-8462852 KIC-8462852 changed the title Segfault fault with invalid or long values in ARC-Authentication-Results headers Segfault with invalid or long values in ARC-Authentication-Results headers May 30, 2023
@glts
Copy link
Contributor

glts commented Jun 14, 2023

@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
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants