net/url, net/http: reject control characters in URLs · golang/go@829c5df (original) (raw)
6 files changed
lines changed
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -583,16 +583,23 @@ func TestFileServerZeroByte(t *testing.T) { | ||
583 | 583 | ts := httptest.NewServer(FileServer(Dir("."))) |
584 | 584 | defer ts.Close() |
585 | 585 | |
586 | -res, err := Get(ts.URL + "/..\x00") | |
586 | +c, err := net.Dial("tcp", ts.Listener.Addr().String()) | |
587 | 587 | if err != nil { |
588 | 588 | t.Fatal(err) |
589 | 589 | } |
590 | -b, err := ioutil.ReadAll(res.Body) | |
590 | +defer c.Close() | |
591 | +_, err = fmt.Fprintf(c, "GET /..\x00 HTTP/1.0\r\n\r\n") | |
592 | +if err != nil { | |
593 | +t.Fatal(err) | |
594 | + } | |
595 | +var got bytes.Buffer | |
596 | +bufr := bufio.NewReader(io.TeeReader(c, &got)) | |
597 | +res, err := ReadResponse(bufr, nil) | |
591 | 598 | if err != nil { |
592 | -t.Fatal("reading Body:", err) | |
599 | +t.Fatal("ReadResponse: ", err) | |
593 | 600 | } |
594 | 601 | if res.StatusCode == 200 { |
595 | -t.Errorf("got status 200; want an error. Body is:\n%s", string(b)) | |
602 | +t.Errorf("got status 200; want an error. Body is:\n%s", got.Bytes()) | |
596 | 603 | } |
597 | 604 | } |
598 | 605 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -59,6 +59,12 @@ func isASCII(s string) bool { | ||
59 | 59 | return true |
60 | 60 | } |
61 | 61 | |
62 | +// isCTL reports whether r is an ASCII control character, including | |
63 | +// the Extended ASCII control characters included in Unicode. | |
64 | +func isCTL(r rune) bool { | |
65 | +return r < ' ' | | |
66 | +} | |
67 | + | |
62 | 68 | func hexEscapeNonASCII(s string) string { |
63 | 69 | newLen := 0 |
64 | 70 | for i := 0; i < len(s); i++ { |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -550,7 +550,12 @@ func (r *Request) write(w io.Writer, usingProxy bool, extraHeaders Header, waitF | ||
550 | 550 | ruri = r.URL.Opaque |
551 | 551 | } |
552 | 552 | } |
553 | -// TODO(bradfitz): escape at least newlines in ruri? | |
553 | +if strings.IndexFunc(ruri, isCTL) != -1 { | |
554 | +return errors.New("net/http: can't write control character in Request.URL") | |
555 | + } | |
556 | +// TODO: validate r.Method too? At least it's less likely to | |
557 | +// come from an attacker (more likely to be a constant in | |
558 | +// code). | |
554 | 559 | |
555 | 560 | // Wrap the writer in a bufio Writer if it's not already buffered. |
556 | 561 | // Don't always call NewWriter, as that forces a bytes.Buffer |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -576,6 +576,17 @@ var reqWriteTests = []reqWriteTest{ | ||
576 | 576 | "User-Agent: Go-http-client/1.1\r\n" + |
577 | 577 | "X-Foo: X-Bar\r\n\r\n", |
578 | 578 | }, |
579 | + | |
580 | +25: { | |
581 | +Req: Request{ | |
582 | +Method: "GET", | |
583 | +URL: &url.URL{ | |
584 | +Host: "www.example.com", | |
585 | +RawQuery: "new\nline", // or any CTL | |
586 | + }, | |
587 | + }, | |
588 | +WantError: errors.New("net/http: can't write control character in Request.URL"), | |
589 | + }, | |
579 | 590 | } |
580 | 591 | |
581 | 592 | func TestRequestWrite(t *testing.T) { |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -513,6 +513,10 @@ func parse(rawurl string, viaRequest bool) (*URL, error) { | ||
513 | 513 | var rest string |
514 | 514 | var err error |
515 | 515 | |
516 | +if strings.IndexFunc(rawurl, isCTL) != -1 { | |
517 | +return nil, errors.New("net/url: invalid control character in URL") | |
518 | + } | |
519 | + | |
516 | 520 | if rawurl == "" && viaRequest { |
517 | 521 | return nil, errors.New("empty url") |
518 | 522 | } |
@@ -1134,3 +1138,9 @@ func validUserinfo(s string) bool { | ||
1134 | 1138 | } |
1135 | 1139 | return true |
1136 | 1140 | } |
1141 | + | |
1142 | +// isCTL reports whether r is an ASCII control character, including | |
1143 | +// the Extended ASCII control characters included in Unicode. | |
1144 | +func isCTL(r rune) bool { | |
1145 | +return r < ' ' | | |
1146 | +} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1738,12 +1738,27 @@ func TestNilUser(t *testing.T) { | ||
1738 | 1738 | } |
1739 | 1739 | |
1740 | 1740 | func TestInvalidUserPassword(t *testing.T) { |
1741 | -_, err := Parse("http://us\\ner:pass\\nword@foo.com/") | |
1741 | +_, err := Parse("http://user^:passwo^rd@foo.com/") | |
1742 | 1742 | if got, wantsub := fmt.Sprint(err), "net/url: invalid userinfo"; !strings.Contains(got, wantsub) { |
1743 | 1743 | t.Errorf("error = %q; want substring %q", got, wantsub) |
1744 | 1744 | } |
1745 | 1745 | } |
1746 | 1746 | |
1747 | +func TestRejectControlCharacters(t *testing.T) { | |
1748 | +tests := []string{ | |
1749 | +"http://foo.com/?foo\\nbar", | |
1750 | +"http\r://foo.com/", | |
1751 | +"http://foo\\x7f.com/", | |
1752 | + } | |
1753 | +for _, s := range tests { | |
1754 | +_, err := Parse(s) | |
1755 | +const wantSub = "net/url: invalid control character in URL" | |
1756 | +if got := fmt.Sprint(err); !strings.Contains(got, wantSub) { | |
1757 | +t.Errorf("Parse(%q) error = %q; want substring %q", s, got, wantSub) | |
1758 | + } | |
1759 | + } | |
1760 | +} | |
1761 | + | |
1747 | 1762 | var escapeBenchmarks = []struct { |
1748 | 1763 | unescaped string |
1749 | 1764 | query string |