cmd/go: refine GOAUTH user parsing to be more strict

This CL enhances the parsing of GOAUTH user based authentication for
improved security.

Updates: #26232

Cq-Include-Trybots: luci.golang.try:gotip-linux-amd64-longtest,gotip-windows-amd64-longtest
Change-Id: Ica57952924020b7bd2670610af8de8ce52dbe92f
Reviewed-on: https://go-review.googlesource.com/c/go/+/644995
Auto-Submit: Sam Thanawalla <samthanawalla@google.com>
Reviewed-by: Damien Neil <dneil@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

src/cmd/go/alldocs.go

@@ -2632,8 +2632,7 @@
 //		Content-Type: text/plain; charset=utf-8
 //		Date: Thu, 07 Nov 2024 18:43:09 GMT
 //
-//	Note: at least for HTTP 1.1, the contents written to stdin can be parsed
+//	Note: it is safe to use net/http.ReadResponse to parse this input.
-//	as an HTTP response.
 //
 // Before the first HTTPS fetch, the go command will invoke each GOAUTH
 // command in the list with no additional arguments and no input.

src/cmd/go/internal/auth/httputils.go

@@ -0,0 +1,173 @@
+// Copyright 2019 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// Code copied from x/net/http/httpguts/httplex.go
+package auth
+
+var isTokenTable = [256]bool{
+	'!':  true,
+	'#':  true,
+	'$':  true,
+	'%':  true,
+	'&':  true,
+	'\'': true,
+	'*':  true,
+	'+':  true,
+	'-':  true,
+	'.':  true,
+	'0':  true,
+	'1':  true,
+	'2':  true,
+	'3':  true,
+	'4':  true,
+	'5':  true,
+	'6':  true,
+	'7':  true,
+	'8':  true,
+	'9':  true,
+	'A':  true,
+	'B':  true,
+	'C':  true,
+	'D':  true,
+	'E':  true,
+	'F':  true,
+	'G':  true,
+	'H':  true,
+	'I':  true,
+	'J':  true,
+	'K':  true,
+	'L':  true,
+	'M':  true,
+	'N':  true,
+	'O':  true,
+	'P':  true,
+	'Q':  true,
+	'R':  true,
+	'S':  true,
+	'T':  true,
+	'U':  true,
+	'W':  true,
+	'V':  true,
+	'X':  true,
+	'Y':  true,
+	'Z':  true,
+	'^':  true,
+	'_':  true,
+	'`':  true,
+	'a':  true,
+	'b':  true,
+	'c':  true,
+	'd':  true,
+	'e':  true,
+	'f':  true,
+	'g':  true,
+	'h':  true,
+	'i':  true,
+	'j':  true,
+	'k':  true,
+	'l':  true,
+	'm':  true,
+	'n':  true,
+	'o':  true,
+	'p':  true,
+	'q':  true,
+	'r':  true,
+	's':  true,
+	't':  true,
+	'u':  true,
+	'v':  true,
+	'w':  true,
+	'x':  true,
+	'y':  true,
+	'z':  true,
+	'|':  true,
+	'~':  true,
+}
+
+// isLWS reports whether b is linear white space, according
+// to http://www.w3.org/Protocols/rfc2616/rfc2616-sec2.html#sec2.2
+//
+//	LWS            = [CRLF] 1*( SP | HT )
+func isLWS(b byte) bool { return b == ' ' || b == '\t' }
+
+// isCTL reports whether b is a control byte, according
+// to http://www.w3.org/Protocols/rfc2616/rfc2616-sec2.html#sec2.2
+//
+//	CTL            = <any US-ASCII control character
+//	                 (octets 0 - 31) and DEL (127)>
+func isCTL(b byte) bool {
+	const del = 0x7f // a CTL
+	return b < ' ' || b == del
+}
+
+// validHeaderFieldName reports whether v is a valid HTTP/1.x header name.
+// HTTP/2 imposes the additional restriction that uppercase ASCII
+// letters are not allowed.
+//
+// RFC 7230 says:
+//
+//	header-field   = field-name ":" OWS field-value OWS
+//	field-name     = token
+//	token          = 1*tchar
+//	tchar = "!" / "#" / "$" / "%" / "&" / "'" / "*" / "+" / "-" / "." /
+//	        "^" / "_" / "`" / "|" / "~" / DIGIT / ALPHA
+func validHeaderFieldName(v string) bool {
+	if len(v) == 0 {
+		return false
+	}
+	for i := 0; i < len(v); i++ {
+		if !isTokenTable[v[i]] {
+			return false
+		}
+	}
+	return true
+}
+
+// validHeaderFieldValue reports whether v is a valid "field-value" according to
+// http://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.2 :
+//
+//	message-header = field-name ":" [ field-value ]
+//	field-value    = *( field-content | LWS )
+//	field-content  = <the OCTETs making up the field-value
+//	                 and consisting of either *TEXT or combinations
+//	                 of token, separators, and quoted-string>
+//
+// http://www.w3.org/Protocols/rfc2616/rfc2616-sec2.html#sec2.2 :
+//
+//	TEXT           = <any OCTET except CTLs,
+//	                  but including LWS>
+//	LWS            = [CRLF] 1*( SP | HT )
+//	CTL            = <any US-ASCII control character
+//	                 (octets 0 - 31) and DEL (127)>
+//
+// RFC 7230 says:
+//
+//	field-value    = *( field-content / obs-fold )
+//	obj-fold       =  N/A to http2, and deprecated
+//	field-content  = field-vchar [ 1*( SP / HTAB ) field-vchar ]
+//	field-vchar    = VCHAR / obs-text
+//	obs-text       = %x80-FF
+//	VCHAR          = "any visible [USASCII] character"
+//
+// http2 further says: "Similarly, HTTP/2 allows header field values
+// that are not valid. While most of the values that can be encoded
+// will not alter header field parsing, carriage return (CR, ASCII
+// 0xd), line feed (LF, ASCII 0xa), and the zero character (NUL, ASCII
+// 0x0) might be exploited by an attacker if they are translated
+// verbatim. Any request or response that contains a character not
+// permitted in a header field value MUST be treated as malformed
+// (Section 8.1.2.6). Valid characters are defined by the
+// field-content ABNF rule in Section 3.2 of [RFC7230]."
+//
+// This function does not (yet?) properly handle the rejection of
+// strings that begin or end with SP or HTAB.
+func validHeaderFieldValue(v string) bool {
+	for i := 0; i < len(v); i++ {
+		b := v[i]
+		if isCTL(b) && !isLWS(b) {
+			return false
+		}
+	}
+	return true
+}

