145 lines
5.3 KiB
Diff
145 lines
5.3 KiB
Diff
From 4be3fc33ef512532b916aa14258087e89eb47347 Mon Sep 17 00:00:00 2001
|
|
From: Russ Cox <rsc@golang.org>
|
|
Date: Wed, 4 Oct 2017 13:24:49 -0400
|
|
Subject: [PATCH] [release-branch.go1.8] net/smtp: fix PlainAuth to refuse to
|
|
send passwords to non-TLS servers
|
|
|
|
PlainAuth originally refused to send passwords to non-TLS servers
|
|
and was documented as such.
|
|
|
|
In 2013, issue #5184 was filed objecting to the TLS requirement,
|
|
despite the fact that it is spelled out clearly in RFC 4954.
|
|
The only possibly legitimate use case raised was using PLAIN auth
|
|
for connections to localhost, and the suggested fix was to let the
|
|
server decide: if it advertises that PLAIN auth is OK, believe it.
|
|
That approach was adopted in CL 8279043 and released in Go 1.1.
|
|
|
|
Unfortunately, this is exactly wrong. The whole point of the TLS
|
|
requirement is to make sure not to send the password to the wrong
|
|
server or to a man-in-the-middle. Instead of implementing this rule,
|
|
CL 8279043 blindly trusts the server, so that if a man-in-the-middle
|
|
says "it's OK, you can send me your password," PlainAuth does.
|
|
And the documentation was not updated to reflect any of this.
|
|
|
|
This CL restores the original TLS check, as required by RFC 4954
|
|
and as promised in the documentation for PlainAuth.
|
|
It then carves out a documented exception for connections made
|
|
to localhost (defined as "localhost", "127.0.0.1", or "::1").
|
|
|
|
Cherry-pick of CL 68170.
|
|
|
|
Change-Id: I1d3729bbd33aa2f11a03f4c000e6bb473164957b
|
|
Reviewed-on: https://go-review.googlesource.com/68023
|
|
Run-TryBot: Russ Cox <rsc@golang.org>
|
|
Reviewed-by: Chris Broadfoot <cbro@golang.org>
|
|
---
|
|
src/net/smtp/auth.go | 33 ++++++++++++++++++---------------
|
|
src/net/smtp/smtp_test.go | 32 ++++++++++++++++++++++----------
|
|
2 files changed, 40 insertions(+), 25 deletions(-)
|
|
|
|
diff --git a/src/net/smtp/auth.go b/src/net/smtp/auth.go
|
|
index 3f1339ebc56..fd1a472f930 100644
|
|
--- a/src/net/smtp/auth.go
|
|
+++ b/src/net/smtp/auth.go
|
|
@@ -44,26 +44,29 @@ type plainAuth struct {
|
|
}
|
|
|
|
// PlainAuth returns an Auth that implements the PLAIN authentication
|
|
-// mechanism as defined in RFC 4616.
|
|
-// The returned Auth uses the given username and password to authenticate
|
|
-// on TLS connections to host and act as identity. Usually identity will be
|
|
-// left blank to act as username.
|
|
+// mechanism as defined in RFC 4616. The returned Auth uses the given
|
|
+// username and password to authenticate to host and act as identity.
|
|
+// Usually identity should be the empty string, to act as username.
|
|
+//
|
|
+// PlainAuth will only send the credentials if the connection is using TLS
|
|
+// or is connected to localhost. Otherwise authentication will fail with an
|
|
+// error, without sending the credentials.
|
|
func PlainAuth(identity, username, password, host string) Auth {
|
|
return &plainAuth{identity, username, password, host}
|
|
}
|
|
|
|
+func isLocalhost(name string) bool {
|
|
+ return name == "localhost" || name == "127.0.0.1" || name == "::1"
|
|
+}
|
|
+
|
|
func (a *plainAuth) Start(server *ServerInfo) (string, []byte, error) {
|
|
- if !server.TLS {
|
|
- advertised := false
|
|
- for _, mechanism := range server.Auth {
|
|
- if mechanism == "PLAIN" {
|
|
- advertised = true
|
|
- break
|
|
- }
|
|
- }
|
|
- if !advertised {
|
|
- return "", nil, errors.New("unencrypted connection")
|
|
- }
|
|
+ // Must have TLS, or else localhost server.
|
|
+ // Note: If TLS is not true, then we can't trust ANYTHING in ServerInfo.
|
|
+ // In particular, it doesn't matter if the server advertises PLAIN auth.
|
|
+ // That might just be the attacker saying
|
|
+ // "it's ok, you can trust me with your password."
|
|
+ if !server.TLS && !isLocalhost(server.Name) {
|
|
+ return "", nil, errors.New("unencrypted connection")
|
|
}
|
|
if server.Name != a.host {
|
|
return "", nil, errors.New("wrong host name")
|
|
diff --git a/src/net/smtp/smtp_test.go b/src/net/smtp/smtp_test.go
|
|
index c48fae6d5ac..15eaca524be 100644
|
|
--- a/src/net/smtp/smtp_test.go
|
|
+++ b/src/net/smtp/smtp_test.go
|
|
@@ -60,29 +60,41 @@ testLoop:
|
|
}
|
|
|
|
func TestAuthPlain(t *testing.T) {
|
|
- auth := PlainAuth("foo", "bar", "baz", "servername")
|
|
|
|
tests := []struct {
|
|
- server *ServerInfo
|
|
- err string
|
|
+ authName string
|
|
+ server *ServerInfo
|
|
+ err string
|
|
}{
|
|
{
|
|
- server: &ServerInfo{Name: "servername", TLS: true},
|
|
+ authName: "servername",
|
|
+ server: &ServerInfo{Name: "servername", TLS: true},
|
|
},
|
|
{
|
|
- // Okay; explicitly advertised by server.
|
|
- server: &ServerInfo{Name: "servername", Auth: []string{"PLAIN"}},
|
|
+ // OK to use PlainAuth on localhost without TLS
|
|
+ authName: "localhost",
|
|
+ server: &ServerInfo{Name: "localhost", TLS: false},
|
|
},
|
|
{
|
|
- server: &ServerInfo{Name: "servername", Auth: []string{"CRAM-MD5"}},
|
|
- err: "unencrypted connection",
|
|
+ // NOT OK on non-localhost, even if server says PLAIN is OK.
|
|
+ // (We don't know that the server is the real server.)
|
|
+ authName: "servername",
|
|
+ server: &ServerInfo{Name: "servername", Auth: []string{"PLAIN"}},
|
|
+ err: "unencrypted connection",
|
|
},
|
|
{
|
|
- server: &ServerInfo{Name: "attacker", TLS: true},
|
|
- err: "wrong host name",
|
|
+ authName: "servername",
|
|
+ server: &ServerInfo{Name: "servername", Auth: []string{"CRAM-MD5"}},
|
|
+ err: "unencrypted connection",
|
|
+ },
|
|
+ {
|
|
+ authName: "servername",
|
|
+ server: &ServerInfo{Name: "attacker", TLS: true},
|
|
+ err: "wrong host name",
|
|
},
|
|
}
|
|
for i, tt := range tests {
|
|
+ auth := PlainAuth("foo", "bar", "baz", tt.authName)
|
|
_, _, err := auth.Start(tt.server)
|
|
got := ""
|
|
if err != nil {
|