[net] internal/http3: modify server to reject invalid headers

[Nicholas Husin (Gerrit)]

Nicholas Husin submitted the change with unreviewed changes

Unreviewed changes

1 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:

```
The name of the file: internal/http3/server_test.go
Insertions: 25, Deletions: 25.

@@ -99,28 +99,28 @@
 		header http.Header
 	}{
 		{
-			name: "invalid header name",
-			header: requestHeader(http.Header{
-				"Name\nEvilInjection": {"Value"},
-			}),
+			name:   "header name with control character",
+			header: http.Header{"name\nevilinjection": {"Value"}},
 		},
 		{
-			name: "invalid pseudo-header name",
-			header: requestHeader(http.Header{
-				":path\nEvilInjection": {"Value"},
-			}),
+			name:   "header name with uppercase character",
+			header: http.Header{"nAme": {"Value"}},
 		},
 		{
-			name: "invalid header value",
-			header: requestHeader(http.Header{
-				"Name": {"Value\nEvilInjection"},
-			}),
+			name:   "pseudo-header name with control character",
+			header: http.Header{":path\nevilinjection": {"Value"}},
 		},
 		{
-			name: "invalid pseudo-header value",
-			header: requestHeader(http.Header{
-				":method": {"Value\nEvilInjection"},
-			}),
+			name:   "pseudo-header name with uppercase character",
+			header: http.Header{":meThod": {"Value"}},
+		},
+		{
+			name:   "header value with control character",
+			header: http.Header{"name": {"Value\nEvilInjection"}},
+		},
+		{
+			name:   "pseudo-header value with control character",
+			header: http.Header{":method": {"Value\nEvilInjection"}},
 		},
 	}
 	for _, tt := range tests {
@@ -130,7 +130,7 @@
 			tc.greet()
 
 			reqStream := tc.newStream(streamTypeRequest)
-			reqStream.writeHeaders(tt.header)
+			reqStream.writeHeaders(requestHeader(tt.header))
 			reqStream.wantError(quic.StreamErrorCode(errH3MessageError))
 		})
 	}
@@ -498,7 +498,7 @@
 		// Client sends an Expect: 100-continue request.
 		reqStream := tc.newStream(streamTypeRequest)
 		reqStream.writeHeaders(requestHeader(http.Header{
-			"Expect": {"100-continue"},
+			"expect": {"100-continue"},
 		}))
 
 		reqStream.wantIdle("stream is idle until server sends an HTTP 100 status")
@@ -530,7 +530,7 @@
 		// Client sends an Expect: 100-continue request.
 		reqStream := tc.newStream(streamTypeRequest)
 		reqStream.writeHeaders(requestHeader(http.Header{
-			"Expect": {"100-continue"},
+			"expect": {"100-continue"},
 		}))
 
 		// Server rejects it.
@@ -555,7 +555,7 @@
 		// Client sends an Expect: 100-continue request.
 		reqStream := tc.newStream(streamTypeRequest)
 		reqStream.writeHeaders(requestHeader(http.Header{
-			"Expect": {"100-continue"},
+			"expect": {"100-continue"},
 		}))
 		// Client sends a body prematurely. This should not happen, unless a
 		// client misbehaves. We do so here anyways so the server handler can
@@ -595,7 +595,7 @@
 		// client indicates a Content-Length of 0.
 		reqStream = tc.newStream(streamTypeRequest)
 		reqStream.writeHeaders(requestHeader(http.Header{
-			"Content-Length": {"0"},
+			"content-length": {"0"},
 		}))
 		reqStream.wantSomeHeaders(http.Header{":status": {"200"}})
 		reqStream.wantData(serverBody)
@@ -631,7 +631,7 @@
 
 		reqStream := tc.newStream(streamTypeRequest)
 		reqStream.writeHeaders(requestHeader(http.Header{
-			"Trailer": {"Client-Trailer-A, Client-Trailer-B"},
+			"trailer": {"Client-Trailer-A, Client-Trailer-B"},
 		}))
 		reqStream.writeData(body)
 		reqStream.writeHeaders(http.Header{
@@ -671,8 +671,8 @@
 
 		reqStream := tc.newStream(streamTypeRequest)
 		reqStream.writeHeaders(requestHeader(http.Header{
-			"Trailer":        {"Client-Trailer-A, Client-Trailer-B"},
-			"Content-Length": {"0"},
+			"trailer":        {"Client-Trailer-A, Client-Trailer-B"},
+			"content-length": {"0"},
 		}))
 		reqStream.writeHeaders(http.Header{
 			"Client-Trailer-A":   {"valuea"},
@@ -863,7 +863,7 @@
 
 			if tt.does100Continue {
 				reqStream.writeHeaders(requestHeader(http.Header{
-					"Expect": {"100-continue"},
+					"expect": {"100-continue"},
 				}))
 				reqStream.wantIdle("stream is idle until server sends an HTTP 100 status")
 				streamIdle <- true
```
```
The name of the file: internal/http3/server.go
Insertions: 27, Deletions: 9.

@@ -324,6 +324,29 @@
 	}
 }
 
+// validWireHeaderFieldName reports whether v is a valid header field
+// name (key). See httpguts.ValidHeaderName for the base rules.
+//
+// Further, http3 says:
+// "A request or response containing uppercase characters in field names MUST
+// be treated as malformed."
+//
+// This function does not validate whether a pseudo-header field name is valid.
+func validWireHeaderFieldName(v string) bool {
+	if len(v) == 0 {
+		return false
+	}
+	for _, r := range v {
+		if !httpguts.IsTokenRune(r) {
+			return false
+		}
+		if 'A' <= r && r <= 'Z' {
+			return false
+		}
+	}
+	return true
+}
+
 type pseudoHeader struct {
 	method    string
 	scheme    string
@@ -332,24 +355,19 @@
 }
 
 func (sc *serverConn) parseHeader(st *stream) (http.Header, pseudoHeader, error) {
-	invalidHeaderErr := &streamError{
-		code:    errH3MessageError,
-		message: "invalid header field",
-	}
-
 	ftype, err := st.readFrameHeader()
 	if err != nil {
 		return nil, pseudoHeader{}, err
 	}
 	if ftype != frameTypeHeaders {
-		return nil, pseudoHeader{}, err
+		return nil, pseudoHeader{}, &streamError{errH3MessageError, "received other frames when expecting HEADERS"}
 	}
 	header := make(http.Header)
 	var pHeader pseudoHeader
 	var dec qpackDecoder
 	if err := dec.decode(st, func(_ indexType, name, value string) error {
 		if !httpguts.ValidHeaderFieldValue(value) {
-			return invalidHeaderErr
+			return &streamError{errH3MessageError, "invalid field value"}
 		}
 		switch name {
 		case ":method":
@@ -361,8 +379,8 @@
 		case ":authority":
 			pHeader.authority = value
 		default:
-			if !httpguts.ValidHeaderFieldName(name) {
-				return invalidHeaderErr
+			if !validWireHeaderFieldName(name) {
+				return &streamError{errH3MessageError, "invalid field name"}
 			}
 			header.Add(name, value)
 		}
```

