[go] net/http: validate structural validity of IP-literal Host headers
pawan kalyan (Gerrit)
pawan kalyan has uploaded the change for review![]( "Open in Gerrit")
Commit message
net/http: validate structural validity of IP-literal Host headers
The validHostHeader function used a lenient byte allowlist that accepted
structurally invalid IP-literals such as [], [12345], and [123g::1].
This change adds structural validation for IP-literals and ports.
Fixes #78172
Change diff
diff --git a/src/net/http/serve_test.go b/src/net/http/serve_test.go
index 9ad3fdf..ae282db 100644
--- a/src/net/http/serve_test.go
+++ b/src/net/http/serve_test.go
@@ -4977,6 +4977,18 @@
{"HTTP/1.0", "Host: first\r\nHost: second\r\n", 400},
{"HTTP/1.0", "Host: \xff\r\n", 400},
+ // IP-literal structural validation per RFC 3986 §3.2.2
+ {"HTTP/1.1", "Host: []\r\n", 400},
+ {"HTTP/1.1", "Host: [::1\r\n", 400},
+ {"HTTP/1.1", "Host: [12345]\r\n", 400},
+ {"HTTP/1.1", "Host: [123g::1]\r\n", 400},
+ {"HTTP/1.1", "Host: [1:2:3:4:5:6:7]\r\n", 400},
+ {"HTTP/1.1", "Host: [1:2:3:4:5:6:7:8:9]\r\n", 400},
+ {"HTTP/1.1", "Host: [v4.192.10.2.1]\r\n", 400},
+ {"HTTP/1.1", "Host: example.com::80\r\n", 400},
+ {"HTTP/1.1", "Host: example.com:80a\r\n", 400},
+ {"HTTP/1.1", "Host: example.com:99999\r\n", 400},
+
// Make an exception for HTTP upgrade requests:
{"PRI * HTTP/2.0", "", 200},
diff --git a/src/vendor/golang.org/x/net/http/httpguts/httplex.go b/src/vendor/golang.org/x/net/http/httpguts/httplex.go
index 9b4de94..15e5411 100644
--- a/src/vendor/golang.org/x/net/http/httpguts/httplex.go
+++ b/src/vendor/golang.org/x/net/http/httpguts/httplex.go
@@ -205,19 +205,61 @@
return true
}
-// ValidHostHeader reports whether h is a valid host header.
+// ValidHostHeader reports whether h is a valid Host header value.
+// It validates structural validity of IP-literals per RFC 3986 §3.2.2
+// in addition to the byte-level character set check.
func ValidHostHeader(h string) bool {
- // The latest spec is actually this:
- //
- // http://tools.ietf.org/html/rfc7230#section-5.4
- // Host = uri-host [ ":" port ]
- //
- // Where uri-host is:
- // http://tools.ietf.org/html/rfc3986#section-3.2.2
- //
- // But we're going to be much more lenient for now and just
- // search for any byte that's not a valid byte in any of those
- // expressions.
+ if strings.HasPrefix(h, "[") {
+ // IP-literal: must have a closing bracket.
+ closeBracket := strings.LastIndex(h, "]")
+ if closeBracket < 0 {
+ return false
+ }
+ ipLiteral := h[1:closeBracket]
+ if len(ipLiteral) == 0 {
+ return false
+ }
+ // IPvFuture starts with 'v'; reject it.
+ if ipLiteral[0] == 'v' || ipLiteral[0] == 'V' {
+ return false
+ }
+ // Strip zone identifier before parsing e.g. "::1%25en0" -> "::1".
+ ipStr := ipLiteral
+ if i := strings.Index(ipStr, "%"); i >= 0 {
+ ipStr = ipStr[:i]
+ }
+ if net.ParseIP(ipStr) == nil {
+ return false
+ }
+ // Optional port after the closing bracket.
+ rest := h[closeBracket+1:]
+ if rest == "" {
+ return true
+ }
+ if rest[0] != ':' {
+ return false
+ }
+ return validHostPort(rest[1:])
+ }
+
+ // Non-IP-literal: reject multiple colons unless it's a bare IPv6 address.
+ if strings.Count(h, ":") > 1 {
+ // Allow bare IPv6 addresses e.g. "::1".
+ if net.ParseIP(h) != nil {
+ return true
+ }
+ return false
+ }
+
+ // Validate port if present.
+ if idx := strings.LastIndex(h, ":"); idx >= 0 {
+ if !validHostPort(h[idx+1:]) {
+ return false
+ }
+ h = h[:idx]
+ }
+
+ // Check remaining bytes against the allowlist.
for i := 0; i < len(h); i++ {
if !validHostByte[h[i]] {
return false
@@ -226,6 +268,24 @@
return true
}
+// validHostPort reports whether port is a valid numeric port (0–65535).
+func validHostPort(port string) bool {
+ if port == "" {
+ return false
+ }
+ n := 0
+ for i := 0; i < len(port); i++ {
+ if port[i] < '0' || port[i] > '9' {
+ return false
+ }
+ n = n*10 + int(port[i]-'0')
+ if n > 65535 {
+ return false
+ }
+ }
+ return true
+}
+
// See the validHostHeader comment.
var validHostByte = [256]bool{
'0': true, '1': true, '2': true, '3': true, '4': true, '5': true, '6': true, '7': true,
Change information
- M src/net/http/serve_test.go
- M src/vendor/golang.org/x/net/http/httpguts/httplex.go
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gopher Robot (Gerrit)
Message from Gopher Robot![]( "Open in Gerrit")
Congratulations on opening your first change. Thank you for your contribution!
Next steps:
A maintainer will review your change and provide feedback. See
https://go.dev/doc/contribute#review for more info and tips to get your
patch through code review.
Most changes in the Go project go through a few rounds of revision. This can be
surprising to people new to the project. The careful, iterative review process
is our way of helping mentor contributors and ensuring that their contributions
have a lasting impact.
During May-July and Nov-Jan the Go project is in a code freeze, during which
little code gets reviewed or merged. If a reviewer responds with a comment like
R=go1.11 or adds a tag like "wait-release", it means that this CL will be
reviewed as part of the next development cycle. See https://go.dev/s/release
for more details.
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicholas Husin (Gerrit)
Nicholas Husin added 4 comments![]( "Open in Gerrit")
Thanks for the CL.
I left some preliminary comments from a quick look. That said, I wouldn't address it yet at this point.
As mentioned in the bug, whether this change is safe to make is still up in the air. So, I wouldn't exactly recommend investing a lot of time in this quite yet until we have more data (which I should be able to get by running the current CL against some tests). I'll keep you updated.
{"HTTP/1.1", "Host: example.com:99999\r\n", 400},Note that your current implementation will return a `400` when `Host` is empty, which is notably the opposite of what we want from the discussion in #77798.
This might be fine, we'll see. Probably worth putting as a test too though.
func ValidHostHeader(h string) bool {I don't think we want to simply add another layer of validation on top of the byte-level validation that already exists.
If we actually decide to do this, we should do it well in one go and implement the entire ABNF parsing logic.
// validHostPort reports whether port is a valid numeric port (0–65535).
func validHostPort(port string) bool {
if port == "" {
return false
}
n := 0
for i := 0; i < len(port); i++ {
if port[i] < '0' || port[i] > '9' {
return false
}
n = n*10 + int(port[i]-'0')
if n > 65535 {
return false
}
}
return true
}Just use `strconv.Atoi`?
Related details
- Damien Neil
- pawan kalyan
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
pawan kalyan (Gerrit)
pawan kalyan added 4 comments![]( "Open in Gerrit")
Thanks for the CL.
I left some preliminary comments from a quick look. That said, I wouldn't address it yet at this point.
As mentioned in the bug, whether this change is safe to make is still up in the air. So, I wouldn't exactly recommend investing a lot of time in this quite yet until we have more data (which I should be able to get by running the current CL against some tests). I'll keep you updated.
Thank you for the review. I'll wait for the internal test results before addressing the remaining comments.
{"HTTP/1.1", "Host: example.com:99999\r\n", 400},Note that your current implementation will return a `400` when `Host` is empty, which is notably the opposite of what we want from the discussion in #77798.
This might be fine, we'll see. Probably worth putting as a test too though.
The existing test case `{"HTTP/1.1", "Host: \r\n", 200}` already covers this and passes with the current implementation. Empty Host correctly returns 200.
func ValidHostHeader(h string) bool {I don't think we want to simply add another layer of validation on top of the byte-level validation that already exists.
If we actually decide to do this, we should do it well in one go and implement the entire ABNF parsing logic.
Understood, will address if the change is deemed safe to proceed.
// validHostPort reports whether port is a valid numeric port (0–65535).
func validHostPort(port string) bool {
if port == "" {
return false
}
n := 0
for i := 0; i < len(port); i++ {
if port[i] < '0' || port[i] > '9' {
return false
}
n = n*10 + int(port[i]-'0')
if n > 65535 {
return false
}
}
return true
}Just use `strconv.Atoi`?
- Nicholas Husin
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicholas Husin (Gerrit)
Nicholas Husin added 1 comment![]( "Open in Gerrit")
{"HTTP/1.1", "Host: example.com:99999\r\n", 400},pawan kalyanNote that your current implementation will return a `400` when `Host` is empty, which is notably the opposite of what we want from the discussion in #77798.
This might be fine, we'll see. Probably worth putting as a test too though.
The existing test case `{"HTTP/1.1", "Host: \r\n", 200}` already covers this and passes with the current implementation. Empty Host correctly returns 200.
Oops right, I think I messed up my buffer while testing your code, thanks.
In that case, the consideration would be: if we were to implement the strict ABNF parsing, should we allow empty `Host`? But, that's a problem for later.
Related details
- pawan kalyan
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
pawan kalyan (Gerrit)
pawan kalyan added 1 comment![]( "Open in Gerrit")
{"HTTP/1.1", "Host: example.com:99999\r\n", 400},pawan kalyanNote that your current implementation will return a `400` when `Host` is empty, which is notably the opposite of what we want from the discussion in #77798.
This might be fine, we'll see. Probably worth putting as a test too though.
Nicholas HusinThe existing test case `{"HTTP/1.1", "Host: \r\n", 200}` already covers this and passes with the current implementation. Empty Host correctly returns 200.
Oops right, I think I messed up my buffer while testing your code, thanks.
In that case, the consideration would be: if we were to implement the strict ABNF parsing, should we allow empty `Host`? But, that's a problem for later.
i think yes, empty Host should still be allowed if we implement strict ABNF parsing, since RFC 9112 3.2 says "a client MUST send an empty Host field when the authority component is missing". So empty Host is valid and should return 200, which is already covered by the existing test case.
Related details
- Nicholas Husin
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |