From: Sam Thanawalla Date: Tue, 28 Jan 2025 16:13:52 +0000 (+0000) Subject: cmd/go: refine GOAUTH user parsing to be more strict X-Git-Tag: go1.24rc3~2^2~2 X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=ce7ea0a6a529ce91327900e29afc3abd620030b4;p=gostls13.git 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 Reviewed-by: Damien Neil LUCI-TryBot-Result: Go LUCI --- diff --git a/src/cmd/go/alldocs.go b/src/cmd/go/alldocs.go index 830bac2b2f..2220863b8e 100644 --- a/src/cmd/go/alldocs.go +++ b/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 -// as an HTTP response. +// Note: it is safe to use net/http.ReadResponse to parse this input. // // Before the first HTTPS fetch, the go command will invoke each GOAUTH // command in the list with no additional arguments and no input. diff --git a/src/cmd/go/internal/auth/httputils.go b/src/cmd/go/internal/auth/httputils.go new file mode 100644 index 0000000000..b8629546d5 --- /dev/null +++ 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 = +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 = +// +// http://www.w3.org/Protocols/rfc2616/rfc2616-sec2.html#sec2.2 : +// +// TEXT = +// LWS = [CRLF] 1*( SP | HT ) +// CTL = +// +// 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 index 0e54a83e31..1a60693a9c 100644 --- 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()) diff --git a/src/cmd/go/internal/auth/userauth_test.go b/src/cmd/go/internal/auth/userauth_test.go index 91a5bb76ec..1b281ed3cd 100644 --- a/src/cmd/go/internal/auth/userauth_test.go +++ b/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) } diff --git a/src/cmd/go/internal/help/helpdoc.go b/src/cmd/go/internal/help/helpdoc.go index 23459ef154..ccc04c25d2 100644 --- a/src/cmd/go/internal/help/helpdoc.go +++ b/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 - as an HTTP response. + Note: it is safe to use net/http.ReadResponse to parse this input. Before the first HTTPS fetch, the go command will invoke each GOAUTH command in the list with no additional arguments and no input.