]> Cypherpunks repositories - gostls13.git/commitdiff
cmd/go: refine GOAUTH user parsing to be more strict
authorSam Thanawalla <samthanawalla@google.com>
Tue, 28 Jan 2025 16:13:52 +0000 (16:13 +0000)
committerGopher Robot <gobot@golang.org>
Thu, 30 Jan 2025 21:39:38 +0000 (13:39 -0800)
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
src/cmd/go/internal/auth/httputils.go [new file with mode: 0644]
src/cmd/go/internal/auth/userauth.go
src/cmd/go/internal/auth/userauth_test.go
src/cmd/go/internal/help/helpdoc.go

index 830bac2b2f896571ffc670678acd432056d51faa..2220863b8edce482342bc2a568be16c82ee90cd9 100644 (file)
 //             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 (file)
index 0000000..b862954
--- /dev/null
@@ -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
+}
index 0e54a83e31873143bbe5dcdfa51acca0967f0901..1a60693a9cd9409c2b22c6c9e7053f99a04c53a4 100644 (file)
@@ -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())
index 91a5bb76ecd3bc1f72239d6f9d21559058e08c00..1b281ed3cdef45d2a9fb5c20cce2b935944d649e 100644 (file)
@@ -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)
        }
index 23459ef15413e377438a56cfc1dde2b51b502a1d..ccc04c25d27d7e560a33960eb09c686d54d2b145 100644 (file)
@@ -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.