[go] net/textproto: reject invalid header keys/values in ReadMIMEHeader
115 views
Skip to first unread message
Damien Neil (Gerrit)
Jun 6, 2022, 4:57:36 PM6/6/22
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Damien Neil has uploaded this change for review.
net/textproto: reject invalid header keys/values in ReadMIMEHeader
Return an error when parsing a MIME header containing bytes in the
key or value outside the set allowed by RFC 7230.
For historical compatibility, accept spaces in keys (but do not
canonicalize the key in this case).
For #53188.
Change-Id: I195319362a2fc69c4e506644f78c5026db070379
---
M src/net/http/serve_test.go
M src/net/textproto/reader.go
M src/net/textproto/reader_test.go
3 files changed, 142 insertions(+), 96 deletions(-)
diff --git a/src/net/http/serve_test.go b/src/net/http/serve_test.go
index cb6312d..7c03c41 100644
--- a/src/net/http/serve_test.go
+++ b/src/net/http/serve_test.go
@@ -6245,7 +6245,7 @@
"fugazi",
"foo-bar",
"unknown",
- "\rchunked",
+ `" chunked"`,
}
for _, badTE := range unsupportedTEs {
diff --git a/src/net/textproto/reader.go b/src/net/textproto/reader.go
index 1f7afc5..8cf984a 100644
--- a/src/net/textproto/reader.go
+++ b/src/net/textproto/reader.go
@@ -512,7 +512,15 @@
if !ok {
return m, ProtocolError("malformed MIME header line: " + string(kv))
}
- key := canonicalMIMEHeaderKey(k)
+ key, ok := canonicalMIMEHeaderKey(k)
+ if !ok {
+ return m, ProtocolError("malformed MIME header line: " + string(kv))
+ }
+ for _, c := range v {
+ if !validHeaderValueByte(c) {
+ return m, ProtocolError("malformed MIME header line: " + string(kv))
+ }
+ }
// As per RFC 7230 field-name is a token, tokens consist of one or more chars.
// We could return a ProtocolError here, but better to be liberal in what we
@@ -589,10 +597,12 @@
return s
}
if upper && 'a' <= c && c <= 'z' {
- return canonicalMIMEHeaderKey([]byte(s))
+ s, _ = canonicalMIMEHeaderKey([]byte(s))
+ return s
}
if !upper && 'A' <= c && c <= 'Z' {
- return canonicalMIMEHeaderKey([]byte(s))
+ s, _ = canonicalMIMEHeaderKey([]byte(s))
+ return s
}
upper = c == '-'
}
@@ -601,7 +611,7 @@
const toLower = 'a' - 'A'
-// validHeaderFieldByte reports whether b is a valid byte in a header
+// validHeaderFieldByte reports whether c is a valid byte in a header
// field name. RFC 7230 says:
//
// header-field = field-name ":" OWS field-value OWS
@@ -609,8 +619,58 @@
// tchar = "!" / "#" / "$" / "%" / "&" / "'" / "*" / "+" / "-" / "." /
// "^" / "_" / "`" / "|" / "~" / DIGIT / ALPHA
// token = 1*tchar
-func validHeaderFieldByte(b byte) bool {
- return int(b) < len(isTokenTable) && isTokenTable[b]
+func validHeaderFieldByte(c byte) bool {
+ // mask is a 128-bit bitmap with 1s for allowed bytes,
+ // so that the byte c can be tested with a shift and an and.
+ // If c >= 128, then 1<<c and 1<<(c-64) will both be zero,
+ // and this function will return false.
+ const mask = 0 |
+ (1<<(10)-1)<<'0' |
+ (1<<(26)-1)<<'a' |
+ (1<<(26)-1)<<'A' |
+ 1<<'!' |
+ 1<<'#' |
+ 1<<'$' |
+ 1<<'%' |
+ 1<<'&' |
+ 1<<'\'' |
+ 1<<'*' |
+ 1<<'+' |
+ 1<<'-' |
+ 1<<'.' |
+ 1<<'^' |
+ 1<<'_' |
+ 1<<'`' |
+ 1<<'|' |
+ 1<<'~'
+ return ((uint64(1)<<c)&(mask&(1<<64-1)) |
+ (uint64(1)<<(c-64))&(mask>>64)) != 0
+}
+
+// validHeaderValueByte reports whether c is a valid byte in a header
+// field value. RFC 7230 says:
+//
+// field-content = field-vchar [ 1*( SP / HTAB ) field-vchar ]
+// field-vchar = VCHAR / obs-text
+// obs-text = %x80-FF
+//
+// RFC 5234 says:
+//
+// HTAB = %x09
+// SP = %x20
+// VCHAR = %x21-7E
+func validHeaderValueByte(c byte) bool {
+ // mask is a 128-bit bitmap with 1s for allowed bytes,
+ // so that the byte c can be tested with a shift and an and.
+ // If c >= 128, then 1<<c and 1<<(c-64) will both be zero.
+ // Since this is the obs-text range, we invert the mask to
+ // create a bitmap with 1s for disallowed bytes.
+ const mask = 0 |
+ (1<<(0x7f-0x21)-1)<<0x21 | // VCHAR: %x21-7E
+ 1<<0x20 | // SP: %x20
+ 1<<0x09 // HTAB: %x09
+ return ((uint64(1)<<c)&^(mask&(1<<64-1)) |
+ (uint64(1)<<(c-64))&^(mask>>64)) == 0
}
// canonicalMIMEHeaderKey is like CanonicalMIMEHeaderKey but is
@@ -619,14 +679,20 @@
//
// For invalid inputs (if a contains spaces or non-token bytes), a
// is unchanged and a string copy is returned.
-func canonicalMIMEHeaderKey(a []byte) string {
+func canonicalMIMEHeaderKey(a []byte) (_ string, ok bool) {
// See if a looks like a header key. If not, return it unchanged.
for _, c := range a {
if validHeaderFieldByte(c) {
continue
}
// Don't canonicalize.
- return string(a)
+ if c == ' ' {
+ // We accept invalid headers with a space before the
+ // colon, but must not canonicalize them.
+ // See https://go.dev/issue/34540.
+ ok = true
+ }
+ return string(a), ok
}
upper := true
@@ -648,9 +714,9 @@
// case, so a copy of a's bytes into a new string does not
// happen in this map lookup:
if v := commonHeader[string(a)]; v != "" {
- return v
+ return v, true
}
- return string(a)
+ return string(a), true
}
// commonHeader interns common header strings.
@@ -704,85 +770,3 @@
commonHeader[v] = v
}
}
-
-// isTokenTable is a copy of net/http/lex.go's isTokenTable.
-// See https://httpwg.github.io/specs/rfc7230.html#rule.token.separators
-var isTokenTable = [127]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,
-}
diff --git a/src/net/textproto/reader_test.go b/src/net/textproto/reader_test.go
index d11d40f..279d86b 100644
--- a/src/net/textproto/reader_test.go
+++ b/src/net/textproto/reader_test.go
@@ -190,15 +190,60 @@
"Foo\r\n\t: foo\r\n\r\n",
"Foo-\n\tBar",
}
-
for _, input := range inputs {
r := reader(input)
- if m, err := r.ReadMIMEHeader(); err == nil {
+ if m, err := r.ReadMIMEHeader(); err == nil || err == io.EOF {
t.Errorf("ReadMIMEHeader(%q) = %v, %v; want nil, err", input, m, err)
}
}
}
+func TestReadMIMEHeaderBytes(t *testing.T) {
+ for i := 0; i <= 0xff; i++ {
+ s := "Foo" + string(rune(i)) + "Bar: foo\r\n\r\n"
+ r := reader(s)
+ wantErr := true
+ switch {
+ case i >= '0' && i <= '9':
+ wantErr = false
+ case i >= 'a' && i <= 'z':
+ wantErr = false
+ case i >= 'A' && i <= 'Z':
+ wantErr = false
+ case i == '!' || i == '#' || i == '$' || i == '%' || i == '&' || i == '\'' || i == '*' || i == '+' || i == '-' || i == '.' || i == '^' || i == '_' || i == '`' || i == '|' || i == '~':
+ wantErr = false
+ case i == ':':
+ // Special case: "Foo:Bar: foo" is the header "Foo".
+ wantErr = false
+ case i == ' ':
+ wantErr = false
+ }
+ m, err := r.ReadMIMEHeader()
+ if err != nil != wantErr {
+ t.Errorf("ReadMIMEHeader(%q) = %v, %v; want error=%v", s, m, err, wantErr)
+ }
+ }
+ for i := 0; i <= 0xff; i++ {
+ s := "Foo: foo" + string(rune(i)) + "bar\r\n\r\n"
+ r := reader(s)
+ wantErr := true
+ switch {
+ case i >= 0x21 && i <= 0x7e:
+ wantErr = false
+ case i == ' ':
+ wantErr = false
+ case i == '\t':
+ wantErr = false
+ case i >= 0x80 && i <= 0xff:
+ wantErr = false
+ }
+ m, err := r.ReadMIMEHeader()
+ if err != nil != wantErr {
+ t.Errorf("ReadMIMEHeader(%q) = %v, %v; want error=%v", s, m, err, wantErr)
+ }
+ }
+}
+
// Test that continued lines are properly trimmed. Issue 11204.
func TestReadMIMEHeaderTrimContinued(t *testing.T) {
// In this header, \n and \r\n terminated lines are mixed on purpose.
@@ -317,7 +362,7 @@
b := []byte("content-Length")
want := "Content-Length"
n := testing.AllocsPerRun(200, func() {
- if x := canonicalMIMEHeaderKey(b); x != want {
+ if x, _ := canonicalMIMEHeaderKey(b); x != want {
t.Fatalf("canonicalMIMEHeaderKey(%q) = %q; want %q", b, x, want)
}
})
To view, visit change 410714. To unsubscribe, or for help writing mail filters, visit settings.
Damien Neil (Gerrit)
Jun 6, 2022, 4:58:19 PM6/6/22
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Patch set 2:Run-TryBot +1
Brad Fitzpatrick (Gerrit)
Jun 17, 2022, 12:17:37 PM6/17/22
to Damien Neil, goph...@pubsubhelper.golang.org, Brad Fitzpatrick, Gopher Robot, golang-co...@googlegroups.com
Attention is currently required from: Damien Neil.
1 comment:
File src/net/http/serve_test.go:
Patch Set #2, Line 6248:
`" chunked"`,did you mean to change this or add another unsupported one?
To view, visit change 410714. To unsubscribe, or for help writing mail filters, visit settings.
Brad Fitzpatrick (Gerrit)
Jun 17, 2022, 12:23:06 PM6/17/22
to Damien Neil, goph...@pubsubhelper.golang.org, Brad Fitzpatrick, Gopher Robot, golang-co...@googlegroups.com
Attention is currently required from: Damien Neil.
3 comments:
File src/net/http/serve_test.go:
Patch Set #2, Line 6248:
`" chunked"`,did you mean to change this or add another unsupported one?
Done
File src/net/textproto/reader.go:
// For invalid inputs (if a contains spaces or non-token bytes), a
// is unchanged and a string copy is returned.
func canonicalMIMEHeaderKey(a []byte) (_ string, ok bool) {
Document what ok means.
And does ok false affect the string result?
if c == ' ' {
// We accept invalid headers with a space before the
// colon, but must not canonicalize them.
// See https://go.dev/issue/34540.
ok = true
}
return string(a), okare there tests for this?
To view, visit change 410714. To unsubscribe, or for help writing mail filters, visit settings.
Ian Lance Taylor (Gerrit)
Jun 24, 2022, 5:25:24 PM6/24/22
to Damien Neil, goph...@pubsubhelper.golang.org, Ian Lance Taylor, Brad Fitzpatrick, Gopher Robot, golang-co...@googlegroups.com
Attention is currently required from: Damien Neil.
1 comment:
Patchset:
@dneil Do we still want this for 1.19?
Note that the CL has picked up a merge conflict.
Thanks.
To view, visit change 410714. To unsubscribe, or for help writing mail filters, visit settings.
Damien Neil (Gerrit)
Jun 30, 2022, 6:56:42 PM6/30/22
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention is currently required from: Damien Neil.
Damien Neil uploaded patch set #3 to this change.
The following approvals got outdated and were removed: Run-TryBot+1 by Damien Neil, TryBot-Result+1 by Gopher Robot
net/textproto: reject invalid header keys/values in ReadMIMEHeader
Return an error when parsing a MIME header containing bytes in the
key or value outside the set allowed by RFC 7230.
For historical compatibility, accept spaces in keys (but do not
canonicalize the key in this case).
For #53188.
Change-Id: I195319362a2fc69c4e506644f78c5026db070379
---
M src/net/http/serve_test.go
M src/net/textproto/reader.go
M src/net/textproto/reader_test.go
3 files changed, 152 insertions(+), 96 deletions(-)
To view, visit change 410714. To unsubscribe, or for help writing mail filters, visit settings.
Damien Neil (Gerrit)
Jun 30, 2022, 6:59:27 PM6/30/22
to goph...@pubsubhelper.golang.org, Ian Lance Taylor, Brad Fitzpatrick, Gopher Robot, golang-co...@googlegroups.com
Attention is currently required from: Brad Fitzpatrick, Ian Lance Taylor.
3 comments:
Patchset:
@dneil Do we still want this for 1.19? […]
This one can wait for 1.20; it's just adding some redundant validation for general hardening.
File src/net/textproto/reader.go:
// For invalid inputs (if a contains spaces or non-token bytes), a
// is unchanged and a string copy is returned.
func canonicalMIMEHeaderKey(a []byte) (_ string, ok bool) {
Document what ok means. […]
Done
if c == ' ' {
// We accept invalid headers with a space before the
// colon, but must not canonicalize them.
// See https://go.dev/issue/34540.
ok = true
}
return string(a), okare there tests for this?
Yes, TestReadMIMEHeaderBytes and other tests for ReadMIMEHeader.
In particular, TestReadMIMEHeaderBytes verifies that we reject header keys with invalid characters other than ' '.
To view, visit change 410714. To unsubscribe, or for help writing mail filters, visit settings.
Brad Fitzpatrick (Gerrit)
Nov 4, 2022, 12:27:48 PM11/4/22
to Damien Neil, goph...@pubsubhelper.golang.org, Brad Fitzpatrick, Gopher Robot, Ian Lance Taylor, golang-co...@googlegroups.com
Attention is currently required from: Damien Neil, Ian Lance Taylor.
Patch set 3:Code-Review +2
Bryan Mills (Gerrit)
Nov 4, 2022, 1:16:10 PM11/4/22
to Damien Neil, goph...@pubsubhelper.golang.org, Bryan Mills, Brad Fitzpatrick, Gopher Robot, Ian Lance Taylor, golang-co...@googlegroups.com
Attention is currently required from: Damien Neil, Ian Lance Taylor.
Patch set 3:Code-Review +1
Reply all
Reply to author
Forward
0 new messages