src/cmd/go/internal/auth/userauth.go

@@ -6,14 +6,11 @@
 package auth
 
 import (
-	"bufio"
-	"bytes"
 	"cmd/internal/quoted"
 	"fmt"
-	"io"
 	"maps"
 	"net/http"
-	"net/textproto"
+	"net/url"
 	"os/exec"
 	"strings"
 )
@@ -42,7 +39,7 @@ func runAuthCommand(command string, url string, res *http.Response) (map[string]
 	if err != nil {
 		return nil, fmt.Errorf("could not run command %s: %v\n%s", command, err, cmd.Stderr)
 	}
-	credentials, err := parseUserAuth(bytes.NewReader(out))
+	credentials, err := parseUserAuth(string(out))
 	if err != nil {
 		return nil, fmt.Errorf("cannot parse output of GOAUTH command %s: %v", command, err)
 	}
@@ -54,53 +51,47 @@ func runAuthCommand(command string, url string, res *http.Response) (map[string]
 // or an error if the data does not follow the expected format.
 // Returns an nil error and an empty map if the data is empty.
 // See the expected format in 'go help goauth'.
-func parseUserAuth(data io.Reader) (map[string]http.Header, error) {
+func parseUserAuth(data string) (map[string]http.Header, error) {
 	credentials := make(map[string]http.Header)
-	reader := textproto.NewReader(bufio.NewReader(data))
+	for data != "" {
-	for {
+		var line string
-		// Return the processed credentials if the reader is at EOF.
+		var ok bool
-		if _, err := reader.R.Peek(1); err == io.EOF {
+		var urls []string
-			return credentials, nil
+		// Parse URLS first.
+		for {
+			line, data, ok = strings.Cut(data, "\n")
+			if !ok {
+				return nil, fmt.Errorf("invalid format: missing empty line after URLs")
+			}
+			if line == "" {
+				break
+			}
+			u, err := url.ParseRequestURI(line)
+			if err != nil {
+				return nil, fmt.Errorf("could not parse URL %s: %v", line, err)
+			}
+			urls = append(urls, u.String())
 		}
-		urls, err := readURLs(reader)
+		// Parse Headers second.
-		if err != nil {
+		header := make(http.Header)
-			return nil, err
+		for {
-		}
+			line, data, ok = strings.Cut(data, "\n")
-		if len(urls) == 0 {
+			if !ok {
-			return nil, fmt.Errorf("invalid format: expected url prefix")
+				return nil, fmt.Errorf("invalid format: missing empty line after headers")
-		}
+			}
-		mimeHeader, err := reader.ReadMIMEHeader()
+			if line == "" {
-		if err != nil {
+				break
-			return nil, err
+			}
-		}
+			name, value, ok := strings.Cut(line, ": ")
-		header := http.Header(mimeHeader)
+			value = strings.TrimSpace(value)
-		// Process the block (urls and headers).
+			if !ok || !validHeaderFieldName(name) || !validHeaderFieldValue(value) {
-		credentialMap := mapHeadersToPrefixes(urls, header)
+				return nil, fmt.Errorf("invalid format: invalid header line")
-		maps.Copy(credentials, credentialMap)
+			}
-	}
+			header.Add(name, value)
-}
-
-// readURLs reads URL prefixes from the given reader until an empty line
-// is encountered or an error occurs. It returns the list of URLs or an error
-// if the format is invalid.
-func readURLs(reader *textproto.Reader) (urls []string, err error) {
-	for {
-		line, err := reader.ReadLine()
-		if err != nil {
-			return nil, err
-		}
-		trimmedLine := strings.TrimSpace(line)
-		if trimmedLine != line {
-			return nil, fmt.Errorf("invalid format: leading or trailing white space")
-		}
-		if strings.HasPrefix(line, "https://") {
-			urls = append(urls, line)
-		} else if line == "" {
-			return urls, nil
-		} else {
-			return nil, fmt.Errorf("invalid format: expected url prefix or empty line")
 		}
+		maps.Copy(credentials, mapHeadersToPrefixes(urls, header))
 	}
+	return credentials, nil
 }
 
 // mapHeadersToPrefixes returns a mapping of prefix → http.Header without
@@ -127,8 +118,8 @@ func buildCommand(command string) (*exec.Cmd, error) {
 func writeResponseToStdin(cmd *exec.Cmd, res *http.Response) error {
 	var output strings.Builder
 	output.WriteString(res.Proto + " " + res.Status + "\n")
-	if err := res.Header.Write(&output); err != nil {
+	for k, v := range res.Header {
-		return err
+		output.WriteString(k + ": " + strings.Join(v, ", ") + "\n")
 	}
 	output.WriteString("\n")
 	cmd.Stdin = strings.NewReader(output.String())

src/cmd/go/internal/auth/userauth_test.go

@@ -7,7 +7,6 @@ package auth
 import (
 	"net/http"
 	"reflect"
-	"strings"
 	"testing"
 )
 
@@ -40,7 +39,7 @@ Data: Test567
 			"Test567",
 		},
 	}
-	credentials, err := parseUserAuth(strings.NewReader(data))
+	credentials, err := parseUserAuth(data)
 	if err != nil {
 		t.Errorf("parseUserAuth(%s): %v", data, err)
 	}
@@ -100,10 +99,55 @@ Authorization: Basic GVuc2VzYW1lYWxhZGRpbjpvc
 Authorization: Basic 1lYWxhZGRplW1lYWxhZGRpbs
 Data: Test567
 
+`,
+		// Continuation in URL line
+		`https://example.com/
+ Authorization: Basic YWxhZGRpbjpvcGVuc2VzYW1l
+`,
+
+		// Continuation in header line
+		`https://example.com
+
+Authorization: Basic YWxhZGRpbjpvcGVuc2VzYW1l
+ Authorization: Basic jpvcGVuc2VzYW1lYWxhZGRpb
+`,
+
+		// Continuation in multiple header lines
+		`https://example.com
+
+Authorization: Basic YWxhZGRpbjpvcGVuc2VzYW1l
+ Authorization: Basic jpvcGVuc2VzYW1lYWxhZGRpb
+ Authorization: Basic dGhpc2lzYWxvbmdzdHJpbmc=
+`,
+
+		// Continuation with mixed spacing
+		`https://example.com
+
+Authorization: Basic YWxhZGRpbjpvcGVuc2VzYW1l
+  Authorization: Basic jpvcGVuc2VzYW1lYWxhZGRpb
+`,
+
+		// Continuation with tab character
+		`https://example.com
+
+Authorization: Basic YWxhZGRpbjpvcGVuc2VzYW1l
+        Authorization: Basic jpvcGVuc2VzYW1lYWxhZGRpb
+`,
+		// Continuation at the start of a block
+		` https://example.com
+
+Authorization: Basic YWxhZGRpbjpvcGVuc2VzYW1l
+`,
+
+		// Continuation after a blank line
+		`https://example.com
+
+
+Authorization: Basic YWxhZGRpbjpvcGVuc2VzYW1l
 `,
 	}
 	for _, tc := range testCases {
-		if credentials, err := parseUserAuth(strings.NewReader(tc)); err == nil {
+		if credentials, err := parseUserAuth(tc); err == nil {
 			t.Errorf("parseUserAuth(%s) should have failed, but got: %v", tc, credentials)
 		}
 	}
@@ -132,7 +176,7 @@ Data: Test567
 			"Test567",
 		},
 	}
-	credentials, err := parseUserAuth(strings.NewReader(data))
+	credentials, err := parseUserAuth(data)
 	if err != nil {
 		t.Errorf("parseUserAuth(%s): %v", data, err)
 	}
@@ -146,7 +190,7 @@ func TestParseUserAuthEmptyHeader(t *testing.T) {
 	data := "https://example.com\n\n\n"
 	// Build the expected header
 	header := http.Header{}
-	credentials, err := parseUserAuth(strings.NewReader(data))
+	credentials, err := parseUserAuth(data)
 	if err != nil {
 		t.Errorf("parseUserAuth(%s): %v", data, err)
 	}
@@ -159,7 +203,7 @@ func TestParseUserAuthEmptyHeader(t *testing.T) {
 func TestParseUserAuthEmpty(t *testing.T) {
 	data := ``
 	// Build the expected header
-	credentials, err := parseUserAuth(strings.NewReader(data))
+	credentials, err := parseUserAuth(data)
 	if err != nil {
 		t.Errorf("parseUserAuth(%s) should have succeeded", data)
 	}

src/cmd/go/internal/help/helpdoc.go

@@ -1050,8 +1050,7 @@ command
 		Content-Type: text/plain; charset=utf-8
 		Date: Thu, 07 Nov 2024 18:43:09 GMT
 
-	Note: at least for HTTP 1.1, the contents written to stdin can be parsed
+	Note: it is safe to use net/http.ReadResponse to parse this input.
-	as an HTTP response.
 
 Before the first HTTPS fetch, the go command will invoke each GOAUTH
 command in the list with no additional arguments and no input.