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 <[email protected]> Reviewed-by: Damien Neil <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]>
golang · Jan 30, 2025 · ce7ea0a · ce7ea0a
1 parent e81f715
commit ce7ea0a
Show file tree

Hide file tree

Showing 5 changed files with 266 additions and 60 deletions.
diff --git a/src/cmd/go/alldocs.go b/src/cmd/go/alldocs.go
diff --git a/src/cmd/go/internal/auth/httputils.go b/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
+}
diff --git a/src/cmd/go/internal/auth/userauth.go b/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 {
-		// Return the processed credentials if the reader is at EOF.
-		if _, err := reader.R.Peek(1); err == io.EOF {
-			return credentials, nil
+	for data != "" {
+		var line string
+		var ok bool
+		var urls []string
+		// 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)
-		if err != nil {
-			return nil, err
-		}
-		if len(urls) == 0 {
-			return nil, fmt.Errorf("invalid format: expected url prefix")
-		}
-		mimeHeader, err := reader.ReadMIMEHeader()
-		if err != nil {
-			return nil, err
-		}
-		header := http.Header(mimeHeader)
-		// Process the block (urls and headers).
-		credentialMap := mapHeadersToPrefixes(urls, header)
-		maps.Copy(credentials, credentialMap)
-	}
-}
-
-// 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")
+		// Parse Headers second.
+		header := make(http.Header)
+		for {
+			line, data, ok = strings.Cut(data, "\n")
+			if !ok {
+				return nil, fmt.Errorf("invalid format: missing empty line after headers")
+			}
+			if line == "" {
+				break
+			}
+			name, value, ok := strings.Cut(line, ": ")
+			value = strings.TrimSpace(value)
+			if !ok || !validHeaderFieldName(name) || !validHeaderFieldValue(value) {
+				return nil, fmt.Errorf("invalid format: invalid header line")
+			}
+			header.Add(name, value)
 		}
+		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 {
-		return err
+	for k, v := range res.Header {
+		output.WriteString(k + ": " + strings.Join(v, ", ") + "\n")
 	}
 	output.WriteString("\n")
 	cmd.Stdin = strings.NewReader(output.String())