From 27647176572c3c844743582f3a7f715f85239787 Mon Sep 17 00:00:00 2001 From: Leo Balduf Date: Sat, 17 Jun 2017 20:30:55 +0200 Subject: [PATCH 1/2] bittorrent: fix out-of-range panics for URL parsing --- bittorrent/params.go | 98 +++++++++++++++------------------------ bittorrent/params_test.go | 44 +++++++++++++----- 2 files changed, 69 insertions(+), 73 deletions(-) diff --git a/bittorrent/params.go b/bittorrent/params.go index a0f60bc..a24d3f6 100644 --- a/bittorrent/params.go +++ b/bittorrent/params.go @@ -88,69 +88,45 @@ func ParseURLData(urlData string) (*QueryParams, error) { // parseQuery parses a URL query into QueryParams. // The query is expected to exclude the delimiting '?'. -func parseQuery(rawQuery string) (*QueryParams, error) { - var ( - keyStart, keyEnd int - valStart, valEnd int +func parseQuery(query string) (q *QueryParams, err error) { + // This is basically url.parseQuery, but with a map[string]string + // instead of map[string][]string for the values. + q = &QueryParams{ + query: query, + infoHashes: nil, + params: make(map[string]string), + } - onKey = true - - q = &QueryParams{ - query: rawQuery, - infoHashes: nil, - params: make(map[string]string), - } - ) - - for i, length := 0, len(rawQuery); i < length; i++ { - separator := rawQuery[i] == '&' || rawQuery[i] == ';' - last := i == length-1 - - if separator || last { - if onKey && !last { - keyStart = i + 1 - continue - } - - if last && !separator && !onKey { - valEnd = i - } - - keyStr, err := url.QueryUnescape(rawQuery[keyStart : keyEnd+1]) - if err != nil { - return nil, err - } - - var valStr string - - if valEnd > 0 { - valStr, err = url.QueryUnescape(rawQuery[valStart : valEnd+1]) - if err != nil { - return nil, err - } - } - - if keyStr == "info_hash" { - if len(valStr) != 20 { - return nil, ErrInvalidInfohash - } - q.infoHashes = append(q.infoHashes, InfoHashFromString(valStr)) - } else { - q.params[strings.ToLower(keyStr)] = valStr - } - - valEnd = 0 - onKey = true - keyStart = i + 1 - - } else if rawQuery[i] == '=' { - onKey = false - valStart = i + 1 - valEnd = 0 - } else if onKey { - keyEnd = i + for query != "" { + key := query + if i := strings.IndexAny(key, "&;"); i >= 0 { + key, query = key[:i], key[i+1:] } else { - valEnd = i + query = "" + } + if key == "" { + continue + } + value := "" + if i := strings.Index(key, "="); i >= 0 { + key, value = key[:i], key[i+1:] + } + key, err = url.QueryUnescape(key) + if err != nil { + return nil, err + } + value, err = url.QueryUnescape(value) + if err != nil { + return nil, err + } + + if key == "info_hash" { + if len(value) != 20 { + return nil, ErrInvalidInfohash + } + q.infoHashes = append(q.infoHashes, InfoHashFromString(value)) + } else { + q.params[strings.ToLower(key)] = value } } diff --git a/bittorrent/params_test.go b/bittorrent/params_test.go index 86da163..5e6c70c 100644 --- a/bittorrent/params_test.go +++ b/bittorrent/params_test.go @@ -27,6 +27,12 @@ var ( InvalidQueries = []string{ "/announce?" + "info_hash=%0%a", } + + // See https://github.com/chihaya/chihaya/issues/334. + shouldNotPanicQueries = []string{ + "/annnounce?" + "info_hash=" + testPeerID + "&a", + "/annnounce?" + "info_hash=" + testPeerID + "&=b?", + } ) func mapArrayEqual(boxed map[string][]string, unboxed map[string]string) bool { @@ -84,26 +90,40 @@ func TestParseInvalidURLData(t *testing.T) { } } +func TestParseShouldNotPanicURLData(t *testing.T) { + for _, parseStr := range shouldNotPanicQueries { + ParseURLData(parseStr) + } +} + func BenchmarkParseQuery(b *testing.B) { + announceStrings := make([]string, 0) + for i := range ValidAnnounceArguments { + announceStrings = append(announceStrings, ValidAnnounceArguments[i].Encode()) + } + b.ResetTimer() for bCount := 0; bCount < b.N; bCount++ { - for parseIndex, parseStr := range ValidAnnounceArguments { - parsedQueryObj, err := parseQuery(parseStr.Encode()) - if err != nil { - b.Error(err, parseIndex) - b.Log(parsedQueryObj) - } + i := bCount % len(announceStrings) + parsedQueryObj, err := parseQuery(announceStrings[i]) + if err != nil { + b.Error(err, i) + b.Log(parsedQueryObj) } } } func BenchmarkURLParseQuery(b *testing.B) { + announceStrings := make([]string, 0) + for i := range ValidAnnounceArguments { + announceStrings = append(announceStrings, ValidAnnounceArguments[i].Encode()) + } + b.ResetTimer() for bCount := 0; bCount < b.N; bCount++ { - for parseIndex, parseStr := range ValidAnnounceArguments { - parsedQueryObj, err := url.ParseQuery(parseStr.Encode()) - if err != nil { - b.Error(err, parseIndex) - b.Log(parsedQueryObj) - } + i := bCount % len(announceStrings) + parsedQueryObj, err := url.ParseQuery(announceStrings[i]) + if err != nil { + b.Error(err, i) + b.Log(parsedQueryObj) } } } From 6e1cfa18d896625a59833e8ddad724c059c3e5ad Mon Sep 17 00:00:00 2001 From: Leo Balduf Date: Sat, 17 Jun 2017 20:31:53 +0200 Subject: [PATCH 2/2] bittorrent: make invalid query escape errors static --- bittorrent/params.go | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/bittorrent/params.go b/bittorrent/params.go index a24d3f6..4e28b73 100644 --- a/bittorrent/params.go +++ b/bittorrent/params.go @@ -5,6 +5,8 @@ import ( "net/url" "strconv" "strings" + + log "github.com/Sirupsen/logrus" ) // Params is used to fetch (optional) request parameters from an Announce. @@ -39,6 +41,10 @@ var ErrKeyNotFound = errors.New("query: value for the provided key does not exis // with invalid length. var ErrInvalidInfohash = ClientError("provided invalid infohash") +// ErrInvalidQueryEscape is returned when a query string contains invalid +// escapes. +var ErrInvalidQueryEscape = ClientError("invalid query escape") + // QueryParams parses a URL Query and implements the Params interface with some // additional helpers. type QueryParams struct { @@ -113,11 +119,21 @@ func parseQuery(query string) (q *QueryParams, err error) { } key, err = url.QueryUnescape(key) if err != nil { - return nil, err + // QueryUnescape returns an error like "invalid escape: '%x'". + // But frontends record these errors to prometheus, which generates + // a lot of time series. + // We log it here for debugging instead. + log.WithFields(log.Fields{"error": err}).Debug("failed to unescape query param key") + return nil, ErrInvalidQueryEscape } value, err = url.QueryUnescape(value) if err != nil { - return nil, err + // QueryUnescape returns an error like "invalid escape: '%x'". + // But frontends record these errors to prometheus, which generates + // a lot of time series. + // We log it here for debugging instead. + log.WithFields(log.Fields{"error": err}).Debug("failed to unescape query param value") + return nil, ErrInvalidQueryEscape } if key == "info_hash" {