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

‘KeepAuthResults = no’ (the default setting) may delete the wrong headers #148

Open
glts opened this issue Mar 16, 2022 · 5 comments
Open
Labels

Comments

@glts
Copy link

glts commented Mar 16, 2022

Authentication-Results headers are deleted by iterating over them in forward direction. But because each deletion shifts headers that come after it, this procedure deletes the wrong headers after the first one. I verified this with Postfix.

https://github.com/trusteddomainproject/OpenDKIM/blob/rel-opendkim-2-11-0-Beta2/opendkim/opendkim.c#L13636-L13639

(My solution: disable this code with KeepAuthResults = yes and prepend a milter https://crates.io/crates/spf-milter that does this correctly)

@Rombobeorn
Copy link

I made the same discovery while trying to figure out how to get RemoveARFrom to work. This defect caused me quite some consternation before I figured out what was going on.

With this line in /etc/opendkim.conf:

RemoveARFrom 2.example,3.example,5.example

this set of header fields:

Authentication-Results: 1.example; dkim=pass
Authentication-Results: 2.example; dkim=pass
Authentication-Results: 3.example; dkim=pass
Authentication-Results: 4.example; dkim=pass
Authentication-Results: 5.example; dkim=pass
Authentication-Results: 6.example; dkim=pass
Authentication-Results: 7.example; dkim=pass
Authentication-Results: 8.example; dkim=pass

gets transformed into this:

Authentication-Results: 1.example; dkim=pass
Authentication-Results: 3.example; dkim=pass
Authentication-Results: 5.example; dkim=pass
Authentication-Results: 6.example; dkim=pass
Authentication-Results: 8.example; dkim=pass

That is, the first matching header field gets removed correctly, but for the second match the one below it gets removed, and for the third match the one two steps below gets removed.

I found this with OpenDKIM 2.11.0-Beta2, Libmilter 8.15.2 and Postfix 3.5.13.

Evidently, when Postfix removes a header field, the ordinal numbers of the following fields change, and Postfix's and OpenDKIM's ideas of the fields' ordinal numbers get out of sync.

When removing elements from a list, it's generally safer to process the list in reverse so that the elements that get renumbered are those you have already processed.

Suggestions for a solution:

1: Count instances of Authentication-Results in mlfi_header. In mlfi_eom, walk the list of header fields backwards and count down instead of up.

2: When an instance to remove is found, push its index to a stack. When done walking the list of header fields, pop indices from the stack and call dkimf_chgheader. The use of a stack causes the last instance to be removed first.

3: If you are confident that all current and future MTAs count the header fields anew each time, then OpenDKIM can compensate by decreasing c if dkimf_chgheader succeeds.

@thegushi thegushi added the bug label Jan 7, 2023
@Rombobeorn
Copy link

CVE-2022-48521 has been assigned to this vulnerability.

@mdomsch
Copy link

mdomsch commented Aug 6, 2023

@vvvllll
Copy link

vvvllll commented Dec 8, 2023

So is the whole TrustedDomainProject dead?

Nothing for years and distributions are only putting few fixes separately.
Opendkim suffers from multiple bugs including this security issue..

@mdomsch
Copy link

mdomsch commented Dec 28, 2023 via email

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

No branches or pull requests

5 participants