From b4db080be81b233093b32d6ba4c9d80d6b1e1308 Mon Sep 17 00:00:00 2001 From: kel Date: Thu, 12 Oct 2017 06:23:00 -0500 Subject: [PATCH 1/9] Fix http status phrase parsing not allow spaces (#4795) (#5312) * Fix http status phrase parsing not allow spaces (#4795) * Update CHANGELOG --- CHANGELOG.asciidoc | 2 ++ packetbeat/protos/http/http_parser.go | 14 ++++----- packetbeat/protos/http/http_test.go | 45 +++++++++++++++++++++++++++ 3 files changed, 54 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index d25b3e03cc46..00f6746576b8 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -36,6 +36,8 @@ https://github.com/elastic/beats/compare/v5.6.8...5.6[Check the HEAD diff] *Packetbeat* +- Fix http status phrase parsing not allow spaces. {pull}5312[5312] + *Winlogbeat* ==== Added diff --git a/packetbeat/protos/http/http_parser.go b/packetbeat/protos/http/http_parser.go index 49ce6e8da065..11d6ffeaa2ae 100644 --- a/packetbeat/protos/http/http_parser.go +++ b/packetbeat/protos/http/http_parser.go @@ -211,15 +211,15 @@ func parseResponseStatus(s []byte) (uint16, []byte, error) { if p == -1 { return 0, nil, errors.New("Not able to identify status code") } - - code, _ := parseInt(s[0:p]) - - p = bytes.LastIndexByte(s, ' ') - if p == -1 { - return uint16(code), nil, errors.New("Not able to identify status code") + statusCode, err := parseInt(s[0:p]) + if err != nil { + return 0, nil, fmt.Errorf("Unable to parse status code from [%s]", s) } phrase := s[p+1:] - return uint16(code), phrase, nil + if len(phrase) == 0 { + return 0, nil, fmt.Errorf("Unable to parse status phrase from [%s]", s) + } + return uint16(statusCode), phrase, nil } func parseVersion(s []byte) (uint8, uint8, error) { diff --git a/packetbeat/protos/http/http_test.go b/packetbeat/protos/http/http_test.go index 989a2b084c16..0f93f86f25ac 100644 --- a/packetbeat/protos/http/http_test.go +++ b/packetbeat/protos/http/http_test.go @@ -525,6 +525,51 @@ func TestHttpParser_301_response(t *testing.T) { assert.Equal(t, 290, msg.contentLength) } +func TestHttpParser_PhraseContainsSpaces(t *testing.T) { + if testing.Verbose() { + logp.LogInit(logp.LOG_DEBUG, "", false, true, []string{"http"}) + } + response_404 := "HTTP/1.1 404 Not Found\r\n" + + "Server: Apache-Coyote/1.1\r\n" + + "Content-Type: text/html;charset=utf-8\r\n" + + "Content-Length: 18\r\n" + + "Date: Mon, 31 Jul 2017 11:31:53 GMT\r\n" + + "\r\n" + + "Http Response Body" + + r, ok, complete := testParse(nil, response_404) + assert.True(t, ok) + assert.True(t, complete) + assert.Equal(t, 18, r.contentLength) + assert.Equal(t, "Not Found", string(r.statusPhrase)) + assert.Equal(t, 404, int(r.statusCode)) + + response_500 := "HTTP/1.1 500 Internal Server Error\r\n" + + "Server: Apache-Coyote/1.1\r\n" + + "Content-Type: text/html;charset=utf-8\r\n" + + "Content-Length: 2\r\n" + + "Date: Mon, 30 Jul 2017 00:00:00 GMT\r\n" + + "\r\n" + + "xx" + r, ok, complete = testParse(nil, response_500) + assert.True(t, ok) + assert.True(t, complete) + assert.Equal(t, 2, r.contentLength) + assert.Equal(t, "Internal Server Error", string(r.statusPhrase)) + assert.Equal(t, 500, int(r.statusCode)) + + broken := "HTTP/1.1 500 \r\n" + + "Server: Apache-Coyote/1.1\r\n" + + "Content-Type: text/html;charset=utf-8\r\n" + + "Content-Length: 2\r\n" + + "Date: Mon, 30 Jul 2017 00:00:00 GMT\r\n" + + "\r\n" + + "xx" + r, ok, complete = testParse(nil, broken) + assert.False(t, ok) + assert.False(t, complete) +} + func TestEatBodyChunked(t *testing.T) { if testing.Verbose() { logp.LogInit(logp.LOG_DEBUG, "", false, true, []string{"http", "httpdetailed"}) From 6c2070c26a2ec3ab134e0236dcc0c7720df62731 Mon Sep 17 00:00:00 2001 From: Pier-Hugues Pellerin Date: Thu, 9 Nov 2017 09:09:31 -0500 Subject: [PATCH 2/9] Add support for space in get request (#5495) This fix an issue when the http request contains a space instead of breaking the line with `bytes.fields` we are finding the start and the end of the URI using the `METHOD` verb and the `HTTP/{VERSION}`. This will allow packet beat to record theses request instead of ignoring them. Fixes: #4974 --- CHANGELOG.asciidoc | 1 + packetbeat/protos/http/http_parser.go | 23 +++++++++------- packetbeat/protos/http/http_test.go | 39 +++++++++++++++++++++++++++ 3 files changed, 54 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index 00f6746576b8..0859b04f81fd 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -37,6 +37,7 @@ https://github.com/elastic/beats/compare/v5.6.8...5.6[Check the HEAD diff] *Packetbeat* - Fix http status phrase parsing not allow spaces. {pull}5312[5312] +- Fix http parse to allow to parse get request with space in the URI. {pull}5495[5495] *Winlogbeat* diff --git a/packetbeat/protos/http/http_parser.go b/packetbeat/protos/http/http_parser.go index 11d6ffeaa2ae..b4a9e562d068 100644 --- a/packetbeat/protos/http/http_parser.go +++ b/packetbeat/protos/http/http_parser.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "time" + "unicode" "github.com/elastic/beats/libbeat/common" "github.com/elastic/beats/libbeat/common/streambuf" @@ -74,8 +75,9 @@ var ( constCRLF = []byte("\r\n") - constClose = []byte("close") - constKeepAlive = []byte("keep-alive") + constClose = []byte("close") + constKeepAlive = []byte("keep-alive") + constHTTPVersion = []byte("HTTP/") nameContentLength = []byte("content-length") nameContentType = []byte("content-type") @@ -145,7 +147,7 @@ func (*parser) parseHTTPLine(s *stream, m *message) (cont, ok, complete bool) { } return false, false, false } - if bytes.Equal(fline[0:5], []byte("HTTP/")) { + if bytes.Equal(fline[0:5], constHTTPVersion) { //RESPONSE m.isRequest = false version = fline[5:8] @@ -160,20 +162,23 @@ func (*parser) parseHTTPLine(s *stream, m *message) (cont, ok, complete bool) { } } else { // REQUEST - slices := bytes.Fields(fline) - if len(slices) != 3 { + afterMethodIdx := bytes.IndexFunc(fline, unicode.IsSpace) + afterRequestURIIdx := bytes.LastIndexFunc(fline, unicode.IsSpace) + + // Make sure we have the VERB + URI + HTTP_VERSION + if afterMethodIdx == -1 || afterRequestURIIdx == -1 || afterMethodIdx == afterRequestURIIdx { if isDebug { debugf("Couldn't understand HTTP request: %s", fline) } return false, false, false } - m.method = common.NetString(slices[0]) - m.requestURI = common.NetString(slices[1]) + m.method = common.NetString(fline[:afterMethodIdx]) + m.requestURI = common.NetString(fline[afterMethodIdx+1 : afterRequestURIIdx]) - if bytes.Equal(slices[2][:5], []byte("HTTP/")) { + if bytes.Equal(fline[afterRequestURIIdx+1:afterRequestURIIdx+len(constHTTPVersion)+1], constHTTPVersion) { m.isRequest = true - version = slices[2][5:] + version = fline[afterRequestURIIdx+len(constHTTPVersion)+1:] } else { if isDebug { debugf("Couldn't understand HTTP version: %s", fline) diff --git a/packetbeat/protos/http/http_test.go b/packetbeat/protos/http/http_test.go index 0f93f86f25ac..c796bc0c742f 100644 --- a/packetbeat/protos/http/http_test.go +++ b/packetbeat/protos/http/http_test.go @@ -699,6 +699,45 @@ func TestEatBodyChunkedWaitCRLF(t *testing.T) { } } +func TestHttpParser_requestURIWithSpace(t *testing.T) { + if testing.Verbose() { + logp.LogInit(logp.LOG_DEBUG, "", false, true, []string{"http", "httpdetailed"}) + } + + http := httpModForTests() + http.hideKeywords = []string{"password", "pass"} + http.parserConfig.sendHeaders = true + http.parserConfig.sendAllHeaders = true + + // Non URL-encoded string, RFC says it should be encoded + data1 := "GET http://localhost:8080/test?password=two secret HTTP/1.1\r\n" + + "Host: www.google.com\r\n" + + "Connection: keep-alive\r\n" + + "User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_7_4) AppleWebKit/537.1 (KHTML, like Gecko) Chrome/21.0.1180.75 Safari/537.1\r\n" + + "Accept: */*\r\n" + + "X-Chrome-Variations: CLa1yQEIj7bJAQiftskBCKS2yQEIp7bJAQiptskBCLSDygE=\r\n" + + "Referer: http://www.google.com/\r\n" + + "Accept-Encoding: gzip,deflate,sdch\r\n" + + "Accept-Language: en-US,en;q=0.8\r\n" + + "Content-Type: application/x-www-form-urlencoded\r\n" + + "Content-Length: 23\r\n" + + "Accept-Charset: ISO-8859-1,utf-8;q=0.7,*;q=0.3\r\n" + + "Cookie: PREF=ID=6b67d166417efec4:U=69097d4080ae0e15:FF=0:TM=1340891937:LM=1340891938:S=8t97UBiUwKbESvVX; NID=61=sf10OV-t02wu5PXrc09AhGagFrhSAB2C_98ZaI53-uH4jGiVG_yz9WmE3vjEBcmJyWUogB1ZF5puyDIIiB-UIdLd4OEgPR3x1LHNyuGmEDaNbQ_XaxWQqqQ59mX1qgLQ\r\n" + + "\r\n" + + "username=ME&pass=twosecret" + tp := newTestParser(http, data1) + + msg, ok, complete := tp.parse() + assert.True(t, ok) + assert.True(t, complete) + rawMsg := tp.stream.data[tp.stream.message.start:tp.stream.message.end] + path, params, err := http.extractParameters(msg, rawMsg) + assert.Nil(t, err) + assert.Equal(t, "/test", path) + assert.Equal(t, string(msg.requestURI), "http://localhost:8080/test?password=two secret") + assert.False(t, strings.Contains(params, "two secret")) +} + func TestHttpParser_censorPasswordURL(t *testing.T) { if testing.Verbose() { logp.LogInit(logp.LOG_DEBUG, "", false, true, []string{"http", "httpdetailed"}) From e2093ccf8393726ada109adfd6022ba63b84e12a Mon Sep 17 00:00:00 2001 From: Timesking Date: Fri, 5 Jan 2018 19:09:17 +0800 Subject: [PATCH 3/9] =?UTF-8?q?Packetbeat,=20mysql=20proto,=20add=20\r=20t?= =?UTF-8?q?o=20trim=20SQLs=20captured=20from=20app=20runnin=E2=80=A6=20(#5?= =?UTF-8?q?572)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Packetbeat, mysql proto, add \r to trim SQLs captured from app running on Windows server. Otherwise method extracted including \r, which is problem. e.g. "SELECT\r\n\t1" * Packetbeat, test case for windows lineending * Packetbeat, changelog for windows lineending --- CHANGELOG.asciidoc | 1 + packetbeat/protos/mysql/mysql.go | 4 +-- .../pcaps/mysql_windows_lineending.pcap | Bin 0 -> 757 bytes .../test_0064_mysql_windows_lineending.py | 24 ++++++++++++++++++ 4 files changed, 27 insertions(+), 2 deletions(-) create mode 100644 packetbeat/tests/system/pcaps/mysql_windows_lineending.pcap create mode 100644 packetbeat/tests/system/test_0064_mysql_windows_lineending.py diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index 0859b04f81fd..84c4fc1e3330 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -38,6 +38,7 @@ https://github.com/elastic/beats/compare/v5.6.8...5.6[Check the HEAD diff] - Fix http status phrase parsing not allow spaces. {pull}5312[5312] - Fix http parse to allow to parse get request with space in the URI. {pull}5495[5495] +- Fix mysql SQL parser to trim `\r` from Windows Server `SELECT\r\n\t1`. {pull}5572[5572] *Winlogbeat* diff --git a/packetbeat/protos/mysql/mysql.go b/packetbeat/protos/mysql/mysql.go index 298756ab6157..ac492918e6e3 100644 --- a/packetbeat/protos/mysql/mysql.go +++ b/packetbeat/protos/mysql/mysql.go @@ -607,8 +607,8 @@ func (mysql *mysqlPlugin) receivedMysqlRequest(msg *mysqlMessage) { // Extract the method, by simply taking the first word and // making it upper case. - query := strings.Trim(msg.query, " \n\t") - index := strings.IndexAny(query, " \n\t") + query := strings.Trim(msg.query, " \r\n\t") + index := strings.IndexAny(query, " \r\n\t") var method string if index > 0 { method = strings.ToUpper(query[:index]) diff --git a/packetbeat/tests/system/pcaps/mysql_windows_lineending.pcap b/packetbeat/tests/system/pcaps/mysql_windows_lineending.pcap new file mode 100644 index 0000000000000000000000000000000000000000..c6219bc12a4d8474ae536546b95d125aada4a10d GIT binary patch literal 757 zcmca|c+)~A1{MYcU}0bca_;r>MQ!tDWC#MXK{z67-#_(Cfai_Q~||77-BZW9-s{z3^}|<9T@D{CcXrj4zh*k)v11v=|Efd zHApZNr5%RY@TK2117r)(-HeRl3=B-nDXD2d6B!K|c Date: Thu, 8 Feb 2018 21:44:36 +0100 Subject: [PATCH 4/9] Fix http parsing of repeated headers (#6325) Packetbeat HTTP protocol parser was not concatenating properly repeated headers in a request or response. This caused corruption in the header in the form of null bytes (\u0000). --- CHANGELOG.asciidoc | 1 + packetbeat/protos/http/http_parser.go | 4 ++-- packetbeat/protos/http/http_test.go | 22 ++++++++++++++++++++++ 3 files changed, 25 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index 84c4fc1e3330..2c4162abca87 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -39,6 +39,7 @@ https://github.com/elastic/beats/compare/v5.6.8...5.6[Check the HEAD diff] - Fix http status phrase parsing not allow spaces. {pull}5312[5312] - Fix http parse to allow to parse get request with space in the URI. {pull}5495[5495] - Fix mysql SQL parser to trim `\r` from Windows Server `SELECT\r\n\t1`. {pull}5572[5572] +- Fix corruption when parsing repeated headers in an HTTP request or response. {pull}6325[6325] *Winlogbeat* diff --git a/packetbeat/protos/http/http_parser.go b/packetbeat/protos/http/http_parser.go index b4a9e562d068..19a476f47afd 100644 --- a/packetbeat/protos/http/http_parser.go +++ b/packetbeat/protos/http/http_parser.go @@ -359,8 +359,8 @@ func (parser *parser) parseHeader(m *message, data []byte) (bool, bool, int) { if val, ok := m.headers[string(headerName)]; ok { composed := make([]byte, len(val)+len(headerVal)+2) off := copy(composed, val) - off = copy(composed[off:], []byte(", ")) - copy(composed[off:], headerVal) + copy(composed[off:], []byte(", ")) + copy(composed[off+2:], headerVal) m.headers[string(headerName)] = composed } else { diff --git a/packetbeat/protos/http/http_test.go b/packetbeat/protos/http/http_test.go index c796bc0c742f..623be3ac7afa 100644 --- a/packetbeat/protos/http/http_test.go +++ b/packetbeat/protos/http/http_test.go @@ -1132,6 +1132,28 @@ func Test_gap_in_body_http1dot0(t *testing.T) { } +func TestHttpParser_composedHeaders(t *testing.T) { + data := "HTTP/1.1 200 OK\r\n" + + "Content-Length: 0\r\n" + + "Date: Tue, 14 Aug 2012 22:31:45 GMT\r\n" + + "Set-Cookie: aCookie=yummy\r\n" + + "Set-Cookie: anotherCookie=why%20not\r\n" + + "\r\n" + http := httpModForTests() + http.parserConfig.sendHeaders = true + http.parserConfig.sendAllHeaders = true + message, ok, complete := testParse(http, data) + + assert.True(t, ok) + assert.True(t, complete) + assert.False(t, message.isRequest) + assert.Equal(t, 200, int(message.statusCode)) + assert.Equal(t, "OK", string(message.statusPhrase)) + header, ok := message.headers["set-cookie"] + assert.True(t, ok) + assert.Equal(t, "aCookie=yummy, anotherCookie=why%20not", string(header)) +} + func testCreateTCPTuple() *common.TCPTuple { t := &common.TCPTuple{ IPLength: 4, From 6dddbd8d340bf8d6316ef736a4a5613515a12926 Mon Sep 17 00:00:00 2001 From: Adrian Serrano Date: Wed, 14 Feb 2018 22:43:49 +0100 Subject: [PATCH 5/9] AMQP: Fix panic during parse of partial message (#6384) A message with a client header consisting on a partial frame (not all data received for this frame) could result in a panic. --- CHANGELOG.asciidoc | 1 + packetbeat/protos/amqp/amqp_parser.go | 5 ++++- packetbeat/protos/amqp/amqp_test.go | 24 ++++++++++++++++++++++++ 3 files changed, 29 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index 2c4162abca87..bbae5eebae4f 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -40,6 +40,7 @@ https://github.com/elastic/beats/compare/v5.6.8...5.6[Check the HEAD diff] - Fix http parse to allow to parse get request with space in the URI. {pull}5495[5495] - Fix mysql SQL parser to trim `\r` from Windows Server `SELECT\r\n\t1`. {pull}5572[5572] - Fix corruption when parsing repeated headers in an HTTP request or response. {pull}6325[6325] +- Fix panic when parsing partial AMQP messages. {pull}6384[6384] *Winlogbeat* diff --git a/packetbeat/protos/amqp/amqp_parser.go b/packetbeat/protos/amqp/amqp_parser.go index 0ec4a39c71d5..00b3cd01a107 100644 --- a/packetbeat/protos/amqp/amqp_parser.go +++ b/packetbeat/protos/amqp/amqp_parser.go @@ -73,7 +73,10 @@ func isProtocolHeader(data []byte) (isHeader bool, version string) { //func to read a frame header and check if it is valid and complete func readFrameHeader(data []byte) (ret *amqpFrame, err bool) { var frame amqpFrame - + if len(data) < 8 { + logp.Warn("Partial frame header, waiting for more data") + return nil, false + } frame.size = binary.BigEndian.Uint32(data[3:7]) if len(data) < int(frame.size)+8 { logp.Warn("Frame shorter than declared size, waiting for more data") diff --git a/packetbeat/protos/amqp/amqp_test.go b/packetbeat/protos/amqp/amqp_test.go index be8d198391a1..1222be569884 100644 --- a/packetbeat/protos/amqp/amqp_test.go +++ b/packetbeat/protos/amqp/amqp_test.go @@ -83,6 +83,30 @@ func TestAmqp_FrameSize(t *testing.T) { } } +// Test that the parser doesn't panic on a partial message that includes +// a client header +func TestAmqp_PartialFrameSize(t *testing.T) { + if testing.Verbose() { + logp.LogInit(logp.LOG_DEBUG, "", false, true, []string{"amqp", "amqpdetailed"}) + } + + amqp := amqpModForTests() + + //incomplete frame + data, err := hex.DecodeString("414d515000060606010000000000") + assert.Nil(t, err) + + stream := &amqpStream{data: data, message: new(amqpMessage)} + ok, complete := amqp.amqpMessageParser(stream) + + if !ok { + t.Errorf("Parsing should not raise an error") + } + if complete { + t.Errorf("message should not be complete") + } +} + func TestAmqp_WrongShortStringSize(t *testing.T) { if testing.Verbose() { logp.LogInit(logp.LOG_DEBUG, "", false, true, []string{"amqp", "amqpdetailed"}) From 728cdd18b03022721768e4d1493f1150d22f8fdb Mon Sep 17 00:00:00 2001 From: Adrian Serrano Date: Thu, 5 Apr 2018 11:50:40 +0200 Subject: [PATCH 6/9] Fix out of bounds access to slice in MongoDB parser (#6256) * Fix out of bounds access to slice in MongoDB parser Ignore MongoDB message and drop the TCP stream if a malformed query / response is received, instead of logging a panic. Closes #5188 * Update CHANGELOG --- CHANGELOG.asciidoc | 1 + packetbeat/protos/mongodb/mongodb_parser.go | 3 ++ packetbeat/protos/mongodb/mongodb_test.go | 31 ++++++++++++++++++++- 3 files changed, 34 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index bbae5eebae4f..49def2c35503 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -41,6 +41,7 @@ https://github.com/elastic/beats/compare/v5.6.8...5.6[Check the HEAD diff] - Fix mysql SQL parser to trim `\r` from Windows Server `SELECT\r\n\t1`. {pull}5572[5572] - Fix corruption when parsing repeated headers in an HTTP request or response. {pull}6325[6325] - Fix panic when parsing partial AMQP messages. {pull}6384[6384] +- Fix out of bounds access to slice in MongoDB parser. {pull}6256[6256] *Winlogbeat* diff --git a/packetbeat/protos/mongodb/mongodb_parser.go b/packetbeat/protos/mongodb/mongodb_parser.go index 6567c250ee0c..4631d5181672 100644 --- a/packetbeat/protos/mongodb/mongodb_parser.go +++ b/packetbeat/protos/mongodb/mongodb_parser.go @@ -341,6 +341,9 @@ func (d *decoder) readDocument() (bson.M, error) { start := d.i documentLength, err := d.readInt32() d.i = start + documentLength + if len(d.in) < d.i { + return nil, errors.New("document out of bounds") + } documentMap := bson.M{} diff --git a/packetbeat/protos/mongodb/mongodb_test.go b/packetbeat/protos/mongodb/mongodb_test.go index f7f69e3ffc25..7bf201e8dafa 100644 --- a/packetbeat/protos/mongodb/mongodb_test.go +++ b/packetbeat/protos/mongodb/mongodb_test.go @@ -7,11 +7,12 @@ import ( "net" "testing" + "github.com/stretchr/testify/assert" + "github.com/elastic/beats/libbeat/common" "github.com/elastic/beats/libbeat/logp" "github.com/elastic/beats/packetbeat/protos" "github.com/elastic/beats/packetbeat/publish" - "github.com/stretchr/testify/assert" ) // Helper function returning a Mongodb module that can be used @@ -333,3 +334,31 @@ func TestMaxDocSize(t *testing.T) { assert.Equal(t, "\"1234 ...\n\"123\"\n\"12\"", res["response"]) } + +// Test for a (recovered) panic parsing document length in request/response messages +func TestDocumentLengthBoundsChecked(t *testing.T) { + if testing.Verbose() { + logp.LogInit(logp.LOG_DEBUG, "", false, true, []string{"mongodb", "mongodbdetailed"}) + } + + mongodb := mongodbModForTests() + + // request and response from tests/pcaps/mongo_one_row.pcap + reqData, err := hex.DecodeString( + // Request message with out of bounds document + "320000000a000000ffffffffd4070000" + + "00000000746573742e72667374617572" + + "616e7473000000000001000000" + + // Document length (including itself) + "06000000" + + // Document (1 byte instead of 2) + "00") + assert.Nil(t, err) + + tcptuple := testTCPTuple() + req := protos.Packet{Payload: reqData} + private := protos.ProtocolData(new(mongodbConnectionData)) + + private = mongodb.Parse(&req, tcptuple, 0, private) + assert.NotNil(t, private, "mongodb parser recovered from a panic") +} From ed4011543be6ede08700d57b3f6fe794169f6f04 Mon Sep 17 00:00:00 2001 From: Julian Reyes Date: Mon, 29 Jan 2018 11:19:57 +0000 Subject: [PATCH 7/9] Packetbeat HTTP parsing fails on empty status phrase. - The function [parseResponseStatus](https://github.com/elastic/beats/blob/81f55f89fd1db748d0063e4e3c0564913e23ee46/packetbeat/protos/http/http_parser.go#L210) now returns the phrase, even if empty, instead of an error. - Added tests cases to http_test.go --- CHANGELOG.asciidoc | 2 + packetbeat/protos/http/http_parser.go | 3 -- packetbeat/protos/http/http_test.go | 55 ++++++++++++++++++++++++++- 3 files changed, 55 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index 49def2c35503..2b5aff599b56 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -57,6 +57,8 @@ https://github.com/elastic/beats/compare/v5.6.8...5.6[Check the HEAD diff] *Packetbeat* +- HTTP parses successfully on empty status phrase. {issue}6176[6176] + *Winlogbeat* ==== Deprecated diff --git a/packetbeat/protos/http/http_parser.go b/packetbeat/protos/http/http_parser.go index 19a476f47afd..06071e61752a 100644 --- a/packetbeat/protos/http/http_parser.go +++ b/packetbeat/protos/http/http_parser.go @@ -221,9 +221,6 @@ func parseResponseStatus(s []byte) (uint16, []byte, error) { return 0, nil, fmt.Errorf("Unable to parse status code from [%s]", s) } phrase := s[p+1:] - if len(phrase) == 0 { - return 0, nil, fmt.Errorf("Unable to parse status phrase from [%s]", s) - } return uint16(statusCode), phrase, nil } diff --git a/packetbeat/protos/http/http_test.go b/packetbeat/protos/http/http_test.go index 623be3ac7afa..b77c263443c3 100644 --- a/packetbeat/protos/http/http_test.go +++ b/packetbeat/protos/http/http_test.go @@ -380,6 +380,54 @@ func TestHttpParser_Response_HTTP_10_without_content_length(t *testing.T) { assert.Equal(t, 4, message.contentLength) } +func TestHttpParser_Response_without_phrase(t *testing.T) { + data := "HTTP/1.1 200 \r\n" + + "Date: Tue, 14 Aug 2012 22:31:45 GMT\r\n" + + "Expires: -1\r\n" + + "Cache-Control: private, max-age=0\r\n" + + "Content-Type: text/html; charset=UTF-8\r\n" + + "Content-Encoding: gzip\r\n" + + "Server: gws\r\n" + + "Content-Length: 0\r\n" + + "X-XSS-Protection: 1; mode=block\r\n" + + "X-Frame-Options: SAMEORIGIN\r\n" + + "\r\n" + r, ok, complete := testParse(nil, data) + + assert.True(t, ok) + assert.True(t, complete) + assert.False(t, r.isRequest) + assert.Equal(t, 200, int(r.statusCode)) + assert.Equal(t, "", string(r.statusPhrase)) + assert.Equal(t, 0, r.contentLength) + + broken := "HTTP/1.1 301 \r\n" + + "Date: Sun, 29 Sep 2013 16:53:59 GMT\r\n" + + "Server: Apache\r\n" + + "Location: http://www.hotnews.ro/\r\n" + + "Vary: Accept-Encoding\r\n" + + "Content-Length: 290\r\n" + + "Connection: close\r\n" + + "Content-Type: text/html; charset=iso-8859-1\r\n" + + "\r\n" + + "\r\n" + + "\r\n" + + "301 Moved Permanently\r\n" + + "\r\n" + + "

Moved Permanently

\r\n" + + "

The document has moved here.

\r\n" + + "
\r\n" + + "
Apache Server at hotnews.ro Port 80
\r\n" + + "" + r, ok, complete = testParse(nil, broken) + + assert.True(t, ok) + assert.True(t, complete) + assert.Equal(t, 290, r.contentLength) + assert.Equal(t, "", string(r.statusPhrase)) + assert.Equal(t, 301, int(r.statusCode)) +} + func TestHttpParser_splitResponse_midBody(t *testing.T) { data1 := "HTTP/1.1 200 OK\r\n" + "Date: Tue, 14 Aug 2012 22:31:45 GMT\r\n" + @@ -566,8 +614,11 @@ func TestHttpParser_PhraseContainsSpaces(t *testing.T) { "\r\n" + "xx" r, ok, complete = testParse(nil, broken) - assert.False(t, ok) - assert.False(t, complete) + assert.True(t, ok) + assert.True(t, complete) + assert.Equal(t, 2, r.contentLength) + assert.Equal(t, "", string(r.statusPhrase)) + assert.Equal(t, 500, int(r.statusCode)) } func TestEatBodyChunked(t *testing.T) { From ce54a5eabcbbf2e222161bf0f52e2123249219ed Mon Sep 17 00:00:00 2001 From: Adrian Serrano Date: Mon, 26 Mar 2018 09:03:36 +0200 Subject: [PATCH 8/9] http: support broken status line (#6631) User reports some HTTP servers may respond with a broken status line that's missing a space between the status code and the optional status phrase. HTTP clients tested already support this behavior. This patch adjusts the http parser so that this deviation from the standard is accepted. Fixes #6176 --- CHANGELOG.asciidoc | 1 + packetbeat/protos/http/http_parser.go | 6 ++- packetbeat/protos/http/http_test.go | 61 +++++++-------------------- 3 files changed, 21 insertions(+), 47 deletions(-) diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index 2b5aff599b56..5f8ed0a6bd63 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -58,6 +58,7 @@ https://github.com/elastic/beats/compare/v5.6.8...5.6[Check the HEAD diff] *Packetbeat* - HTTP parses successfully on empty status phrase. {issue}6176[6176] +- HTTP parser supports broken status line. {pull}6631[6631] *Winlogbeat* diff --git a/packetbeat/protos/http/http_parser.go b/packetbeat/protos/http/http_parser.go index 06071e61752a..6e58db75a4e9 100644 --- a/packetbeat/protos/http/http_parser.go +++ b/packetbeat/protos/http/http_parser.go @@ -212,15 +212,17 @@ func parseResponseStatus(s []byte) (uint16, []byte, error) { debugf("parseResponseStatus: %s", s) } + var phrase []byte p := bytes.IndexByte(s, ' ') if p == -1 { - return 0, nil, errors.New("Not able to identify status code") + p = len(s) + } else { + phrase = s[p+1:] } statusCode, err := parseInt(s[0:p]) if err != nil { return 0, nil, fmt.Errorf("Unable to parse status code from [%s]", s) } - phrase := s[p+1:] return uint16(statusCode), phrase, nil } diff --git a/packetbeat/protos/http/http_test.go b/packetbeat/protos/http/http_test.go index b77c263443c3..214e6e151b0b 100644 --- a/packetbeat/protos/http/http_test.go +++ b/packetbeat/protos/http/http_test.go @@ -4,6 +4,7 @@ package http import ( "bytes" + "fmt" "net" "regexp" "strings" @@ -381,51 +382,21 @@ func TestHttpParser_Response_HTTP_10_without_content_length(t *testing.T) { } func TestHttpParser_Response_without_phrase(t *testing.T) { - data := "HTTP/1.1 200 \r\n" + - "Date: Tue, 14 Aug 2012 22:31:45 GMT\r\n" + - "Expires: -1\r\n" + - "Cache-Control: private, max-age=0\r\n" + - "Content-Type: text/html; charset=UTF-8\r\n" + - "Content-Encoding: gzip\r\n" + - "Server: gws\r\n" + - "Content-Length: 0\r\n" + - "X-XSS-Protection: 1; mode=block\r\n" + - "X-Frame-Options: SAMEORIGIN\r\n" + - "\r\n" - r, ok, complete := testParse(nil, data) - - assert.True(t, ok) - assert.True(t, complete) - assert.False(t, r.isRequest) - assert.Equal(t, 200, int(r.statusCode)) - assert.Equal(t, "", string(r.statusPhrase)) - assert.Equal(t, 0, r.contentLength) - - broken := "HTTP/1.1 301 \r\n" + - "Date: Sun, 29 Sep 2013 16:53:59 GMT\r\n" + - "Server: Apache\r\n" + - "Location: http://www.hotnews.ro/\r\n" + - "Vary: Accept-Encoding\r\n" + - "Content-Length: 290\r\n" + - "Connection: close\r\n" + - "Content-Type: text/html; charset=iso-8859-1\r\n" + - "\r\n" + - "\r\n" + - "\r\n" + - "301 Moved Permanently\r\n" + - "\r\n" + - "

Moved Permanently

\r\n" + - "

The document has moved here.

\r\n" + - "
\r\n" + - "
Apache Server at hotnews.ro Port 80
\r\n" + - "" - r, ok, complete = testParse(nil, broken) - - assert.True(t, ok) - assert.True(t, complete) - assert.Equal(t, 290, r.contentLength) - assert.Equal(t, "", string(r.statusPhrase)) - assert.Equal(t, 301, int(r.statusCode)) + for idx, testCase := range []struct { + ok, complete bool + code int + request string + }{ + {true, true, 200, "HTTP/1.1 200 \r\nContent-Length: 0\r\n\r\n"}, + {true, true, 301, "HTTP/1.1 301\r\nContent-Length: 0\r\n\r\n"}, + } { + msg := fmt.Sprintf("failed test case[%d]: \"%s\"", idx, testCase.request) + r, ok, complete := testParse(nil, testCase.request) + assert.Equal(t, testCase.ok, ok, msg) + assert.Equal(t, testCase.complete, complete, msg) + assert.Equal(t, testCase.code, int(r.statusCode), msg) + assert.Equal(t, "", string(r.statusPhrase), msg) + } } func TestHttpParser_splitResponse_midBody(t *testing.T) { From 117e90f2d1ec8fa5578654296d7df94b912a75db Mon Sep 17 00:00:00 2001 From: Adrian Serrano Date: Thu, 5 Apr 2018 01:26:27 +0200 Subject: [PATCH 9/9] Fix minor panic in http parser (#6750) There was a bounds check error in parsing HTTP responses. A malformed response line in the form "HTTP/1.1\r\n" would cause a panic when parsed. Related to #6409 --- CHANGELOG.asciidoc | 2 ++ packetbeat/protos/http/http_parser.go | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index 5f8ed0a6bd63..d3d1ef408617 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -42,6 +42,8 @@ https://github.com/elastic/beats/compare/v5.6.8...5.6[Check the HEAD diff] - Fix corruption when parsing repeated headers in an HTTP request or response. {pull}6325[6325] - Fix panic when parsing partial AMQP messages. {pull}6384[6384] - Fix out of bounds access to slice in MongoDB parser. {pull}6256[6256] +- Fix sniffer hanging on exit under Linux. {pull}6535[6535] +- Fix bounds check error in http parser causing a panic. {pull}6750[6750] *Winlogbeat* diff --git a/packetbeat/protos/http/http_parser.go b/packetbeat/protos/http/http_parser.go index 6e58db75a4e9..f09e1ca3f94f 100644 --- a/packetbeat/protos/http/http_parser.go +++ b/packetbeat/protos/http/http_parser.go @@ -141,7 +141,7 @@ func (*parser) parseHTTPLine(s *stream, m *message) (cont, ok, complete bool) { var version []byte var err error fline := s.data[s.parseOffset:i] - if len(fline) < 8 { + if len(fline) < 9 { if isDebug { debugf("First line too small") }