net: LookupMX behaviour broken #46979
Comments
seankhliao
added
the
NeedsDecision
|
These changes (https://golang.org/cl/323131 and cherry picks) were made to address a security issue. See #46241 and https://www.whitesourcesoftware.com/vulnerability-database/CVE-2021-33195. Marking as release blocker for 1.17 to decide whether there is anything to change here. CC @golang/osp-team @golang/security |
|
@horkhe do you have an example of why a server would legitimately return a set of MX records with some "invalid" ones, or do you have an example of any real-world servers doing that? That is, what breaks due to the filtering? |
|
Change https://golang.org/cl/332094 mentions this issue: |
To be clear, invalid even after we fix validation to allow null records. |
|
@gopherbot please open a backport issue for Go 1.16, this is a regression in the last minor release. |
|
Backport issue(s) opened: #46999 (for 1.16). Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases. |
|
@rcs full disclosure, I am a developer from Mailgun by Pathwire. We send emails... quite a few of them. Obviously we query MX records a lot. Occasionally we see domains that have several MX records sprinkled with invalid ones. Those are obviously human errors. I cannot really give you a real world example of those records, but here is a domain that I created to run tests - test-unicode.definbox.com. It was inspired by a real configuration that we stumbled up on in our line of business. The original domain has already been fixed by the owner. So the answer to your question is: there is no legitimate reason to configure a bad record, but one human error should not prevent users from seen all the correct MX records if any. Thanks a lot for a quick turn around on this issue. |
|
@FiloSottile If I understand correctly, Go 1.15 is also affected, and so it also needs a backport candidate. Opened #47012. |
Bypass hostname validity checking when a null mx record is returned as, defined in RFC 7505. Updates #46979 Change-Id: Ibe683bd6b47333a8ff30909fb2680ec8e10696ef Reviewed-on: https://go-review.googlesource.com/c/go/+/332094 Trust: Roland Shoemaker <roland@golang.org> Trust: Katie Hockman <katie@golang.org> Run-TryBot: Roland Shoemaker <roland@golang.org> TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Katie Hockman <katie@golang.org>
|
Change https://golang.org/cl/332371 mentions this issue: |
|
Change https://golang.org/cl/332372 mentions this issue: |
|
@horkhe Thanks for the extra information. |
|
Change https://golang.org/cl/332549 mentions this issue: |
Bypass hostname validity checking when a null mx record is returned as, defined in RFC 7505. Updates #46979 Updates #46999 Change-Id: Ibe683bd6b47333a8ff30909fb2680ec8e10696ef Reviewed-on: https://go-review.googlesource.com/c/go/+/332094 Trust: Roland Shoemaker <roland@golang.org> Trust: Katie Hockman <katie@golang.org> Run-TryBot: Roland Shoemaker <roland@golang.org> TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Katie Hockman <katie@golang.org> (cherry picked from commit 03761ed) Reviewed-on: https://go-review.googlesource.com/c/go/+/332371 Run-TryBot: Katie Hockman <katie@golang.org>
Bypass hostname validity checking when a null mx record is returned as, defined in RFC 7505. Updates #46979 Updates #47012 Change-Id: Ibe683bd6b47333a8ff30909fb2680ec8e10696ef Reviewed-on: https://go-review.googlesource.com/c/go/+/332094 Trust: Roland Shoemaker <roland@golang.org> Trust: Katie Hockman <katie@golang.org> Run-TryBot: Roland Shoemaker <roland@golang.org> TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Katie Hockman <katie@golang.org> (cherry picked from commit 03761ed) Reviewed-on: https://go-review.googlesource.com/c/go/+/332372 Run-TryBot: Katie Hockman <katie@golang.org>
|
Change https://golang.org/cl/333330 mentions this issue: |
|
Change https://golang.org/cl/333331 mentions this issue: |
…nstead of hard failing Instead of hard failing on a single bad record, filter the bad records and return anything valid. This only applies to the methods which can return multiple records, LookupMX, LookupNS, LookupSRV, and LookupAddr. When bad results are filtered out, also return an error, indicating that this filtering has happened. Updates #46241 Updates #46979 Fixes #46999 Change-Id: I6493e0002beaf89f5a9795333a93605abd30d171 Reviewed-on: https://go-review.googlesource.com/c/go/+/332549 Trust: Roland Shoemaker <roland@golang.org> Reviewed-by: Filippo Valsorda <filippo@golang.org> (cherry picked from commit 296ddf2) Reviewed-on: https://go-review.googlesource.com/c/go/+/333330 Run-TryBot: Roland Shoemaker <roland@golang.org> TryBot-Result: Go Bot <gobot@golang.org>
…nstead of hard failing Instead of hard failing on a single bad record, filter the bad records and return anything valid. This only applies to the methods which can return multiple records, LookupMX, LookupNS, LookupSRV, and LookupAddr. When bad results are filtered out, also return an error, indicating that this filtering has happened. Updates #46241 Updates #46979 Fixes #47012 Change-Id: I6493e0002beaf89f5a9795333a93605abd30d171 Reviewed-on: https://go-review.googlesource.com/c/go/+/332549 Trust: Roland Shoemaker <roland@golang.org> Reviewed-by: Filippo Valsorda <filippo@golang.org> (cherry picked from commit 296ddf2) Reviewed-on: https://go-review.googlesource.com/c/go/+/333331 Run-TryBot: Roland Shoemaker <roland@golang.org>
dmitshur
removed
the
NeedsDecision
dmitshur
added
the
NeedsFix
|
Thank you, guys! |
Change https://golang.org/cl/322230 introduced in release 1.16.5 and backported to 1.15.13, altered
net.LookupMXbehaviour that breaks the old contract in at least two ways:&net.DNSError{Err: "MX target is invalid", Name: name}error.&net.DNSError{Err: "MX target is invalid", Name: name}is returned. So it is impossible to distinguish a domain explicitly forbidding SMTP connections from a domain with misconfigured MX records.I believe users of
net.LookupXXXfamily have come to expect that those methods return whatever DNS servers on the Internet have configured. We understand that those records can be incorrect and sometimes they are, but mostly due to human errors. Following the Robustness Principal we try to salvage useful pieces, ignore and/or auto-correct bad records. But this change robbed us of that option. So I think https://golang.org/cl/322230 is a regression and should be rolled back, alternatively you can providenet.RawLookupXXXmethods, or addSkipVerify boolfield tonet.Resolverthat would allow disabling verification of retrieved records.The text was updated successfully, but these errors were encountered: