-
Notifications
You must be signed in to change notification settings - Fork 56
Fix multiple addresses in From vulnerability #48
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
Fix multiple addresses in From vulnerability #48
Conversation
|
Thanks @panpilkarz Let's hope that someone from trusteddomainproject is reacting to this, |
|
In light of https://tools.ietf.org/html/rfc7489#section-6.6.1 this looks way too simple. From a quick look I'd say that the proposed fix only parses the first domain and it does not parse/verify each domain as specified in the RFC. |
- please note that it might only be a partial fix, see trusteddomainproject/OpenDMARC#48 (comment) PR: 240505 Reported by: protonmail Approved by: ports-secteam (delphij) Obtained from: trusteddomainproject/OpenDMARC#48 MFH: 2019Q3 Security: https://protonmail.com/blog/bellingcat-cyberattack-phishing/ git-svn-id: svn+ssh://svn.freebsd.org/ports/head@512093 35697150-7ecd-e111-bb59-0022644237b5
mail/opendmarc: fix multiple addresses in From vulnerability - please note that it might only be a partial fix, see trusteddomainproject/OpenDMARC#48 (comment) PR: 240505 Reported by: protonmail Approved by: ports-secteam (delphij) Obtained from: trusteddomainproject/OpenDMARC#48 Security: https://protonmail.com/blog/bellingcat-cyberattack-phishing/
- please note that it might only be a partial fix, see trusteddomainproject/OpenDMARC#48 (comment) PR: 240505 Reported by: protonmail Approved by: ports-secteam (delphij) Obtained from: trusteddomainproject/OpenDMARC#48 MFH: 2019Q3 Security: https://protonmail.com/blog/bellingcat-cyberattack-phishing/
- please note that it might only be a partial fix, see trusteddomainproject/OpenDMARC#48 (comment) PR: 240505 Reported by: protonmail Approved by: ports-secteam (delphij) Obtained from: trusteddomainproject/OpenDMARC#48 MFH: 2019Q3 Security: https://protonmail.com/blog/bellingcat-cyberattack-phishing/ git-svn-id: svn+ssh://svn.freebsd.org/ports/head@512093 35697150-7ecd-e111-bb59-0022644237b5
|
I would recommend distros and others apply this (I'm about to do it in Debian). I tested this by creating a DKIM signed multi-from message. The message was After patching, the result is DMARC fail for the first domain listed (same one I did a code inspection and there is existing code to error out when more than one |
|
CVE-2019-16378 was assigned for this issue. |
|
For the record I am applying this to the Fedora / EPEL package too; it will be in opendmarc-1.3.2-1 on Fedora / EPEL. I think this is the same as https://sourceforge.net/p/opendmarc/tickets/235/ in the sourceforge tracker. I am unsure what status this github repo has and whether any maintainer is watching it. |
|
It's related, but the problem is bigger than that. When I was trying to replicate the attack, I used DKIM only. I used multiple From values to get an incorrect result. Something like:
mail from: good.example.whatever
...
DKIM signature with d=example.bad
...
From: example.com, example.bad
The result was DMARC pass for example.com.
I believe, but haven't tested, that the referenced issue on sourceforge is another instance about being confused about what the authenticated identity is. It would be useful for someone to test with this patch and see if they can still replicate the issue that's reported there.
Scott K
|
|
Background: When OpenDMARC was written, the posture of the DMARC community was that this case is so rare that such messages should simply be rejected because, as the RFC says, the kinds of email DMARC aims to deal with never encounter this use case. Accordingly, the filter and library were written on the assumption that they would never see this use case. Discussion: As far as I can tell, this patch is really a no-op. The current value passed to this function contains the whole header field value, which is then parsed to find the domain. The patch proposes to pass just the domain, but it's the same domain, so nothing really changes. Proposal: If upstream MTAs are not rejecting multi-valued From fields as expected, we'll have to teach this filter to parse and process them. |
|
When this was posted, I tested opendmarc both with and without this patch and the behavior is different (see above: #48 (comment)), so for whatever reason it's not a no-op. I tested passing mail through postfix and libmilter, so it was representative of what can happen on a live system, not an artificial test architecture. I don't think 'fix all the MTAs' is a good solution, so I do think it needs addressing here. You already have code to error out in the multi-From case, I think it's enough to apply this change to get it triggered. |
|
@mskucherawy is working on a different fix to this issue. However, I am leaving this open as the patch is in the wild and will need to be conflict-merged in the future. |
|
It may be obvious, but since there seems to be some confusion about why the patch works (and it confused me as well), I just wanted to point out: The reason that it works (looking at opendmarc/opendmarc.c) is that at all places in the code where the From header value is used the Thus, when a DKIM PASS result is stored later and it matches with whatever was identified by libopendmarc as From domain but that does not agree with what was parsed by the local code, we get the wrong result. In addition to patching any multi-From problems, I think it would be a good idea to use the actual From domain used by libopendmarc when reporting results, e.g., writing the AR-header (I added a function to do just this: #63.) |
|
I made a different fix for this in commit bdcda9b. We're already parsing the From field and extracting possibly multiple values, but currently we would quietly take the first one and continue. With the patch, messages with multi-valued From fields are ignored completely by the filter, though there is a new option to reject them instead if that's preferred. The latter provides strict compliance with the advice of RFC 7489 Section 6.6.1; the former is the less-disruptive alternative that also avoids making an inconsistent determination. |
|
Why don't you guys just use a 3rd party lib that parses rfc5322 fields instead of trying to reinvent the wheel? |
|
I know this has been "hanging around" for a while and I believe it is superceded by Murray's recent patch for the multi-from problem. I'm going to mark this wont patch as I think we have an effective solution now. IF THIS IS IN ERROR -- we can always create a new PR against 1.4.x main branch and I'll have @thegushi review it. -Martin ( patch guru ) |
|
We also did apply the one-line patch that FreeBSD and OpenBSD have been using, here: https://github.com/openbsd/ports/blob/master/mail/opendmarc/patches/patch-opendmarc_opendmarc_c |
mail/opendmarc: fix multiple addresses in From vulnerability - please note that it might only be a partial fix, see trusteddomainproject/OpenDMARC#48 (comment) PR: 240505 Reported by: protonmail Approved by: ports-secteam (delphij) Obtained from: trusteddomainproject/OpenDMARC#48 Security: https://protonmail.com/blog/bellingcat-cyberattack-phishing/
|
I am running EDIT: I found the answer on https://ubuntu.com/security/cves?q=&package=opendmarc |
This PR fixes vulnerability that allows opendmarc to pass as
example.comsince spf and dkim were notexample.comin the following example:Input:
Output: