Fix ARC header parsing crashes: SIGABRT on malformed tokens and SIGSEGV on large RSA key signatures#296
Open
thegushi wants to merge 3 commits into
Open
Fix ARC header parsing crashes: SIGABRT on malformed tokens and SIGSEGV on large RSA key signatures#296thegushi wants to merge 3 commits into
thegushi wants to merge 3 commits into
Conversation
added 2 commits
May 11, 2026 19:19
opendmarc-arcares.c, opendmarc-arcseal.c: strip_whitespace() returns NULL when a token exceeds 512 chars; guard each call site so overlong or malformed ARC header values return -1 rather than segfaulting via a NULL pointer passed to strlcpy(). Also remove the dependency on opendmarc.h (only needed for MIN_OF, now defined locally) so these parsers can be compiled and tested without libmilter. opendmarc.c: free the allocated arcares_header/arcseal_header structs before continuing when the parse fails, plugging two small heap leaks. libopendmarc/tests/test_arcares.c: new unit tests covering valid parse and the overlong-token crash paths for all three functions.
…g headers (issues #183, #186, #222, #236) Two distinct crash modes in ARC header parsing, both rooted in the same parser pattern: SIGABRT (#222, #186): Malformed headers like "ARC-Seal: i=1; none" produce a token with no '=' sign. strsep() sets token_ptr to NULL, which was then passed directly to strip_whitespace(), triggering assert(string != NULL). Fix: guard token_ptr == NULL after strsep() and return -1 in all three parse functions (opendmarc_arcseal_parse, opendmarc_arcares_parse, opendmarc_arcares_arc_parse). SIGSEGV (#183, #236): RSA 3072-bit (and larger) key signatures produce base64 values >= 512 bytes. strip_whitespace() returned NULL when the token hit the OPENDMARC_*_MAX_TOKEN_LEN limit, and that NULL was then passed to strlcpy(). Fix: remove the artificial length cap from both strip_whitespace() functions; the input is already bounded by the 4096-byte MAXHEADER_LEN buffer, and strlcpy() handles truncation into struct fields. Also bump OPENDMARC_ARCSEAL_MAX_LONG_VALUE_LEN and OPENDMARC_ARCARES_MAX_LONG_VALUE_LEN from 512 to 2048 so that struct fields can hold signatures from RSA 4096-bit and larger keys without truncation. Regression tests added for all crash paths. Supersedes PR #223, PR #225, PR #277.
This was referenced May 12, 2026
Open
thegushi
added
the
needs-testing
This was referenced May 12, 2026
Closed
Open
…g (issue #238) Two related issues causing Gmail (and other) ARC-Authentication-Results headers to be silently rejected: Unknown authentication methods (e.g. dara=, bimi=) caused the entire AAR header to be rejected. The default: case in all three ARC parsers set result = -1, so a header containing any unrecognized method would return failure even if all the known fields (dkim=, spf=, dmarc=) parsed correctly. Changed to break so unknown methods are silently skipped. Multi-line ARC headers use \r\n for line endings before continuation whitespace. The strspn() calls that strip leading whitespace used " \n\t" but omitted \r, leaving a stray \r at the start of tokens in folded headers. Added \r to all four strspn() character sets across both parsers. Regression tests added for both cases.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
token_ptr == NULLafterstrsep()inopendmarc_arcseal_parse,opendmarc_arcares_parse, andopendmarc_arcares_arc_parse— a token with no=sign (e.g.ARC-Seal: i=1; none) previously causedassert(string != NULL)insidestrip_whitespace().strip_whitespace()functions — RSA 3072-bit (and larger) key signatures produceb=values ≥512 bytes, causingstrip_whitespace()to returnNULLwhich was then passed tostrlcpy().OPENDMARC_ARCSEAL_MAX_LONG_VALUE_LENandOPENDMARC_ARCARES_MAX_LONG_VALUE_LENfrom 512 → 2048 to accommodate RSA 4096-bit (and larger) signatures without truncation.Supersedes #223, #225, #277. Closes #183, #186, #222, #236. Related: #241, #242.
Test plan
make checkpasses (8/8 tests includingtest_arcares)test_arcaresincludes regression tests for both SIGABRT paths (missing=) and SIGSEGV paths (520-byte token exceeding old 512-byte limit)