Change information

Commit message:

internal/http3: modify server to reject headers containing invalid characters

When a header contains an invalid character, treat it as a stream error
of type H3_MESSAGE_ERROR per RFC 9114. This is done so control
characters cannot be used to inject new headers by a malicious client,
as an example.

For golang/go#70914

Change-Id: I43a1e3db3fc0688055293e90f33bf3a46a6a6964

Reviewed-on: https://go-review.googlesource.com/c/net/+/771101

LUCI-TryBot-Result: golang...@luci-project-accounts.iam.gserviceaccount.com <golang...@luci-project-accounts.iam.gserviceaccount.com>

Reviewed-by: Nicholas Husin <hu...@google.com>

Reviewed-by: Damien Neil <dn...@google.com>

Files:

• M internal/http3/server.go
• M internal/http3/server_test.go

Change size: M

Delta: 2 files changed, 81 insertions(+), 9 deletions(-)

Branch: refs/heads/master

Submit Requirements:

• Code-Review: +1 by Nicholas Husin, +2 by Damien Neil
• TryBots-Pass: LUCI-TryBot-Result+1 by golang...@luci-project-accounts.iam.gserviceaccount.com

Open in Gerrit

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: merged

Gerrit-Project: net

Gerrit-Branch: master

Gerrit-Change-Id: I43a1e3db3fc0688055293e90f33bf3a46a6a6964

Gerrit-Change-Number: 771101

Gerrit-PatchSet: 3

Gerrit-Owner: Nicholas Husin <n...@golang.org>

Gerrit-Reviewer: Damien Neil <dn...@google.com>

Gerrit-Reviewer: Nicholas Husin <hu...@google.com>

Gerrit-Reviewer: Nicholas Husin <n...@golang.org>

Gerrit-Reviewer: golang...@luci-project-accounts.iam.gserviceaccount.com <golang...@luci-project-accounts.iam.gserviceaccount.com>