trusteddomainproject / OpenDKIM

Other
94 stars 51 forks source link

Authentication-Results fields are not removed correctly #185

Closed mdomsch closed 1 year ago

mdomsch commented 1 year ago

From: https://bugzilla.redhat.com/show_bug.cgi?id=2223270 and https://bugzilla.redhat.com/show_bug.cgi?id=2223352

Description of problem: When OpenDKIM removes fake Authentication-Results fields (as required in https://www.rfc-editor.org/rfc/rfc8601#section-5), it doesn't account for the fact that – at least in Postfix – this changes the ordinal numbers of the following header fields, so it passes the wrong number to the MTA for the second and following header fields it removes. If there are more than one fake Authentication-Results fields, then OpenDKIM leaves some of them in place. Thus a fake Authentication-Results field can bypass OpenDKIM, and be relied on by other programs as if it had been added by OpenDKIM. An email message may be accepted when by policy it should be rejected, and/or the recipient can be tricked into believing that the sender is someone they trust.

It seems unlikely that the vulnerability will be fixed upstream. Sysadmins should know that Authentication-Results from OpenDKIM can't be trusted unless some other program removes fake Authentication-Results fields from incoming messages before OpenDKIM processes them.

Version-Release number of selected component: 2.11.0-0.34

How reproducible: deterministic

Steps to Reproduce:

  1. this line in /etc/opendkim.conf: RemoveARFrom 2.example,3.example,5.example

  2. an email with 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

Actual results: The above 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.

Expected results: Authentication-Results: 1.example; dkim=pass Authentication-Results: 4.example; dkim=pass Authentication-Results: 6.example; dkim=pass Authentication-Results: 7.example; dkim=pass Authentication-Results: 8.example; dkim=pass

A note for anyone who wants to develop a patch: The Libmilter API documentation doesn't specify whether removing a header field renumbers the following header fields, so hypothetically different MTAs could do it differently without violating the API specification. The safe way to handle the ambiguity is to remove header fields in reverse order.

glts commented 1 year ago

Duplicate of #148

mdomsch commented 1 year ago

Closing as duplicate as noted.